Keep ScriptProcessorNode from premature GC with ScriptWrappable::hasPendingActivity()

Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

With this patch, scriptprocessornode-premature-death.html is now correctly passing. Thus the expected text was fixed accordingly.

BUG=577431, 500335, 360378, 121654, 82795
TEST=LayoutTests/webaudio/scriptprocessornode-premature-death.html (revised expected file)

Committed: https://crrev.com/246a91f12971f2f6e4ba0b68fdbf5efa8afb344e
Cr-Commit-Position: refs/heads/master@{#379586}

[hongchan, 2016-03-03 22:04:02]

Description was changed from

==========
Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

The test for this change must be performed manually, because ScriptProcessorNode
is not compatible with OfflineAudioContext and the only consistently
reproducible test case relies on getUserMedia() source stream.

BUG=577431,500335,360378,121654,82795
TEST=ManualTests/webaudio/scriptprocessornode-premature-gc.html
==========

to

==========
Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

The test for this change must be performed manually, because ScriptProcessorNode
is not compatible with OfflineAudioContext and the only consistently
reproducible test case relies on getUserMedia() source stream.

BUG=577431,500335,360378,121654,82795
TEST=ManualTests/webaudio/scriptprocessornode-premature-gc.html
==========

[hongchan, 2016-03-03 22:04:03]

hongchan@chromium.org changed reviewers:
+ rtoy@chromium.org

[hongchan, 2016-03-03 22:04:21]

hongchan@chromium.org changed reviewers:
+ haraken@chromium.org

[hongchan, 2016-03-03 22:05:57]

PTAL.

The CL is very similar to https://codereview.chromium.org/1593763002/, but the
testing pattern is changed due to the limitation of ScriptProcessorNode.

[Raymond Toy, 2016-03-03 22:50:52]

Please change the subject to say what you're solving, not how you're solving it.

Can you add a test (manual or otherwise) to demonstrate that ScriptProcessorNode
will get collected if onaudioprocess is NOT assigned?

[haraken, 2016-03-03 23:34:16]

https://codereview.chromium.org/1762103002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/1762103002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:110: bool
hasPendingActivity() const final;

I'm not sure how this works. Don't you need to add 'override'?

[hongchan, 2016-03-04 17:31:02]

On 2016/03/03 22:50:52, Raymond Toy wrote:
> Please change the subject to say what you're solving, not how you're solving
it.

Done.

> Can you add a test (manual or otherwise) to demonstrate that
ScriptProcessorNode
> will get collected if onaudioprocess is NOT assigned?

Actually this question got me thinking and I found this test:
scriptprocessornode-premature-death.html
With internal.observeGC(), I think we might be able to use this method to
automate the test. One thing I am not certain is the functionality of that test
helper method. I will report back to this CL.

[hongchan, 2016-03-04 18:08:58]

On 2016/03/03 23:34:16, haraken wrote:
>
https://codereview.chromium.org/1762103002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h (right):
> 
>
https://codereview.chromium.org/1762103002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:110: bool
> hasPendingActivity() const final;
> 
> I'm not sure how this works. Don't you need to add 'override'?

Similarly, I was following the convention from other parts of WebAudio. I
believe this node is the end of inheritance tree and nothing will inherit from
this, so I used final. I do not mind changing it to override. WDYT? FWIW, both
final and override compile okay and run all the test correctly.

[hongchan, 2016-03-04 18:10:55]

Description was changed from

==========
Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

The test for this change must be performed manually, because ScriptProcessorNode
is not compatible with OfflineAudioContext and the only consistently
reproducible test case relies on getUserMedia() source stream.

BUG=577431,500335,360378,121654,82795
TEST=ManualTests/webaudio/scriptprocessornode-premature-gc.html
==========

to

==========
Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

With this patch, scriptprocessornode-premature-death.html is now correctly
passing. Thus the expected text was fixed accordingly.

BUG=577431,500335,360378,121654,82795
TEST=LayoutTests/webaudio/scriptprocessornode-premature-death.html (revised
expected file)
==========

[hongchan, 2016-03-04 18:15:12]

The CQ bit was checked by hongchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 18:16:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762103002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762103002/20001

[hongchan, 2016-03-04 18:16:11]

After finding the existing test for premature GC, the manual test is dropped and
fixed the 'fail-passing' test expectation.

[commit-bot: I haz the power, 2016-03-04 19:37:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-04 19:37:01]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-03-05 03:24:58]

On 2016/03/04 18:16:11, hoch wrote:
> After finding the existing test for premature GC, the manual test is dropped
and
> fixed the 'fail-passing' test expectation.

ah, okay. LGTM.

[Raymond Toy, 2016-03-07 16:47:15]

lgtm

[hongchan, 2016-03-07 16:47:31]

The CQ bit was checked by hongchan@chromium.org

[commit-bot: I haz the power, 2016-03-07 16:47:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762103002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762103002/20001

[commit-bot: I haz the power, 2016-03-07 18:02:26]

Description was changed from

==========
Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

With this patch, scriptprocessornode-premature-death.html is now correctly
passing. Thus the expected text was fixed accordingly.

BUG=577431,500335,360378,121654,82795
TEST=LayoutTests/webaudio/scriptprocessornode-premature-death.html (revised
expected file)
==========

to

==========
Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

With this patch, scriptprocessornode-premature-death.html is now correctly
passing. Thus the expected text was fixed accordingly.

BUG=577431,500335,360378,121654,82795
TEST=LayoutTests/webaudio/scriptprocessornode-premature-death.html (revised
expected file)
==========

[commit-bot: I haz the power, 2016-03-07 18:02:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-07 18:03:38]

Description was changed from

==========
Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

With this patch, scriptprocessornode-premature-death.html is now correctly
passing. Thus the expected text was fixed accordingly.

BUG=577431,500335,360378,121654,82795
TEST=LayoutTests/webaudio/scriptprocessornode-premature-death.html (revised
expected file)
==========

to

==========
Add ScriptWrappable::hasPendingActivity to ScriptProcessorNode

|onaudioprocess| event in ScriptProcessor node is not fired once the JS
reference of the node goes out of local scope.

This CL is to keep the 'out-of-scope' reference from being collected when
it has an active |onaudioprocess| handler.

With this patch, scriptprocessornode-premature-death.html is now correctly
passing. Thus the expected text was fixed accordingly.

BUG=577431,500335,360378,121654,82795
TEST=LayoutTests/webaudio/scriptprocessornode-premature-death.html (revised
expected file)

Committed: https://crrev.com/246a91f12971f2f6e4ba0b68fdbf5efa8afb344e
Cr-Commit-Position: refs/heads/master@{#379586}
==========

[commit-bot: I haz the power, 2016-03-07 18:03:39]

Patchset 2 (id:??) landed as
https://crrev.com/246a91f12971f2f6e4ba0b68fdbf5efa8afb344e
Cr-Commit-Position: refs/heads/master@{#379586}