Oilpan: Use transition types for HTMLMediaElement and make AudioSourceProviderClient a GC mixin.

Oilpan: Use transition types for HTMLMediaElement and make AudioSourceProviderClient a GC mixin.

BUG=357163
R=haraken@chromium.org

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

[haraken, 2014-05-21 16:35:58]

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:373: ASSERT(!m_audioSourceNode);

I'm curious why you added this ASSERT. If m_audioSourceNode is really guaranteed
to be 0 here, we don't need to have the following code in
clearMediaPlayerAndAudioSourceProviderClient.

if (m_audioSourceNode)
  m_audioSourceNode->lock();
...;
if (m_audioSourceNode)
  m_audioSourceNode->unlock();

(I added the lock/unlock to fix some threading issues a long time ago.)

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:517:
RawPtrWillBeMember<AudioSourceProviderClient> m_audioSourceNode;

Shouldn't this be a weak member?

[zerny-chromium, 2014-05-22 05:54:31]

Thanks for the review.

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:373: ASSERT(!m_audioSourceNode);
On 2014/05/21 16:35:58, haraken wrote:
> 
> I'm curious why you added this ASSERT. If m_audioSourceNode is really
guaranteed
> to be 0 here, we don't need to have the following code in
> clearMediaPlayerAndAudioSourceProviderClient.
> 
> if (m_audioSourceNode)
>   m_audioSourceNode->lock();
> ...;
> if (m_audioSourceNode)
>   m_audioSourceNode->unlock();
> 
> (I added the lock/unlock to fix some threading issues a long time ago.)

Exactly. That is why I inlined the reduced code in the oilpan build and added
the assert to the non-oilpan build. It really must be the case since the audio
node has a ref-ptr to the element and upon destruction calls
setAudioSourceNode(0).

I could inline clearMediaPlayerAndAudioSourceProviderClient for both builds.
That would probably be more clear.

The reason why the locking is needed in
clearMediaPlayerAndAudioSourceProviderClient is because of its use outside of
the destructor.

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:517:
RawPtrWillBeMember<AudioSourceProviderClient> m_audioSourceNode;
> Shouldn't this be a weak member?

It appears to be raw simply to break what would otherwise be a ref cycle. I
can't see any apparent reason that the audioSourceNode must be reclaimed if only
the HTMLMediaElement is referencing it.

[haraken, 2014-05-22 07:33:35]

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:517:
RawPtrWillBeMember<AudioSourceProviderClient> m_audioSourceNode;
On 2014/05/22 05:54:32, zerny-chromium wrote:
> > Shouldn't this be a weak member?
> 
> It appears to be raw simply to break what would otherwise be a ref cycle. I
> can't see any apparent reason that the audioSourceNode must be reclaimed if
only
> the HTMLMediaElement is referencing it.

In the current implementation, it looks like that HTMLMediaElement can outlive
AudioSourceProviderClient. To keep the behavior, I guess this pointer needs to
be weak. (Also the comment in line 516 implies that this point is weak.)

[zerny-chromium, 2014-05-22 07:39:48]

On 2014/05/22 07:33:35, haraken wrote:
>
https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
> File Source/core/html/HTMLMediaElement.h (right):
> 
>
https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
> Source/core/html/HTMLMediaElement.h:517:
> RawPtrWillBeMember<AudioSourceProviderClient> m_audioSourceNode;
> On 2014/05/22 05:54:32, zerny-chromium wrote:
> > > Shouldn't this be a weak member?
> > 
> > It appears to be raw simply to break what would otherwise be a ref cycle. I
> > can't see any apparent reason that the audioSourceNode must be reclaimed if
> only
> > the HTMLMediaElement is referencing it.
> 
> In the current implementation, it looks like that HTMLMediaElement can outlive
> AudioSourceProviderClient. To keep the behavior, I guess this pointer needs to
> be weak. (Also the comment in line 516 implies that this point is weak.)

I agree that the current behavior is that the element can outlive the client,
but the comment on 516 explicitly says that it is weak to avoid the ref-count
cycle. For all I can see the behavior is simple a product of trying to break the
ref-cycle.

[haraken, 2014-05-22 07:48:02]

On 2014/05/22 07:39:48, zerny-chromium wrote:
> On 2014/05/22 07:33:35, haraken wrote:
> >
>
https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
> > File Source/core/html/HTMLMediaElement.h (right):
> > 
> >
>
https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
> > Source/core/html/HTMLMediaElement.h:517:
> > RawPtrWillBeMember<AudioSourceProviderClient> m_audioSourceNode;
> > On 2014/05/22 05:54:32, zerny-chromium wrote:
> > > > Shouldn't this be a weak member?
> > > 
> > > It appears to be raw simply to break what would otherwise be a ref cycle.
I
> > > can't see any apparent reason that the audioSourceNode must be reclaimed
if
> > only
> > > the HTMLMediaElement is referencing it.
> > 
> > In the current implementation, it looks like that HTMLMediaElement can
outlive
> > AudioSourceProviderClient. To keep the behavior, I guess this pointer needs
to
> > be weak. (Also the comment in line 516 implies that this point is weak.)
> 
> I agree that the current behavior is that the element can outlive the client,
> but the comment on 516 explicitly says that it is weak to avoid the ref-count
> cycle. For all I can see the behavior is simple a product of trying to break
the
> ref-cycle.

