Keep AudioScheduledSourceNode alive until onended is called.

Keep AudioScheduledSourceNode alive until onended is called.

Also, if the document has already gone away, we want to avoid firing
the event at all. This is similar to what ScriptProcessorNode does
with events.

BUG=269753
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157615

[haraken, 2013-09-05 23:42:44]

We should avoid manual ref()/deref(). As discussed offline, we should use
ActiveDOMObject::hasPendingActivity.

https://codereview.chromium.org/23596014/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):

https://codereview.chromium.org/23596014/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:184: ref();

You can just set m_playing to true, and...

https://codereview.chromium.org/23596014/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:204: deref();

... set m_playing to false here.

Then you can implement hasPendingActivity() { return m_playing; }

Doesn't it work?

[Raymond Toy (Google), 2013-09-05 23:48:46]

On 2013/09/05 23:42:44, haraken wrote:
> We should avoid manual ref()/deref(). As discussed offline, we should use
> ActiveDOMObject::hasPendingActivity.
> 
>
https://codereview.chromium.org/23596014/diff/1/Source/modules/webaudio/Audio...
> File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):
> 
>
https://codereview.chromium.org/23596014/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioScheduledSourceNode.cpp:184: ref();
> 
> You can just set m_playing to true, and...
> 
>
https://codereview.chromium.org/23596014/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioScheduledSourceNode.cpp:204: deref();
> 
> ... set m_playing to false here.
> 
> Then you can implement hasPendingActivity() { return m_playing; }
> 
> Doesn't it work?

Let me try it....

[Raymond Toy (Google), 2013-09-06 16:56:12]

PTAL.  This uses ActiveDOMObject as suggested, but it doesn't work.  Debug
builds fail with the assert:

ASSERTION FAILED: (*iter)->suspendIfNeededCalled()

Release builds crash.

I notice that hasPendingActivity never seems to be called.

[haraken, 2013-09-06 17:23:21]

On 2013/09/06 16:56:12, rtoy wrote:
> PTAL.  This uses ActiveDOMObject as suggested, but it doesn't work.  Debug
> builds fail with the assert:
> 
> ASSERTION FAILED: (*iter)->suspendIfNeededCalled()
> 
> Release builds crash.
> 
> I notice that hasPendingActivity never seems to be called.

The implementation is not following the programming pattern of ActiveDOMObject.
For example, you need to call ActiveDOMObject::setPendingActivity(),
unsetPendingActivity(). You might need to override suspend(), stop(), resume()
etc. Please see how HTMLMediaDocument is implemented: 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Raymond Toy (Google), 2013-09-06 20:12:34]

PTAL again.  As discussed offline, I've added the
setPendingActivity/unsetPendingActivity as needed and also added
suspendIfNeeded.

I ran the test in the bug for several minutes in both debug and release builds. 
No issues.

[haraken, 2013-09-06 20:21:35]

You need to add [ActiveDOMObjet] IDL attribute to AudioScheduledSourceNode.idl
and OscillatorNode.cpp.

I'm not familiar with the lifetime of webaudio objects and how
ActiveDOMObject::suspend(), stop(), resume() are overridden in these webaudio
objects. I'd like to have kbr take a look.

[Ken Russell (switch to Gerrit), 2013-09-06 21:33:46]

I looked through the ActiveDOMObject interface and don't think that any of the
methods it defines need to be overridden. There's an unfortunate overloading
between ActiveDOMObject::stop() and AudioScheduledSourceNode::stop(double) but
it shouldn't cause problems and AudioScheduledSourceNode shouldn't need to
override suspend/resume/stop.

LGTM with haraken's IDL concerns addressed. Also assuming this has been
*thoroughly* tested with existing web audio tests and content.

[haraken, 2013-09-06 21:39:59]

On 2013/09/06 21:33:46, Ken Russell wrote:
> I looked through the ActiveDOMObject interface and don't think that any of the
> methods it defines need to be overridden. There's an unfortunate overloading
> between ActiveDOMObject::stop() and AudioScheduledSourceNode::stop(double) but
> it shouldn't cause problems and AudioScheduledSourceNode shouldn't need to
> override suspend/resume/stop.
> 
> LGTM with haraken's IDL concerns addressed. Also assuming this has been
> *thoroughly* tested with existing web audio tests and content.

Maybe stop() needs to call unsetPendingActivity() ?

[Raymond Toy (Google), 2013-09-06 21:59:20]

On 2013/09/06 20:21:35, haraken wrote:
> You need to add [ActiveDOMObjet] IDL attribute to AudioScheduledSourceNode.idl
> and OscillatorNode.cpp.
> 
> I'm not familiar with the lifetime of webaudio objects and how
> ActiveDOMObject::suspend(), stop(), resume() are overridden in these webaudio
> objects. I'd like to have kbr take a look.

