WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
112521
ScriptProcessorNode is garbage collected while still active if unreachable (breaks multiple webaudio test)
https://bugs.webkit.org/show_bug.cgi?id=112521
Summary
ScriptProcessorNode is garbage collected while still active if unreachable (b...
Russell McClellan
Reported
2013-03-17 14:10:00 PDT
When garbage collection runs, any ScriptProcessorNode's bindings are garbage collected if the JS binding is unreachable. This can happen for instance if the ScriptProcessorNode is created inside a function. Garbage collecting the binding will cause any event listeners to also be garbage collected, which will mean they won't be able to process any more audio. Steps to reproduce: 1) create a ScriptProcessorNode from inside a function 2) attach an onaudioprocess handler 3) attach the script processor node to the audio graph 4) leave the function 5) trigger garbage collection After garbage collection occurs, the onaudioprocess handler will be deleted and will never be called again. This was reported in chrome as issue 82795:
https://code.google.com/p/chromium/issues/detail?id=82795
, but it's bug in webkit. It happens in every version since it's a problem with the garbage collection scheme.
Attachments
Patch
(17.28 KB, patch)
2013-03-17 14:53 PDT
,
Russell McClellan
no flags
Details
Formatted Diff
Diff
Patch
(7.28 KB, patch)
2013-03-17 21:29 PDT
,
Russell McClellan
no flags
Details
Formatted Diff
Diff
Patch
(7.09 KB, patch)
2013-03-17 21:57 PDT
,
Russell McClellan
no flags
Details
Formatted Diff
Diff
Patch
(8.73 KB, patch)
2013-03-18 17:07 PDT
,
Russell McClellan
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Russell McClellan
Comment 1
2013-03-17 14:53:05 PDT
Created
attachment 193473
[details]
Patch
Kentaro Hara
Comment 2
2013-03-17 17:22:15 PDT
Comment on
attachment 193473
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=193473&action=review
A right way to fix the GC problem would be: (1) Make AudioNode an ActiveDOMObject. (2) Override hasPendingActivity() like this: bool AudioNode::hasPendingActivity() { return !m_isDisabled && (m_connectionRefCount > 0); } GC is implemented so that it skips active DOM objects that have pending activities, so you can use the mechanism. See mediasource/MediaSource.{h,cpp} for more details. Then you don't need to add custom bindings.
> LayoutTests/webaudio/javascriptaudionode.html:87 > +// borrowed from other tests. > +function gc() > +{ > + if (window.GCController) > + return GCController.collect(); > + > + for (var i = 0; i < 10000; i++) { > + var s = new String("abc"); > + } > +}
You can include fast/js/resources/js-test-pre.js and use gc() defined there.
Russell McClellan
Comment 3
2013-03-17 21:29:34 PDT
Created
attachment 193487
[details]
Patch
Kentaro Hara
Comment 4
2013-03-17 21:43:35 PDT
Comment on
attachment 193487
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=193487&action=review
Looks good with nits.
> Source/WebCore/ChangeLog:10 > + Fix for issue where ScriptProcessorNodes (and AudioNode js wrappers generally) > + would be garbage collected before their time. Made AudioNode an ActiveDOMElement > + marked pending if there are any open audio connections. > +
https://bugs.webkit.org/show_bug.cgi?id=112521
> + > + Reviewed by NOBODY (OOPS!). > + > + Test: webaudio/javascriptaudionode.html
Nit: WebKit convention is a bug title (which should be concise), a bug URL, a reviewer, a description, and a test.
> LayoutTests/webaudio/javascriptaudionode.html:89 > + testFailed("onaudioprocess was called not called on unreachable but connected script node.");
Typo: was not called
> LayoutTests/webaudio/javascriptaudionode.html:90 > + if(audioprocessWasCalled) { > + testPassed("onaudioprocess was called even though node was unreachable."); > + } else { > + testFailed("onaudioprocess was called not called on unreachable but connected script node."); > + }
This can simply be shouldBeTrue('audioprocessWasCalled');
Russell McClellan
Comment 5
2013-03-17 21:57:58 PDT
Created
attachment 193488
[details]
Patch
Kentaro Hara
Comment 6
2013-03-17 22:06:10 PDT
Comment on
attachment 193488
[details]
Patch Thanks!
WebKit Review Bot
Comment 7
2013-03-17 22:54:02 PDT
Comment on
attachment 193488
[details]
Patch Clearing flags on attachment: 193488 Committed
r146033
: <
http://trac.webkit.org/changeset/146033
>
WebKit Review Bot
Comment 8
2013-03-17 22:54:07 PDT
All reviewed patches have been landed. Closing bug.
Chris Rogers
Comment 9
2013-03-17 23:01:26 PDT
Kentaro, I haven't had a chance to understand completely what implications this has for all AudioNodes. Will the garbage collections of normal (not ScriptProcessorNode objects) be delayed?
Kentaro Hara
Comment 10
2013-03-17 23:05:15 PDT
(In reply to
comment #9
)
> Kentaro, I haven't had a chance to understand completely what implications this has for all AudioNodes. Will the garbage collections of normal (not ScriptProcessorNode objects) be delayed?
A GC treats active DOM objects as alive if hasPendingActivity() of the active DOM objects returns true. Given that hasPendingActivity() returns false in normal cases (it returns true only when '!m_isDisabled && (m_connectionRefCount > 0)' is true), the change won't affect normal cases.
Chris Rogers
Comment 11
2013-03-17 23:19:26 PDT
Has there been any verification that garbage collections is still happening correctly for dynamic nodes such as AudioBufferSourceNode? For example in the granular demo, is the gc still happening correctly:
http://chromium.googlecode.com/svn/trunk/samples/audio/granular.html
I'd be more comfortable if we could actually manually test that the gc is indeed happening in these cases. I'm not saying the patch is incorrect, but we should be extremely careful to test these types of things.
Kentaro Hara
Comment 12
2013-03-17 23:26:50 PDT
(In reply to
comment #11
)
> Has there been any verification that garbage collections is still happening correctly for dynamic nodes such as AudioBufferSourceNode? For example in the granular demo, is the gc still happening correctly: >
http://chromium.googlecode.com/svn/trunk/samples/audio/granular.html
> > I'd be more comfortable if we could actually manually test that the gc is indeed happening in these cases.
If you're suspicious of the change, let's revert it. (I'm sorry I didn't give you a chance to comment on the patch.) Would it be possible to add granular.html (or somewhat minimized version of granular.html) to layout tests? Manual tests are too fragile since people won't test them when they land patches.
Russell McClellan
Comment 13
2013-03-17 23:54:40 PDT
(In reply to
comment #11
)
> Has there been any verification that garbage collections is still happening correctly for dynamic nodes such as AudioBufferSourceNode? For example in the granular demo, is the gc still happening correctly: >
http://chromium.googlecode.com/svn/trunk/samples/audio/granular.html
> > I'd be more comfortable if we could actually manually test that the gc is indeed happening in these cases.
Yes, I added some printfs to audiobuffersourcenode and tested that the C++ side objects were getting freed before I submitted it. I just double-checked with granular.html and it seemed like garbage collection was happening correctly (that is, things were getting deleted at the same rate they were created)
> I'm not saying the patch is incorrect, but we should be extremely careful to test these types of things.
Yeah, I couldn't think of a great automated test for detecting that something was deleted correctly - I did try my best to test everything manually. My original patch only affected ScriptProcessorNodes, but I changed it on Kentaro's recommendation (I agree with him that "pending activity" is better because the logic isn't duplicated between chrome and safari). One point in favor of the current patch is that if audionodes ever become eventlisteners they will need a scheme like this or else they will have the same problem as ScriptProcessorNodes. (i.e., their event listeners will get deleted even though they are supposed to fire in the future).
Kentaro Hara
Comment 14
2013-03-18 01:35:52 PDT
Reverted
r146033
for reason: web audio tests are broken Committed
r146040
: <
http://trac.webkit.org/changeset/146040
>
Kentaro Hara
Comment 15
2013-03-18 01:37:41 PDT
These webaudio tests are broken:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=webaudio%2Faudiobuffersource-playbackState.html%2Cwebaudio%2Fbiquad-highshelf.html%2Cwebaudio%2Foscillator-sine.html%2Cwebaudio%2Faudioparam-connect-audioratesignal.html%2Cwebaudio%2Fpanner-equalpower.html%2Cwebaudio%2Faudiobuffersource-start.html%2Cwebaudio%2Fup-mixing-mono-51.html%2Cwebaudio%2Faudiobuffersource-channels.html%2Cwebaudio%2Fdynamicscompressor-basic.html%2Cwebaudio%2Frealtimeanalyser-fft-sizing.html%2Cwebaudio%2Fmediastreamaudiodestinationnode.html%2Cwebaudio%2Fbiquad-lowshelf.html%2Cwebaudio%2Fmediaelementaudiosourcenode-gc.html%2Cwebaudio%2Faudionode-connect-order.html%2Cwebaudio%2Fnote-grain-on-timing.html%2Cwebaudio%2Faudiobuffersource-loop-points.html%2Cwebaudio%2Fup-mixing-stereo-51.html%2Cwebaudio%2Faudioparam-linearRampToValueAtTime.html%2Cwebaudio%2Foscillator-custom.html%2Cwebaudio%2Fbiquad-peaking.html%2Cwebaudio%2Fjavascriptaudionode-zero-input-channels.html%2Cwebaudio%2Fdistance-inverse.html%2Cwebaudio%2Fbiquad-getFrequencyResponse.html%2Cwebaudio%2Fconvolution-mono-mono.html%2Cwebaudio%2Fstereo2mono-down-mixing.html%2Cwebaudio%2Faudioparam-summingjunction.html%2Cwebaudio%2Fdelaynode.html%2Cwebaudio%2Fdelaynode-max-nondefault-delay.html%2Cwebaudio%2Faudiochannelmerger-basic.html%2Cwebaudio%2Fjavascriptaudionode-downmix8-2channel-input.html%2Cwebaudio%2Fdecode-audio-data-basic.html%2Cwebaudio%2Foscillator-triangle.html%2Cwebaudio%2Faudiochannelsplitter.html%2Cwebaudio%2Fdelaynode-maxdelaylimit.html%2Cwebaudio%2Fmixing.html%2Cwebaudio%2Fbiquad-allpass.html%2Cwebaudio%2Faudioparam-setValueAtTime.html
Russell McClellan
Comment 16
2013-03-18 07:08:54 PDT
Sorry! I was only testing the release builds on my machine which don't have this assert. It looks like there's a call "suspendIfNeeded" that must be called on every activeDOMObject - the solution in other places is to just call that on each ActiveDOMObject as it is created (i.e. see AudioContext where also suspend doesn't do anything). This is unfortunate as AudioNodes are just using ActiveDOMObjects as a convenience for garbage collection and suspending them doesn't actually mean anything. I guess the options are a) only derive from "ActiveDOMElement" in ScriptProcessorNode as that's the only place it's actually needed currently. Add the suspendIfNeeded call in the create function. If AudioNode ever becomes an EventTarget, we'll have to remember to add it there, too, lest garbage collection goes wrong in the same way. b) add a "suspendIfNeeded" call to all the AudioNode create functions. This feels to me like a lot of extra boilerplate but it seems to be what's done elsewhere in the codebase. c) add a "suspendIfNeeded" call to AudioNode's constructor. This is bad because derived classes won't be able to override suspend, as the "this" pointer is of type AudioNode inside the AudioNode constructor. probably I lean towards (a) for now as that makes the minimal change that will fix the bug.
Russell McClellan
Comment 17
2013-03-18 17:07:59 PDT
Created
attachment 193697
[details]
Patch
Chris Rogers
Comment 18
2013-03-18 17:12:33 PDT
(In reply to
comment #16
)
> Sorry! I was only testing the release builds on my machine which don't have this assert. It looks like there's a call "suspendIfNeeded" that must be called on every activeDOMObject - the solution in other places is to just call that on each ActiveDOMObject as it is created (i.e. see AudioContext where also suspend doesn't do anything). This is unfortunate as AudioNodes are just using ActiveDOMObjects as a convenience for garbage collection and suspending them doesn't actually mean anything. > > I guess the options are > a) only derive from "ActiveDOMElement" in ScriptProcessorNode as that's the only place it's actually needed currently. Add the suspendIfNeeded call in the create function. If AudioNode ever becomes an EventTarget, we'll have to remember to add it there, too, lest garbage collection goes wrong in the same way.
Actually, AudioNode is supposed to be an EventTarget according to recent discussions:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=20764
So maybe it's a good idea to implement that first?
> b) add a "suspendIfNeeded" call to all the AudioNode create functions. This feels to me like a lot of extra boilerplate but it seems to be what's done elsewhere in the codebase. > c) add a "suspendIfNeeded" call to AudioNode's constructor. This is bad because derived classes won't be able to override suspend, as the "this" pointer is of type AudioNode inside the AudioNode constructor. > > probably I lean towards (a) for now as that makes the minimal change that will fix the bug.
Kentaro Hara
Comment 19
2013-03-18 17:13:56 PDT
(In reply to
comment #18
)
> So maybe it's a good idea to implement that first?
Sounds reasonable to me.
Alexey Proskuryakov
Comment 20
2013-09-17 15:03:14 PDT
This breaks webaudio/javascriptaudionode.html regression test. Updated TestExpectations in <
http://trac.webkit.org/r156005
>.
Alexey Proskuryakov
Comment 21
2013-09-17 15:03:35 PDT
<
rdar://problem/15014121
>
Alexey Proskuryakov
Comment 22
2014-04-15 09:23:41 PDT
***
Bug 120148
has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 23
2014-04-15 09:29:54 PDT
Updated TestExpectations in
http://trac.webkit.org/r167309
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug