MediaElementAudioSourceNode::dispose() should call HTMLMediaElement::setAudioSourceNode(0) in oilpa…

MediaElementAudioSourceNode::dispose() should call HTMLMediaElement::setAudioSourceNode(0) in oilpan builds

Currently HTMLMediaElement::setAudioSourceNode(0) is called only in non-oilpan builds.
However, there is no reason why oilpan builds cannot call HTMLMediaElement::setAudioSourceNode(0).
It's better to call it both in oilpan builds and non-oilpan builds and reduce #if branches.

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

[haraken, 2014-08-11 07:01:46]

PTAL

[zerny-chromium, 2014-08-11 07:37:07]

This change requires that it is guaranteed that MediaElementAudioSourceNode is
always disposed prior to destruction which the dtor ASSERT(!isInitialized())
seems to imply. Could we just as well inline unintialize() in dispose?

Maybe we should add a comment that we rely on dispose always being called.
Otherwise, LGTM.

[haraken, 2014-08-11 07:48:25]

On 2014/08/11 07:37:07, zerny-chromium wrote:
> This change requires that it is guaranteed that MediaElementAudioSourceNode is
> always disposed prior to destruction which the dtor ASSERT(!isInitialized())
> seems to imply. Could we just as well inline unintialize() in dispose?
> 
> Maybe we should add a comment that we rely on dispose always being called.
> Otherwise, LGTM.

Good point... probably I was wrong. If AudioContext and AudioNodes die together,
AudioNode::dispose() is not called. Then we cannot rely on the
AudioNode::dispose() always being called. Then this CL is doing a wrong thing.

[tkent, 2014-08-11 08:53:00]

On 2014/08/11 07:48:25, haraken wrote:
> On 2014/08/11 07:37:07, zerny-chromium wrote:
> > This change requires that it is guaranteed that MediaElementAudioSourceNode
is
> > always disposed prior to destruction which the dtor ASSERT(!isInitialized())
> > seems to imply. Could we just as well inline unintialize() in dispose?
> > 
> > Maybe we should add a comment that we rely on dispose always being called.
> > Otherwise, LGTM.
> 
> Good point... probably I was wrong. If AudioContext and AudioNodes die
together,
> AudioNode::dispose() is not called. Then we cannot rely on the
> AudioNode::dispose() always being called. Then this CL is doing a wrong thing.

AudioNode::dispose() is always called. AudioContext::hasPendingActivity returns
true until clear() is called, and clear() calls AudioNode::dispose() for all of
live AudioNodes.

[tkent, 2014-08-11 08:54:04]

On 2014/08/11 08:53:00, tkent wrote:
> AudioNode::dispose() is always called. AudioContext::hasPendingActivity
returns
> true until clear() is called, and clear() calls AudioNode::dispose() for all
of
> live AudioNodes.

AudioNode::clear is called in ActiveDOMObject::stop().

[haraken, 2014-08-11 09:23:17]

On 2014/08/11 08:54:04, tkent wrote:
> On 2014/08/11 08:53:00, tkent wrote:
> > AudioNode::dispose() is always called. AudioContext::hasPendingActivity
> returns
> > true until clear() is called, and clear() calls AudioNode::dispose() for all
> of
> > live AudioNodes.
> 
> AudioNode::clear is called in ActiveDOMObject::stop().

Thanks for the details. I uploaded a CL to add an ASSERT to verify that
AudioNode::dispose() is always called before the AudioNode is destructed here
(https://codereview.chromium.org/455353002/).

Also added a comment to this CL as Ian suggests. PTAL.

[tkent, 2014-08-11 10:10:39]

lgtm

[haraken, 2014-08-11 10:28:32]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-11 10:29:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/456273002/40001

[commit-bot: I haz the power, 2014-08-11 11:47:45]

Change committed as 179939