browser.documentURI goes out of sync when doing same-document navigation in e10s

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: whimboo, Assigned: mconley)

Tracking

({regression})

35 Branch
mozilla41
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10sm7+, firefox41 fixed)

Details

(URL)

Attachments

(5 attachments)

Today I was informed that activating Adobe Flash for the following video on photos.google.com does not work. I tested this myself on Linux and I can verify this breakage. I found that this is actually regression between Firefox 34 and 35.

As Georg told me there were e10s related click-to-play changes as done by Mike for Firefox 35 on bug 899347. It looks like that this could be the regression. I don't have the time for a full regression test, so maybe Mike can have a look and test?

Steps:

1. Load https://goo.gl/photos/3Tckx7BJn3zcK2j79
2. Click the little play icon
3. For the video click "Activate Adobe Flash"

With step 3 nothing happens, and the door hanger does not appear.

Interestingly it works when you load the video directly via 

https://photos.google.com/share/AF1QipNzDMs8_-2g4ySbfI2t--0bgQJVmWPgl-OzE5wt-nzyiFe3XNT0sausLadILRK4ug/photo/AF1QipNaiuS6IYzTDeROnW4FCkyhUSXIj44xJueu89v9?key=eVBiWVB0SkNHZUt2U3VVeUY1UnFvVWxNb29oT09R

So maybe its the redirect or the dynamic loading of the video. Btw I do not see any JS errors in the browser console.
Flags: needinfo?(mconley)
(Assignee)

Comment 1

4 years ago
Hey Henrik,

I'm having some difficulty reproducing this - Google seems to only want to serve up the HTML5 version of this video. How do I get it to serve up Flash? I've tried OS X, Windows and Linux, and got the same result with each running 38.
Flags: needinfo?(mconley) → needinfo?(hskupin)
Mike, flipping the following prefs to false in about:config should effectively disable HTML5 video playback forcing google to serve up a flash player.

media.fragmented-mp4.enabled
media.fragmented-mp4.exposed
media.webm.enabled
media.ogg.enabled
media.mediasource.enabled
media.windows-media-foundation.enabled (Windows only)
media.gstreamer.enabled                (Linux only)
media.apple.mp4.enabled                (Mac only)
The problem right now is that the video is no longer available. I will get in contact with the author and see how to get a similar one up.
Flags: needinfo?(hskupin)
I actually found a video of my own which reproduces it. So please have a look at https://goo.gl/photos/EcjuHj5M8jiuo7Jw6.
I can confirm that this is happening on Windows 7 with the latest nightly with the video from comment #4 and using the directions in comment #2 to force the flash player.  My guess is that since the plugin is in a different document (an iframe) and there is an absolutely positioned div (that contains a video poster image) in the top level document that completely covers the iframe, Firefox is having some issue determining the visibility of the plugin.  Deleting the overlaying div reveals the click-to-play UI where the flash player is located, but clicking on it does nothing.
(Assignee)

Updated

4 years ago
Flags: needinfo?(mconley)
(Assignee)

Comment 7

4 years ago
Sorry, I'm still having trouble reproducing this on my Windows 7 machine, even with the prefs in comment 2 set.

Here's what I do on Fx 38.0.5:

1) Set the prefs as indicated on Windows 7
2) Ensure that Flash is set to Ask to Activate in about:addons
3) Visit https://goo.gl/photos/EcjuHj5M8jiuo7Jw6
4) Click the little play icon in the top right corner of the video

ER:

I should see the click-to-play popup notification doorhanger allowing me to activate the plugin.

AR:

I see a forever-spinner on the video, and nothing else happens. Video never plays, nor does the popup show up.

Maybe this is YouTube screwing up in serving up the page? I'll try again tomorrow.
Created attachment 8615783 [details]
broken-click-to-play.mp4

Mike, I've attached a video to show you what I'm seeing, which I think is also what the reporter is seeing.

The plugin icon in the url bar never shows up, ever.  I don't get a info bar drop down about a hidden plugin either.  Deleting the poster image with the spinner overlaying the plugin (which is being served in an iframe) reveals the click-to-play UI, but clicking on it doesn't work.
(Assignee)

Comment 9

4 years ago
Ok, yes, I am seeing that behaviour.

