Oilpan: have the media element safely close its MediaSource.

Oilpan: have the media element safely close its MediaSource.

The media element must notify its attached MediaSource upon being
finalized, as the MediaSource must then transition to a closed state
and fire the corresponding event. Hence, upon media element finalization,
it will conditionally notify its MediaSource (if any) object if the
latter survived the GC. If neither survived, an explicit close isn't
needed.

With that in place, the media element no longer keeps a RefPtr to its
MediaSource (with Oilpan enabled.) Consequently, ref-forwarding over
HTMLMediaSource is no longer needed nor supported.

R=haraken@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177532

[sof, 2014-07-02 19:47:58]

Please take a look.

As requested, tidy up the relationship between HTMLMediaElement and
(HTML)MediaSource with Oilpan enabled.

fwiw, I like the use of a weak callback here.

[haraken, 2014-07-03 03:25:37]

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:269:
m_attachedElement->setCloseMediaSourceWhenFinalizing();

Don't you need to clear m_attachedElement here?

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:269:
m_attachedElement->setCloseMediaSourceWhenFinalizing();

By the way, wouldn't it be possible to call closeMediaSource() here instead of
setting the flag and let the HTMLMediaElement's destructor call
closeMediaSource()?

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:109: removedFromMediaSource();

Just help me understand: What is this change for?

[sof, 2014-07-03 06:22:24]

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:269:
m_attachedElement->setCloseMediaSourceWhenFinalizing();
On 2014/07/03 03:25:37, haraken wrote:
> 
> By the way, wouldn't it be possible to call closeMediaSource() here instead of
> setting the flag and let the HTMLMediaElement's destructor call
> closeMediaSource()?

That will bring about the allocation of an Event, which we're not allowed inside
weak callbacks, right?

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:269:
m_attachedElement->setCloseMediaSourceWhenFinalizing();
On 2014/07/03 03:25:37, haraken wrote:
> 
> Don't you need to clear m_attachedElement here?

The close() will do that, so I didn't duplicate it here.

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:109: removedFromMediaSource();
On 2014/07/03 03:25:37, haraken wrote:
> 
> Just help me understand: What is this change for?

If MediaSource and HTMLMediaElement die at the same time, then MediaSource will
not be explicitly closed. This takes care of the lack of such wrt reset'ing the
MediaSource's SourceBuffers.

[haraken, 2014-07-03 06:40:31]

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:269:
m_attachedElement->setCloseMediaSourceWhenFinalizing();
On 2014/07/03 06:22:24, sof wrote:
> On 2014/07/03 03:25:37, haraken wrote:
> > 
> > By the way, wouldn't it be possible to call closeMediaSource() here instead
of
> > setting the flag and let the HTMLMediaElement's destructor call
> > closeMediaSource()?
> 
> That will bring about the allocation of an Event, which we're not allowed
inside
> weak callbacks, right?

Makes sense.

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:269:
m_attachedElement->setCloseMediaSourceWhenFinalizing();
On 2014/07/03 06:22:24, sof wrote:
> On 2014/07/03 03:25:37, haraken wrote:
> > 
> > Don't you need to clear m_attachedElement here?
> 
> The close() will do that, so I didn't duplicate it here.

ah, got it; I'd prefer explicitly clearing out m_attachedElement here as well
though.

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:109: removedFromMediaSource();
On 2014/07/03 06:22:24, sof wrote:
> On 2014/07/03 03:25:37, haraken wrote:
> > 
> > Just help me understand: What is this change for?
> 
> If MediaSource and HTMLMediaElement die at the same time, then MediaSource
will
> not be explicitly closed. This takes care of the lack of such wrt reset'ing
the
> MediaSource's SourceBuffers.

Hmm, in my understanding, SourceBuffers are owned by the MediaSource, so the
SourceBuffers always die with the MediaSource. Then we don't need to call
removedFromMediaSource() in ~SourceBuffer().