Just help me understand: Why do you think that the HTMLMediaElement and the
AudioSourceProviderClient should live and die together? It seems a bit strange
to me that the element keeps a "client" alive.

[Mads Ager (chromium), 2014-05-22 08:02:55]

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:373: ASSERT(!m_audioSourceNode);
On 2014/05/22 05:54:32, zerny-chromium wrote:
> On 2014/05/21 16:35:58, haraken wrote:
> > 
> > I'm curious why you added this ASSERT. If m_audioSourceNode is really
> guaranteed
> > to be 0 here, we don't need to have the following code in
> > clearMediaPlayerAndAudioSourceProviderClient.
> > 
> > if (m_audioSourceNode)
> >   m_audioSourceNode->lock();
> > ...;
> > if (m_audioSourceNode)
> >   m_audioSourceNode->unlock();
> > 
> > (I added the lock/unlock to fix some threading issues a long time ago.)
> 
> Exactly. That is why I inlined the reduced code in the oilpan build and added
> the assert to the non-oilpan build. It really must be the case since the audio
> node has a ref-ptr to the element and upon destruction calls
> setAudioSourceNode(0).
> 
> I could inline clearMediaPlayerAndAudioSourceProviderClient for both builds.
> That would probably be more clear.
> 
> The reason why the locking is needed in
> clearMediaPlayerAndAudioSourceProviderClient is because of its use outside of
> the destructor.

Maybe we can split this into two methods:

clearMediaPlayerAndAudioSourceProviderClientWithoutLocking()
    - do the actual clearing

clearMediaPlayerAndAudioSourceProviderClient():
   - lock
   - clearMediaPlayerAndAudioSourceProviderClientWithoutLocking()
   - unlock

The call the one without locking here and have the assert that m_audioSourceNode
is NULL for the non-oilpan build.

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:517:
RawPtrWillBeMember<AudioSourceProviderClient> m_audioSourceNode;
On 2014/05/22 07:33:36, haraken wrote:
> On 2014/05/22 05:54:32, zerny-chromium wrote:
> > > Shouldn't this be a weak member?
> > 
> > It appears to be raw simply to break what would otherwise be a ref cycle. I
> > can't see any apparent reason that the audioSourceNode must be reclaimed if
> only
> > the HTMLMediaElement is referencing it.
> 
> In the current implementation, it looks like that HTMLMediaElement can outlive
> AudioSourceProviderClient. To keep the behavior, I guess this pointer needs to
> be weak. (Also the comment in line 516 implies that this point is weak.)

I think what that comment is saying is that this is a raw pointer because
m_audioSourceNode has a RefPtr to us and therefore it would introduce a cycle to
use a RefPtr here. The use of the work "weak" here just means that it is a "raw"
pointer.

I agree with Ian that these objects should just live and die together. As far as
I can tell this pointer is raw just to break a reference cycle between the
HTMLMediaElement and the AudioSourceNode that is providing audio for this
element.

[zerny-chromium, 2014-05-22 08:23:12]

On 2014/05/22 08:02:55, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
> Source/core/html/HTMLMediaElement.cpp:373: ASSERT(!m_audioSourceNode);
> On 2014/05/22 05:54:32, zerny-chromium wrote:
> > On 2014/05/21 16:35:58, haraken wrote:
> > > 
> > > I'm curious why you added this ASSERT. If m_audioSourceNode is really
> > guaranteed
> > > to be 0 here, we don't need to have the following code in
> > > clearMediaPlayerAndAudioSourceProviderClient.
> > > 
> > > if (m_audioSourceNode)
> > >   m_audioSourceNode->lock();
> > > ...;
> > > if (m_audioSourceNode)
> > >   m_audioSourceNode->unlock();
> > > 
> > > (I added the lock/unlock to fix some threading issues a long time ago.)
> > 
> > Exactly. That is why I inlined the reduced code in the oilpan build and
added
> > the assert to the non-oilpan build. It really must be the case since the
audio
> > node has a ref-ptr to the element and upon destruction calls
> > setAudioSourceNode(0).
> > 
> > I could inline clearMediaPlayerAndAudioSourceProviderClient for both builds.
> > That would probably be more clear.
> > 
> > The reason why the locking is needed in
> > clearMediaPlayerAndAudioSourceProviderClient is because of its use outside
of
> > the destructor.
> 
> Maybe we can split this into two methods:
> 
> clearMediaPlayerAndAudioSourceProviderClientWithoutLocking()
>     - do the actual clearing
> 
> clearMediaPlayerAndAudioSourceProviderClient():
>    - lock
>    - clearMediaPlayerAndAudioSourceProviderClientWithoutLocking()
>    - unlock
> 
> The call the one without locking here and have the assert that
m_audioSourceNode
> is NULL for the non-oilpan build.
> 
>
https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
> File Source/core/html/HTMLMediaElement.h (right):
> 
>
https://codereview.chromium.org/294053006/diff/1/Source/core/html/HTMLMediaEl...
> Source/core/html/HTMLMediaElement.h:517:
> RawPtrWillBeMember<AudioSourceProviderClient> m_audioSourceNode;
> On 2014/05/22 07:33:36, haraken wrote:
> > On 2014/05/22 05:54:32, zerny-chromium wrote:
> > > > Shouldn't this be a weak member?
> > > 
> > > It appears to be raw simply to break what would otherwise be a ref cycle.
I
> > > can't see any apparent reason that the audioSourceNode must be reclaimed
if
> > only
> > > the HTMLMediaElement is referencing it.
> > 
> > In the current implementation, it looks like that HTMLMediaElement can
outlive
> > AudioSourceProviderClient. To keep the behavior, I guess this pointer needs
to
> > be weak. (Also the comment in line 516 implies that this point is weak.)
> 
> I think what that comment is saying is that this is a raw pointer because
> m_audioSourceNode has a RefPtr to us and therefore it would introduce a cycle
to
> use a RefPtr here. The use of the work "weak" here just means that it is a
"raw"
> pointer.
> 
> I agree with Ian that these objects should just live and die together. As far
as
> I can tell this pointer is raw just to break a reference cycle between the
> HTMLMediaElement and the AudioSourceNode that is providing audio for this
> element.

