[webkit-reviews] review denied: [Bug 61947] AudioContext needs non-blocking call to create AudioBuffer from audio file data : [Attachment 97669] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied  review:
Bug 61947: AudioContext needs non-blocking call to create AudioBuffer from
audio file data
https://bugs.webkit.org/show_bug.cgi?id=61947

Attachment 97669: Patch
https://bugs.webkit.org/attachment.cgi?id=97669&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97669&action=review

Sorry to override but I don't think this is correct.  Do we have to pass
refptr<>s to callbacks to the second thread?  That seems likely to be racy
since our RefCounted implementation is not threadsafe.

> Source/WebCore/webaudio/AsyncAudioDecoder.cpp:67
> +    OwnPtr<DecodingTask> decodingTask = adoptPtr(new DecodingTask(audioData,
sampleRate, successCallback, errorCallback));
> +    m_queue.append(decodingTask.release()); // note that ownership of the
task is effectively taken by the queue.

This worries me because we're passing two RefPtr<>s to another thread.	Is it
safe to drop the last reference to these callbacks from somewhere other than
the main thread?  The answer is nearly always no, and our RefPtr<>
implementation is not thread-safe without some sort of additional
synchronization around the refcount.