633051 - Video controls break if you go back to a page containing a video document

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

STR:

1. Open <http://mozillalabs.com/prospector/2011/02/09/time-to-stop-managing-tabs/>.
2. Click on the link with the title "video" so that you'll be redirected to this page: <http://videos-cdn.mozilla.net/labs/prospector/homeDashContext.webm> which is a video document.
3. Click play.
4. Go back.
5. Go forward.

The video will stop playing.  Clicking on the play button doesn't do anything.  Clicking on the mute button doesn't do anything.  The volume slider seems to work, but I can't make sure, since the video is not playing.  The space button on the keyboard doesn't work.

If you use the context menu items, they will toggle their menu (for example, Play changes to Pause and vice versa) but none of them work.  Basically, the video would be useless until you reload.

Comment 1

[Matthew Gregan [:kinetik]]

I couldn't reproduce this at first, but once I waited for the video to download completely it was possible to reproduce.  It sounds very similar to bug 630761, except the reporter claims that happens on live (unending) streams.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> but once I waited for the video to download completely it was possible to
> reproduce.

Does this only happen if the page comes from bfcache, then?

Comment 3

[Matthew Gregan [:kinetik]]

Yes, it looks like when nsHTMLMediaElement::NotifyOwnerDocumentActivityChanged() is called as we're restoring the video document, ownerDoc->IsVisible() returns false so we don't unpause and unsuspend the decoder.

Comment 4

[(no longer active)]

(In reply to comment #2)
> > but once I waited for the video to download completely it was possible to
> > reproduce.
> 
> Does this only happen if the page comes from bfcache, then?

This matches what I experienced, yes (although I have not debugged this).

Comment 5

[Matthew Gregan [:kinetik]]

The problem is that nsDocLoad::OnStopRequest never fires.  This happens because nsLoadGroup::RemoveRequest (called from nsDocShell::FinishRestore) doesn't call the listener's OnStopRequest when the request has the flag LOAD_BACKGROUND.  The media request acquires this flag in nsMediaStream::MoveLoadsToBackground when nsHTMLMEdiaElement::FirstFrameLoaded runs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Do you mean that nsDocLoader::OnStopRequest never fires?

Are we ending up in a situation where there is only one load in flight and it's LOAD_BACKGROUND and we haven't caled nsDocLoader::OnStopRequest yet for some reason?

Comment 7

[Matthew Gregan [:kinetik]]

Yeah, nsDocLoad is a typo.  There's only one request added (and removed) from the nsLoadGroup when restoring the video document from session history.  Since it's LOAD_BACKGROUND, the block at http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsLoadGroup.cpp#670 is skipped, which is where nsDocLoader::OnStopRequest would usually be called from.

Presumably the fix is to avoid marking media requests LOAD_BACKGROUND when they originate from a video document.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why are we adding/removing anything at all?  Or are we adding/removing the document channel in this case (and this is the fake add/remove that docshell does when restoring)?

Comment 9

[Matthew Gregan [:kinetik]]

It's the document channel in the fake add/remove case, the remove is here: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6817

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, ok.

So what we should probably do in the media code is to unset the LOAD_BACKGROUND flag on the channel when it sends OnStopRequest or something like that (possibly async?).  That should do the right thing, I think...

If we don't set LOAD_BACKGROUND in the fullpage media case, then the throbber will keep spinning until the media loads fully, which is not what we want.

Maybe we need a more than just two states (background/foreground) in necko...

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Remove LOAD_BACKGROUND from the media request when OnStopRequest fires.

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4f3ad7dc6839

Comment 14

[u279076]

Cannot reproduce with Firefox 4.0b12pre 20110218.