I can try :-)

The HTMLMediaElement which is Node-derived potentially has a
MediaElementAudioSourceNode which is AudioNode-derived and in addition it
implements the AudioSourceClient interface. The audio source can be set/cleared
by setAudioSource. Setting it will hand off the node as a client to the
audioSourceProvider. So the "client aspect" is wrt the audioSourceProvider and
that does not have a lifetime preserving reference (ie, that is still weak).

MediaElementAudioSourceNode always has an element and will (in non-oilpan)
set/clear the back pointer on create/delete.

In summary the MediaElementAudioSourceNode could *not* have a ref pointer, and
because of this it is allowed that it dies once only the HTMLMediaNode is
referencing it. In order for this weakness to matter the web-audio nodes would
need to be removed/torn down prior to the rest of the document and cause the
media element to "loose" its source and the client to be deregistered from the
audioSourceProvider. I seems reasonable that if we have a handle on the
HTMLMediaElement that should keep its attached AudioSourceNode alive.

@rtoy Do you know if audio nodes should be weakly observed so that they can be
destructed prior to the referencing HTMLMediaNode dying?

[haraken, 2014-05-22 08:31:16]

+acolwell

[zerny-chromium, 2014-06-03 06:25:33]

acolwell@ Do you know if the pointer from HTMLMediaElement to
MediaElementAudioSourceNode needs to have weak behavior? I.e., must it be such
that the audio source node is destructed as soon as only the node references it?

In the oilpan build, we would like to make both pointers between
HTMLMediaElement and MediaElementAudioSourceNode strongly traced, such that if
they are set/non-null the two objects will die together.

Any other comments you might have to this CL are most welcome.

[acolwell GONE FROM CHROMIUM, 2014-06-03 20:18:01]

On 2014/06/03 06:25:33, zerny-chromium wrote:
> acolwell@ Do you know if the pointer from HTMLMediaElement to
> MediaElementAudioSourceNode needs to have weak behavior? I.e., must it be such
> that the audio source node is destructed as soon as only the node references
it?
> 
> In the oilpan build, we would like to make both pointers between
> HTMLMediaElement and MediaElementAudioSourceNode strongly traced, such that if
> they are set/non-null the two objects will die together.
> 
> Any other comments you might have to this CL are most welcome.

How urgent is this particular work? I'd actually like to see this WebAudio code
moved out of the HTMLMediaElement code into a supplemental object in the
webaudio module like the encryptedmedia code. I'd also like to see the locking
and ownership stuff cleaned up. I'd prefer that work to be done before
oilpanification if possible since I think it might make things easier.

[haraken, 2014-06-03 23:26:15]

On 2014/06/03 20:18:01, acolwell wrote:
> On 2014/06/03 06:25:33, zerny-chromium wrote:
> > acolwell@ Do you know if the pointer from HTMLMediaElement to
> > MediaElementAudioSourceNode needs to have weak behavior? I.e., must it be
such
> > that the audio source node is destructed as soon as only the node references
> it?
> > 
> > In the oilpan build, we would like to make both pointers between
> > HTMLMediaElement and MediaElementAudioSourceNode strongly traced, such that
if
> > they are set/non-null the two objects will die together.
> > 
> > Any other comments you might have to this CL are most welcome.
> 
> How urgent is this particular work? I'd actually like to see this WebAudio
code
> moved out of the HTMLMediaElement code into a supplemental object in the
> webaudio module like the encryptedmedia code. I'd also like to see the locking
> and ownership stuff cleaned up. I'd prefer that work to be done before
> oilpanification if possible since I think it might make things easier.

oh, this sounds a nice clean-up and will simplify the oilpanification!

In order to start performance evaluation for the Node hierarchy in oilpan builds
(we need to remove all RefPtrs to the Node hierarchy), we might want to make
this change by the end of this month.

[zerny-chromium, 2014-06-04 05:04:52]

On 2014/06/03 23:26:15, haraken wrote:
> On 2014/06/03 20:18:01, acolwell wrote:
> > On 2014/06/03 06:25:33, zerny-chromium wrote:
> > > acolwell@ Do you know if the pointer from HTMLMediaElement to
> > > MediaElementAudioSourceNode needs to have weak behavior? I.e., must it be
> such
> > > that the audio source node is destructed as soon as only the node
references
> > it?
> > > 
> > > In the oilpan build, we would like to make both pointers between
> > > HTMLMediaElement and MediaElementAudioSourceNode strongly traced, such
that
> if
> > > they are set/non-null the two objects will die together.
> > > 
> > > Any other comments you might have to this CL are most welcome.
> > 
> > How urgent is this particular work? I'd actually like to see this WebAudio
> code
> > moved out of the HTMLMediaElement code into a supplemental object in the
> > webaudio module like the encryptedmedia code. I'd also like to see the
locking
> > and ownership stuff cleaned up. I'd prefer that work to be done before
> > oilpanification if possible since I think it might make things easier.
> 
> oh, this sounds a nice clean-up and will simplify the oilpanification!
> 
> In order to start performance evaluation for the Node hierarchy in oilpan
builds
> (we need to remove all RefPtrs to the Node hierarchy), we might want to make
> this change by the end of this month.

