Closed Bug 1572798 Opened 5 years ago Closed 4 years ago

Video element created using document.open() doesn't work

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Version:
67 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox-esr78 --- wontfix
firefox84 --- wontfix
firefox85 --- wontfix
firefox86 --- fixed

People

(Reporter: dtgjyhdty, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

document.open() that creates a video element
661 bytes, text/html
Details
Bug 1572798 - Should call MaybeActiveMediaComponents from SetScriptGlobalObject if becoming visible.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1572798 - Call MaybeActiveMediaComponents only if the visibility state changed.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1572798 - Make content browsing contexts for tabs start as inactive. r=nika
47 bytes, text/x-phabricator-request
Details | Review
Updated test-case
659 bytes, text/html
Details

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  1. Open the attached test.html in Firefox.
  2. Click on the Open video link.
  3. Click play to play the video.
    Note: I used some sample video I found, but it's reproducible with any video file.

Actual results:

The video doesn't play.

Expected results:

The video should play.

It worked fine until Firefox 67. I ran mozregression to find the regression and got the following:
2019-08-09T20:00:46: INFO : Narrowed inbound regression window from [2624b1bd, 03d6fa61] (3 builds) to [2624b1bd, a01586b6] (2 builds) (~1 steps left)
2019-08-09T20:00:47: DEBUG : Starting merge handling...
2019-08-09T20:00:47: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=a01586b62cf510bb165057e0bea9a45cc76e961e&full=1
2019-08-09T20:00:47: DEBUG : Found commit message:
Bug 1489308 part 10. Remove some document.open handling in outer window that's no longer needed. r=mccr8
Differential Revision: https://phabricator.services.mozilla.com/D18077

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Audio/Video: Playback → DOM: Core & HTML
OS: Unspecified → All
Regressed by: 1489308
Hardware: Unspecified → All

It works for me on Nightly, does it work for you? Thanks so much for the regression range and filing btw :)

Flags: needinfo?(bzbarsky)

Ok it doesn't work on 68, I'll find a fix range before bothering Boris ;)

Flags: needinfo?(bzbarsky)

Oh, so I can repro on a clean profile but it works on my regular profile, strange...

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

It doesn't work for me in Nightly. Same result with my regular profile and a clean profile.

I can also reproduce the issue on Nightly70.0a1 Windows10.
And I found a strange thing, once after turn to video fullscreen, the video is playing as expected.

We're hitting this code here...

I'll take a quick look before heading out for the weekend.

Assignee: nobody → emilio

I bet this is not expected:

(rr) p mWindow
$6 = [(nsGlobalWindowOuter *) 0x7f20cb3379a0]
(rr) up
#1  0x00007f20e6a3a044 in mozilla::dom::HTMLMediaElement::AudioChannelAgentCallback::NotifyAudioChannelAgent (this=0x7f20cb473900, aPlaying=true) at /home/emilio/src/moz/gecko-3/dom/html/HTMLMediaElement.cpp:1193
#2  0x00007f20e6a279fc in mozilla::dom::HTMLMediaElement::AudioChannelAgentCallback::UpdateAudioChannelPlayingState (this=0x7f20cb473900, aForcePlaying=true) at /home/emilio/src/moz/gecko-3/dom/html/HTMLMediaElement.cpp:1006
#3  0x00007f20e6a24fc2 in mozilla::dom::HTMLMediaElement::AudioChannelAgentCallback::IsPlaybackBlocked (this=0x7f20cb473900) at /home/emilio/src/moz/gecko-3/dom/html/HTMLMediaElement.cpp:1137
#4  0x00007f20e69f9ea3 in mozilla::dom::HTMLMediaElement::AudioChannelAgentDelayingPlayback (this=0x7f20ccedf000) at /home/emilio/src/moz/gecko-3/dom/html/HTMLMediaElement.cpp:3781
(rr) p OwnerDoc()->GetWindow()
$7 = (nsGlobalWindowOuter *) 0x7f20cb337980

Hmm, or maybe it is.

