Bug 204087 – AudioScheduledSourceNodes leak if they have an attached onended EventTarget

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

AudioScheduledSourceNodes leak if they have an attached onended EventTarget

[Keith Miller]

Created attachment 383306 [details]
Patch

[Keith Miller]

Created attachment 383308 [details]
Patch

[Keith Miller]

Created attachment 383309 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Jer Noble]

Comment on attachment 383309 [details]
Patch

LGTM. Nice to disambiguate node->start(when) from ActiveDOMObject::start(). Windows EWS failure seems unrelated.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Comment on attachment 383309 [details]
Patch

Restoring r-.

[Ryosuke Niwa]

Comment on attachment 383309 [details]
Patch

I mean r+

[Keith Miller]

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...

[Keith Miller]

Created attachment 383384 [details]
Patch for landing

[Keith Miller]

Created attachment 383385 [details]
Patch for landing

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Keith Miller]

(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...

[Keith Miller]

Committed r252389: <https://trac.webkit.org/changeset/252389>

[Radar WebKit Bug Importer]

<rdar://problem/57139042>

[Keith Miller]

*** Bug 203624 has been marked as a duplicate of this bug. ***