[sof, 2014-07-03 06:52:25]

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:109: removedFromMediaSource();
On 2014/07/03 06:40:31, haraken wrote:
> On 2014/07/03 06:22:24, sof wrote:
> > On 2014/07/03 03:25:37, haraken wrote:
> > > 
> > > Just help me understand: What is this change for?
> > 
> > If MediaSource and HTMLMediaElement die at the same time, then MediaSource
> will
> > not be explicitly closed. This takes care of the lack of such wrt reset'ing
> the
> > MediaSource's SourceBuffers.
> 
> Hmm, in my understanding, SourceBuffers are owned by the MediaSource, so the
> SourceBuffers always die with the MediaSource. Then we don't need to call
> removedFromMediaSource() in ~SourceBuffer().

That matches my understanding of ownership (via SourceBufferList.) But close()
(via onReadyStateChange()) will explicitly call removeFromMediaSource() on them,
so if it doesn't, someone else will.

Hmm, looking at all that SourceBuffer::abortIfUpdating() will potentially do,
that's too much - no need to fire events, for instance. Will address that.

[sof, 2014-07-03 08:22:48]

On 2014/07/03 06:52:25, sof wrote:
>
https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
> File Source/modules/mediasource/SourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasour...
> Source/modules/mediasource/SourceBuffer.cpp:109: removedFromMediaSource();
> On 2014/07/03 06:40:31, haraken wrote:
> > On 2014/07/03 06:22:24, sof wrote:
> > > On 2014/07/03 03:25:37, haraken wrote:
> > > > 
> > > > Just help me understand: What is this change for?
> > > 
> > > If MediaSource and HTMLMediaElement die at the same time, then MediaSource
> > will
> > > not be explicitly closed. This takes care of the lack of such wrt
reset'ing
> > the
> > > MediaSource's SourceBuffers.
> > 
> > Hmm, in my understanding, SourceBuffers are owned by the MediaSource, so the
> > SourceBuffers always die with the MediaSource. Then we don't need to call
> > removedFromMediaSource() in ~SourceBuffer().
> 
> That matches my understanding of ownership (via SourceBufferList.) But close()
> (via onReadyStateChange()) will explicitly call removeFromMediaSource() on
them,
> so if it doesn't, someone else will.
> 
> Hmm, looking at all that SourceBuffer::abortIfUpdating() will potentially do,
> that's too much - no need to fire events, for instance. Will address that.

Done now, thanks for pushing back on the initial change. Going through the
actions performed by removedFromMediaSource(), they're not needed when
finalizing in the Oilpan case.

We do need to disable the asserts only - left a comment trying to explain why.

[haraken, 2014-07-03 09:46:05]

+acolwell

LGTM

https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:267: // notify its MediaSource during
finalization by calling close().

Also let's add a comment that m_attachedElement is cleared when the
HTMLMediaElement is destructed (or simply add code to clear m_attachedElement
here).

[sof, 2014-07-03 09:49:00]

https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:267: // notify its MediaSource during
finalization by calling close().
On 2014/07/03 09:46:05, haraken wrote:
> 
> Also let's add a comment that m_attachedElement is cleared when the
> HTMLMediaElement is destructed (or simply add code to clear m_attachedElement
> here).

Oh, I'm sorry - I merely forgot to follow up on your earlier suggestion. I agree
that we should clear it in the weak callback; will do.

[sof, 2014-07-03 09:59:18]

On 2014/07/03 09:49:00, sof wrote:
>
https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasour...
> File Source/modules/mediasource/MediaSource.cpp (right):
> 
>
https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasour...
> Source/modules/mediasource/MediaSource.cpp:267: // notify its MediaSource
during
> finalization by calling close().
> On 2014/07/03 09:46:05, haraken wrote:
> > 
> > Also let's add a comment that m_attachedElement is cleared when the
> > HTMLMediaElement is destructed (or simply add code to clear
m_attachedElement
> > here).
> 
> Oh, I'm sorry - I merely forgot to follow up on your earlier suggestion. I
agree
> that we should clear it in the weak callback; will do.

Done now. I will wait overnight for acolwell's feedback.

[sof, 2014-07-04 09:37:59]

Sending this one along; will ofc followup should there be later arriving
feedback.

[sof, 2014-07-04 09:38:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-04 09:38:37]

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

[commit-bot: I haz the power, 2014-07-04 10:23:45]

Change committed as 177532