I think I got a fix.

This was a latent bug.

Windows start blocking media by default (see the
media.block-autoplay-until-in-foreground pref). If the document becomes
visible from GetScriptHandlingObject(), we hand-rolled our own
UpdateVisibilityState and forgot to call MaybeActiveMediaComponents (which
unblocks media playback).

I haven't checked with a pre-regression build, but given how the code is
structured, probably a following call to UpdateVisibilityState wallpapered it
(since MaybeActiveMediaComponents is called even when the visibility state has
not changed, which looks a bit suspect).

I'm not 100% sure how to add an automated test for this, but it ought to be
possible, so I can look into it sometime next week.

All sets of mVisibilityState go through this function now, and not having a
window implies that the visibility state must be hidden, so this should be sound
now.

I'll land this separately anyway since it's slightly more risky.

So my patch here causes some tests to fail. This is because the way we create browser tabs works like:

  • We start with nsDocShell::mIsActive = true and nsWebBrowser::mIsActive = true.
  • Then we create an about:blank content-viewer. We end up in SetScriptGlobalObject, and determine that the document is visible.
  • Then we receive SetDocShellIsActive(false) and propagate as needed, but by the time that happens we've already unblocked autoplay.

Boris, should we start with nsDocShell::mIsActive = false (presumably along with nsWebBrowser::mIsActive)? That looks like a p. scary change...

Ideally we should be able to know from the start whether a browser is active or not, I'd think...

Flags: needinfo?(bzbarsky)

That would be ideal; one issue is that the active state is known in the parent process, but the docshell is created in the child process. So we'd need to communicate the active state as part of docshell creation. We can do that, and if we restrict it to just <browser>-created docshells it might not even be too scary. We'd need to do something about the current <browser> docShellISActive API,which only works if there is already a docshell; some other way of storing the active state before the docshell is created would be needed.

Is the test issue that tests are then testing the autoplay state of the initial about:blank document, and not doing any loads after the SetDocShellIsActive(false) call?

Flags: needinfo?(bzbarsky)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #14)

Is the test issue that tests are then testing the autoplay state of the initial about:blank document, and not doing any loads after the SetDocShellIsActive(false) call?

No, the issue is that once we unblock the media playback, we never re-consider that state. Thus by loading the initial about:blank document we always unblock media playback forever, regardless of any other later loads in the same nsGlobalWindowOuter.

Wait, we store the unblocked state on the nsGlobalWindowOuter? That seems pretty odd in itself...

Yes, that does sound odd, I guess it should be a per document thing really...

Priority: -- → P2

Media blocking wants to autoplay stuff in a window as soon as any document in
the window becomes visible (where visible depends on
Document::mVisibilityState).

Having content docshells start as active means that if the initial about:blank
load happens before chrome has fixed-up the active-ness, it may be considered
"active", and thus we'll update autoplay for the whole session.

In order to prevent that, right now we don't unblock autoplay from
Document::SetScriptGlobalObject1, but that may actually be the only time we
can actually update the visibility, and thus it can cause correctness issues
like this bug.

Chrome code already seems to activate docshells and such properly after
creation, so this only changes the default and prevents us from being in the
active state unnecessarily before chrome code fixes it up.

Media blocking wants to autoplay stuff in a window as soon as any document in
the window becomes visible (where visible depends on
Document::mVisibilityState).

Having content docshells start as active means that if the initial about:blank
load happens before chrome has fixed-up the active-ness, it may be considered
"active", and thus we'll update autoplay for the whole session.

In order to prevent that, right now we don't unblock autoplay from
Document::SetScriptGlobalObject1, but that may actually be the only time we
can actually update the visibility, and thus it can cause correctness issues
like this bug.

Chrome code already seems to activate docshells and such properly after
creation, so this only changes the default and prevents us from being in the
active state unnecessarily before chrome code fixes it up.

Attached file Bug 1572798 - Test. (obsolete) —

This test times out without the previous patches.

Attachment #9086089 - Attachment is obsolete: true