gfritzsche - do you want this one? I'm kinda bogged down with e10s stuff, but I might eventually get to this.
Flags: needinfo?(mconley) → needinfo?(gfritzsche)
Sorry Mike, i'm definitely too busy with Telemetry right now.
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 11

4 years ago
Alright, I'll see if I can get to this.
Assignee: nobody → mconley
(Assignee)

Comment 12

4 years ago
So I've dug into this a bit. It looks as if in framescripts, content.document.location / content.document.documentURI.spec go out of sync anytime the content uses window.history.pushState.

That means that the location that we send up via the ShowClickToPlayNotification doesn't match the browser.documentURI check, so we bail out.

I have a feeling this is related to bug 1173215, which I coincidentally heard about recently for a completely different reason.
Unfortunately, Bug 1173215 doesn't seem to have had any affect on this.  It is still broken in today's nightly.
(Assignee)

Comment 14

4 years ago
Yeah, I'm seeing the same thing. :(

It does look like it's in the same neighbourhood though. I'm attempting to come up with a minimal testcase to show our DOM / Document Navigation people.
(Assignee)

Comment 15

4 years ago
(In reply to Trevor Rowbotham from comment #13)
> Unfortunately, Bug 1173215 doesn't seem to have had any affect on this.  It
> is still broken in today's nightly.

Ah, but it doesn't appear to be broken if e10s is disabled!

Can you confirm by disabling multi-process mode (Preferences > General > "Enable multi-process Nightly")?
Flags: needinfo?(smokey101stair)
(Assignee)

Updated

4 years ago
Keywords: regressionwindow-wanted
(Assignee)

Comment 16

4 years ago
Created attachment 8623099 [details] [diff] [review]
Add browser_multiple_pushState.js regression test
(Assignee)

Comment 17

4 years ago
So it looks like multiple calls to pushState with e10s enabled don't update the browser's document URI. See attached test case.
Yes, I can confirm that it works as expected when disabling e10s.

I found that it partially works with e10s enabled if I have the Click to Play per-element[1] add-on installed.  The add-on makes the click-to-play ui behind the poster image/loading spinner work, but the plugin icon in the url bar still did not show up.

[1] https://addons.mozilla.org/en-US/firefox/addon/click-to-play-per-element
Flags: needinfo?(smokey101stair)
Bug 1135714 seems very similar to this but is non-e10s.
(Assignee)

Comment 20

4 years ago
Ok, I think I know what's going on.

The problem is that the way that remote browsers get their documentURI's updated is broken when content uses window.history.pushState.

This is what my test case demonstrates.

The reason that this is broken is because of how pushState fires onLocationChange and update documentURI's, and how remote browsers get informed that the documentURI has changed.

The first thing to know is that remote browsers get their documentURI's updated in response to onLocationChange being fired in the content process, which gets messaged up to the parent. This is the _only_ way that documentURI's get updated for the remote browser.

The second thing to know is that when pushState is used, nsDocShell fires onLocationChange, and _then_ updates documentURI in the content process.

This means that the message that gets sent up to the parent regarding the value of content.documentURIObject.spec is out of date - nsDocShell is about to update it. But when nsDocShell updates it, the message has been sent to the parent already, and no other message is sent up about the update to the documentURIObject.

So with pushState, the browser.documentURI is always off by one.
(Assignee)

Comment 21

4 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #20)
> So with pushState, the browser.documentURI is always off by one.

This appears to also be true when the user causes a same-document navigation that fires LOCATION_CHANGE_SAME_DOCUMENT - for example, clicking on a link that adds an anchor ref to the current URL. The documentURI also goes out of date here.
(Assignee)

Comment 22

4 years ago
So, as far as I can tell, two choices:

1) Have nsDocShell update the document URI before it sends the onLocationChange.
2) Have browser-child.js send the aLocationURI up as the documentURI in the event that flags contain LOCATION_CHANGE_SAME_DOCUMENT
(Assignee)

Comment 23

4 years ago
Mutating this bug to what's actually going wrong.
tracking-e10s: --- → ?
Summary: Click-to-Play for Adobe Flash not working when loading video not directly → browser.documentURI goes out of sync when doing same-document navigation in e10s
(Assignee)

Comment 24

4 years ago
Created attachment 8623682 [details]
MozReview Request: Bug 1170488 - Document URI should be updated before sending out onLocationChange. r=smaug

