WebAudio Node JS wrappers should not be collected if events can be fired
Created attachment 368835 [details] Patch
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.
<rdar://problem/47897230>
Created attachment 368890 [details] Patch
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 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
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
Created attachment 369124 [details] Allow GC for closed AudioContexts
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.
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 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.
(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.
(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.
Created attachment 369139 [details] Patch for landing
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.
(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 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
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
Created attachment 369160 [details] test without gum
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 on attachment 369160 [details] test without gum Clearing flags on attachment: 369160 Committed r244977: <https://trac.webkit.org/changeset/244977>
All reviewed patches have been landed. Closing bug.
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.
(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.