Bug 197533 - WebAudio Node JS wrappers should not be collected if events can be fired
Summary: WebAudio Node JS wrappers should not be collected if events can be fired
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-02 15:30 PDT by youenn fablet
Modified: 2019-05-07 15:03 PDT (History)
8 users (show)

See Also:


Attachments
Patch (17.31 KB, patch)
2019-05-02 16:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (17.43 KB, patch)
2019-05-02 21:19 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.76 MB, application/zip)
2019-05-02 23:28 PDT, Build Bot
no flags Details
Allow GC for closed AudioContexts (17.86 KB, patch)
2019-05-06 08:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (10.00 KB, patch)
2019-05-06 11:01 PDT, youenn fablet
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.16 MB, application/zip)
2019-05-06 11:52 PDT, Build Bot
no flags Details
test without gum (9.80 KB, patch)
2019-05-06 12:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-05-02 15:30:18 PDT
WebAudio Node JS wrappers should not be collected if events can be fired
Comment 1 youenn fablet 2019-05-02 16:01:12 PDT
Created attachment 368835 [details]
Patch
Comment 2 Build Bot 2019-05-02 16:02:43 PDT
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.
Comment 3 youenn fablet 2019-05-02 16:06:42 PDT
<rdar://problem/47897230>
Comment 4 youenn fablet 2019-05-02 21:19:28 PDT
Created attachment 368890 [details]
Patch
Comment 5 Build Bot 2019-05-02 21:21:31 PDT
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.
Comment 6 Build Bot 2019-05-02 23:28:53 PDT
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
Comment 7 Build Bot 2019-05-02 23:28:55 PDT
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
Comment 8 youenn fablet 2019-05-06 08:44:41 PDT
Created attachment 369124 [details]
Allow GC for closed AudioContexts
Comment 9 Build Bot 2019-05-06 08:47:45 PDT
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.
Comment 10 Sam Weinig 2019-05-06 10:16:30 PDT
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?
Comment 11 Jer Noble 2019-05-06 10:17:50 PDT
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.
Comment 12 youenn fablet 2019-05-06 10:18:09 PDT
(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.
Comment 13 Sam Weinig 2019-05-06 10:23:46 PDT
(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.
Comment 14 youenn fablet 2019-05-06 11:01:53 PDT
Created attachment 369139 [details]
Patch for landing
Comment 15 Build Bot 2019-05-06 11:04:21 PDT
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.
Comment 16 youenn fablet 2019-05-06 11:05:04 PDT
(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.
Comment 17 Build Bot 2019-05-06 11:52:49 PDT
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
Comment 18 Build Bot 2019-05-06 11:52:51 PDT
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
Comment 19 youenn fablet 2019-05-06 12:59:50 PDT
Created attachment 369160 [details]
test without gum
Comment 20 Build Bot 2019-05-06 13:01:51 PDT
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.
Comment 21 WebKit Commit Bot 2019-05-06 14:14:18 PDT
Comment on attachment 369160 [details]
test without gum

Clearing flags on attachment: 369160

Committed r244977: <https://trac.webkit.org/changeset/244977>
Comment 22 WebKit Commit Bot 2019-05-06 14:14:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Geoffrey Garen 2019-05-06 14:24:30 PDT
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.
Comment 24 youenn fablet 2019-05-07 15:03:40 PDT
(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.