Attribute added.

[Raymond Toy (Google), 2013-09-06 22:02:23]

On 2013/09/06 21:33:46, Ken Russell wrote:
> I looked through the ActiveDOMObject interface and don't think that any of the
> methods it defines need to be overridden. There's an unfortunate overloading
> between ActiveDOMObject::stop() and AudioScheduledSourceNode::stop(double) but
> it shouldn't cause problems and AudioScheduledSourceNode shouldn't need to
> override suspend/resume/stop.
> 
> LGTM with haraken's IDL concerns addressed. Also assuming this has been
> *thoroughly* tested with existing web audio tests and content.

The layout tests pass, but I definitely need to test this on other content. 
I'll get back when I feel confident that nothing is broken.

[haraken, 2013-09-06 22:53:06]

On 2013/09/06 22:02:23, rtoy wrote:
> On 2013/09/06 21:33:46, Ken Russell wrote:
> > I looked through the ActiveDOMObject interface and don't think that any of
the
> > methods it defines need to be overridden. There's an unfortunate overloading
> > between ActiveDOMObject::stop() and AudioScheduledSourceNode::stop(double)
but
> > it shouldn't cause problems and AudioScheduledSourceNode shouldn't need to
> > override suspend/resume/stop.
> > 
> > LGTM with haraken's IDL concerns addressed. Also assuming this has been
> > *thoroughly* tested with existing web audio tests and content.
> 
> The layout tests pass, but I definitely need to test this on other content. 
> I'll get back when I feel confident that nothing is broken.

Thanks, the only remaining concern to me is whether we should call
unsetPendingActivity() in stop() or not.

[Raymond Toy (Google), 2013-09-06 23:11:15]

On 2013/09/06 22:53:06, haraken wrote:
> On 2013/09/06 22:02:23, rtoy wrote:
> > On 2013/09/06 21:33:46, Ken Russell wrote:
> > > I looked through the ActiveDOMObject interface and don't think that any of
> the
> > > methods it defines need to be overridden. There's an unfortunate
overloading
> > > between ActiveDOMObject::stop() and AudioScheduledSourceNode::stop(double)
> but
> > > it shouldn't cause problems and AudioScheduledSourceNode shouldn't need to
> > > override suspend/resume/stop.
> > > 
> > > LGTM with haraken's IDL concerns addressed. Also assuming this has been
> > > *thoroughly* tested with existing web audio tests and content.
> > 
> > The layout tests pass, but I definitely need to test this on other content. 
> > I'll get back when I feel confident that nothing is broken.
> 
> Thanks, the only remaining concern to me is whether we should call
> unsetPendingActivity() in stop() or not.

I don't think so.  This stop has nothing to do with ActiveDOMObject::stop; it
doesn't actually cause it to stop.  That happens sometime later in finish(),
which will call the setPendingActivity if there's an onended listener.

[haraken, 2013-09-07 00:22:14]

On 2013/09/06 23:11:15, rtoy wrote:
> I don't think so.  This stop has nothing to do with ActiveDOMObject::stop; it
> doesn't actually cause it to stop.  That happens sometime later in finish(),
> which will call the setPendingActivity if there's an onended listener.

I mean, we should probably inherit ActiveDOMObject::stop() and call
unsetPendingActivity() in the stop(). Otherwise, the ref() set in the
setPendingActivity() is not cleared when a ScriptExecutionContext stops. (You'll
need to rename AudioScheduledSourceNode::stop() then.)

Note: I'm not familiar with the lifetime of webaudio objects and don't know if
we really need to override ActiveDOMObject::stop(). I just want you to clarify
the point.

[Raymond Toy (Google), 2013-09-09 17:41:57]

On 2013/09/07 00:22:14, haraken wrote:
> On 2013/09/06 23:11:15, rtoy wrote:
> > I don't think so.  This stop has nothing to do with ActiveDOMObject::stop;
it
> > doesn't actually cause it to stop.  That happens sometime later in finish(),
> > which will call the setPendingActivity if there's an onended listener.
> 
> I mean, we should probably inherit ActiveDOMObject::stop() and call
> unsetPendingActivity() in the stop(). Otherwise, the ref() set in the
> setPendingActivity() is not cleared when a ScriptExecutionContext stops.
(You'll
> need to rename AudioScheduledSourceNode::stop() then.)
> 
> Note: I'm not familiar with the lifetime of webaudio objects and don't know if
> we really need to override ActiveDOMObject::stop(). I just want you to clarify
> the point.

Good point.  I've implemented this now.

[haraken, 2013-09-09 17:58:51]

LGTM with a comment.

https://codereview.chromium.org/23596014/diff/29001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):

https://codereview.chromium.org/23596014/diff/29001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:215: if
(m_notifyingEndedListener)