In that case, lets make MediaElementAudioSourceNode::m_mediaElement a
Persistent<HTMLMediaElement> with a FIXME: Oilpan: about the refactoring.

That should unblock work for both projects.

[zerny-chromium, 2014-06-04 05:58:34]

On 2014/06/04 05:04:52, zerny-chromium wrote:
> On 2014/06/03 23:26:15, haraken wrote:
> > On 2014/06/03 20:18:01, acolwell wrote:
> > > On 2014/06/03 06:25:33, zerny-chromium wrote:
> > > > acolwell@ Do you know if the pointer from HTMLMediaElement to
> > > > MediaElementAudioSourceNode needs to have weak behavior? I.e., must it
be
> > such
> > > > that the audio source node is destructed as soon as only the node
> references
> > > it?
> > > > 
> > > > In the oilpan build, we would like to make both pointers between
> > > > HTMLMediaElement and MediaElementAudioSourceNode strongly traced, such
> that
> > if
> > > > they are set/non-null the two objects will die together.
> > > > 
> > > > Any other comments you might have to this CL are most welcome.
> > > 
> > > How urgent is this particular work? I'd actually like to see this WebAudio
> > code
> > > moved out of the HTMLMediaElement code into a supplemental object in the
> > > webaudio module like the encryptedmedia code. I'd also like to see the
> locking
> > > and ownership stuff cleaned up. I'd prefer that work to be done before
> > > oilpanification if possible since I think it might make things easier.
> > 
> > oh, this sounds a nice clean-up and will simplify the oilpanification!
> > 
> > In order to start performance evaluation for the Node hierarchy in oilpan
> builds
> > (we need to remove all RefPtrs to the Node hierarchy), we might want to make
> > this change by the end of this month.
> 
> In that case, lets make MediaElementAudioSourceNode::m_mediaElement a
> Persistent<HTMLMediaElement> with a FIXME: Oilpan: about the refactoring.
> 
> That should unblock work for both projects.

I've looked at this a bit more and I don't think that the persistent is a good
idea since it will be on a managed on-heap object (AudioSourceNode). This can
lead to leaks in the same way as RefPtr and is likely to do so with oilpan
enabled.

acolwell@ I understand your reasons for wanting to do the migration in a
"pre-oilpan" setting, but both the Node hierarchy, the AudioNode hierarchy and
the Supplement(able) structures are already using transition types. So it is
pretty much unavoidable to tackle oilpan as part of the refactoring. That said,
we are more than happy to help out with that.

I think it would be best to get this CL landed if it actually does what we want.
It is also directly inline with your thoughts on using a supplement, since a
supplement is just a special kind of GC-mixin and this CL makes the
AudioSourceProviderClient such a GC-mixin.

The outstanding question is what the lifetime is between HTMLMediaElement and
MediaElementAudioSourceNode. Can they simply be a strongly connected, which is
simple (this cycle is a non-issue for oilpan) or must the HTMLMediaElement be
able to outlive its "attached" AudioSourceProviderClient, which is the behavior
of the existing code. I'm unsure if that behavior is simply a side-effect of
breaking the ref cycle.

[acolwell GONE FROM CHROMIUM, 2014-06-04 19:24:59]

On 2014/06/04 05:58:34, zerny-chromium wrote:
> On 2014/06/04 05:04:52, zerny-chromium wrote:
> > On 2014/06/03 23:26:15, haraken wrote:
> > > On 2014/06/03 20:18:01, acolwell wrote:
> > > > On 2014/06/03 06:25:33, zerny-chromium wrote:
> > > > > acolwell@ Do you know if the pointer from HTMLMediaElement to
> > > > > MediaElementAudioSourceNode needs to have weak behavior? I.e., must it
> be
> > > such
> > > > > that the audio source node is destructed as soon as only the node
> > references
> > > > it?
> > > > > 
> > > > > In the oilpan build, we would like to make both pointers between
> > > > > HTMLMediaElement and MediaElementAudioSourceNode strongly traced, such
> > that
> > > if
> > > > > they are set/non-null the two objects will die together.
> > > > > 
> > > > > Any other comments you might have to this CL are most welcome.
> > > > 
> > > > How urgent is this particular work? I'd actually like to see this
WebAudio
> > > code
> > > > moved out of the HTMLMediaElement code into a supplemental object in the
> > > > webaudio module like the encryptedmedia code. I'd also like to see the
> > locking
> > > > and ownership stuff cleaned up. I'd prefer that work to be done before
> > > > oilpanification if possible since I think it might make things easier.
> > > 
> > > oh, this sounds a nice clean-up and will simplify the oilpanification!
> > > 
> > > In order to start performance evaluation for the Node hierarchy in oilpan
> > builds
> > > (we need to remove all RefPtrs to the Node hierarchy), we might want to
make
> > > this change by the end of this month.
> > 
> > In that case, lets make MediaElementAudioSourceNode::m_mediaElement a
> > Persistent<HTMLMediaElement> with a FIXME: Oilpan: about the refactoring.
> > 
> > That should unblock work for both projects.
> 
> I've looked at this a bit more and I don't think that the persistent is a good
> idea since it will be on a managed on-heap object (AudioSourceNode). This can
> lead to leaks in the same way as RefPtr and is likely to do so with oilpan
> enabled.
> 
> acolwell@ I understand your reasons for wanting to do the migration in a
> "pre-oilpan" setting, but both the Node hierarchy, the AudioNode hierarchy and
> the Supplement(able) structures are already using transition types. So it is
> pretty much unavoidable to tackle oilpan as part of the refactoring. That
said,
> we are more than happy to help out with that.

