1586129 - Set mIsNavigating unconditionally in nsDocShell::LoadURI(nsDocShellLoadState..

Status: NEW

(Core :: DOM: Navigation, enhancement, P2)

Description

[Matt Woodrow (:mattwoodrow)]

We currently set mIsNavigating in some of the callers of this LoadURI variant, but not others.

This has required adding a bunch of extra plumbing to preserve the semantics in bug 1578624.

Unfortunately, making it unconditional results in browser_urlbar_locationchange_urlbar_edit_dos.js failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=748b126f1f9d28d726debee70c691d2da20699d&selectedJob=269689472

I'll look into this, and figure out if we can fix this test and then simplify the code.

Comment 1

[Matt Woodrow (:mattwoodrow)]

https://pernos.co/debug/DyN4fST8C2J_81DxfRqBIw/index.html

Comment 2

[Matt Woodrow (:mattwoodrow)]

The failing test is writing to location.hash in the content process, and checking that this doesn't result in a change to the URL bar.

The call stack to IsNavigating being visible is this:

::Location::SetHash () at /Location.cpp:176
::LocationBase::SetURI () at /LocationBase.cpp:150
::BrowsingContext::LoadURI () at /BrowsingContext.cpp:851
{virtual override thunk({offset(-416)}, nsDocShell::LoadURI)} ()
nsDocShell::LoadURI () at /nsDocShell.cpp:803
nsDocShell::InternalLoad () at /nsDocShell.cpp:9326
nsDocShell::HandleSameDocumentNavigation () at /nsDocShell.cpp:9011
nsDocShell::OnNewURI () at /nsDocShell.cpp:11048
nsDocShell::SetCurrentURI () at /nsDocShell.cpp:1256
nsDocLoader::FireOnLocationChange () at /nsDocLoader.cpp:1368
{virtual override thunk({offset(-8)}, nsBrowserStatusFilter::OnLocationChange)} () at /nsBrowserStatusFilter.cpp:0
nsBrowserStatusFilter::OnLocationChange () at /nsBrowserStatusFilter.cpp:215
{virtual override thunk({offset(-464)}, ::BrowserChild::OnLocationChange)} ()
::BrowserChild::OnLocationChange () at /BrowserChild.cpp:3648
nsIDocShell::GetIsNavigating () at /nsIDocShell.h:940
{virtual override thunk({offset(-416)}, nsDocShell::GetIsNavigating)} () at /nsDocShell.cpp:0
nsDocShell::GetIsNavigating () at /nsDocShell.cpp:3685

We then share this value to the parent process, and decide not to update the URL bar at this point: https://searchfox.org/mozilla-central/rev/6866d3a650c826f1cabd123663e84b95ee743701/browser/base/content/tabbrowser.js#5851

Given that, I think BrowsingContext::LoadURI needs to differentiate between 'navigating' and 'non-navigating' loads in some way, unless we find a different way of implementing this behaviour.

We could maybe remove the flag from the cross-process BrowsingContext/WindowGlobal bits, and assume it's always a 'navigating' load for those cases, but it's not super obvious that this will be correct as we add more consumers.

Any ideas Kris?

Comment 3

[Matt Woodrow (:mattwoodrow)]

I considered trying to make LoadURI determine internally whether this was a 'navigating' load (by detecting if it's just a hash change, which we already do).

That then makes this test fail: https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/browser/components/urlbar/tests/browser/browser_urlbarHashChangeProxyState.js#82

It looks like we add a new hash to gURLBar, and then we don't update the URL when we get onLocationChange (since we have a user typed value, the location change is non-navigating). That's ok, since the typed value and the new location are identical, but we're still marked as a user typed value.

We then do a second hash change, but this time from within the content process by forcing a click on a link. Again we get onLocationChange, and don't update the URL bar since we still have a 'user typed value', and the hash change is non-navigating. This time the test fails, since the typed URL is from the previous subtest.

Possibly the front-end code needs to differentiate between an onLocationChange in response to a load that they initiated (update the URL bar, clear user typed value state) and an onLocationChange that the page initiated (keep the user typed URL bar contents if the change was just a hash change/non-navigating). I'm not sure how to do that though.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5276c79c34bfb5f1c707fa6c72b1a3857b775084&selectedJob=271196003

Comment 4

[Matt Woodrow (:mattwoodrow)]

Gijs, do you have any ideas on how the frontend could differentiate between these two different hash change types?

Comment 5

[:Gijs (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)

Gijs, do you have any ideas on how the frontend could differentiate between these two different hash change types?

I'm a bit confused. We do currently distinguish between them, using exactly the isNavigating property that the summary of this bug wants to get rid of (but on browser, in browser-custom-element.js , whereas the original thing is in docshell).

We can replace it with pretty much anything else, as long as it's accessible from the various web progress listeners. For that, it'd need to be either part of some of the load state things (loadcontext, loadflags, loadstate, whatever they're all called - we have too many of them with unclear distinctions... but that's a separate issue) or something that we can ask the docshell about and/or track on the <browser> so we can ask about it then. The flag/loadcontext thing would be better because it doesn't suffer from data races, which I suspect the <browser> variant does (already) because it involves IPC.

Does... that answer your question? :-)

Comment 6

[Matt Woodrow (:mattwoodrow)]

What is the intended definition of isNavigating?

The documentation for nsIDocShell::isNavigating suggests that all loadURI calls will set it.

From debugging the test failures, it appears that the behaviour that we're relying on is that it's true for all loadURI calls that aren't just a hash change, and also true for hash only changes that originate from the frontend/parent process. It's false within loadURI if it's just a hash change that the document itself made (writing to location.hash). There may be more ways to classify the 'originator', but that's all we have test coverage for.

My main question is whether we can simplify this definition (to either being all loadURI calls as currently documented, or all loadURI calls excluding hash only changes), and then have front-end code figure out the 'originator' bit itself using a different mechanism.

From the docshell side, that simplies things considerably, since the 'originator' concept isn't really something that's defined.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Note that this bug isn't intending to get rid of isNavigating itself, but rather the internal navigating bool we pass around all over the place.

The value of this appears to be fairly arbitrarily picked for most call chains, but manages to ensure that the above test-covered behaviour (always true for cross-process messages originating from the frontend, and always false for location.hash writes).

Comment 8

[:Gijs (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)

What is the intended definition of isNavigating?

The purpose is separating out user-initiated navigations/loads from non-user-initiated ones. So in theory, true for browser.isNavigating means the location/state change is a result of user action (typically in the URL bar or some other part of browser UI), false means it is not.

Ultimately, I think the main (possibly only?) thing we use it for is to determine whether user-altered URL bar data can be replaced with the current URI or not. It's all about the rather annoying fact that the URL bar is meant to be both an inputfield and show state - and ideally not ever lose user data. bug 360511 and bug 1272317 and bug 1267289 and bug 1241085 are all interesting edgecases in this dance.

If we're using this for something else, that'd be useful to know - I can't quickly see anything in searchfox but it's midnight here so I may be tired and missing something, and this code is hairy.

The documentation for nsIDocShell::isNavigating suggests that all loadURI calls will set it.

All of the ones from frontend code likely do; for things like assigning to location.hash inside the page, probably not. Unsure about e.g. back/forward in history etc.

In theory, it might be nice if we could propagate user-interaction state (which we currently track already in DOM/event code / popup blocking terms) into it when assignments to location.hash or other in-content navigations happen as a result of user action in the page (e.g. clicking something that runs some JS), but I don't believe we currently do so.

From debugging the test failures, it appears that the behaviour that we're relying on is that it's true for all loadURI calls that aren't just a hash change, and also true for hash only changes that originate from the frontend/parent process. It's false within loadURI if it's just a hash change that the document itself made (writing to location.hash). There may be more ways to classify the 'originator', but that's all we have test coverage for.

I'm not sure about the non-hashchange case; it's possible we're using something else (like uri.equalsExceptRef or something) to determine we want to clear the location bar in that case, I'd have to dig into the code again - but we do end up clearing the location bar "even" for non-browser-UI-initiated loads (which you can trivially see by trying it out). Otherwise, yes this sounds correct.

My main question is whether we can simplify this definition (to either being all loadURI calls as currently documented, or all loadURI calls excluding hash only changes), and then have front-end code figure out the 'originator' bit itself using a different mechanism.

I mean, what kind of different mechanism would that be? Ultimately, I'm not sure there's any way to associate something to a load except via some form (flag, extra param, stuff on the loadcontext/state/principal) of input to the loadURI call (or the call that ends up calling loadURI). Can you give an example of the type of thing you have in mind? It feels like I'm not really following what you are getting at here.

Comment 9

[Chris Peterson [:cpeterson]]

Does this DocumentChannel bug need to block shipping Fission MVP?

Comment 10

[Matt Woodrow (:mattwoodrow)]

No, it's just cleanup, doesn't need to block anything.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:hsinyi, since the bug has high priority and recent activity, could you have a look please?

For more information, please visit auto_nag documentation.