798249 - Changing URL locations while a page is loading temporarily reverts the URL

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Visit a slow loading page
While the page is loading, type in a different URL in the location bar and hit Enter.

Expected:
The typed value will remain

Actual:
The typed value is reverted to the loading URL until some time later in the new request.

This affects users perceived responsiveness since the status of the location bar is one of the ways that users can track the progress of the page's load (for example, pages that redirect upon loading). Reverting back to the current location can also cause confusion as users wonder if their attempt to redirect their navigation was successful.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This may be caused by bug 724599's patch.

Comment 2

[Alice0775 White]

Steps to reproduce:
1.Paste https://bugzilla.mozilla.org/attachment.cgi?id=666224 in the location bar and hit Enter.
   (If an warning dialog pops up, cllick OK)
2.Paste https://developer.mozilla.org/en/firefox_16_for_developers in the location bar and hit Enter.

Regression window(cached m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/1965a2c89d61
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120328 Firefox/14.0a1 ID:20120328183752
Bad:
http://hg.mozilla.org/mozilla-central/rev/563ea372f000
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120329 Firefox/14.0a1 ID:20120329024856
Pushlog;
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1965a2c89d61&tochange=563ea372f000

Comment 4

[[e-mail]]

Also clicking stop button permanently reverts the url. Seems to be caused by same bug 724599

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Jared why did you file this as perf?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

To get it to show up in the workweek query. It is a snappy issue.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

We're currently clearing userTypedValue when we get a network STATE_STOP regardless of which request is being stopped.

We should check that the page being stopped is the new page being requested and not the current one.

Gavin, do you know of a way to differentiate requests within mTabsProgressListener.onStateChange? I tried comparing aRequest.QueryInterface(nsIChannel).URI to this.mBrowser.userTypedValue, but that doesn't work and I'm not sure that we can rely on userTypedValue being equal to the actual URI being requested (for example, maybe we tack on a www. prefix or a trailing slash).

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Felipe, would you be able to answer the questions in comment #7 or redirect me to someone who can?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Sorry I lost this needinfo :(

It sounds like what we want to do is ignore STOP requests that are the result of load starts that we triggered. I don't know of a great way to do that offhand. What's the order of events that we get from the WPL in this case? Does the STOP for the initial load come before the start, or after?

Comment 10

[:Felipe Gomes (needinfo for replies!)]

aWebProgress.DOMWindow contains a reference to the window related to the notification. Maybe you can use that. It will be a bit more painful for e10s but you could try a non-e10s proof-of-concept easily.

Also worth checking if aWebProgress.isLoadingDocument can help

Comment 11

[:Gavin Sharp [email: [e-mail]]]

This is backlog+, but the e10s bug that's similar that you're probably noticing on Nightly now is separate, and tracked in bug 1096459.

Comment 12

[:Gijs (out for now; he/him)]

As far as I can tell, the issue here is that this code:

https://dxr.mozilla.org/mozilla-central/rev/c2256ee8ae9a8ee0bf7ab49a8b1924720d846cc7/browser/base/content/tabbrowser.xml#678-688

resets the location bar value once we stop the original load.

We seem to not distinguish whether this load corresponds to the value in the location bar when doing this and/or whether this is happening because (presumably: in nsDocShell's InternalLoad, ie here: https://dxr.mozilla.org/mozilla-central/rev/c2256ee8ae9a8ee0bf7ab49a8b1924720d846cc7/docshell/base/nsDocShell.cpp#10280-10309 ) we call Stop as a result of a new load in the same docshell, or as a result of the user hitting the stop button, or because the page stopped navigation in some way (e.g. our old friend beforeunload!).

Marco, do you know what would be the best way of checking what's happening and not being so eager to reset the value here?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

I suspect we're getting a Stop on the first webprogress but might not know that a new load is coming immediately after and that the source is a userTypedValue.

Comment 14

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> I suspect we're getting a Stop on the first webprogress but might not know
> that a new load is coming immediately after and that the source is a
> userTypedValue.

Are you asserting that we can't know that in here? Because it seems like we should be able to figure that out, one way or another... In fact, it seems plausible there are several indicators:
- the userTypedClear counter
- docshell loads should be called from our JS anyway, so we should know that we just tried to load something, and this code should all be running sync off the stop() call, so setting a bool and unsetting it after the load returns might work;
- aStatus seems to always be NS_BINDING_ABORTED (but maybe we use that for other things as well?)
- there might be others ?

I'm just not sure which is the most reliable and/or not confused with other things, which is why I pinged Marco... :-)

Comment 15

[:Gijs (out for now; he/him)]

In fact, thinking about this more, it seems reasonable to add a flag to docshell / webprogress / webprogresslistener that specifically indicates that this (ie a new load in the same docshell) is why stop is being called (on the original load). That seems like the most "proper" solution, and such flags should also help with spoofing concerns in bug 610357 and bug 770382 (which, AFAICT, are kind of similar, and started when we began with using about:newtab instead of about:blank).

Comment 16

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> In fact, thinking about this more, it seems reasonable to add a flag to
> docshell / webprogress / webprogresslistener that specifically indicates
> that this (ie a new load in the same docshell) is why stop is being called
> (on the original load).

... except that this code threads through no-argument Stop() on nsDocLoader, so "pass a flag" isn't as straightforward as it sounds. :-(

Comment 17

[Marco Bonardo [:mak]]

Sorry, I don't know more than what you already figured out.
userTypedClear sounds like may be used, but it looks fragile imo (temporary and double increments to skip over temporary increments...)
I'm also not sure I'd rely on NS_BINDING_ABORTED, it's used almost everywhere, doesn't sound like a good source to distinguish a stop reason. Maybe you could ask someone with more knowlege of nsDocLoader what they suggest to pass through an information, like a stop reason.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] from comment #17)
> Sorry, I don't know more than what you already figured out.
> userTypedClear sounds like may be used, but it looks fragile imo (temporary
> and double increments to skip over temporary increments...)
> I'm also not sure I'd rely on NS_BINDING_ABORTED, it's used almost
> everywhere, doesn't sound like a good source to distinguish a stop reason.
> Maybe you could ask someone with more knowlege of nsDocLoader what they
> suggest to pass through an information, like a stop reason.

OK. I'll have a look. I think the nsDocLoader thing is sufficiently non-trivial because of the amount of indirection that I'd like to look if there's a simpler solution first. Might be a few days until I get to this though.

Comment 20

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 798249 - track when we're 'inside' a loadURI call when dealing with Stop() calls resulting from the same, r?mconley

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

Comment 21

[:Gijs (out for now; he/him)]

https://reviewboard.mozilla.org/r/34995/#review31553

::: browser/base/content/browser.js:905
(Diff revision 1)
> +  browser.lastRequestedURI = makeURI(data.uri);

It seems you can't pass an nsIURI in a message? I thought you could... but anyway, I decided to pass a spec instead. If there's a better way to do this, I'm all ears.

::: browser/base/content/test/urlbar/browser_urlbar_stop_pending.js:21
(Diff revision 1)
> +  yield new Promise(resolve => setTimeout(resolve, 200));

r?mconley for the e10s bits here, which include the fact that this test always passes in e10s, even before my changes. This is odd, because when I manually open a regular e10s nightly profile, load some page (1), load a slow page (2), then load another page (3), I can *see* the URL bar switching back to page 1 once I try to load page 3. It seems in the test we're somehow not hitting that, but only in e10s. I don't know how to figure out why.

As it is, the test will catch regressions in non-e10s, but I don't know if that's long for this world. :-\

::: browser/base/moz.build:26
(Diff revision 1)
> +    'content/test/urlbar/browser.ini',

I'd like to empty out some of the location bar tests to this in a separate bug. content/test/general/ is a giant mishmash of things which doesn't help anybody (including intermittents, finding relevant tests, etc.)

::: docshell/base/nsDocShell.cpp:10292
(Diff revision 1)
> +    // For root docshells, tell the web browser going ahead with a new load,
> +    // and do this before calling Stop() so the browser has a chance of not
> +    // messing with the UI because of the call to Stop():
> +    nsCOMPtr<nsIDocShellTreeItem> root;
> +    GetSameTypeRootTreeItem(getter_AddRefs(root));
> +    bool isRoot = root.get() == static_cast<nsIDocShellTreeItem*>(this);
> +    if (browserChrome3 && isRoot) {
> +      browserChrome3->OnBeforeStopForNewLoad(this, aURI);
> +    }

This is arguably the most important part of the patch. I'm making docshell itself take care of sending this information.

I'm not particularly pleased about doing this, but I can't think of a better way. The url bar quite often passes things like "foo.com/" and "some text" into docshell, which does all the hard work of figuring out what to actually request. Which means that correlating this fully on the frontend side of things isn't really possible without duplicating all that docshell logic turning those random strings into URLs, which seems very icky and prone to missing edgecases if the flags ever change or depend on other circumstances than just the input. Doing everything in the front-end also wouldn't really deal well with loads that are denied by the (before)unload handlers etc.

The downside of this approach (apart from the idl changes and generally hacking docshell for what at face value is a frontend problem) is that now this will apply for any load, not just loads initiated from the URL bar. Because other loads don't normally change the URL bar anyway, I think that's fine and doesn't really impact anything materially, but it would be good if Olli and Marco could confirm that this change is sane.

A more long-term solution would be moving all the URL processing out of docshell and then doing this on the frontend only, but that feels like a much scarier/larger change. OTOH, it could potentially lead to simpler code.

Comment 22

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

(In reply to :Gijs Kruitbosch from comment #21)
> The downside of this approach (apart from the idl changes and generally
> hacking docshell for what at face value is a frontend problem) is that now
> this will apply for any load, not just loads initiated from the URL bar.
> Because other loads don't normally change the URL bar anyway, I think that's
> fine and doesn't really impact anything materially,
Don't understand the comment "other loads don't normally change the URL bar".
Sure loading any new top level page ends up changing url bar.


Is onStateChange not enough here? I mean do we not get enough information from
STATE_START/STOP ?

Comment 23

[:Gijs (out for now; he/him)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #22)
> (In reply to :Gijs Kruitbosch from comment #21)
> > The downside of this approach (apart from the idl changes and generally
> > hacking docshell for what at face value is a frontend problem) is that now
> > this will apply for any load, not just loads initiated from the URL bar.
> > Because other loads don't normally change the URL bar anyway, I think that's
> > fine and doesn't really impact anything materially,
> Don't understand the comment "other loads don't normally change the URL bar".
> Sure loading any new top level page ends up changing url bar.

Yes, but not as soon as the request starts.

> Is onStateChange not enough here? I mean do we not get enough information
> from
> STATE_START/STOP ?

See comment #12 / comment #14 . Essentially, AFAICT there is no sane way to distinguish the STOP from the pending load from any other pending STOP.

What happens is, we load 1 page, that finishes, that stays in the URL bar. Now we start loading a second page. If that happens from the URL bar, the new page's URL is in the URL bar. Now if you type in the URL bar some more before the second load finishes, and hit enter (trying to load a 3rd page), docshell stops the 2nd load. Right now, when a stop happens and the load was not successful, we reset the URL in the URL bar. This makes sense if you try loading a page and then hit stop or cases like this. However, we need to distinguish that case from this one, where the 3rd load is cancelling the 2nd one, and to just leave the user's input in the URL bar while we load page number 3.

Does that make more sense?

Comment 24

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

(In reply to :Gijs Kruitbosch from comment #23) 
> See comment #12 / comment #14 . Essentially, AFAICT there is no sane way to
> distinguish the STOP from the pending load from any other pending STOP.
The STOP happening _during_ the method call to start a new load on docshell?

> 
> What happens is, we load 1 page, that finishes, that stays in the URL bar.
> Now we start loading a second page. If that happens from the URL bar, the
> new page's URL is in the URL bar. Now if you type in the URL bar some more
> before the second load finishes, and hit enter (trying to load a 3rd page),
> docshell stops the 2nd load. Right now, when a stop happens and the load was
> not successful, we reset the URL in the URL bar. This makes sense if you try
> loading a page and then hit stop or cases like this. However, we need to
> distinguish that case from this one, where the 3rd load is cancelling the
> 2nd one, and to just leave the user's input in the URL bar while we load
> page number 3.
> 
> Does that make more sense?
This is what I've been thinking.

(though I wonder what the UI should show if 2nd page is loading, then 3rd is started and then stop is pressed before any of 3rd has been loaded.)

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #24)
> (In reply to :Gijs Kruitbosch from comment #23) 
> > See comment #12 / comment #14 . Essentially, AFAICT there is no sane way to
> > distinguish the STOP from the pending load from any other pending STOP.
> The STOP happening _during_ the method call to start a new load on docshell?

Yes, though "during" is relative in e10s land, as far as chrome is concerned. :-(

> > What happens is, we load 1 page, that finishes, that stays in the URL bar.
> > Now we start loading a second page. If that happens from the URL bar, the
> > new page's URL is in the URL bar. Now if you type in the URL bar some more
> > before the second load finishes, and hit enter (trying to load a 3rd page),
> > docshell stops the 2nd load. Right now, when a stop happens and the load was
> > not successful, we reset the URL in the URL bar. This makes sense if you try
> > loading a page and then hit stop or cases like this. However, we need to
> > distinguish that case from this one, where the 3rd load is cancelling the
> > 2nd one, and to just leave the user's input in the URL bar while we load
> > page number 3.
> > 
> > Does that make more sense?
> This is what I've been thinking.
> 
> (though I wonder what the UI should show if 2nd page is loading, then 3rd is
> started and then stop is pressed before any of 3rd has been loaded.)

It will use browser.currentURI, which depends on whether or not the 2nd page has loaded anything at all or not (ie if the 1st document has unloaded).

Comment 26

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

(In reply to :Gijs Kruitbosch from comment #25)
> Yes, though "during" is relative in e10s land, as far as chrome is
> concerned. :-(
But you're anyhow dealing with content process side scripts here, so you can synchronously detect it.
Or do we initiate page loads in e10s using nsIFrameLoader::loadURI on parent process, and not using
some frame script which then calls docshell's uri loading method in child process?

Comment 27

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

Looks like we're using RemoteWebNavigation.js to load uris, so getting state changes while passing url to docshell should work in e10 and non-e10.

Comment 28

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

Comment on attachment 8719541 [details]
MozReview Request: Bug 798249 - track when we're 'inside' a loadURI call when dealing with Stop() calls resulting from the same, r?mconley

So unless I've made some mistake, this bug could be fixed without adding more clutter to docshell.
If you disagree, or think that the other approach is too complicated, please explain and ask review again.

Comment 29

[:Gijs (out for now; he/him)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #27)
> Looks like we're using RemoteWebNavigation.js to load uris, so getting state
> changes while passing url to docshell should work in e10 and non-e10.

But we quite often don't pass a URL to docshell, but incomplete fragments or keyword searches, see comment #21:

(In reply to :Gijs Kruitbosch from comment #21)
> I'm not particularly pleased about doing this, but I can't think of a better
> way. The url bar quite often passes things like "foo.com/" and "some text"
> into docshell, which does all the hard work of figuring out what to actually
> request. Which means that correlating this fully on the frontend side of
> things isn't really possible without duplicating all that docshell logic
> turning those random strings into URLs, which seems very icky and prone to
> missing edgecases if the flags ever change or depend on other circumstances
> than just the input. Doing everything in the front-end also wouldn't really
> deal well with loads that are denied by the (before)unload handlers etc.

Comment 30

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

(In reply to :Gijs Kruitbosch from comment #29)
> (In reply to Olli Pettay [:smaug] (high review load) from comment #27)
> > Looks like we're using RemoteWebNavigation.js to load uris, so getting state
> > changes while passing url to docshell should work in e10 and non-e10.
> 
> But we quite often don't pass a URL to docshell, but incomplete fragments or
> keyword searches, see comment #21:
So? If you get a STOP while initiating a load (whether it is with fragment or whatever), you don't update url bar based on the stop.
Or do you want some different behavior.

Comment 31

[:Gijs (out for now; he/him)]

Comment on attachment 8719541 [details]
MozReview Request: Bug 798249 - track when we're 'inside' a loadURI call when dealing with Stop() calls resulting from the same, r?mconley

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

Comment 32

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #21)
> ::: browser/base/content/test/urlbar/browser_urlbar_stop_pending.js:21
> (Diff revision 1)
> > +  yield new Promise(resolve => setTimeout(resolve, 200));
> 
> r?mconley for the e10s bits here, which include the fact that this test
> always passes in e10s, even before my changes. This is odd, because when I
> manually open a regular e10s nightly profile, load some page (1), load a
> slow page (2), then load another page (3), I can *see* the URL bar switching
> back to page 1 once I try to load page 3. It seems in the test we're somehow
> not hitting that, but only in e10s. I don't know how to figure out why.
> 
> As it is, the test will catch regressions in non-e10s, but I don't know if
> that's long for this world. :-\

Fixed this by using about:home as the initial page. Seems like about:robots loads in the parent process (?) as does about:mozilla and about:supports... well, anyway.

This does indicate that things are "not quite the same" when we switch processes for a load. I tried to deal with this in the patch, but it seems we route process-changing loads through session restore (!?) and I got lost trying to find the precise point where we load stuff and ensuring the sensible thing happens in that case - it almost seems like we always use a content script in that case, even if we're loading in the parent process, which doesn't make a whole lot of sense to me. So I set the inLoadURI property on the browser in _loadURIWithOptions (and remove it there, too) and moved on. Either way, it seems from the test passing before any changes were made that that case might have already behaved correctly.

Comment 33

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

Comment on attachment 8719541 [details]
MozReview Request: Bug 798249 - track when we're 'inside' a loadURI call when dealing with Stop() calls resulting from the same, r?mconley

https://reviewboard.mozilla.org/r/34995/#review31987

I guess this is one of those weird cases where we have to support two wildly different ways of doing things due to e10s. :/

Thanks for this Gijs - I'm really happy someone is taking a stab at this. I've got some comments - see below!

::: browser/base/content/browser.js:876
(Diff revision 2)
> +    browser.inLoadURI = false;

Maybe only set this if we're dealing with a non-remote browser.

::: browser/base/content/test/urlbar/browser_urlbar_stop_pending.js:6
(Diff revision 2)
> +add_task(function*() {

Can you please add a docstring that describes what this tests?

::: browser/base/content/test/urlbar/browser_urlbar_stop_pending.js:30
(Diff revision 2)
> +  gBrowser.removeCurrentTab();

Please use:

```JavaScript
yield BrowserTestUtils.removeTab(tab);
```

to make sure all of the messages from this tab have finished being handled.

::: toolkit/modules/RemoteWebProgress.jsm:201
(Diff revision 2)
> +      webProgress._inLoadURI = json.inLoadURI;

To me, it's a bit odd that we're stuffing this in to the RemoteWebNavigation, but not exposing it via the interface, and not doing the same thing for nsIWebNavigation for non-remote browsers.

Perhaps we can set the remote <xul:browser>'s loadURI directly instead, and that way we don't have to check the remoteness of the browser in tabbrowser.xml - we can probably just use the same code-paths for the remote and non-remote cases.

Comment 34

[:Gijs (out for now; he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #33)
> ::: toolkit/modules/RemoteWebProgress.jsm:201
> (Diff revision 2)
> > +      webProgress._inLoadURI = json.inLoadURI;
> 
> To me, it's a bit odd that we're stuffing this in to the
> RemoteWebNavigation, but not exposing it via the interface, and not doing
> the same thing for nsIWebNavigation for non-remote browsers.

The problem is that these progress listeners are called through an XPCOM filter web progress listeners, which means these are going through XPCOM, which means we can't just add a param or whatever, without adjusting umpteen interfaces which have been the same since methusalem, which I'd rather not do.

> Perhaps we can set the remote <xul:browser>'s loadURI directly instead, and
> that way we don't have to check the remoteness of the browser in
> tabbrowser.xml - we can probably just use the same code-paths for the remote
> and non-remote cases.

I'm not sure what "set the remote <xul:browser>'s loadURI" means. Can you expand on this suggestion?

Comment 35

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

(In reply to :Gijs Kruitbosch from comment #34)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #33)
> > ::: toolkit/modules/RemoteWebProgress.jsm:201
> > (Diff revision 2)
> > > +      webProgress._inLoadURI = json.inLoadURI;
> > 
> > To me, it's a bit odd that we're stuffing this in to the
> > RemoteWebNavigation, but not exposing it via the interface, and not doing
> > the same thing for nsIWebNavigation for non-remote browsers.
> 
> The problem is that these progress listeners are called through an XPCOM
> filter web progress listeners, which means these are going through XPCOM,
> which means we can't just add a param or whatever, without adjusting umpteen
> interfaces which have been the same since methusalem, which I'd rather not
> do.

Yeah, I totally sympathize - and I definitely don't want you to add it to the interface. :)

I just wonder if it makes more sense to do things in a similar way for both e10s and non-e10s - as in, stashing the value in the <xul:browser> itself.

> 
> > Perhaps we can set the remote <xul:browser>'s loadURI directly instead, and
> > that way we don't have to check the remoteness of the browser in
> > tabbrowser.xml - we can probably just use the same code-paths for the remote
> > and non-remote cases.
> 
> I'm not sure what "set the remote <xul:browser>'s loadURI" means. Can you
> expand on this suggestion?

Sorry - I miswrote - I should have said "inLoadURI" instead of "loadURI". What I mean is, you stash the inLoadURI value in the <xul:browser> for the non-remote case. Can we not do the same thing for the remote case?

Comment 36

[Marco Bonardo [:mak]]

Gijs, I'm really busy trying to figure out hags on shutdown/startup in current Beta, I'm not sure I have enough time to review this complex patch soon.

Comment 37

[:Gijs (out for now; he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=16409630f876

Comment 38

[:Gijs (out for now; he/him)]

Comment on attachment 8719541 [details]
MozReview Request: Bug 798249 - track when we're 'inside' a loadURI call when dealing with Stop() calls resulting from the same, r?mconley

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

Comment 39

[:Gijs (out for now; he/him)]

Comment on attachment 8719541 [details]
MozReview Request: Bug 798249 - track when we're 'inside' a loadURI call when dealing with Stop() calls resulting from the same, r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34995/diff/3-4/

Comment 40

[:Gijs (out for now; he/him)]

https://reviewboard.mozilla.org/r/34995/#review34687

::: toolkit/content/browser-child.js:330
(Diff revision 4)
> +      WebProgressListener.sendLoadCallResult(loadCallResult);

FWIW, I'm likely to need the details I'm passing here in bug 1217387 anyway. I needed the message here to ensure we immediately reset the browser's inLoadURI value as soon as the loadURIWithOptions call returns, even in the e10s case.

Comment 41

[:Gijs (out for now; he/him)]

Comment on attachment 8719541 [details]
MozReview Request: Bug 798249 - track when we're 'inside' a loadURI call when dealing with Stop() calls resulting from the same, r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34995/diff/4-5/

Comment 42

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

Comment on attachment 8719541 [details]
MozReview Request: Bug 798249 - track when we're 'inside' a loadURI call when dealing with Stop() calls resulting from the same, r?mconley

https://reviewboard.mozilla.org/r/34995/#review35037

Really happy you found a way to write an automated test for this.

I think you can probably smash all of my open issues by just removing the object being sent to the LoadResult message handler.

Thanks, Gijs!

::: browser/base/content/test/general/head.js:401
(Diff revision 5)
> +        /* Hammer time. */

Glad we're keeping this comment. :)

::: toolkit/content/browser-child.js:327
(Diff revision 5)
> +        exception,

I don't think this is going to work - I don't think we can serialize Exceptions like this. Maybe send up the Exception message instead?

Actually, the message handler doesn't even seem to be doing anything with this stuff. Can we just remove it?

::: toolkit/content/browser-child.js:328
(Diff revision 5)
> +        baseURI

Nit: Trailing comma please.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2331bc8e11a7

Comment 44

[:Gijs (out for now; he/him)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/614ad511d861 for orange (about:home doesn't like being unloaded shortly after it's been loaded because it does async stuff straight afterwards) and (presumably co-incidental, as in resulting from that issue) leaks.

Comment 45

[:Gijs (out for now; he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=577318fc480c

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0cdad8ac7513

Comment 47

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #45)
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=577318fc480c

All happy and green with that change, so let's try again:

https://hg.mozilla.org/integration/fx-team/rev/0cdad8ac7513

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

\o/

+100XP awarded to Gijs

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cdad8ac7513

Comment 50

[tabmix.onemen]

With today Nightly i get this error

TypeError: json is undefined RemoteWebProgress.jsm:189:9

aMessage.name Content:LoadURIResult 
aMessage.json undefined


browser-child.js send message without any data !!
>  sendLoadCallResult() {
>    this._send("Content:LoadURIResult");
>  },

Comment 51

[:Gijs (out for now; he/him)]

(In reply to tabmix.onemen from comment #50)
> With today Nightly i get this error
> 
> TypeError: json is undefined RemoteWebProgress.jsm:189:9
> 
> aMessage.name Content:LoadURIResult 
> aMessage.json undefined
> 
> 
> browser-child.js send message without any data !!
> >  sendLoadCallResult() {
> >    this._send("Content:LoadURIResult");
> >  },

It was meant to do that. I already fixed this in bug 1254657 but that didn't make today's Nightly.

Comment 52

[arni2033]

STR:
1. Type http://www.rp-online.de/ in urlbar, press Enter
2. When content appears, paste http://briangrinstead.com/files/sleepy.php in urlbar and press Enter

AR:  URL in addressbar is   http://www.rp-online.de/
ER:  Url in addressbar is   http://briangrinstead.com/files/sleepy.php


Affected:   Win7_64, Nightly 48, 32bit, ID 20160308030418
Fixeв on:   Win7_64, Nightly 48, 32bit, ID 20160309030419

Comment 53

[u595641]

bug 1354796

has brought this back :(

Comment 54

[Berend De Schouwer]

I can duplicate something similar in 50 release (which doesn't have #1354796), 53 release (which has #1354796) and nightly (which has a fix for #1354796)

On a slow loading site, change the URL, hit enter, and before it completes hit "stop" can show either the previous URL or about:blank.

In Firefox 50:
- open a slow (no syn/ack) site using "right click, new tab"
- go to new tab
- change URL to another site
- hit Enter
- before the load finishes, hit Esc
- URLBar = new site (correct)

In 53 and Nightly:
- open a slow (no syn/ack) site using "right click, new tab"
- go to new tab
- change URL to another site
- hit Enter
- before the load finishes, hit Esc
- URLBar = about:blank (incorrect)

In 50, 53 and Nightly:
- open a new tab
- go to a slow (no syn/ack) site
- change URL
- hit enter
- before the load finishes, hit Esc
- URLBar = new site (correct)

In 50, 53 and Nightly:
- open a new tab
- go to a slow (syn/ack, but slow/not cached) site
- change URL
- hit enter
- before the load finishes, hit Esc
- URLBar = old site (incorrect)

I've tried my best to open with empty profiles, so there's no cache.

To get the old site I have to hit Esc fairly quickly (as in, sub-one-second)

No syn/ack means no HTTP transfers have started.  No data is there to display.  Slow but not cached means HTTP transfers have started but not finished.  There likely will be some incomplete data to display.

If the load of the new site gets beyond a certain point, hitting Esc will result in the new site being partially displayed, and the new site being in the URL bar.

Comment 55

[:Gijs (out for now; he/him)]

(In reply to Sami from comment #53)
> bug 1354796
> 
> has brought this back :(

Then we should file a new bug, not reopen ones that have been fixed for well over a year.