Summary: | AudioScheduledSourceNodes leak if they have an attached onended EventTarget | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, eric.carlson, esprehn+autocc, ews-watchlist, glenn, jer.noble, kondapallykalyan, philipj, rniwa, sergio, shortsands.ceo, simon.fraser, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Keith Miller
2019-11-11 15:13:24 PST
Created attachment 383306 [details]
Patch
Created attachment 383308 [details]
Patch
Created attachment 383309 [details]
Patch
Comment on attachment 383309 [details] Patch Attachment 383309 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13242206 New failing tests: imported/blink/fast/events/panScroll-crash.html Created attachment 383347 [details]
Archive of layout-test-results from ews214 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 383309 [details]
Patch
LGTM. Nice to disambiguate node->start(when) from ActiveDOMObject::start(). Windows EWS failure seems unrelated.
Comment on attachment 383309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383309&action=review The patch looks sensible from GC lifetime perspective but Jer should take a look at it too. > Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:192 > + unsetPendingActivity(*this); This should happen after the event is dispatched. Otherwise we have a race of this code & event dispatching code. Also use makePendingActivity instead of set/unsetPendingActivity, which also refs the object. > Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:76 > + setPendingActivity(*this); Creating a pending activity in the constructor doesn’t seem right. Also, set/unsetPendingActivity ref’s this object. I think a better approach is to use makePendingActivity and store the token as a member > LayoutTests/webaudio/finished-audio-buffer-source-nodes-should-be-collectable-expected.txt:8 > +PASS AudioBufferSourceNode was collected after calling onended. We should also add a test to make sure the JS wrapper of source node doesn’t get prematurely collected. Comment on attachment 383309 [details]
Patch
Restoring r-.
Comment on attachment 383309 [details]
Patch
I mean r+
Comment on attachment 383309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383309&action=review >> Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:192 >> + unsetPendingActivity(*this); > > This should happen after the event is dispatched. > Otherwise we have a race of this code & event dispatching code. > Also use makePendingActivity instead of set/unsetPendingActivity, which also refs the object. Sure, that makes sense. I'll put it inside a WTF::scope so we always release ourself. >> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:76 >> + setPendingActivity(*this); > > Creating a pending activity in the constructor doesn’t seem right. > Also, set/unsetPendingActivity ref’s this object. > I think a better approach is to use makePendingActivity and store the token as a member I can use makePendingActivity(). Although, I don't think that automates anything since I think everywhere I setPendingActivity I need to set the member and everywhere I currently unsetPendingActivity I'll need to clear the RefPtr. Maybe it's clearer though? I can also move the set to a the create method in the subclasses but that seems like an anti-pattern? All the other logic for setting/unsetting is in this class. Too bad WebCore doesn't have a finishCreation() that goes up the class hierarchy... >> LayoutTests/webaudio/finished-audio-buffer-source-nodes-should-be-collectable-expected.txt:8 >> +PASS AudioBufferSourceNode was collected after calling onended. > > We should also add a test to make sure the JS wrapper of source node doesn’t get prematurely collected. Sure, although, I'm not exactly sure what cases are the interesting ones... Created attachment 383384 [details]
Patch for landing
Created attachment 383385 [details]
Patch for landing
Comment on attachment 383385 [details] Patch for landing Rejecting attachment 383385 [details] from commit-queue. New failing tests: webaudio/finished-audio-buffer-source-nodes-should-be-collectable.html Full output: https://webkit-queues.webkit.org/results/13243934 Created attachment 383395 [details]
Archive of layout-test-results from webkit-cq-03 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03 Port: mac-highsierra Platform: Mac OS X 10.13.6
(In reply to WebKit Commit Bot from comment #13) > Comment on attachment 383385 [details] > Patch for landing > > Rejecting attachment 383385 [details] from commit-queue. > > New failing tests: > webaudio/finished-audio-buffer-source-nodes-should-be-collectable.html > Full output: https://webkit-queues.webkit.org/results/13243934 Whoops, I changed the test reason but didn't rebaseline... Committed r252389: <https://trac.webkit.org/changeset/252389> *** Bug 203624 has been marked as a duplicate of this bug. *** |