Emilio, you mentioned the media.block-autoplay-until-in-foreground pref. If I change it to false, it does resolve the issue. But do you think I can implement some kind of workaround (until your fix is ready) to make it work without changing the pref?

Flags: needinfo?(emilio)
Blocks: 1582042

Yes, any kind of other load that isn't the initial about:blank (like opening a same-origin empty file or something) should work.

Depends on: 1578379
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
See Also: → 1669736

It looks like it was fixed in Firefox 83 or an earlier version. It doesn't happen anymore with the same code I used when I submitted this bug.

Attached file Updated test-case —
Flags: needinfo?(emilio)

So bug 1643204 fixed this, but it's a bit of a wallpaper, because we still have an inconsistent media vs. visibilitystate until the document.open load finishes...

Matt

Depends on: 1643204

Err, Matt, wanted to ask... it is a bit weird that bug 1643204 makes us fire pageshow but not load.

The reason firing PageShow also works is because pageshow also calls UpdateVisibilityState, which will have the side effect of calling MaybeActiveMediaComponents.

Only Gecko seems to pass the test that you added on that bug: https://wpt.fyi/results/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/pageshow-event.window.html

Should I land https://phabricator.services.mozilla.com/D41438 (which is, I think, a better fix) and back out bug 1643204?

Flags: needinfo?(matt.woodrow)
Attachment #9086090 - Attachment is obsolete: true
Attachment #9086092 - Attachment is obsolete: true
Blocks: 1685201

Moved this question to bug 1685201.

Flags: needinfo?(matt.woodrow)

And for context: I couldn't land this at the time because of issues with the docshell active flag which should be resolved with bug 1635914.

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4de429b6ad64 Should call MaybeActiveMediaComponents from SetScriptGlobalObject if becoming visible. r=bzbarsky,farre
See Also: → 1685163

Hi, emilio, I just wonder if you have any plan to land those patches again? Thank you!

I need to look into the failures and such. If you have the time to dig it'd be lovely.

Sure, I can take a look. Set NI to myself.

Flags: needinfo?(alwu)
Attachment #9086090 - Attachment description: Bug 1572798 - Make content docshells start as inactive. → Bug 1572798 - Make content browsing contexts start as inactive. r=farre
Attachment #9086090 - Attachment is obsolete: false
Attachment #9086090 - Attachment description: Bug 1572798 - Make content browsing contexts start as inactive. r=farre → Bug 1572798 - Make content browsing contexts for tabs start as inactive. r=nika
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f2d3a6800638 Make content browsing contexts for tabs start as inactive. r=nika
Flags: needinfo?(emilio)
Keywords: leave-open
Flags: needinfo?(emilio)

Sorry I have been busy on other bugs these days so didn't get time to debug the backout. But seems you already found the reason and got the new solution, so clear NI.

Flags: needinfo?(alwu)
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/cfdebb863454 Explicitly check for top contexts because devtools creates it.
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/975f6057245a Make content browsing contexts for tabs start as inactive. r=nika
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d9d973f94b15 Make content browsing contexts for tabs start as inactive. r=nika
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/35fa43e126e5 Should call MaybeActiveMediaComponents from SetScriptGlobalObject if becoming visible. r=bzbarsky,farre

Should leave-open still be set? I think https://phabricator.services.mozilla.com/D41439 still needs to land.

All sets of mVisibilityState go through this function now, and not
having a window implies that the visibility state must be hidden, so
this should be sound now.

I'll land this separately anyway since it's slightly more risky.

Attachment #9198843 - Attachment is obsolete: true
Keywords: leave-open

(In reply to Mathew Hodson from comment #44)

Should leave-open still be set? I think https://phabricator.services.mozilla.com/D41439 still needs to land.

Yep, I had clicked the land button a while ago, but it needed a rebase, thanks for the ping! :)

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90549a9217d3 Call MaybeActiveMediaComponents only if the visibility state changed. r=bzbarsky,farre

https://hg.mozilla.org/mozilla-central/rev/90549a9217d3

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox86: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: