Closed Bug 633051 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b12

People

(Reporter: ehsan, Assigned: kinetik)

References

Details

Attachments

(1 file)

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.
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.
> 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?
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.
(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).
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
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.
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?
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.
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)?
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
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...
Attached patch patch v0Splinter Review
Remove LOAD_BACKGROUND from the media request when OnStopRequest fires.
Attachment #512311 - Flags: review?(roc)
Attachment #512311 - Flags: approval2.0?
Attachment #512311 - Flags: review?(roc)
Attachment #512311 - Flags: review+
Attachment #512311 - Flags: approval2.0?
Attachment #512311 - Flags: approval2.0+
Whiteboard: [needs landing]
Duplicate of this bug: 622248
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4f3ad7dc6839
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b12
Cannot reproduce with Firefox 4.0b12pre 20110218.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.