Bug 99647 – HTMLMediaPlayer should free m_player when src is set/changed

• Component: New Bugs
• Status: RESOLVED FIXED

[Ami Fischman]

HTMLMediaPlayer should free m_player on error.

[Ami Fischman]

Created attachment 169278 [details]
Patch

[Ami Fischman]

FTR, this issue was raised in http://crbug.com/154546
(because chrome enjoys spawning several threads per MediaPlayer, making delaying their teardown an expensive proposition).

[WebKit Review Bot]

Comment on attachment 169278 [details]
Patch

Attachment 169278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14395756

New failing tests:
compositing/geometry/limit-layer-bounds-transformed.html
platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-clipping-ancestor.html
platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-positioned.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/limit-layer-bounds-positioned.html
platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-overflow-root.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-transformed.html
platform/chromium/virtual/softwarecompositing/layer-creation/scroll-partial-update.html
compositing/geometry/limit-layer-bounds-overflow-root.html
platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-positioned-transition.html
platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/layer-creation/scroll-partial-update.html

[Ami Fischman]

Comment on attachment 169278 [details]
Patch

(bot failures are clearly unrelated)

[Eric Carlson]

Comment on attachment 169278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169278&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:4324
> +    if (event->type() == eventNames().errorEvent)
> +        m_player = nullptr;

An error event doesn't mean that the media player doesn't have any valid data, eg. losing the connection to the server will fire an error event with some media engines. This will change will *always* delete the media player and lose all state.

[Ami Fischman]