How about just using hasPendingActivity() instead of introducing
m_notifyingEndedListener?

[Raymond Toy (Google), 2013-09-09 18:23:44]

https://codereview.chromium.org/23596014/diff/29001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):

https://codereview.chromium.org/23596014/diff/29001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:215: if
(m_notifyingEndedListener)
On 2013/09/09 17:58:51, haraken wrote:
> 
> How about just using hasPendingActivity() instead of introducing
> m_notifyingEndedListener?

Done. m_notifyingEndedListener removed.

[haraken, 2013-09-09 18:29:16]

LGTM

[Raymond Toy (Google), 2013-09-09 18:42:02]

Thanks for the review!

Ken, do you want to take a final look?  

I need to do a bit more testing with this final version, but testing with
Friday's version showed no issues.

[Ken Russell (switch to Gerrit), 2013-09-09 19:45:47]

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:202:
unsetPendingActivity(this);
This looks wrong now. What happens if we navigate away from the document, stop()
is called first, and the audio processing thread afterward finishes handling the
node and notifyEnded() is called on the main thread? As far as I can see both
unsetPendingActivity calls should be conditional. I don't know the conditions
under which notifyEnded will be called but please explain why this code is
correct.

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioScheduledSourceNode.h (right):

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioScheduledSourceNode.h:62: void stopNote(double
when);
I'm 99.9% sure this renaming isn't necessary. stop() and stop(double) are
different methods and there should be no issue with stop() overriding
ActiveDOMObject's stop() while leaving this named stop(double).

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioScheduledSourceNode.h:74: virtual
ScriptExecutionContext* scriptExecutionContext() const OVERRIDE;
Please add a comment that these are for ActiveDOMObject.

[Ken Russell (switch to Gerrit), 2013-09-09 21:30:04]

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:184:
setPendingActivity(this);
Sorry, I failed to realize this before. This can't be called here -- this code
runs on the audio thread, and ActiveDOMObject, Node, EventTarget, etc., aren't
thread-safe.

I think that we'll have to figure out some other solution. Perhaps the node
can't transition to the FINISHED state until the onended callback has been
called on the main thread.

[Ken Russell (switch to Gerrit), 2013-09-09 21:34:30]

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):

https://codereview.chromium.org/23596014/diff/37001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:184:
setPendingActivity(this);
On 2013/09/09 21:30:05, Ken Russell wrote:
> Sorry, I failed to realize this before. This can't be called here -- this code
> runs on the audio thread, and ActiveDOMObject, Node, EventTarget, etc., aren't
> thread-safe.
> 
> I think that we'll have to figure out some other solution. Perhaps the node
> can't transition to the FINISHED state until the onended callback has been
> called on the main thread.

Apologies, I think I'm wrong. AudioNode::ref is specifically thread-safe. Let's
follow up offline.

[Ken Russell (switch to Gerrit), 2013-09-09 21:55:11]

After offline discussion, it's certain that we do not want to make
AudioScheduledSourceNode an ActiveDOMObject. AudioContext is already an
ActiveDOMObject. All that is desired is to protect the AudioScheduledSourceNode
between the call to finish() on the audio thread and the call to notifyEnded()
on the main thread.

Slava recommended allocating a tiny task object to be used as the argument to
notifyEndedDispatch, which contains a RefPtr inside. That's the pattern used in
Source/core/fileapi/BlobRegistry.cpp and would be more bulletproof.

[Raymond Toy (Google), 2013-09-11 16:47:49]

On 2013/09/09 21:55:11, Ken Russell wrote:
> After offline discussion, it's certain that we do not want to make
> AudioScheduledSourceNode an ActiveDOMObject. AudioContext is already an
> ActiveDOMObject. All that is desired is to protect the
AudioScheduledSourceNode
> between the call to finish() on the audio thread and the call to notifyEnded()
> on the main thread.
> 
> Slava recommended allocating a tiny task object to be used as the argument to
> notifyEndedDispatch, which contains a RefPtr inside. That's the pattern used
in
> Source/core/fileapi/BlobRegistry.cpp and would be more bulletproof.

ActiveDOMObject removed, and a tiny task object created, as suggested.

[Ken Russell (switch to Gerrit), 2013-09-11 17:08:30]

lgtm

[haraken, 2013-09-11 17:14:18]

This looks nicer. LGTM.

[Raymond Toy (Google), 2013-09-11 17:23:09]

Thanks, everyone, for the review.  

And for the record, all of the webaudio layout tests pass and I ran many of the
webaudio demos without any problems.  The test case from the bug report runs
fine for many minutes. (It use to crash almost instantly.)

[commit-bot: I haz the power, 2013-09-11 17:43:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/23596014/45001

[commit-bot: I haz the power, 2013-09-11 22:46:40]

Change committed as 157615