Oilpan: prevent player from accessing its media element during finalization

Oilpan: prevent player from accessing its media element during finalization

When finalizing the media element, its media player is also cleared
out. With Oilpan enabled, that media player object must not touch the
media element while destructing its "client", as it is not in a
valid state (its heap object member may have been finalized already.)

Arrange for that to not happen by having the media element enter a
'finalizing' state, which is explicitly checked for when the player
attempts to access the media element during destruction.

This is a shorter-term solution until the media player object itself
is moved to the Oilpan heap; http://crbug.com/378229 for handling that.

R=haraken@chromium.org,ager@chromium.org
BUG=377567
TEST=media/track/track-removal-crash.html
TEST=media/audio-delete-while-slider-thumb-clicked.html
TEST=http/tests/media/media-source/mediasource-closed-on-htmlmediaelement-destruction.html

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

[sof, 2014-05-27 10:49:46]

Please take a look.

I don't see a way around not introducing this internal state
flag for the media element.

[haraken, 2014-05-27 11:06:37]

I wonder why WebMediaPlayerClientImpl doesn't keep a strong member to the
HTMLMediaElement (i.e., WebMediaPlayerClientImpl::m_client). If the client and
the media element die together, probably we won't need to call
playerDestroyed().

[Mads Ager (chromium), 2014-05-27 11:08:56]

We have:

WebCore::MediaPlayerClient* m_client;

in WebMediaPlayerClientImpl.h. This can only point to an HTMLMediaElement and
MediaPlayerClient should be a GarbageCollectedMixin and this needs to be either
a Member or a WeakMember. Based on this code review it looks like we can make
this a weak pointer that will be cleared out in weak processing. Then when we
get to this path we will get a 0 media element in player destroyed and we can do
a null check there as well instead of the explicit flag.

Would that work?

[Mads Ager (chromium), 2014-05-27 11:09:29]

On 2014/05/27 11:08:56, Mads Ager (chromium) wrote:
> We have:
> 
> WebCore::MediaPlayerClient* m_client;
> 
> in WebMediaPlayerClientImpl.h. This can only point to an HTMLMediaElement and
> MediaPlayerClient should be a GarbageCollectedMixin and this needs to be
either
> a Member or a WeakMember. Based on this code review it looks like we can make
> this a weak pointer that will be cleared out in weak processing. Then when we
> get to this path we will get a 0 media element in player destroyed and we can
do
> a null check there as well instead of the explicit flag.
> 
> Would that work?

And if we can make it a Member and have them live and die together that would be
even better as Haraken points out!

[sof, 2014-05-27 11:20:29]

On 2014/05/27 11:06:37, haraken wrote:
> I wonder why WebMediaPlayerClientImpl doesn't keep a strong member to the
> HTMLMediaElement (i.e., WebMediaPlayerClientImpl::m_client). If the client and
> the media element die together, probably we won't need to call
> playerDestroyed().

Oh, there's nothing stopping the use of a GC class for WebMediaPlayerClientImpl.
Much better.

[sof, 2014-05-27 12:35:35]

On 2014/05/27 11:20:29, sof wrote:
> On 2014/05/27 11:06:37, haraken wrote:
> > I wonder why WebMediaPlayerClientImpl doesn't keep a strong member to the
> > HTMLMediaElement (i.e., WebMediaPlayerClientImpl::m_client). If the client
and
> > the media element die together, probably we won't need to call
> > playerDestroyed().
> 
> Oh, there's nothing stopping the use of a GC class for
WebMediaPlayerClientImpl.
> Much better.

Hmm, there is. This will require exposing the MediaPlayer factory, an exported
API, as returning PassOwnWillBeRawPtr<>.

That is not allowed as it would require the caller to register it as a GC root
(one way or another); correct?

[sof, 2014-05-27 14:43:11]

On 2014/05/27 12:35:35, sof wrote:
> On 2014/05/27 11:20:29, sof wrote:
> > On 2014/05/27 11:06:37, haraken wrote:
> > > I wonder why WebMediaPlayerClientImpl doesn't keep a strong member to the
> > > HTMLMediaElement (i.e., WebMediaPlayerClientImpl::m_client). If the client
> and
> > > the media element die together, probably we won't need to call
> > > playerDestroyed().
> > 
> > Oh, there's nothing stopping the use of a GC class for
> WebMediaPlayerClientImpl.
> > Much better.
> 
> Hmm, there is. This will require exposing the MediaPlayer factory, an exported
> API, as returning PassOwnWillBeRawPtr<>.
> 
> That is not allowed as it would require the caller to register it as a GC root
> (one way or another); correct?

From this it follows:
 
=> MediaPlayer cannot be on the heap.
=> WebMediaPlayerClientImpl cannot either.
=> WebMediaPlayerClientImpl::m_client cannot be a Member.

As a HTMLMediaElement holds onto its MediaPlayer with an OwnPtr, and passes in
itself as the client object when creating the player, WebMediaPlayerClientImpl
cannot use a Persistent to refer back to the client, as that would create a
leak.

When clearing the player, we tear down the player object without first detaching
the client -- its lifetime is assumed to extend beyond the player's. Which it
does in a non-Oilpan setting, but this no longer holds true with Oilpan: some of
its heap-allocated members (e.g., supplementable hash table) may have been
finalized already.

