[webkit-reviews] review requested: [Bug 204087] AudioScheduledSourceNodes leak if they have an attached onended EventTarget : [Attachment 383309] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has asked  for review:
Bug 204087: AudioScheduledSourceNodes leak if they have an attached onended
EventTarget
https://bugs.webkit.org/show_bug.cgi?id=204087

Attachment 383309: Patch

https://bugs.webkit.org/attachment.cgi?id=383309&action=review




--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 383309
  --> https://bugs.webkit.org/attachment.cgi?id=383309
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-e
xpected.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.