Bug 112521 - ScriptProcessorNode is garbage collected while still active if unreachable (breaks multiple webaudio test)
Summary: ScriptProcessorNode is garbage collected while still active if unreachable (b...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 120148 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-03-17 14:10 PDT by Russell McClellan
Modified: 2015-06-12 10:31 PDT (History)
18 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Russell McClellan 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.
Comment 1 Russell McClellan 2013-03-17 14:53:05 PDT
Created attachment 193473 [details]
Patch
Comment 2 Kentaro Hara 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.
Comment 3 Russell McClellan 2013-03-17 21:29:34 PDT
Created attachment 193487 [details]
Patch
Comment 4 Kentaro Hara 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');
Comment 5 Russell McClellan 2013-03-17 21:57:58 PDT
Created attachment 193488 [details]
Patch
Comment 6 Kentaro Hara 2013-03-17 22:06:10 PDT
Comment on attachment 193488 [details]
Patch

Thanks!
Comment 7 WebKit Review Bot 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>
Comment 8 WebKit Review Bot 2013-03-17 22:54:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Chris Rogers 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?
Comment 10 Kentaro Hara 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.
Comment 11 Chris Rogers 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.
Comment 12 Kentaro Hara 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.
Comment 13 Russell McClellan 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).
Comment 14 Kentaro Hara 2013-03-18 01:35:52 PDT
Reverted r146033 for reason:

web audio tests are broken

Committed r146040: <http://trac.webkit.org/changeset/146040>
Comment 15 Kentaro Hara 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
Comment 16 Russell McClellan 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.
Comment 17 Russell McClellan 2013-03-18 17:07:59 PDT
Created attachment 193697 [details]
Patch
Comment 18 Chris Rogers 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.
Comment 19 Kentaro Hara 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.
Comment 20 Alexey Proskuryakov 2013-09-17 15:03:14 PDT
This breaks webaudio/javascriptaudionode.html regression test.

Updated TestExpectations in <http://trac.webkit.org/r156005>.
Comment 21 Alexey Proskuryakov 2013-09-17 15:03:35 PDT
<rdar://problem/15014121>
Comment 22 Alexey Proskuryakov 2014-04-15 09:23:41 PDT
*** Bug 120148 has been marked as a duplicate of this bug. ***
Comment 23 Alexey Proskuryakov 2014-04-15 09:29:54 PDT
Updated TestExpectations in http://trac.webkit.org/r167309