Bug 204087 - AudioScheduledSourceNodes leak if they have an attached onended EventTarget
Summary: AudioScheduledSourceNodes leak if they have an attached onended EventTarget
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
: 203624 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-11-11 15:13 PST by Keith Miller
Modified: 2019-11-14 10:46 PST (History)
14 users (show)

See Also:


Attachments
Patch (30.29 KB, patch)
2019-11-11 15:42 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (30.33 KB, patch)
2019-11-11 15:58 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (30.54 KB, patch)
2019-11-11 16:10 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (14.21 MB, application/zip)
2019-11-12 04:18 PST, EWS Watchlist
no flags Details
Patch for landing (33.42 KB, patch)
2019-11-12 14:36 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (33.41 KB, patch)
2019-11-12 14:37 PST, Keith Miller
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-03 for mac-highsierra (3.46 MB, application/zip)
2019-11-12 15:50 PST, WebKit Commit Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2019-11-11 15:13:24 PST
AudioScheduledSourceNodes leak if they have an attached onended EventTarget
Comment 1 Keith Miller 2019-11-11 15:42:12 PST
Created attachment 383306 [details]
Patch
Comment 2 Keith Miller 2019-11-11 15:58:52 PST
Created attachment 383308 [details]
Patch
Comment 3 Keith Miller 2019-11-11 16:10:05 PST
Created attachment 383309 [details]
Patch
Comment 4 EWS Watchlist 2019-11-12 04:18:05 PST
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
Comment 5 EWS Watchlist 2019-11-12 04:18:07 PST
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 6 Jer Noble 2019-11-12 10:37:22 PST
Comment on attachment 383309 [details]
Patch

LGTM. Nice to disambiguate node->start(when) from ActiveDOMObject::start(). Windows EWS failure seems unrelated.
Comment 7 Ryosuke Niwa 2019-11-12 10:39:47 PST
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 8 Ryosuke Niwa 2019-11-12 10:40:08 PST
Comment on attachment 383309 [details]
Patch

Restoring r-.
Comment 9 Ryosuke Niwa 2019-11-12 10:40:39 PST
Comment on attachment 383309 [details]
Patch

I mean r+
Comment 10 Keith Miller 2019-11-12 12:29:04 PST
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...
Comment 11 Keith Miller 2019-11-12 14:36:51 PST
Created attachment 383384 [details]
Patch for landing
Comment 12 Keith Miller 2019-11-12 14:37:47 PST
Created attachment 383385 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2019-11-12 15:50:25 PST
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
Comment 14 WebKit Commit Bot 2019-11-12 15:50:27 PST
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
Comment 15 Keith Miller 2019-11-12 18:00:29 PST
(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...
Comment 16 Keith Miller 2019-11-12 18:07:36 PST
Committed r252389: <https://trac.webkit.org/changeset/252389>
Comment 17 Radar WebKit Bug Importer 2019-11-12 18:08:18 PST
<rdar://problem/57139042>
Comment 18 Keith Miller 2019-11-14 10:46:10 PST
*** Bug 203624 has been marked as a duplicate of this bug. ***