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.
Created attachment 193473 [details] Patch
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.
Created attachment 193487 [details] Patch
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');
Created attachment 193488 [details] Patch
Comment on attachment 193488 [details] Patch Thanks!
Comment on attachment 193488 [details] Patch Clearing flags on attachment: 193488 Committed r146033: <http://trac.webkit.org/changeset/146033>
All reviewed patches have been landed. Closing bug.
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?
(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.
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.
(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.
(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).
Reverted r146033 for reason: web audio tests are broken Committed r146040: <http://trac.webkit.org/changeset/146040>
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
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.
Created attachment 193697 [details] Patch
(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.
(In reply to comment #18) > So maybe it's a good idea to implement that first? Sounds reasonable to me.
This breaks webaudio/javascriptaudionode.html regression test. Updated TestExpectations in <http://trac.webkit.org/r156005>.
<rdar://problem/15014121>
*** Bug 120148 has been marked as a duplicate of this bug. ***
Updated TestExpectations in http://trac.webkit.org/r167309