OK.

> 
> I think it would be best to get this CL landed if it actually does what we
want.
> It is also directly inline with your thoughts on using a supplement, since a
> supplement is just a special kind of GC-mixin and this CL makes the
> AudioSourceProviderClient such a GC-mixin.

OK. SGTM.

> 
> The outstanding question is what the lifetime is between HTMLMediaElement and
> MediaElementAudioSourceNode. Can they simply be a strongly connected, which is
> simple (this cycle is a non-issue for oilpan) or must the HTMLMediaElement be
> able to outlive its "attached" AudioSourceProviderClient, which is the
behavior
> of the existing code. I'm unsure if that behavior is simply a side-effect of
> breaking the ref cycle.

I'm pretty sure the HTMLMediaElement can outlive the
MediaElementAudioSourceNode. For example it could be in the document, but the
MEASN gets detached from the node graph. I believe this should cause MEASN to
get GCed, but the HTMLMediaElement definitely shouldn't be cleaned up. I believe
it should just stop routing its audio to MEASN when this happens.

[Mads Ager (chromium), 2014-06-06 06:14:45]

On 2014/06/04 19:24:59, acolwell wrote:
> On 2014/06/04 05:58:34, zerny-chromium wrote:
> > On 2014/06/04 05:04:52, zerny-chromium wrote:
> > > On 2014/06/03 23:26:15, haraken wrote:
> > > > On 2014/06/03 20:18:01, acolwell wrote:
> > > > > On 2014/06/03 06:25:33, zerny-chromium wrote:
> > > > > > acolwell@ Do you know if the pointer from HTMLMediaElement to
> > > > > > MediaElementAudioSourceNode needs to have weak behavior? I.e., must
it
> > be
> > > > such
> > > > > > that the audio source node is destructed as soon as only the node
> > > references
> > > > > it?
> > > > > > 
> > > > > > In the oilpan build, we would like to make both pointers between
> > > > > > HTMLMediaElement and MediaElementAudioSourceNode strongly traced,
such
> > > that
> > > > if
> > > > > > they are set/non-null the two objects will die together.
> > > > > > 
> > > > > > Any other comments you might have to this CL are most welcome.
> > > > > 
> > > > > How urgent is this particular work? I'd actually like to see this
> WebAudio
> > > > code
> > > > > moved out of the HTMLMediaElement code into a supplemental object in
the
> > > > > webaudio module like the encryptedmedia code. I'd also like to see the
> > > locking
> > > > > and ownership stuff cleaned up. I'd prefer that work to be done before
> > > > > oilpanification if possible since I think it might make things easier.
> > > > 
> > > > oh, this sounds a nice clean-up and will simplify the oilpanification!
> > > > 
> > > > In order to start performance evaluation for the Node hierarchy in
oilpan
> > > builds
> > > > (we need to remove all RefPtrs to the Node hierarchy), we might want to
> make
> > > > this change by the end of this month.
> > > 
> > > In that case, lets make MediaElementAudioSourceNode::m_mediaElement a
> > > Persistent<HTMLMediaElement> with a FIXME: Oilpan: about the refactoring.
> > > 
> > > That should unblock work for both projects.
> > 
> > I've looked at this a bit more and I don't think that the persistent is a
good
> > idea since it will be on a managed on-heap object (AudioSourceNode). This
can
> > lead to leaks in the same way as RefPtr and is likely to do so with oilpan
> > enabled.
> > 
> > acolwell@ I understand your reasons for wanting to do the migration in a
> > "pre-oilpan" setting, but both the Node hierarchy, the AudioNode hierarchy
and
> > the Supplement(able) structures are already using transition types. So it is
> > pretty much unavoidable to tackle oilpan as part of the refactoring. That
> said,
> > we are more than happy to help out with that.
> 
> OK.
> 
> > 
> > I think it would be best to get this CL landed if it actually does what we
> want.
> > It is also directly inline with your thoughts on using a supplement, since a
> > supplement is just a special kind of GC-mixin and this CL makes the
> > AudioSourceProviderClient such a GC-mixin.
> 
> OK. SGTM.
> 
> > 
> > The outstanding question is what the lifetime is between HTMLMediaElement
and
> > MediaElementAudioSourceNode. Can they simply be a strongly connected, which
is
> > simple (this cycle is a non-issue for oilpan) or must the HTMLMediaElement
be
> > able to outlive its "attached" AudioSourceProviderClient, which is the
> behavior
> > of the existing code. I'm unsure if that behavior is simply a side-effect of
> > breaking the ref cycle.
> 
> I'm pretty sure the HTMLMediaElement can outlive the
> MediaElementAudioSourceNode. For example it could be in the document, but the
> MEASN gets detached from the node graph. I believe this should cause MEASN to
> get GCed, but the HTMLMediaElement definitely shouldn't be cleaned up. I
believe
> it should just stop routing its audio to MEASN when this happens.

