Bug 80681 - [JSC] AudioDestinationNode event notification error on AudioContext
Summary: [JSC] AudioDestinationNode event notification error on AudioContext
Status: RESOLVED DUPLICATE of bug 102356
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 61355
  Show dependency treegraph
Reported: 2012-03-09 00:05 PST by Philippe Normand
Modified: 2012-12-29 08:21 PST (History)
5 users (show)

See Also:

Patch (5.08 KB, patch)
2012-03-12 01:38 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Proposed patch (5.17 KB, patch)
2012-04-03 07:08 PDT, Philippe Normand
ggaren: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-03-09 00:05:23 PST
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.
Comment 1 Philippe Normand 2012-03-12 01:38:30 PDT
Created attachment 131293 [details]
Comment 2 WebKit Review Bot 2012-03-12 01:40:21 PDT
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.
Comment 3 Philippe Normand 2012-04-03 07:08:04 PDT
Created attachment 135325 [details]
Proposed patch
Comment 4 Philippe Normand 2012-04-03 07:11:54 PDT
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 :(
Comment 5 Geoffrey Garen 2012-04-03 09:39:54 PDT
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.
Comment 6 Philippe Normand 2012-04-03 09:42:48 PDT
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.
Comment 7 Philippe Normand 2012-12-29 08:21:10 PST

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