Oilpan: have media element prefinalizer handle all finalization.

Oilpan: have media element prefinalizer handle all finalization.

It is unsafe to access a to-be-finalized object, which is a
possibility for media elements with lazy sweeping enabled.
The access in that case being made by the embedder media
player updating and notifying the media element.

Prevent that from happening by promptly/eagerly releasing
the player object.

With prefinalizers now being able to allocate, extend r197339 and
finalize the media player and perform the other Oilpan finalization
steps during media element prefinalization.

R=haraken
BUG=502863
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197628

[sof, 2015-06-22 13:05:02]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-06-22 13:05:03]

please take a look.

[haraken, 2015-06-22 13:25:39]

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

[haraken, 2015-06-22 13:25:40]

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

https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:460: if (m_closeMediaSourceWhenFinalizing)

Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we can
unconditionally call closeMediaSource() since the MediaSource is guaranteed to
be alive at this point (during the finalizer).

[sof, 2015-06-22 13:31:56]

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

https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:460: if (m_closeMediaSourceWhenFinalizing)
On 2015/06/22 13:25:40, haraken wrote:
> 
> Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we can
> unconditionally call closeMediaSource() since the MediaSource is guaranteed to
> be alive at this point (during the finalizer).

I would prefer not to, as it would call MediaSource::close() on a dying object.

[haraken, 2015-06-22 13:43:36]

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

https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:460: if (m_closeMediaSourceWhenFinalizing)
On 2015/06/22 13:31:56, sof wrote:
> On 2015/06/22 13:25:40, haraken wrote:
> > 
> > Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we can
> > unconditionally call closeMediaSource() since the MediaSource is guaranteed
to
> > be alive at this point (during the finalizer).
> 
> I would prefer not to, as it would call MediaSource::close() on a dying
object.

Makes sense; Then would it be a bit tidier to directly check
isHeapObjectAlive(m_mediaSource) and decide to call closeMediaSource or not?

[sof, 2015-06-22 13:53:31]

On 2015/06/22 13:43:36, haraken wrote:
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> Source/core/html/HTMLMediaElement.cpp:460: if
(m_closeMediaSourceWhenFinalizing)
> On 2015/06/22 13:31:56, sof wrote:
> > On 2015/06/22 13:25:40, haraken wrote:
> > > 
> > > Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we can
> > > unconditionally call closeMediaSource() since the MediaSource is
guaranteed
> to
> > > be alive at this point (during the finalizer).
> > 
> > I would prefer not to, as it would call MediaSource::close() on a dying
> object.
> 
> Makes sense; Then would it be a bit tidier to directly check
> isHeapObjectAlive(m_mediaSource) and decide to call closeMediaSource or not?

Not sure it would be vastly tidier; the MediaSource still needs to keep a weak
reference & callback.

[haraken, 2015-06-22 14:10:53]

On 2015/06/22 13:53:31, sof wrote:
> On 2015/06/22 13:43:36, haraken wrote:
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > File Source/core/html/HTMLMediaElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > Source/core/html/HTMLMediaElement.cpp:460: if
> (m_closeMediaSourceWhenFinalizing)
> > On 2015/06/22 13:31:56, sof wrote:
> > > On 2015/06/22 13:25:40, haraken wrote:
> > > > 
> > > > Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we can
> > > > unconditionally call closeMediaSource() since the MediaSource is
> guaranteed
> > to
> > > > be alive at this point (during the finalizer).
> > > 
> > > I would prefer not to, as it would call MediaSource::close() on a dying
> > object.
> > 
> > Makes sense; Then would it be a bit tidier to directly check
> > isHeapObjectAlive(m_mediaSource) and decide to call closeMediaSource or not?
> 
> Not sure it would be vastly tidier; the MediaSource still needs to keep a weak
> reference & callback.

Why can't we use default weak processing for the MediaSource::m_attachedElement?
I guess we can do something like:

class MediaSource {
  WeakMember<HTMLMediaElement> m_attachedElement;  // default weak processing
};

class HTMLMediaElement {
  void dispose() {
    if (!isHeapObjectAlive(m_mediaSource))
      m_mediaSource = nullptr;
    clearMediaSource();
  }
};

[sof, 2015-06-22 14:18:24]

On 2015/06/22 14:10:53, haraken wrote:
> On 2015/06/22 13:53:31, sof wrote:
> > On 2015/06/22 13:43:36, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > > File Source/core/html/HTMLMediaElement.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > > Source/core/html/HTMLMediaElement.cpp:460: if
> > (m_closeMediaSourceWhenFinalizing)
> > > On 2015/06/22 13:31:56, sof wrote:
> > > > On 2015/06/22 13:25:40, haraken wrote:
> > > > > 
> > > > > Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we can
> > > > > unconditionally call closeMediaSource() since the MediaSource is
> > guaranteed
> > > to
> > > > > be alive at this point (during the finalizer).
> > > > 
> > > > I would prefer not to, as it would call MediaSource::close() on a dying
> > > object.
> > > 
> > > Makes sense; Then would it be a bit tidier to directly check
> > > isHeapObjectAlive(m_mediaSource) and decide to call closeMediaSource or
not?
> > 
> > Not sure it would be vastly tidier; the MediaSource still needs to keep a
weak
> > reference & callback.
> 
> Why can't we use default weak processing for the
MediaSource::m_attachedElement?

You can, but it still involves a callback, just pre-packaged.

Anyway, will take care of this to make some progress here.

