ScriptProcessorNode is garbage collected while still active if unreachable (breaks multiple webaudio test)

• Component: Web Audio
• Status: NEW ---

[Russell McClellan]

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.

[Russell McClellan]

Created attachment 193473 [details]
Patch

[Kentaro Hara]

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]

Created attachment 193487 [details]
Patch

[Kentaro Hara]

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]

Created attachment 193488 [details]
Patch

[Kentaro Hara]

Comment on attachment 193488 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 193488 [details]
Patch

Clearing flags on attachment: 193488

Committed r146033: <http://trac.webkit.org/changeset/146033>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Rogers]

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]

(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]

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]

(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]

(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]

Reverted r146033 for reason:

web audio tests are broken

Committed r146040: <http://trac.webkit.org/changeset/146040>

[Kentaro Hara]

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]

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]

Created attachment 193697 [details]
Patch

[Chris Rogers]

(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]

(In reply to comment #18)
> So maybe it's a good idea to implement that first?

Sounds reasonable to me.

[Alexey Proskuryakov]

This breaks webaudio/javascriptaudionode.html regression test.

Updated TestExpectations in <http://trac.webkit.org/r156005>.

[Alexey Proskuryakov]

<rdar://problem/15014121>

[Alexey Proskuryakov]

*** Bug 120148 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Updated TestExpectations in http://trac.webkit.org/r167309