Bug 197533 – WebAudio Node JS wrappers should not be collected if events can be fired

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[youenn fablet]

WebAudio Node JS wrappers should not be collected if events can be fired

[youenn fablet]

Created attachment 368835 [details]
Patch

[EWS Watchlist]

Attachment 368835 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSScriptProcessorNodeCustom.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/js/JSOscillatorNodeCustom.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/js/JSAudioBufferSourceNodeCustom.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

<rdar://problem/47897230>

[youenn fablet]

Created attachment 368890 [details]
Patch

[EWS Watchlist]

Attachment 368890 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSScriptProcessorNodeCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/js/JSOscillatorNodeCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/js/JSAudioBufferSourceNodeCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 368890 [details]
Patch

Attachment 368890 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12078269

New failing tests:
imported/blink/compositing/layer-creation/iframe-clip-removed.html
svg/repaint/remove-background-property-on-root.html
legacy-animation-engine/fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html

[EWS Watchlist]

Created attachment 368907 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[youenn fablet]

Created attachment 369124 [details]
Allow GC for closed AudioContexts

[EWS Watchlist]

Attachment 369124 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSScriptProcessorNodeCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/js/JSOscillatorNodeCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/js/JSAudioBufferSourceNodeCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

When you say this is covered by existing tests, do those tests fail 100% of the time due to this issue, or just occasionally (as GC bugs tend do)? If it's just occasionally, can you construct tests that fail 100% of the time using any of our gc internals?

[Jer Noble]

Comment on attachment 369124 [details]
Allow GC for closed AudioContexts

r=me, with nit:

AudioNode.idl already has a "GenerateIsReachable=Impl", which /should/ keep the wrapper alive as long as the node itself is alive, so at the very least, your patch should incorporate those existing isReachableFromOpaqueRoots(). But since AudioNode is also an EventTarget, it is probably a better idea just to do a custom isReachable against AudioNode itself, rather than different implementations for three different subclasses of AudioNode. One implementation seems like a better idea from a code maintenance standpoint, and the benefit of having different "reason" information for each subclass doesn't seem worth it in comparison.

[youenn fablet]

(In reply to Sam Weinig from comment #10)
> When you say this is covered by existing tests, do those tests fail 100% of
> the time due to this issue, or just occasionally (as GC bugs tend do)? If
> it's just occasionally, can you construct tests that fail 100% of the time
> using any of our gc internals?

These are flaky tests. I will try to write a dedicated test.

[Sam Weinig]

(In reply to youenn fablet from comment #12)
> (In reply to Sam Weinig from comment #10)
> > When you say this is covered by existing tests, do those tests fail 100% of
> > the time due to this issue, or just occasionally (as GC bugs tend do)? If
> > it's just occasionally, can you construct tests that fail 100% of the time
> > using any of our gc internals?
> 
> These are flaky tests. I will try to write a dedicated test.

Thanks. Given how hard these gc/wrapper bugs are, I think it is particularly important that we have good test coverage for them.

[youenn fablet]

Created attachment 369139 [details]
Patch for landing

[EWS Watchlist]

Attachment 369139 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSAudioNodeCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to Jer Noble from comment #11)
> Comment on attachment 369124 [details]
> Allow GC for closed AudioContexts
> 
> r=me, with nit:
> 
> AudioNode.idl already has a "GenerateIsReachable=Impl", which /should/ keep
> the wrapper alive as long as the node itself is alive, so at the very least,
> your patch should incorporate those existing isReachableFromOpaqueRoots().
> But since AudioNode is also an EventTarget, it is probably a better idea
> just to do a custom isReachable against AudioNode itself, rather than
> different implementations for three different subclasses of AudioNode. One
> implementation seems like a better idea from a code maintenance standpoint,
> and the benefit of having different "reason" information for each subclass
> doesn't seem worth it in comparison.

Good idea, patch looks nicer now.

[EWS Watchlist]

Comment on attachment 369139 [details]
Patch for landing

Attachment 369139 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12113361

New failing tests:
webaudio/webaudio-gc.html

[EWS Watchlist]

Created attachment 369152 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[youenn fablet]

Created attachment 369160 [details]
test without gum

[EWS Watchlist]

Attachment 369160 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSAudioNodeCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 369160 [details]
test without gum

Clearing flags on attachment: 369160

Committed r244977: <https://trac.webkit.org/changeset/244977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 369160 [details]
test without gum

View in context: https://bugs.webkit.org/attachment.cgi?id=369160&action=review

> Source/WebCore/bindings/js/JSAudioNodeCustom.cpp:53
> +    if (node.hasEventListeners()) {
> +        if (UNLIKELY(reason))
> +            *reason = "AudioNode has event listeners";
> +        return true;
> +    }

This is a memory leak. If I create audio nodes with event listeners in a loop, they will not be collected until I leave the page.

[youenn fablet]

(In reply to Geoffrey Garen from comment #23)
> Comment on attachment 369160 [details]
> test without gum
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369160&action=review
> 
> > Source/WebCore/bindings/js/JSAudioNodeCustom.cpp:53
> > +    if (node.hasEventListeners()) {
> > +        if (UNLIKELY(reason))
> > +            *reason = "AudioNode has event listeners";
> > +        return true;
> > +    }
> 
> This is a memory leak. If I create audio nodes with event listeners in a
> loop, they will not be collected until I leave the page.

It cannot be GCed until the end of the document or JS closes the AudioContext.
Also, while JSScriptProcessorNode can be GCed, audio nodes have their own ref counting and I am not sure GCing the JSScriptProcessorNode will allow freeing the corresponding ScriptProcessorNode until the AudioContext is stopped anyway.

Note also that currently, AudioContext/OfflineAudioContext can never be GCed, even if closed. We should probably be able to gc AudioContext, at least when closed.

We could try to improve the web audio code to better rely on JS GC and stop relying on web audio dedicated ref/deref mechansim.

For instance, audio node graphs that do not contain active output nodes (sound output, script processor node) connected directly or indirectly to active input nodes (OscillatorNode, MediaStreamAudioSourceNode) could be GCed.