(In reply to comment #5)
> (From update of attachment 169278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169278&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4324
> > +    if (event->type() == eventNames().errorEvent)
> > +        m_player = nullptr;
> 
> An error event doesn't mean that the media player doesn't have any valid data, eg. losing the connection to the server will fire an error event with some media engines. This will change will *always* delete the media player and lose all state.

I see.  Do you have any suggestions for where the right place to put this is?
(the goal, other than "real" error handling, is to be able to know that setting mediaElement.src="" in JS means resources held by any previously-playing engine underlying mediaElement have been released).

[Eric Carlson]

(In reply to comment #6)
> 
> I see.  Do you have any suggestions for where the right place to put this is?
> (the goal, other than "real" error handling, is to be able to know that setting mediaElement.src="" in JS means resources held by any previously-playing engine underlying mediaElement have been released).

Only clearing it when m_readyState < HAVE_METADATA should be safe, except that when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is defined you should *never* delete it.

You might also try clearing it in HTMLMediaElement::parseAttribute when when the src attribute is set to "", but please check the spec first to see if it has anything specific to say about what to do in that situation.

[Ami Fischman]

(In reply to comment #7)
> Only clearing it when m_readyState < HAVE_METADATA should be safe, except that when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is defined you should *never* delete it.

That doesn't work for me, because in the case I'm concerned with m_readyState==4.
Reading the spec, ISTM setting src invokes the resource load algorithm, which among other things resets readyState to HAVE_NOTHING, dismisses pending callbacks and algorithms, and generally starts from a clean slate.  What am I missing that says already-downloaded state should be preserved?

In fact, looking at the code a bit closer, it seems prepareForLoad() already does all this cleanup, but it is being stymied by the second clause in this test:
    if ((loadType & MediaResource) && !(m_pendingLoadFlags & MediaResource)) {
in scheduleLoad().  Commenting out the !(m_pendingLoadFlags & MediaResource) part makes the problem go away (even without the explicit clearing of m_player).  
Do you know what that part is doing?

> You might also try clearing it in HTMLMediaElement::parseAttribute when when the src attribute is set to "", but please check the spec first to see if it has anything specific to say about what to do in that situation.

It doesn't say anything about the empty string specifically, but this triggering on parseAttribute seems strange, considering the prepareForLoad() business already seems to implement the verbiage I want to rely on from the spec.

[Eric Carlson]

(In reply to comment #8)
> (In reply to comment #7)
> > Only clearing it when m_readyState < HAVE_METADATA should be safe, except that when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is defined you should *never* delete it.
> 
> That doesn't work for me, because in the case I'm concerned with m_readyState==4.

What causes the error event to be fired when readyState is HAVE_ENOUGH_DATA?

> Reading the spec, ISTM setting src invokes the resource load algorithm, which among other things resets readyState to HAVE_NOTHING, dismisses pending callbacks and algorithms, and generally starts from a clean slate.  What am I missing that says already-downloaded state should be preserved?
> 

I don't know that the spec says already downloaded state should be preserved, but it seems common sense to me. For example, I think a user would be very surprised if all media data was purged because the connection to the server was lost after downloading most of a file.

> In fact, looking at the code a bit closer, it seems prepareForLoad() already does all this cleanup, but it is being stymied by the second clause in this test:
>     if ((loadType & MediaResource) && !(m_pendingLoadFlags & MediaResource)) {
> in scheduleLoad().  Commenting out the !(m_pendingLoadFlags & MediaResource) part makes the problem go away (even without the explicit clearing of m_player).  
> Do you know what that part is doing?
> 
  scheduleLoad is called to set up loading of the media resource and text track(s) so m_pendingLoadFlags keeps track of which load has already been scheduled so we don't, e.g. set up to load the media resource and then do it again when we see that there are text tracks to load. It is also necessary to allow us to iterate through candidate <source> elements without resetting the state machine each time.

  m_pendingLoadFlags is cleared in loadTimerFired, so it *should* allow prepareForLoad to be called again when src changes. What prevents it? 

> > You might also try clearing it in HTMLMediaElement::parseAttribute when when the src attribute is set to "", but please check the spec first to see if it has anything specific to say about what to do in that situation.
> 
> It doesn't say anything about the empty string specifically, but this triggering on parseAttribute seems strange, considering the prepareForLoad() business already seems to implement the verbiage I want to rely on from the spec.

Looking at the code I see that parseAttribute already calls prepareForLoad when src is set to "" so my suggestion should be unnecessary.

[Ami Fischman]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Only clearing it when m_readyState < HAVE_METADATA should be safe, except that when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is defined you should *never* delete it.
> > That doesn't work for me, because in the case I'm concerned with m_readyState==4.
> What causes the error event to be fired when readyState is HAVE_ENOUGH_DATA?

I'm not sure what you're asking, but you can easily repro by dropping this in ManualTests/:
<html>
<head>
<script>
var v;
function handleCanplay() {
  v.src = "";
}

function go() {
  v = document.createElement('video');
  v.addEventListener('canplay', handleCanplay);
  v.controls = "controls";
  v.src = "../LayoutTests/media/content/test-25fps.mp4";
  v.load();
  document.body.appendChild(v);
}
</script>
</head>
<body onload="go()">
</body>
</html>

> > In fact, looking at the code a bit closer, it seems prepareForLoad() already does all this cleanup, but it is being stymied by the second clause in this test:
> >     if ((loadType & MediaResource) && !(m_pendingLoadFlags & MediaResource)) {
> > in scheduleLoad().  Commenting out the !(m_pendingLoadFlags & MediaResource) part makes the problem go away (even without the explicit clearing of m_player).  
> > Do you know what that part is doing?
> > 
>   scheduleLoad is called to set up loading of the media resource and text track(s) so m_pendingLoadFlags keeps track of which load has already been scheduled so we don't, e.g. set up to load the media resource and then do it again when we see that there are text tracks to load. It is also necessary to allow us to iterate through candidate <source> elements without resetting the state machine each time.

Hmm.  Sounds like m_pLF needs to be cleared earlier, but I'm not sure where.

>   m_pendingLoadFlags is cleared in loadTimerFired, so it *should* allow prepareForLoad to be called again when src changes. What prevents it? 

I'm not sure what the intended timing is, but the above test case emits this sequence of logs (with a bit of extra LOGging added, but no logic changes from ToT):

HTMLMediaElement::scheduleLoad
HTMLMediaElement::prepareForLoad
AMI: m_player being written; currently: (nil)
HTMLMediaElement::setShouldDelayLoadEvent(true)
HTMLMediaElement::prepareForLoad
AMI: m_player being written; currently: 0x7f206dbbea20
HTMLMediaElement::setReadyState(0) - current state is 0,
HTMLMediaElement::setReadyState(1) - current state is 0,
HTMLMediaElement::setReadyState(4) - current state is 1,
HTMLMediaElement::setShouldDelayLoadEvent(false)
HTMLMediaElement::scheduleLoad
AMI: HTMLMediaElement::loadTimerFired()
HTMLMediaElement::setShouldDelayLoadEvent(true)
HTMLMediaElement::setShouldDelayLoadEvent(false)
AMI: loadTimerFired clearing m_pendingLoadFlags, which was: 1

I think you're saying you expect loadTimerFired to be fired before the second scheduleLoad(), so m_pLF should be 0 by the time that second scheduleLoad fires?

> > > You might also try clearing it in HTMLMediaElement::parseAttribute when when the src attribute is set to "", but please check the spec first to see if it has anything specific to say about what to do in that situation.
> > It doesn't say anything about the empty string specifically, but this triggering on parseAttribute seems strange, considering the prepareForLoad() business already seems to implement the verbiage I want to rely on from the spec.
> Looking at the code I see that parseAttribute already calls prepareForLoad when src is set to "" so my suggestion should be unnecessary.

I'm not seeing that; what I see is selectMediaResource() handling src="" specially, but not how that leads to prepareForLoad.

[Ami Fischman]

Eric,
I went back to the spec and ISTM it says any set/change of the src attribute should drop all state from any previously-playing source:

4.8.10.2 says "If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm"
4.8.10.5 (step 8) says "Playback of any previously playing media resource for this element stops."  Coupled with step 4 of the load algo I read this to mean discard any buffered data from a previous value of src.

Given that, WDYT of simply adding dropping the player when src is set in parseAttribute?  Uploading a patch that does this for discussion.

[Ami Fischman]

Created attachment 171940 [details]
Patch

[Eric Carlson]

Comment on attachment 171940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171940&action=review

Thanks for keeping at this!

> Source/WebCore/html/HTMLMediaElement.cpp:374
> +            m_loadTimer.stop();
> +            m_progressEventTimer.stop();
> +            m_playbackProgressTimer.stop();
> +            m_pendingLoadFlags &= ~MediaResource;
> +            m_loadState = WaitingForSource;
> +#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> +            m_player.clear();
> +#endif

We do all of these in HTMLMediaElement::userCancelledLoad, although m_pendingLoadFlags is set to 0. It would be nice to have both use a shared function that took a flags mask.

[Ami Fischman]

Comment on attachment 171940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171940&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:374
>> +#endif
> 
> We do all of these in HTMLMediaElement::userCancelledLoad, although m_pendingLoadFlags is set to 0. It would be nice to have both use a shared function that took a flags mask.

Yeah, I thought about it, but wasn't sure about it.
clearMediaLoadingState() ?
Given that I only need to clear MediaResource from m_pLF (setting it to 0 here is wrong) and given uCL is already using a helper for stopping 2 of the timers (but not the third) I opted for explicitness and to leave unification to a pass that understands how these things are supposed to be related.

If that's ok w/ you, please CQ+.
If you want me to make a unification pass as part of this CL, let me know and I'll take a crack at it.

[Eric Carlson]

Comment on attachment 171940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171940&action=review

>>> Source/WebCore/html/HTMLMediaElement.cpp:374
>>> +#endif
>> 
>> We do all of these in HTMLMediaElement::userCancelledLoad, although m_pendingLoadFlags is set to 0. It would be nice to have both use a shared function that took a flags mask.
> 
> Yeah, I thought about it, but wasn't sure about it.
> clearMediaLoadingState() ?
> Given that I only need to clear MediaResource from m_pLF (setting it to 0 here is wrong) and given uCL is already using a helper for stopping 2 of the timers (but not the third) I opted for explicitness and to leave unification to a pass that understands how these things are supposed to be related.
> 
> If that's ok w/ you, please CQ+.
> If you want me to make a unification pass as part of this CL, let me know and I'll take a crack at it.

I think we might as well do it now, there is enough "fix this later" crud in HTMLMediaElement already :-)

Something like this should work:

void HTMLMediaElement::clearMediaPlayer(unsigned flags)
{
#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
    m_player.clear();
#endif
    stopPeriodicTimers();
    m_loadTimer.stop();

    m_pendingLoadFlags &= ~flags;
    m_loadState = WaitingForSource;
}

[Ami Fischman]

Created attachment 171946 [details]
Patch

[Ami Fischman]

(In reply to comment #15)
> I think we might as well do it now, there is enough "fix this later" crud in HTMLMediaElement already :-)

Mmm :)

> Something like this should work:
> void HTMLMediaElement::clearMediaPlayer(unsigned flags)

It requires a -1 trick, but done.

[Eric Carlson]

Comment on attachment 171946 [details]
Patch

Thank you!

[Build Bot]

Comment on attachment 171946 [details]
Patch

Attachment 171946 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14687664

[WebKit Review Bot]

Comment on attachment 171946 [details]
Patch

Clearing flags on attachment: 171946

Committed r133252: <http://trac.webkit.org/changeset/133252>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 101010

[Ami Fischman]

(the threatened rollout for which this bug was reopened was replaced with a forward-fix, so this was never reverted)