Bug 1170488 - Update Document URI before sending out onLocationChange for same page navigation. r?smaug
Attachment #8623682 - Flags: review?(bugs)
(Assignee)

Comment 25

4 years ago
Created attachment 8623683 [details]
MozReview Request: Bug 1170488 - Add browser_multiple_pushState.js regression test. r=smaug

Bug 1170488 - Add browser_multiple_pushState.js regression test. r?smaug
Attachment #8623683 - Flags: review?(bugs)
Comment on attachment 8623682 [details]
MozReview Request: Bug 1170488 - Document URI should be updated before sending out onLocationChange. r=smaug

This makes AddState and historyNavBetweenSameDoc behave inconsistently. Currently SetDocumentURI seems to be called in both cases after
SetCurrentURI, but you change AddState's behavior.
So, either keep the current behavior, or change both cases
Attachment #8623682 - Flags: review?(bugs) → review-
Attachment #8623683 - Flags: review?(bugs) → review+
(Assignee)

Comment 27

4 years ago
Comment on attachment 8623682 [details]
MozReview Request: Bug 1170488 - Document URI should be updated before sending out onLocationChange. r=smaug

Bug 1170488 - Document URI should be updated before sending out onLocationChange. r?smaug
Attachment #8623682 - Attachment description: MozReview Request: Bug 1170488 - Update Document URI before sending out onLocationChange for same page navigation. r?smaug → MozReview Request: Bug 1170488 - Document URI should be updated before sending out onLocationChange. r?smaug
Attachment #8623682 - Flags: review- → review?(bugs)
(Assignee)

Comment 28

4 years ago
Comment on attachment 8623683 [details]
MozReview Request: Bug 1170488 - Add browser_multiple_pushState.js regression test. r=smaug

Bug 1170488 - Add browser_multiple_pushState.js regression test. r=smaug
Attachment #8623683 - Attachment description: MozReview Request: Bug 1170488 - Add browser_multiple_pushState.js regression test. r?smaug → MozReview Request: Bug 1170488 - Add browser_multiple_pushState.js regression test. r=smaug
Attachment #8623683 - Flags: review+
Attachment #8623682 - Flags: review?(bugs) → review+
(Assignee)

Updated

4 years ago
Attachment #8623682 - Attachment description: MozReview Request: Bug 1170488 - Document URI should be updated before sending out onLocationChange. r?smaug → MozReview Request: Bug 1170488 - Document URI should be updated before sending out onLocationChange. r=smaug
Attachment #8623682 - Flags: review+
(Assignee)

Comment 29

4 years ago
Comment on attachment 8623682 [details]
MozReview Request: Bug 1170488 - Document URI should be updated before sending out onLocationChange. r=smaug

Bug 1170488 - Document URI should be updated before sending out onLocationChange. r=smaug
(Assignee)

Comment 30

4 years ago
Comment on attachment 8623683 [details]
MozReview Request: Bug 1170488 - Add browser_multiple_pushState.js regression test. r=smaug

Bug 1170488 - Add browser_multiple_pushState.js regression test. r=smaug
tracking-e10s: ? → m7+
https://hg.mozilla.org/mozilla-central/rev/53d80bc541e2
https://hg.mozilla.org/mozilla-central/rev/8eb956e0ce98
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
I can confirm that this is fixed on the latest nightly.

I did notice, however, that there is no hidden plugin notification.  Seems like the hidden plugin notification should be showing up here since the plugin is completely hidden behind another element, even though I hate that bar that pops up.  Should this notification be showing?
Flags: needinfo?(mconley)
(Assignee)

Comment 34

4 years ago
(In reply to Trevor Rowbotham from comment #33)
> 
> I did notice, however, that there is no hidden plugin notification.  Seems
> like the hidden plugin notification should be showing up here since the
> plugin is completely hidden behind another element, even though I hate that
> bar that pops up.  Should this notification be showing?

I don't believe so. If I understand what this page is doing correctly, what's happening is that when the page determines that HTML5 video cannot be used, it does some re-arranging of the DOM, and then shows an iframe that loads the Flash player. Once this occurs, the Flash player is in the fore-ground and loaded, the user is able to see and click on it. So we never get to the point where the plugin is actually hidden, since up until the point it's shown, it was never loaded.
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.