Bug 80681 – [JSC] AudioDestinationNode event notification error on AudioContext

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Philippe Normand]

While working on getting the WebAudio layout tests up in the GTK port I came up with this:

#0  0xb41008b6 in WebCore::JSEventListener::jsFunction (this=0xa14eeae0, scriptExecutionContext=0xa14cf920)
    at ../../Source/WebCore/bindings/js/JSEventListener.h:84
84	        ASSERT(m_wrapper || !m_jsFunction);

#0  0xb41008b6 in WebCore::JSEventListener::jsFunction (this=0xa14eeae0, scriptExecutionContext=0xa14cf920)
    at ../../Source/WebCore/bindings/js/JSEventListener.h:84
#1  0xb410003d in WebCore::JSEventListener::handleEvent (this=0xa14eeae0, scriptExecutionContext=0xa14cf920, event=0xa141b2a8)
    at ../../Source/WebCore/bindings/js/JSEventListener.cpp:80
#2  0xb4348683 in WebCore::EventTarget::fireEventListeners (this=0x9f6351ac, event=0xa141b2a8, d=0x9f63527c, entry=...)
    at ../../Source/WebCore/dom/EventTarget.cpp:231
#3  0xb4348525 in WebCore::EventTarget::fireEventListeners (this=0x9f6351ac, event=0xa141b2a8) at ../../Source/WebCore/dom/EventTarget.cpp:198
#4  0xb43483be in WebCore::EventTarget::dispatchEvent (this=0x9f6351ac, event=...) at ../../Source/WebCore/dom/EventTarget.cpp:177
#5  0xb4a88cf9 in WebCore::AudioContext::fireCompletionEvent (this=0x9f635198) at ../../Source/WebCore/webaudio/AudioContext.cpp:778
#6  0xb4a9f583 in WebCore::OfflineAudioDestinationNode::notifyComplete (this=0x9f633020)
    at ../../Source/WebCore/webaudio/OfflineAudioDestinationNode.cpp:165
#7  0xb4a9f541 in WebCore::OfflineAudioDestinationNode::notifyCompleteDispatch (userData=0x9f633020)
    at ../../Source/WebCore/webaudio/OfflineAudioDestinationNode.cpp:159
#8  0xb7397733 in WTF::dispatchFunctionsFromMainThread () at ../../Source/JavaScriptCore/wtf/MainThread.cpp:156
#9  0xb73974a0 in WTF::timeoutFired () at ../../Source/JavaScriptCore/wtf/gtk/MainThreadGtk.cpp:43
#10 0xb321b9df in g_timeout_dispatch (source=0xa1cb5d0, callback=0xb7397489 <WTF::timeoutFired(gpointer)>, user_data=0x0) at gmain.c:3854
#11 0xb321ac7a in g_main_dispatch (context=0x9e0c058) at gmain.c:2510
#12 g_main_context_dispatch (context=0x9e0c058) at gmain.c:3047
#13 0xb321b085 in g_main_context_iterate (dispatch=1, block=-1289582320, context=0x9e0c058, self=<optimized out>) at gmain.c:3118
#14 g_main_context_iterate (context=0x9e0c058, block=-1289582320, dispatch=1, self=<optimized out>) at gmain.c:3055
#15 0xb321b4cb in g_main_loop_run (loop=0x9f61ab90) at gmain.c:3312
#16 0xb37e28b5 in gtk_main () at gtkmain.c:1358
#17 0x0809c471 in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:715
#18 0x0809bad7 in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:498
#19 0x0809e74f in main (argc=2, argv=0xbf9143c4) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1389

It seems that the AudioDestinationNode tries to notify a no longer active AudioContext.

[Philippe Normand]

Created attachment 131293 [details]
Patch

[WebKit Review Bot]

Attachment 131293 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/webaudio/AudioContext.h:82:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 135325 [details]
Proposed patch

[Philippe Normand]

CCing Geoffrey Garren for this WebAudio/JSC issue.

If you can have a look at this patch it would be great, I had some good results with it but I wonder if the approach is correct, it's probably too naive :(

[Geoffrey Garen]

Comment on attachment 135325 [details]
Proposed patch

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.

[Philippe Normand]

Thanks for the review! I'll rework the patch as you advised and try to get Chris Rogers eyes on the updated patch as well.

[Philippe Normand]

*** This bug has been marked as a duplicate of bug 102356 ***