Jumping in from the side since Ian is on vacation today. :-)

Yes, you can definitely explicitly pull these objects apart at which point they
can die independently. At this point there is a raw pointer which means that one
of these objects can actually die before the other one even when they are still
attached. That does not seem intentional to me, but we would like to verify. So
the question is if the AudioSourceProviderClient should be able to die even when
it is attached to an HTMLMediaElement? I would guess that while they are
attached none of them should die if the other one is alive. Only when they are
explicitly detached from each other should they be able to die independently.
Does that sound right?

[acolwell GONE FROM CHROMIUM, 2014-06-09 22:10:40]

On 2014/06/06 06:14:45, Mads Ager (chromium) wrote:
> On 2014/06/04 19:24:59, acolwell wrote:
> > On 2014/06/04 05:58:34, zerny-chromium wrote:
> > > On 2014/06/04 05:04:52, zerny-chromium wrote:
> > > > On 2014/06/03 23:26:15, haraken wrote:
> > > > > On 2014/06/03 20:18:01, acolwell wrote:
> > > > > > On 2014/06/03 06:25:33, zerny-chromium wrote:
> > > > > > > acolwell@ Do you know if the pointer from HTMLMediaElement to
> > > > > > > MediaElementAudioSourceNode needs to have weak behavior? I.e.,
must
> it
> > > be
> > > > > such
> > > > > > > that the audio source node is destructed as soon as only the node
> > > > references
> > > > > > it?
> > > > > > > 
> > > > > > > In the oilpan build, we would like to make both pointers between
> > > > > > > HTMLMediaElement and MediaElementAudioSourceNode strongly traced,
> such
> > > > that
> > > > > if
> > > > > > > they are set/non-null the two objects will die together.
> > > > > > > 
> > > > > > > Any other comments you might have to this CL are most welcome.
> > > > > > 
> > > > > > How urgent is this particular work? I'd actually like to see this
> > WebAudio
> > > > > code
> > > > > > moved out of the HTMLMediaElement code into a supplemental object in
> the
> > > > > > webaudio module like the encryptedmedia code. I'd also like to see
the
> > > > locking
> > > > > > and ownership stuff cleaned up. I'd prefer that work to be done
before
> > > > > > oilpanification if possible since I think it might make things
easier.
> > > > > 
> > > > > oh, this sounds a nice clean-up and will simplify the oilpanification!
> > > > > 
> > > > > In order to start performance evaluation for the Node hierarchy in
> oilpan
> > > > builds
> > > > > (we need to remove all RefPtrs to the Node hierarchy), we might want
to
> > make
> > > > > this change by the end of this month.
> > > > 
> > > > In that case, lets make MediaElementAudioSourceNode::m_mediaElement a
> > > > Persistent<HTMLMediaElement> with a FIXME: Oilpan: about the
refactoring.
> > > > 
> > > > That should unblock work for both projects.
> > > 
> > > I've looked at this a bit more and I don't think that the persistent is a
> good
> > > idea since it will be on a managed on-heap object (AudioSourceNode). This
> can
> > > lead to leaks in the same way as RefPtr and is likely to do so with oilpan
> > > enabled.
> > > 
> > > acolwell@ I understand your reasons for wanting to do the migration in a
> > > "pre-oilpan" setting, but both the Node hierarchy, the AudioNode hierarchy
> and
> > > the Supplement(able) structures are already using transition types. So it
is
> > > pretty much unavoidable to tackle oilpan as part of the refactoring. That
> > said,
> > > we are more than happy to help out with that.
> > 
> > OK.
> > 
> > > 
> > > I think it would be best to get this CL landed if it actually does what we
> > want.
> > > It is also directly inline with your thoughts on using a supplement, since
a
> > > supplement is just a special kind of GC-mixin and this CL makes the
> > > AudioSourceProviderClient such a GC-mixin.
> > 
> > OK. SGTM.
> > 
> > > 
> > > The outstanding question is what the lifetime is between HTMLMediaElement
> and
> > > MediaElementAudioSourceNode. Can they simply be a strongly connected,
which
> is
> > > simple (this cycle is a non-issue for oilpan) or must the HTMLMediaElement
> be
> > > able to outlive its "attached" AudioSourceProviderClient, which is the
> > behavior
> > > of the existing code. I'm unsure if that behavior is simply a side-effect
of
> > > breaking the ref cycle.
> > 
> > I'm pretty sure the HTMLMediaElement can outlive the
> > MediaElementAudioSourceNode. For example it could be in the document, but
the
> > MEASN gets detached from the node graph. I believe this should cause MEASN
to
> > get GCed, but the HTMLMediaElement definitely shouldn't be cleaned up. I
> believe
> > it should just stop routing its audio to MEASN when this happens.
> 
> Jumping in from the side since Ian is on vacation today. :-)
> 
> Yes, you can definitely explicitly pull these objects apart at which point
they
> can die independently. At this point there is a raw pointer which means that
one
> of these objects can actually die before the other one even when they are
still
> attached. That does not seem intentional to me, but we would like to verify.
So
> the question is if the AudioSourceProviderClient should be able to die even
when
> it is attached to an HTMLMediaElement? I would guess that while they are
> attached none of them should die if the other one is alive. Only when they are
> explicitly detached from each other should they be able to die independently.
> Does that sound right?

