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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 2

[Trevor Rowbotham [:rowbot]]

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)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I actually found a video of my own which reproduces it. So please have a look at https://goo.gl/photos/EcjuHj5M8jiuo7Jw6.

Comment 5

[Trevor Rowbotham [:rowbot]]

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.

Comment 6

[Trevor Rowbotham [:rowbot]]

Last good: 2014-09-30
First bad: 2014-10-01
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c24470b6b3a&tochange=14665b1de5ee

Suspect: Bug 1069075

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 8

[Trevor Rowbotham [:rowbot]]

Attachment: 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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Sorry Mike, i'm definitely too busy with Telemetry right now.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Alright, I'll see if I can get to this.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 13

[Trevor Rowbotham [:rowbot]]

Unfortunately, Bug 1173215 doesn't seem to have had any affect on this.  It is still broken in today's nightly.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(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")?

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Add browser_multiple_pushState.js regression test

Comment 17

[Mike Conley (:mconley) (:⚙️)]

So it looks like multiple calls to pushState with e10s enabled don't update the browser's document URI. See attached test case.

Comment 18

[Trevor Rowbotham [:rowbot]]

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

Comment 19

[Benjamin Smedberg]

Bug 1135714 seems very similar to this but is non-e10s.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

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

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Mutating this bug to what's actually going wrong.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1170488 - Add browser_multiple_pushState.js regression test. r=smaug

Bug 1170488 - Add browser_multiple_pushState.js regression test. r?smaug

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 27

[Mike Conley (:mconley) (:⚙️)]

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

Comment 28

[Mike Conley (:mconley) (:⚙️)]

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

Comment 29

[Mike Conley (:mconley) (:⚙️)]

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

Comment 30

[Mike Conley (:mconley) (:⚙️)]

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

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53d80bc541e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb956e0ce98

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53d80bc541e2
https://hg.mozilla.org/mozilla-central/rev/8eb956e0ce98

Comment 33

[Trevor Rowbotham [:rowbot]]

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?

Comment 34

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1170488.html