[haraken, 2015-06-22 14:19:48]

On 2015/06/22 14:18:24, sof wrote:
> On 2015/06/22 14:10:53, haraken wrote:
> > On 2015/06/22 13:53:31, sof wrote:
> > > On 2015/06/22 13:43:36, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > > > File Source/core/html/HTMLMediaElement.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > > > Source/core/html/HTMLMediaElement.cpp:460: if
> > > (m_closeMediaSourceWhenFinalizing)
> > > > On 2015/06/22 13:31:56, sof wrote:
> > > > > On 2015/06/22 13:25:40, haraken wrote:
> > > > > > 
> > > > > > Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we can
> > > > > > unconditionally call closeMediaSource() since the MediaSource is
> > > guaranteed
> > > > to
> > > > > > be alive at this point (during the finalizer).
> > > > > 
> > > > > I would prefer not to, as it would call MediaSource::close() on a
dying
> > > > object.
> > > > 
> > > > Makes sense; Then would it be a bit tidier to directly check
> > > > isHeapObjectAlive(m_mediaSource) and decide to call closeMediaSource or
> not?
> > > 
> > > Not sure it would be vastly tidier; the MediaSource still needs to keep a
> weak
> > > reference & callback.
> > 
> > Why can't we use default weak processing for the
> MediaSource::m_attachedElement?
> 
> You can, but it still involves a callback, just pre-packaged.
> 
> Anyway, will take care of this to make some progress here.

ok, LGTM.

[sof, 2015-06-22 15:03:35]

On 2015/06/22 14:19:48, haraken wrote:
> On 2015/06/22 14:18:24, sof wrote:
> > On 2015/06/22 14:10:53, haraken wrote:
> > > On 2015/06/22 13:53:31, sof wrote:
> > > > On 2015/06/22 13:43:36, haraken wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > > > > File Source/core/html/HTMLMediaElement.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > > > > Source/core/html/HTMLMediaElement.cpp:460: if
> > > > (m_closeMediaSourceWhenFinalizing)
> > > > > On 2015/06/22 13:31:56, sof wrote:
> > > > > > On 2015/06/22 13:25:40, haraken wrote:
> > > > > > > 
> > > > > > > Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we
can
> > > > > > > unconditionally call closeMediaSource() since the MediaSource is
> > > > guaranteed
> > > > > to
> > > > > > > be alive at this point (during the finalizer).
> > > > > > 
> > > > > > I would prefer not to, as it would call MediaSource::close() on a
> dying
> > > > > object.
> > > > > 
> > > > > Makes sense; Then would it be a bit tidier to directly check
> > > > > isHeapObjectAlive(m_mediaSource) and decide to call closeMediaSource
or
> > not?
> > > > 
> > > > Not sure it would be vastly tidier; the MediaSource still needs to keep
a
> > weak
> > > > reference & callback.
> > > 
> > > Why can't we use default weak processing for the
> > MediaSource::m_attachedElement?
> > 
> > You can, but it still involves a callback, just pre-packaged.
> > 
> > Anyway, will take care of this to make some progress here.
> 
> ok, LGTM.

done, i believe, but it'll have to run past the bots.

[haraken, 2015-06-22 15:14:48]

On 2015/06/22 15:03:35, sof wrote:
> On 2015/06/22 14:19:48, haraken wrote:
> > On 2015/06/22 14:18:24, sof wrote:
> > > On 2015/06/22 14:10:53, haraken wrote:
> > > > On 2015/06/22 13:53:31, sof wrote:
> > > > > On 2015/06/22 13:43:36, haraken wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > > > > > File Source/core/html/HTMLMediaElement.cpp (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1193383002/diff/1/Source/core/html/HTMLMediaE...
> > > > > > Source/core/html/HTMLMediaElement.cpp:460: if
> > > > > (m_closeMediaSourceWhenFinalizing)
> > > > > > On 2015/06/22 13:31:56, sof wrote:
> > > > > > > On 2015/06/22 13:25:40, haraken wrote:
> > > > > > > > 
> > > > > > > > Maybe can we remove m_closeMediaSourceWhenFinalizing? I guess we
> can
> > > > > > > > unconditionally call closeMediaSource() since the MediaSource is
> > > > > guaranteed
> > > > > > to
> > > > > > > > be alive at this point (during the finalizer).
> > > > > > > 
> > > > > > > I would prefer not to, as it would call MediaSource::close() on a
> > dying
> > > > > > object.
> > > > > > 
> > > > > > Makes sense; Then would it be a bit tidier to directly check
> > > > > > isHeapObjectAlive(m_mediaSource) and decide to call closeMediaSource
> or
> > > not?
> > > > > 
> > > > > Not sure it would be vastly tidier; the MediaSource still needs to
keep
> a
> > > weak
> > > > > reference & callback.
> > > > 
> > > > Why can't we use default weak processing for the
> > > MediaSource::m_attachedElement?
> > > 
> > > You can, but it still involves a callback, just pre-packaged.
> > > 
> > > Anyway, will take care of this to make some progress here.
> > 
> > ok, LGTM.
> 
> done, i believe, but it'll have to run past the bots.

Thanks, still LGTM

[sof, 2015-06-23 06:12:38]

Have looked through the results; noisier than wanted, but have triangulated with
other builds on the same bots, and this looks fine.

Sending it along.

[sof, 2015-06-23 06:12:44]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-23 06:12:55]

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

[commit-bot: I haz the power, 2015-06-23 06:16:36]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197628