I think MEASN should be allowed to die if JS does not have a reference either
directly or via a WebAudio node graph. As long as MEASN is alive, it should keep
HME alive. If MEASN dies and no JS or DOM references the HME and the HME is not
playing, then I think it should also die. If HME is still playing, then it needs
to stay alive just like it would if WebAudio was never involved. Does that make
sense and/or clarify things?  You might want to have rtoy@ weigh in as well
since I believe he is more familiar w/ the WebAudio spec than I am.

[zerny-chromium, 2014-06-10 16:43:07]

Thanks for the feedback acolwell. I've updated the CL to use a weak member and
added a FIXME about the possibility of using a strong member instead. I'll
create a bug report on this and CC rtoy. In the meantime, we should land this
which will unblock tkent from removing ref-counting completely from Node and its
decedents. PTAL.

[acolwell GONE FROM CHROMIUM, 2014-06-10 17:07:18]

lgtm

https://codereview.chromium.org/294053006/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/294053006/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:379:
clearMediaPlayerAndAudioSourceProviderClientWithoutLocking();
This makes me nervous. This is safe because we can't get here if MEASN is still
holding a reference to this object and MEASN won't get destructed unless it is
detached from the WebAudio node graph an therefore it isn't possible for a call
to come in from the audio processing thread? 

Assuming what I've said here is correct, it would be nice to have a comment here
outlining why it is ok to not lock.

https://codereview.chromium.org/294053006/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3135: void
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClient()
nit: How about just moving the body of this function into clearMediaPlayer()
since that appears to be the only call site?

[zerny-chromium, 2014-06-10 19:42:30]

Thanks for the comments.

https://codereview.chromium.org/294053006/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/294053006/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:379:
clearMediaPlayerAndAudioSourceProviderClientWithoutLocking();
On 2014/06/10 17:07:17, acolwell wrote:
> This makes me nervous. This is safe because we can't get here if MEASN is
still
> holding a reference to this object and MEASN won't get destructed unless it is
> detached from the WebAudio node graph an therefore it isn't possible for a
call
> to come in from the audio processing thread? 
> 
> Assuming what I've said here is correct, it would be nice to have a comment
here
> outlining why it is ok to not lock.

Yes. The HTMLMediaElement can only die after the MEASN is gone, so we can assert
that in the non-oilpan build. With oilpan, it can be the case that both objects
are being finalized in the same GC round. Therefor, the destructor can not
access m_audioSourceNode which is potentially already finalized or will be soon.

I've added a comment to that effect.

https://codereview.chromium.org/294053006/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3135: void
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClient()
On 2014/06/10 17:07:17, acolwell wrote:
> nit: How about just moving the body of this function into clearMediaPlayer()
> since that appears to be the only call site?

Done.

[tkent, 2014-06-10 20:55:18]

platform/ lgtm

[haraken, 2014-06-11 01:19:50]

LGTM with comments.

https://codereview.chromium.org/294053006/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/294053006/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3674: audioSourceProvider()->setClient(0);

This looks OK but it would be better to just call setAudioSourceNode:

  if (!visitor->isAlive(m_audioSourceNode))
    setAudioSourceNode(0);

https://codereview.chromium.org/294053006/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/MediaStreamAudioSourceNode.h (right):

https://codereview.chromium.org/294053006/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/MediaStreamAudioSourceNode.h:67: RefPtr<MediaStream>
m_mediaStream;

MediaStream is on-heap, so you can use a Member for this.

https://codereview.chromium.org/294053006/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/MediaStreamAudioSourceNode.h:68:
RefPtr<MediaStreamTrack> m_audioTrack;

Ditto. MediaStreamTrack is on-heap.

[zerny-chromium, 2014-06-11 06:26:58]

Thanks for the reviews.

Haraken, I'm OOO the rest of the day, so if you agree with keeping the weak
processing as is, could you hit the QC bit?

https://codereview.chromium.org/294053006/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/294053006/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3674: audioSourceProvider()->setClient(0);
On 2014/06/11 01:19:49, haraken wrote:
> 
> This looks OK but it would be better to just call setAudioSourceNode:
> 
>   if (!visitor->isAlive(m_audioSourceNode))
>     setAudioSourceNode(0);

I don't think so, since m_audioSourceNode is dying and setAudioSourceNode would
do a ref (via lock).

https://codereview.chromium.org/294053006/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/MediaStreamAudioSourceNode.h (right):

https://codereview.chromium.org/294053006/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/MediaStreamAudioSourceNode.h:67: RefPtr<MediaStream>
m_mediaStream;
On 2014/06/11 01:19:49, haraken wrote:
> 
> MediaStream is on-heap, so you can use a Member for this.

Done.

https://codereview.chromium.org/294053006/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/MediaStreamAudioSourceNode.h:68:
RefPtr<MediaStreamTrack> m_audioTrack;
On 2014/06/11 01:19:50, haraken wrote:
> 
> Ditto. MediaStreamTrack is on-heap.

Done.

[haraken, 2014-06-11 06:28:58]

