Closed
Bug 633051
Opened 14 years ago
Closed 14 years ago
Video controls break if you go back to a page containing a video document
Categories
(Core :: Audio/Video, defect)
Tracking
()
VERIFIED
FIXED
mozilla2.0b12
People
(Reporter: ehsan.akhgari, Assigned: kinetik)
References
Details
Attachments
(1 file)
(deleted),
patch
|
roc
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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•14 years ago
|
||
> 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?
Assignee | ||
Comment 3•14 years ago
|
||
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.
Reporter | ||
Comment 4•14 years ago
|
||
(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 | ||
Updated•14 years ago
|
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•14 years ago
|
||
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•14 years ago
|
||
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?
Assignee | ||
Comment 7•14 years ago
|
||
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•14 years ago
|
||
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)?
Assignee | ||
Comment 9•14 years ago
|
||
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•14 years ago
|
||
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...
Assignee | ||
Comment 11•14 years ago
|
||
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+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs landing]
Reporter | ||
Updated•14 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 13•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b12
Comment 14•14 years ago
|
||
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.
Description
•