So, instead of putting the media element into a custom 'finalizing' state with
Oilpan enabled, finalization should detach the player from its client before
finalizing it. The player will then not be able to access it & go wrong.

Will put up a patch based on that.. background explanation hopefully useful to
motivate why it is so.

[sof, 2014-05-27 15:15:23]

On 2014/05/27 14:43:11, sof wrote:
..
> 
> Will put up a patch based on that.. background explanation hopefully useful to
> motivate why it is so.

Now uploaded, preferable? Please take a look.

[haraken, 2014-05-27 23:09:09]

The approach looks OK, but calling detach() in a destructor is a bit nasty. It
means that we execute different code paths depending on the order in which
~HTMLMediaElement() and ~WebMediaPlayer() are called. This will work but is a
bit weird.

> Hmm, there is. This will require exposing the MediaPlayer factory, an exported
> API, as returning PassOwnWillBeRawPtr<>.
> 
> That is not allowed as it would require the caller to register it as a GC root
> (one way or another); correct?

I'd like to understand the reason why we cannot make MediaPlayer heap-allocated.
As far as I see, MediaPlayer::create is used only by HTMLMediaElement. Who is
using MediaPlayer::create from outside?

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

https://codereview.chromium.org/303593002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:367: // Oilpan: the player must be
released, but it cannot safely access

but => because ?

https://codereview.chromium.org/303593002/diff/20001/Source/web/WebMediaPlaye...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/303593002/diff/20001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:152: ASSERT(m_client || !layer);

Nit: I wonder why you need this ASSERT.

[sof, 2014-05-28 05:28:05]

On 2014/05/27 23:09:09, haraken wrote:
> The approach looks OK, but calling detach() in a destructor is a bit nasty. It
> means that we execute different code paths depending on the order in which
> ~HTMLMediaElement() and ~WebMediaPlayer() are called. This will work but is a
> bit weird.
> 
> > Hmm, there is. This will require exposing the MediaPlayer factory, an
exported
> > API, as returning PassOwnWillBeRawPtr<>.
> > 
> > That is not allowed as it would require the caller to register it as a GC
root
> > (one way or another); correct?
> 
> I'd like to understand the reason why we cannot make MediaPlayer
heap-allocated.
> As far as I see, MediaPlayer::create is used only by HTMLMediaElement. Who is
> using MediaPlayer::create from outside?
> 

It could be that current (Blink) embedders don't need the ability (or will be),
I suppose. Media folks being Cc:ed might know..?

[sof, 2014-05-28 05:28:17]

https://codereview.chromium.org/303593002/diff/20001/Source/web/WebMediaPlaye...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/303593002/diff/20001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:152: ASSERT(m_client || !layer);
On 2014/05/27 23:09:09, haraken wrote:
> 
> Nit: I wonder why you need this ASSERT.

To check that the embedder only tries to clear the layer when in a detached
state. Which the WebMediaPlayer does when it is torn down in the destructor
above.

Exposing a detached state on this client object isn't quite as
contained&restricted as I would have hoped for. Been going back&forth on this
overnight, and I think I prefer the first approach.

[sof, 2014-05-28 06:08:34]

Removing any exported API requires careful consideration. I think we need a
solution sooner here to address media-related flakiness with Oilpan.

I suggest we do what's in patchset #1, but create a separate bug on this & leave
behind a FIXME.

[haraken, 2014-05-28 06:16:16]

On 2014/05/28 06:08:34, sof wrote:
> Removing any exported API requires careful consideration. I think we need a
> solution sooner here to address media-related flakiness with Oilpan.
> 
> I suggest we do what's in patchset #1, but create a separate bug on this &
leave
> behind a FIXME.

FIXME + the patch set 1 LGTM.

Both patch set 1 and patch set 2 are nasty, but I'd prefer the patch set 1
because it's clearer how nasty it is :)

[Mads Ager (chromium), 2014-05-28 09:09:44]

I'm fine with that as well LGTM.

Having slept on this I also think that we should be able to move the
WebMediaPlayerClientImpl to the oilpan heap. The create method returns an OwnPtr
and that has to be used internally in blink code as far as I know. I don't think
we ever give the embedder OwnPtrs - they are blink internal only. That would
indicate that WebMediaPlayerClientImpl instances are only used internally and
can therefore be on the heap.

[sof, 2014-05-28 11:11:13]

On 2014/05/28 09:09:44, Mads Ager (chromium) wrote:
> I'm fine with that as well LGTM.
> 
> Having slept on this I also think that we should be able to move the
> WebMediaPlayerClientImpl to the oilpan heap. The create method returns an
OwnPtr
> and that has to be used internally in blink code as far as I know. I don't
think
> we ever give the embedder OwnPtrs - they are blink internal only. That would
> indicate that WebMediaPlayerClientImpl instances are only used internally and
> can therefore be on the heap.

Created http://crbug.com/378229 - might well be within short reach to move the
player onto the heap, let's see.

Updated the patch & going ahead with this one for now.

[sof, 2014-05-28 11:28:56]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-28 11:29:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/303593002/40001

[commit-bot: I haz the power, 2014-05-28 12:22:14]

Change committed as 174964