> Thanks for the reviews.
> 
> Haraken, I'm OOO the rest of the day, so if you agree with keeping the weak
> processing as is, could you hit the QC bit?
> 
>
https://codereview.chromium.org/294053006/diff/40001/Source/core/html/HTMLMed...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/294053006/diff/40001/Source/core/html/HTMLMed...
> Source/core/html/HTMLMediaElement.cpp:3674:
audioSourceProvider()->setClient(0);
> On 2014/06/11 01:19:49, haraken wrote:
> > 
> > This looks OK but it would be better to just call setAudioSourceNode:
> > 
> >   if (!visitor->isAlive(m_audioSourceNode))
> >     setAudioSourceNode(0);
> 
> I don't think so, since m_audioSourceNode is dying and setAudioSourceNode
would
> do a ref (via lock).

Makes sense. Will push it to a commit queue.

[haraken, 2014-06-11 06:29:02]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-11 06:29:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/294053006/60001

[commit-bot: I haz the power, 2014-06-11 06:29:54]

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

[commit-bot: I haz the power, 2014-06-11 06:29:56]

Failed to apply patch for Source/core/html/HTMLMediaElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLMediaElement.cpp
  Hunk #5 FAILED at 3663.
  1 out of 5 hunks FAILED -- saving rejects to file
Source/core/html/HTMLMediaElement.cpp.rej

Patch:       Source/core/html/HTMLMediaElement.cpp
Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp
b/Source/core/html/HTMLMediaElement.cpp
index
3b781bf062425814208b18e67bcd5c3da9712c68..367d587732a04f7b2b179edf8f37be62f2daac8e
100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -284,7 +284,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName&
tagName, Document& docum
     , m_textTracks(nullptr)
     , m_ignoreTrackDisplayUpdate(0)
 #if ENABLE(WEB_AUDIO)
-    , m_audioSourceNode(0)
+    , m_audioSourceNode(nullptr)
 #endif
 {
     ASSERT(RuntimeEnabledFeatures::mediaEnabled());
@@ -372,7 +372,13 @@ HTMLMediaElement::~HTMLMediaElement()
     // crbug.com/378229
     m_isFinalizing = true;
 #endif
-    clearMediaPlayerAndAudioSourceProviderClient();
+
+    // The m_audioSourceNode is either dead already or it is dying together
with
+    // this HTMLMediaElement which it strongly keeps alive.
+#if !ENABLE(OILPAN)
+    ASSERT(!m_audioSourceNode);
+#endif
+    clearMediaPlayerAndAudioSourceProviderClientWithoutLocking();
 
 #if !ENABLE(OILPAN)
     document().decrementLoadEventDelayCount();
@@ -3119,22 +3125,13 @@ void HTMLMediaElement::userCancelledLoad()
     updateActiveTextTrackCues(0);
 }
 
-void HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClient()
+void
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking()
 {
 #if ENABLE(WEB_AUDIO)
-    if (m_audioSourceNode)
-        m_audioSourceNode->lock();
-
     if (audioSourceProvider())
         audioSourceProvider()->setClient(0);
 #endif
-
     m_player.clear();
-
-#if ENABLE(WEB_AUDIO)
-    if (m_audioSourceNode)
-        m_audioSourceNode->unlock();
-#endif
 }
 
 void HTMLMediaElement::clearMediaPlayer(int flags)
@@ -3145,7 +3142,17 @@ void HTMLMediaElement::clearMediaPlayer(int flags)
 
     m_delayingLoadForPreloadNone = false;
 
-    clearMediaPlayerAndAudioSourceProviderClient();
+#if ENABLE(WEB_AUDIO)
+    if (m_audioSourceNode)
+        m_audioSourceNode->lock();
+#endif
+
+    clearMediaPlayerAndAudioSourceProviderClientWithoutLocking();
+
+#if ENABLE(WEB_AUDIO)
+    if (m_audioSourceNode)
+        m_audioSourceNode->unlock();
+#endif
 
     stopPeriodicTimers();
     m_loadTimer.stop();
@@ -3656,8 +3663,15 @@ void HTMLMediaElement::trace(Visitor* visitor)
     visitor->trace(m_nextChildNodeToConsider);
     visitor->trace(m_textTracks);
     visitor->trace(m_textTracksWhenResourceSelectionBegan);
+    visitor->registerWeakMembers<HTMLMediaElement,
&HTMLMediaElement::clearWeakMembers>(this);
     WillBeHeapSupplementable<HTMLMediaElement>::trace(visitor);
     HTMLElement::trace(visitor);
 }
 
+void HTMLMediaElement::clearWeakMembers(Visitor* visitor)
+{
+    if (!visitor->isAlive(m_audioSourceNode) && audioSourceProvider())
+        audioSourceProvider()->setClient(0);
+}
+
 }

[zerny-chromium, 2014-06-12 06:44:36]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-06-12 06:45:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/294053006/80001

[commit-bot: I haz the power, 2014-06-12 07:54:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)

[commit-bot: I haz the power, 2014-06-12 08:12:13]

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

[commit-bot: I haz the power, 2014-06-12 08:12:14]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)

[zerny-chromium, 2014-06-12 08:24:18]

The CQ bit was checked by zerny@chromium.org

[zerny-chromium, 2014-06-12 08:25:24]

The CQ bit was unchecked by zerny@chromium.org

[zerny-chromium, 2014-06-12 08:27:03]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-06-12 08:27:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/294053006/120001

[commit-bot: I haz the power, 2014-06-12 09:37:57]

Change committed as 176013