1272317 - [e10s] URL bar hash not updated after navigating to about:debugging or other parent process page from a content process page (e.g. website)

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Julian Descottes [:jdescottes]]

Attachment: about-debugging-hash-issue.gif

STRs (see attachment):
- load a web page (any page will do https://www.mozilla.org/en-US/)
- type about:debugging#workers in the URL bar
- press ENTER
- in about:debugging menu select "Add-ons"

ER : The addons panel should be displayed and the URL should be updated to "about:debugging#addons"
AR : Even though the panel is displayed and window.location.hash becomes "#addons", the URL bar still displays "about:debugging#workers"

After running through mozregression, it looks like the issue comes from Bug 1267289.

Comment 1

[Julian Descottes [:jdescottes]]

Looks like the issue is not specific to about:debugging, I could reproduce it with about:preferences as well.

When clicking on a menu-item in about:debugging, we trigger a navigation by doing

> window.location.hash = "#" + panelId;

This issue only happens when first opening a webpage and then navigating to about:debugging.
Going from about:preferences to about:debugging does not trigger this issue for instance.

Gijs: Any idea what could be wrong here?

Comment 2

[:Gijs (he/him)]

As I'm assigned, this won't drop off my radar so I'm clearing needinfo.

FTR, I've looked at what's going on here and it's... tricky. Basically, we set the 'typed' value on the browser and in the URL bar (specifically, here: https://dxr.mozilla.org/mozilla-central/rev/d0be57e84807ce0853b2406de7ff6abb195ac898/browser/components/sessionstore/SessionStore.jsm#824-832 ) because no load is pending at the point where we restore the tab. Changing this, by indicating we've started a load based on what happens in navigateAndRestore (and the things it calls in either process) fixes this but breaks other tests (particularly tests added in the dep, or sessionstore tests, depending on what we do).

On the good news site, I have an automated test for this case (obviously failing against current fx-team tip) so that should make getting a solution here sometime this week feasible... but I have other regressions / urgent things on my plate as well, so it might not be today/tomorrow.

It feels like specialcasing the "switch to parent process" case would work but would be pretty hacky... but I'm not sure if there's a better solution. It also doesn't feel like I understand *why* that case is special yet.

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> It feels like specialcasing the "switch to parent process" case would work
> but would be pretty hacky... but I'm not sure if there's a better solution.
> It also doesn't feel like I understand *why* that case is special yet.

Right. I do now. The normal sequence of events when switching processes is something like:

1 collect tab state
2 switch remoteness - this copies user set content data anyway
3 set the URL of the loading page in the URL bar
4 start load, and send a message when this is done,
5 from that message, restore the URL bar to any user-set content after starting the load, unless other user-set content is (already) present on the browser
6 the load finishes, and clears the stuff set in step 3 unless we set something else in step 5.

In the case described in comment #0 (which, fwiw, is a little intermittent for me, which makes sense because it's race conditions all the way down!) 6 happens before 5. Makes sense, because loading about: pages is fast (maybe even sync, I'm not sure). This is unfortunate because now we set the URL bar to a "custom" value which means it then doesn't update (gets out of whack).

TBH, re-reading this stuff, I *think* that if we decide to switch process for a URI load, we should just not bother with step 5 at all, because there shouldn't be user-set data to copy over as the load is a direct result of what the user did anyway... It's necessary for "actual" session store, but not anything else. I'll see what that breaks...

Comment 4

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1272317 - fix URL bar state when switching to a non-remote browser, r?mconley

Review commit: https://reviewboard.mozilla.org/r/53226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53226/

Comment 5

[:Gijs (he/him)]

Comment on attachment 8753345 [details]
MozReview Request: Bug 1272317 - fix URL bar state when switching to a non-remote browser, r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53226/diff/1-2/

Comment 6

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

Comment on attachment 8753345 [details]
MozReview Request: Bug 1272317 - fix URL bar state when switching to a non-remote browser, r?mconley

https://reviewboard.mozilla.org/r/53226/#review50060

Looks good!

::: browser/components/sessionstore/test/browser.ini:226
(Diff revision 2)
>  [browser_multiple_navigateAndRestore.js]
>  run-if = e10s
>  [browser_async_window_flushing.js]
>  [browser_forget_async_closings.js]
>  [browser_newtab_userTypedValue.js]
> +[browser_parentProcessRestoreHash.js]

This test is going to break in the non-e10s case - you should probably add `run-if = e10s`

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f7e2e84c0c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7f7e2e84c0c

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c560b2cb8b97

Comment 10

[Forhad Hossain]

I have reproduced the bug in Nightly 49.0a1 (2016-05-12) with the instruction from comment 0 and windows 32 bit

Bug is fixed now on Latest Beta 48.0b6 (Build ID:20160706215822)
User Agent:Mozilla/5.0 (Windows NT 6.3; rv:48.0) Gecko/20100101 Firefox/48.0

& fixed Latest Developer Edition 49.0a2 (2016-07-11)(Build ID:20160711004013)
User Agent:Mozilla/5.0 (Windows NT 6.3; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160708]

Comment 11

[Rezaul Huque Nayeem]

Reproduced this bug in firefox nightly 49.0a1 (2016-05-12) with ubuntu 16.04 (64 bit)

Verified as this bug fixed with latest firefox Beta 48.0b9 (Build ID: 20160718142219)
Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0 

and latest firefox aurora 49.0a2 (Build ID: 20160726004006)
Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0