[webkit-reviews] review denied: [Bug 80681] [JSC] AudioDestinationNode event notification error on AudioContext : [Attachment 135325] Proposed patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 80681: [JSC] AudioDestinationNode event notification error on AudioContext
https://bugs.webkit.org/show_bug.cgi?id=80681

Attachment 135325: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=135325&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=135325&action=review


> Source/WebCore/Modules/webaudio/AudioContext.h:84
> +	   bool running(destination() ? destination()->running() : false);

WebKit style is to use "=" instead of constructor syntax for "running" here.
But this probably won't matter in the end -- see below.

> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:88
>	   m_startedRendering = true;
> +	   m_running = true;
>	   ref(); // See corresponding deref() call in
notifyCompleteDispatch().

I think "m_running = false" should go wherever we call "deref()" and destroy
our audio thread, rather than in notifyComplete(). I'm not familiar with this
code, but from the outside it seems like some chains of function calls could
cancel the audio without calling notifyComplete(), which would leave a zombie
audio node.

Even better: ActiveDOMObject packages up these concepts for you. Instead of
manually calling ref() and maintaining a separate boolean, you can just call
"setPendingActivity". That will ref() your object and cause
hasPendingActivity() to start returning true. No need to keep an m_running
variable, no need to override hasPendingActivity(). Then, call
"unsetPendingActivity" wherever you stop playing and cancel the audio thread.