Summary: | [JSC] AudioDestinationNode event notification error on AudioContext | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED DUPLICATE | ||||||||
Severity: | Normal | CC: | crogers, ggaren, pnormand, s.choi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 61355 | ||||||||
Attachments: |
|
Description
Philippe Normand
2012-03-09 00:05:23 PST
Created attachment 131293 [details]
Patch
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.
Created attachment 135325 [details]
Proposed patch
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 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. 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. *** This bug has been marked as a duplicate of bug 102356 *** |