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

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

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

This is effectively a clone of 798249 because people are reporting that's been regressed by 1354796.


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



(In reply to Berend De Schouwer from bug 798249 comment #54)
> 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.

This doesn't make sense. 53 doesn't, as far as I can tell, have the fix for 1354796 at all. It sounds like it was regressed by something else.

Comment 1

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

Sami, can you outline exact steps you're using to reproduce this bug, and if you checked for a formal regression window or if 1354796 is just a guess at what broke this?

Comment 2

[Berend De Schouwer]

I would guess it was regressed by the same thing that caused -- not fixed -- 1354796.  That would make sense (override URL bar from document window.)

If that's true, the fix for 1354796 is incomplete.

But I would also like more specific tests.  No syn/ack (will timeout) vs. slow (will complete); and load first URL from href vs. load first URL from user input.

Comment 3

[Dão Gottwald [:dao]]

protip:
If you write "bug xxxxxx" rather than just "xxxxxx", Bugzilla will automatically link to the bug and reveal the bug summary on hover 8-)

Comment 4

[u595641]

(In reply to :Gijs from comment #1)
> Sami, can you outline exact steps you're using to reproduce this bug, and if
> you checked for a formal regression window or if 1354796 is just a guess at
> what broke this?

Just throttle the connection to say 15kbps,
load group of bookmarks,
wait a few seconds and then follow the STR you mentioned.
Does not happen in non e but with e 4 cp 1 gpu 1 other , it's easily reproducible when content process are high value.
mozregressionpointed pointed to it but there were a few other bug which did not seem relevant.
Right now traveling or would have run it again and give all the ranges like some bugs which were not view able for permissions missing or something like from September last.

Comment 5

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

(In reply to Sami from comment #4)
> (In reply to :Gijs from comment #1)
> > Sami, can you outline exact steps you're using to reproduce this bug, and if
> > you checked for a formal regression window or if 1354796 is just a guess at
> > what broke this?
> 
> Just throttle the connection to say 15kbps,

How?

> load group of bookmarks,
How big a group? With what sites?

> wait a few seconds and then follow the STR you mentioned.

Which of the 4?

> Does not happen in non e but with e 4 cp 1 gpu 1 other , it's easily
> reproducible when content process are high value.

What does this mean?

> mozregressionpointed pointed to it

What (which bug?) is 'it'?

> but there were a few other bug which did
> not seem relevant.
> Right now traveling or would have run it again and give all the ranges like
> some bugs which were not view able for permissions missing or something like
> from September last.

bug 1354796 isn't from September, though bug 1284395 is (and is now open)... My understanding was that the issue reported in bug 1354796 is now resolved on nightly. Is it not for you? Or is it partly resolved? It's hard to tell without clarity about what exact steps you're using.

Comment 6

[u595641]

(In reply to :Gijs from comment #5)
> (In reply to Sami from comment #4)
> > (In reply to :Gijs from comment #1)
> > > Sami, can you outline exact steps you're using to reproduce this bug, and if
> > > you checked for a formal regression window or if 1354796 is just a guess at
> > > what broke this?
> > 
> > Just throttle the connection to say 15kbps,
> 
> How?

use any software like bandwidth limiter or 2g connection

> > load group of bookmarks,
> How big a group? With what sites?

10-15 bookmarks
 
> > wait a few seconds and then follow the STR you mentioned.
> 
> Which of the 4?


53 & nightly one

> > Does not happen in non e but with e 4 cp 1 gpu 1 other , it's easily
> > reproducible when content process are high value.
> 
> What does this mean?

4 content processes 1 gpu process , when content processes are higher than 8 its easily seen.
 
> > mozregressionpointed pointed to it
> 
> What (which bug?) is 'it'?

some security bug? not allowed to see it.
 
> > but there were a few other bug which did
> > not seem relevant.
> > Right now traveling or would have run it again and give all the ranges like
> > some bugs which were not view able for permissions missing or something like
> > from September last.
> 
> bug 1354796 isn't from September, though bug 1284395 is (and is now open)...
> My understanding was that the issue reported in bug 1354796 is now resolved
> on nightly. Is it not for you? Or is it partly resolved? It's hard to tell
> without clarity about what exact steps you're using.

yes these are new bugs but it all started with sept bug,
resurfaced in v52+

but can still reproduce in nightly 55

Comment 7

[Berend De Schouwer]

I ran mozregression, and got this:

92:56.02 INFO: Oh noes, no (more) inbound revisions :(
92:56.02 INFO: Last good revision: cde7a9badcab194e25adf5fc48a991f6fa41fb87
92:56.02 INFO: First bad revision: a45cfe898352daa445ebc702273c73c56b3ef3a1
92:56.02 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cde7a9badcab194e25adf5fc48a991f6fa41fb87&tochange=a45cfe898352daa445ebc702273c73c56b3ef3a1
92:59.15 INFO: Looks like the following bug has the  changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1284395


This is for the first behaviour:

- 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

In Firefox 50 this ends up with the user-entered URL bar.  In Firefox Nightly this ends up with about:blank.


This is the only regression I could find related to bug 798249  The other behaviours are inconsistent (sometimes about:blank, sometimes the new info, sometimes the old info); but aren't a regression per se, because the behaviour is inconsistent in the same way in Firefox 50. (inconsistent in a consistent way.)

Comment 8

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

(In reply to Berend De Schouwer from comment #7)
> This is for the first behaviour:
> 
> - open a slow (no syn/ack) site using "right click, new tab"

I was never able to do this successfully in bug 1354796. Can you elaborate on how I would reproduce this?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to :Gijs from comment #8)
> (In reply to Berend De Schouwer from comment #7)
> > This is for the first behaviour:
> > 
> > - open a slow (no syn/ack) site using "right click, new tab"
> 
> I was never able to do this successfully in bug 1354796. Can you elaborate
> on how I would reproduce this?

http://locahost:10000/ for instance, or any unallocated address in your local network (and the default port)

On Win, this will timeout in 21 seconds.  On some linux distros, this will timeout in 120 seconds, what will trigger our internal timeout (on application level, not the OS TCP stack's)

To trigger the application level timeout (ours) sooner, change "network.http.connection-timeout" to e.g. 3 (it's in seconds).

Comment 10

[Honza Bambas (:mayhemer)]

Note that the app level timeout cancellation error code has been changed in bug 1354796 which probably effects this bug (in good or bad, not sure anymore).

Comment 11

[Berend De Schouwer]

(In reply to :Gijs from comment #8)
> (In reply to Berend De Schouwer from comment #7)
> > This is for the first behaviour:
> > 
> > - open a slow (no syn/ack) site using "right click, new tab"
> 
> I was never able to do this successfully in bug 1354796. Can you elaborate
> on how I would reproduce this?

Setup a firewall.  eg. iptables --protocol tcp --destination 1.2.3.4/32 --jump DROP

I can try with a CGI script that has a "sleep 30" before sending a HTTP header.


Off-topic: Does Firefox have any bad comms unit tests?

Comment 12

[Berend De Schouwer]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> (In reply to :Gijs from comment #8)
> > (In reply to Berend De Schouwer from comment #7)
> > > This is for the first behaviour:
> > > 
> > > - open a slow (no syn/ack) site using "right click, new tab"
> > 
> > I was never able to do this successfully in bug 1354796. Can you elaborate
> > on how I would reproduce this?
> 
> http://locahost:10000/ for instance, or any unallocated address in your
> local network (and the default port)

Localhost can quite often result in a TCP RST, which will report an immediate error.  That's what it does on my Linux machine.

Comment 13

[Berend De Schouwer]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> Note that the app level timeout cancellation error code has been changed in
> bug 1354796 which probably effects this bug (in good or bad, not sure
> anymore).

No.

This regression is present before and after bug 1354796 fix in Nightly.  That's what running mozregression was for.

The cause for bug 1354796 also triggered this regression.

Comment 14

[Berend De Schouwer]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> To trigger the application level timeout (ours) sooner, change
> "network.http.connection-timeout" to e.g. 3 (it's in seconds).

This will make it more difficult to override the URL, since a timeout will occur sooner.  You'll only have 3 seconds to select the URL bar and submit a new request.

This will also hide bug 1354796 since that was caused by a mismatch between Firefox and OS socket timeouts.

Comment 15

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

(In reply to Berend De Schouwer from comment #11)
> (In reply to :Gijs from comment #8)
> > (In reply to Berend De Schouwer from comment #7)
> > > This is for the first behaviour:
> > > 
> > > - open a slow (no syn/ack) site using "right click, new tab"
> > 
> > I was never able to do this successfully in bug 1354796. Can you elaborate
> > on how I would reproduce this?
> 
> Setup a firewall.  eg. iptables --protocol tcp --destination 1.2.3.4/32
> --jump DROP

I'm sure I'm being dumb here, but this doesn't work:

gijs@linux-vm:~$ iptables --protocol tcp --destination 1.2.3.4/32 --jump DROP
iptables v1.6.0: no command specified
Try `iptables -h' or 'iptables --help' for more information.

Reading the help had me try:

gijs@linux-vm:~$ sudo iptables --append INPUT --protocol tcp --destination 1.2.3.4/32 --jump DROP

which doesn't seem to affect my browser in any way - ditto for chain "OUTPUT".

"Setup a firewall" can mean a lot of things. I don't really have time to immerse myself in iptables documentation, and potentially still end up with a different setup from yours. Please can you be more explicit?

(In reply to Berend De Schouwer from comment #11)
> I can try with a CGI script that has a "sleep 30" before sending a HTTP
> header.

Wouldn't that still report back that the tcp connection had been established at the point where the cgi script accepts the connection, before it starts sending anything back?

> Off-topic: Does Firefox have any bad comms unit tests?

The location bar has some tests for similar cases already, yes. I don't know about the network layer itself and unittests - Honza would be better-placed to answer that.

Comment 16

[Berend De Schouwer]

(In reply to :Gijs from comment #15)
> (In reply to Berend De Schouwer from comment #11)
> > (In reply to :Gijs from comment #8)
> > > (In reply to Berend De Schouwer from comment #7)
> > > > This is for the first behaviour:
> > > > 
> > > > - open a slow (no syn/ack) site using "right click, new tab"
> > > 
> > > I was never able to do this successfully in bug 1354796. Can you elaborate
> > > on how I would reproduce this?
> > 
> > Setup a firewall.  eg. iptables --protocol tcp --destination 1.2.3.4/32
> > --jump DROP
> 
> I'm sure I'm being dumb here, but this doesn't work:
> 
> gijs@linux-vm:~$ iptables --protocol tcp --destination 1.2.3.4/32 --jump DROP
> iptables v1.6.0: no command specified
> Try `iptables -h' or 'iptables --help' for more information.
> 
> Reading the help had me try:
> 
> gijs@linux-vm:~$ sudo iptables --append INPUT --protocol tcp --destination
> 1.2.3.4/32 --jump DROP
> 
> which doesn't seem to affect my browser in any way - ditto for chain
> "OUTPUT".

Apologies.  It was for OUTPUT.

In this case, unlike bug 1354796, I was simply attempting to have enough time to overwrite the URL bar with new user input, not trigger a TCP error.

I was also attempting to have enough time consistently for a regression test, so I simply dropped all packets.

Comment 17

[Berend De Schouwer]

(In reply to :Gijs from comment #0)
> This is effectively a clone of 798249 because people are reporting that's
> been regressed by 1354796.
> 
> 
> Original 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.


Can someone confirm that this is an exact regression?

While the behaviour *was* changed, I do not think this is an exact regression.

I think I tried to help by giving a more exact description of the changed behaviour.  So I described what *I saw*.  Just like bug 610357 there might be more than one bug, or more than one definition of the bug.


Behaviours I can see (going back to Firefox 17), after typing enter:
1. URL bar immediately shows old URL, then loads new URL, then shows new URL
2. URL bar stays on new URL while loading, loads, shows new URL
3. URL bar stays on new URL while loading, user interrupted, shows old URL
4. URL bar stays on new URL while loading, user interrupted, shows new URL
5. URL bar stays on new URL while loading, user interrupted, shows about:blank

I can make 2, 3, 4, 5 happen with Nightly.
I can make 1, 2, 3 happen with Firefox 34 (I think) 
I can make 2, 3, 4 happen with Firefox 50

(older Firefox is limited due to SSL errors)

What is the expected behaviour?

Comment 18

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

I ended up downloading gufw and just dropping everything, because Ubuntu's ufw firewall stuff is messy and seems to override the iptables config.

Now none of my loads succeed. But if I do the following:

1) open: data:text/html,<a href="http://www.google.com/">Click</a>
2) right click the link
3) click 'open link in new tab'
4) switch to this tab
5) in the url bar, put 'www.mozilla.org' and hit enter
6) some seconds later, hit [esc] on the keyboard to stop the load

the URL bar still says 'www.mozilla.org'


What's different in your steps? (The tab title is wrong on nightly only, that's bug 1367630, and an unrelated regression.)

Comment 19

[Berend De Schouwer]

(In reply to :Gijs from comment #18)
> I ended up downloading gufw and just dropping everything, because Ubuntu's
> ufw firewall stuff is messy and seems to override the iptables config.
> 
> Now none of my loads succeed. But if I do the following:
> 
> 1) open: data:text/html,<a href="http://www.google.com/">Click</a>
> 2) right click the link
> 3) click 'open link in new tab'
> 4) switch to this tab
> 5) in the url bar, put 'www.mozilla.org' and hit enter
> 6) some seconds later, hit [esc] on the keyboard to stop the load
> 
> the URL bar still says 'www.mozilla.org'
> 
> 
> What's different in your steps? (The tab title is wrong on nightly only,
> that's bug 1367630, and an unrelated regression.)

The only difference I can think off is timing.

If mozilla.org is slow (multi-seconds before HTML starts) I get about:blank
If mozilla.org is fast I get mozilla.org

But if you're dropping mozilla.org, ...

Comment 20

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

(In reply to Berend De Schouwer from comment #17)
> Behaviours I can see (going back to Firefox 17), after typing enter:
> 1. URL bar immediately shows old URL, then loads new URL, then shows new URL
> 2. URL bar stays on new URL while loading, loads, shows new URL
> 3. URL bar stays on new URL while loading, user interrupted, shows old URL
> 4. URL bar stays on new URL while loading, user interrupted, shows new URL
> 5. URL bar stays on new URL while loading, user interrupted, shows
> about:blank
> 
> I can make 2, 3, 4, 5 happen with Nightly.
> I can make 1, 2, 3 happen with Firefox 34 (I think) 
> I can make 2, 3, 4 happen with Firefox 50
> 
> (older Firefox is limited due to SSL errors)
> 
> What is the expected behaviour?

I'm a bit confused because the scenarios between (1/2) and (3/4/5) are different, right (whether the user interrupts a load or not)?

I think the ideal behaviour is (2) or (4) (depending on whether the user interrupts), and bug 798249 was supposed to fix (1) so that we'd have that. It sounds like that worked (in that (1) no longer happens in 50/nightly). It's quite possible there are further issues with what happens when loads are interrupted, and that's besides the more general issue of bug 610357 which is potentially partially responsible for (5).


This whole thing is messy. If there's a specific case that regressed due to bug 1284395 (which is what comment #7 implies) then we can track and hopefully fix that in this bug, but it would be really helpful if we were super clear on what exactly that scenario is.

This is why I keep banging on about having exact steps, because getting this right is hard and it's not just a case of "oh, let's just fix all of it". I would love to do that, but getting a URL bar to behave right is much harder than it might appear, mostly because it mixes user input and a security indication that is supposed to tell a user what they're looking at, and that should ideally never be out-of-step with what is in the content window. This is why the cases of cancelled loads are tricky: if the document in the content is from foo.com, even if you typed bar.com in the URL bar and hit enter, if you cancel the bar.com load, what we display depends on whether the content document is still from foo.com (or has been unloaded and replaced with a (potentially empty) document for bar.com.


(In reply to Berend De Schouwer from comment #19)
> > What's different in your steps? (The tab title is wrong on nightly only,
> > that's bug 1367630, and an unrelated regression.)
> 
> The only difference I can think off is timing.
> 
> If mozilla.org is slow (multi-seconds before HTML starts) I get about:blank
> If mozilla.org is fast I get mozilla.org
> 
> But if you're dropping mozilla.org, ...

I am. But I've just realized... what document is your link on? Is it an http(s) document? That might make a difference here.

Comment 21

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

(In reply to :Gijs from comment #20)
> (In reply to Berend De Schouwer from comment #19)
> > > What's different in your steps? (The tab title is wrong on nightly only,
> > > that's bug 1367630, and an unrelated regression.)
> > 
> > The only difference I can think off is timing.
> > 
> > If mozilla.org is slow (multi-seconds before HTML starts) I get about:blank
> > If mozilla.org is fast I get mozilla.org
> > 
> > But if you're dropping mozilla.org, ...
> 
> I am. But I've just realized... what document is your link on? Is it an
> http(s) document? That might make a difference here.

Yes, I can reproduce if the initial link is on a normal document rather than a manually loaded data: URI.

Comment 22

[Berend De Schouwer]

(In reply to :Gijs from comment #20)
> (In reply to Berend De Schouwer from comment #19)
> > > What's different in your steps? (The tab title is wrong on nightly only,
> > > that's bug 1367630, and an unrelated regression.)
> > 
> > The only difference I can think off is timing.
> > 
> > If mozilla.org is slow (multi-seconds before HTML starts) I get about:blank
> > If mozilla.org is fast I get mozilla.org
> > 
> > But if you're dropping mozilla.org, ...
> 
> I am. But I've just realized... what document is your link on? Is it an
> http(s) document? That might make a difference here.

It is http

Comment 23

[Berend De Schouwer]

(In reply to :Gijs from comment #20)
> (In reply to Berend De Schouwer from comment #17)
> > Behaviours I can see (going back to Firefox 17), after typing enter:
> > 1. URL bar immediately shows old URL, then loads new URL, then shows new URL
> > 2. URL bar stays on new URL while loading, loads, shows new URL
> > 3. URL bar stays on new URL while loading, user interrupted, shows old URL
> > 4. URL bar stays on new URL while loading, user interrupted, shows new URL
> > 5. URL bar stays on new URL while loading, user interrupted, shows
> > about:blank
> > 
> > I can make 2, 3, 4, 5 happen with Nightly.
> > I can make 1, 2, 3 happen with Firefox 34 (I think) 
> > I can make 2, 3, 4 happen with Firefox 50
> > 
> > (older Firefox is limited due to SSL errors)
> > 
> > What is the expected behaviour?
> 
> I'm a bit confused because the scenarios between (1/2) and (3/4/5) are
> different, right (whether the user interrupts a load or not)?

That's why I asked the question.  My understanding of bug 798249 was that it was filed as 1, but turned into 3.

3, 4 and 5 can be triggered on partial loads, depending on how quick you are.  So it may be necessary to file multiple bugreports.  There is, however, no point in splitting them if that's the expected behaviour, and it results in a WONTFIX.

Comment 24

[Berend De Schouwer]

(In reply to :Gijs from comment #20) 
> This is why I keep banging on about having exact steps, because getting this
> right is hard and it's not just a case of "oh, let's just fix all of it".

That's bug 0

Comment 25

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

The root cause here is that, in the scenario we're testing:

(In reply to :Gijs from comment #18)

0) drop all outgoing network packets

> 1) open a page with a link like this <a href="http://www.google.com/">Click</a> that has a codebase principal (ie is served from http(s)://somewhere )
> 2) right click the link
> 3) click 'open link in new tab'
> 4) switch to this tab

At this point, the tab contains about:blank that's inherited the principal of the page that had the link.

Prior to bug 1284395, it was just whatever principal any old tab that loads its initial about:blank ends up with (probably a null principal).

> 5) in the url bar, put 'www.mozilla.org' and hit enter
> 6) some seconds later, hit [esc] on the keyboard to stop the load


At this point, this code: https://dxr.mozilla.org/mozilla-central/rev/f7adbf457ee20eeffde72694e0d17d73616e3cfd/browser/base/content/tabbrowser.xml#713-723

decides that (a) the load isn't successful (which is true) and (b) that *the tab isn't empty*. This is now the case because of bug 1284395. As a result, we reset the URL bar.

(In my original steps in comment #18 where I couldn't reproduce, when using a data: URI, the page will have a null principal which satisfies the isTabEmpty() function, which means we treat the tab as empty, which means we won't re-set the URL bar here, which is why I originally couldn't reproduce)



There are two tempting fixes. One is to treat these about:blanks that have some other principal as empty. Unfortunately that would create security issues, because although in this case about:blank is independent from the page on which the link existed, there are other steps where that would not be the case, and we don't want to show an empty URL bar for a page that's web-controlled.

So the correct fix, I guess, is to avoid changing the principal of about:blank in cases where we're sure the web page doesn't control the resulting document. Specifically, in the right click -> open in new tab/window case when not using a URL that inherits principals (ie where the URL itself provided by the original page runs code, so data: and javascript: URIs).

I think this has always been broken for the <a href="foo" target="_blank">link</a> case - that is, using the same steps but target=_blank (and not the context menu) to open the initial new tab. That displays about:blank for me initially, even on Firefox 50, which comes back squarely to bug 610357.

Comment 26

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

Attachment: Bug 1366203 - revert about blank creation for URIs that don't inherit principals,

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

Comment 27

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

Comment on attachment 8872369 [details]
Bug 1366203 - revert about blank creation for URIs that don't inherit principals,

https://reviewboard.mozilla.org/r/143856/#review147662

Thanks Gijs!

::: browser/base/content/test/urlbar/browser_urlbar_stop_pending.js:43
(Diff revision 1)
>    obs.disconnect();
>    obs = null;
>    await BrowserTestUtils.removeTab(tab);
>  });
>  
> +add_task(async function() {

Mind adding a quick comment above this test describing what it's testing?

::: browser/base/content/test/urlbar/browser_urlbar_stop_pending.js:45
(Diff revision 1)
> +  socket.init(9999, true, -1);
> +  let socket2 = Cc["@mozilla.org/network/server-socket;1"].createInstance(Ci.nsIServerSocket);
> +  socket2.init(9998, true, -1);

I'm a tad worried about hardcoding ports here - it'd be great if we could dynamically bind to a port we _know_ will be free...

According to nsIServerSocket.idl, if we pass -1 as the port, it'll choose one that's free, and then we can get the port via `socket.port`. Can we go that route instead?

::: browser/base/content/test/urlbar/browser_urlbar_stop_pending.js:64
(Diff revision 1)
> +  let newTabPromise = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen");
> +  // Middle click the link:
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#clickme", { button: 1 }, tab.linkedBrowser);
> +  // get new tab, switch to it
> +  let newTab = (await newTabPromise).target;
> +  await BrowserTestUtils.switchTab(gBrowser, newTab);

Can't we wait for this tab with BrowserTestUtils.waitForNewTab, or did I miss something?

Comment 28

[u595641]

(In reply to Berend De Schouwer from comment #17)
> (In reply to :Gijs from comment #0)
> > This is effectively a clone of 798249 because people are reporting that's
> > been regressed by 1354796.
> > 
> > 
> > Original 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.
> 
> 
> Can someone confirm that this is an exact regression?
> 
> While the behaviour *was* changed, I do not think this is an exact
> regression.
> 
> I think I tried to help by giving a more exact description of the changed
> behaviour.  So I described what *I saw*.  Just like bug 610357 there might
> be more than one bug, or more than one definition of the bug.
> 
> 
> Behaviours I can see (going back to Firefox 17), after typing enter:
> 1. URL bar immediately shows old URL, then loads new URL, then shows new URL
> 2. URL bar stays on new URL while loading, loads, shows new URL
> 3. URL bar stays on new URL while loading, user interrupted, shows old URL
> 4. URL bar stays on new URL while loading, user interrupted, shows new URL
> 5. URL bar stays on new URL while loading, user interrupted, shows
> about:blank
> 
> I can make 2, 3, 4, 5 happen with Nightly.
> I can make 1, 2, 3 happen with Firefox 34 (I think) 
> I can make 2, 3, 4 happen with Firefox 50
> 
> (older Firefox is limited due to SSL errors)
> 
> What is the expected behaviour?

Look at this it's a bit clear explanation and similar to what facing on my end.

Comment 29

[Berend De Schouwer]

(In reply to Sami from comment #28)
> Look at this it's a bit clear explanation and similar to what facing on my
> end.

What info do you need from me?  I wrote that comment; I have looked at it :)

When you say similar, do you mean exact or just similar, and what behaviour do you expect?  You commented that bug 
798249 was back, so I'd like to know exactly what behaviour you see, and what behaviour you expect instead.

Anyway, thanks Gijs for fixing the exact behaviour that *I* see.

Comment 30

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

Comment on attachment 8872369 [details]
Bug 1366203 - revert about blank creation for URIs that don't inherit principals,

https://reviewboard.mozilla.org/r/143856/#review147662

> Mind adding a quick comment above this test describing what it's testing?

Yep, sorry for missing that out.

> I'm a tad worried about hardcoding ports here - it'd be great if we could dynamically bind to a port we _know_ will be free...
> 
> According to nsIServerSocket.idl, if we pass -1 as the port, it'll choose one that's free, and then we can get the port via `socket.port`. Can we go that route instead?

Nice catch! Yes, that works.

I also realized that when you just load a URL in a new tab via middle-click / context menu, and then hit [stop] before the socket responds, that is also broken, and also fixed by this patch (ie the second URL load isn't necessary - I guess we're working with it because of the origins of bug 798249). This makes sense given the earlier diagnosis I posted in comment #25... so I guess I'll add a separate test for that, too.

> Can't we wait for this tab with BrowserTestUtils.waitForNewTab, or did I miss something?

`waitForNewTab` waits for onLocationChange to fire, which won't happen until the http request starts, which AIUI won't happen until the server sends headers, which in this test never happens. Certainly practically speaking, when I tried to use it, the test got stuck at that point.

Comment 31

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

Comment on attachment 8872369 [details]
Bug 1366203 - revert about blank creation for URIs that don't inherit principals,

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

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d98a318d6ec
revert about blank creation for URIs that don't inherit principals, r=mconley

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d98a318d6ec

Comment 34

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

Comment on attachment 8872369 [details]
Bug 1366203 - revert about blank creation for URIs that don't inherit principals,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1284395
[User impact if declined]: dataloss in the URL bar if a request for a URL opened via middle-click/accel-click/context menu + open in new tab/window/whatever doesn't complete
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not manually, but I'm pretty confident in the automated tests.
[Needs manual test from QE? If yes, steps to reproduce]: if this is landing late in beta, it'd probably be a sensible idea, yes.

STR:
1. drop outgoing network traffic. Various ways of doing this:
- install and run gufw on Linux (turn on ufw via: sudo ufw enable) and simply flip the switch on 'outgoing' traffic to 'deny'
- connect up to your usual wifi, but disconnect wifi at the network end
- run a local port but don't accept any connections. E.g. run this in the browser console:
socket = Cc["@mozilla.org/network/server-socket;1"].createInstance(Ci.nsIServerSocket);
socket.init(-1, true, -1);
socket.port

will return a port number, and then you should be able to reproduce not being able to connect by putting:

localhost:<portnumber>

in the location bar.

2. create a local html file with a simple HTML link (<a href="...">) pointing to an unreachable address
3. open the local html file (either via file: or served by a local http server)
4. middle click the link / right click and open in new tab / etc. to open the link in a different tab/window
5. switch to that tab/window
6. hit 'stop' (escape key, stop toolbar button, etc.)

ER:
the location bar still contains the original address where the link was supposed to go
AR:
the location bar contains about:blank

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not too risky given the cost/benefit
[Why is the change risky/not risky?]:  We have tests for the security fix that regressed this, and they continue to pass. We also have a lot of location bar tests, and this patch adds new tests. They all seem to pass fine, too. The majority of this patch is test changes - the actual code changes are relatively small.
[String changes made/needed]: no

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8872369 [details]
Bug 1366203 - revert about blank creation for URIs that don't inherit principals,

This seems to be a severe enough problem to warrant an uplift to beta54.

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Andrei, while this has automated test coverage, since it's a pretty commonly used core functionality, could you please have the team test this a bit thoroughly during b13 sign-offs? Thanks!

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Gijs, should we consider uplifting this to ESR52 as well?

Comment 38

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

Attachment: Patch for beta

Comment 39

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

Comment on attachment 8873543 [details] [diff] [review]
Patch for beta

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
common functionality
User impact if declined:
dataloss in URL bar if you use right-click + open in new tab/window/whatever, and then the request fails
Fix Landed on Version: 55, uplift to 54
Risk to taking this patch (and alternatives if risky): reeeeaaasonably low, given size of patch, and automated tests. Alternative is leaving 52esr unfixed. The automated tests and added QE will help reduce this risk.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 40

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

Attachment: Patch for beta

Now with less async function because beta/esr.

Comment 41

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

Comment on attachment 8873545 [details] [diff] [review]
Patch for beta

(In reply to :Gijs from comment #39)
> Comment on attachment 8873543 [details] [diff] [review]
> Patch for beta
> 
> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR
> consideration:
> common functionality that is broken.
> User impact if declined:
> dataloss in URL bar if you use right-click + open in new
> tab/window/whatever, and then the request fails
> Fix Landed on Version: 55, uplift to 54
> Risk to taking this patch (and alternatives if risky): reeeeaaasonably low,
> given size of patch, and automated tests. Alternative is leaving 52esr
> unfixed. The automated tests and added QE will help reduce this risk.
> String or UUID changes made by this patch: none
> 
> See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more
> info.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/10fd5abbf547

Comment 43

[u595893]

Attachment: Clipboard01.jpg

Comment 44

[u595893]

When the connection times-out the url is retained but cannot be reloaded or refreshed
Please look at the screen shot.
Please Fix

Comment 45

[u595893]

Refresh using refresh button or kb shortcut.

Comment 46

[u595893]

another bug,
restarting during this the tabs are discarded and auto-removed from the session and url is lost aka Data-loss
use restart shortcut or any addon

Please file bug if not correct bug.

Comment 47

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

(In reply to shellye from comment #44)
> When the connection times-out the url is retained but cannot be reloaded or
> refreshed

The screenshot is not helpful - I have no idea what happened there. At a guess, this issue pre-dates the regression this bug fixes - I can reproduce something vaguely similar (where about:blank remains loaded without an error page) on Firefox 51. The issue here is that the network request aborts without ever loading anything - it's supposed to load an error page instead. This is if you don't abort the request yourself. My understanding was that bug 1354796 should fix this. And indeed, on Linux at least, I can't reproduce on an up-to-date Nightly 55 when trying to connect to an unreachable site - I correctly get a "The connection has timed out" error page and the refresh button / try again works correctly.

If you're still seeing this issue on Nightly please file a new bug WITH DETAILED STEPS TO REPRODUCE, like what platform you're seeing this on, exactly how you're connecting / how the connection fails (are you manually hitting stop/esc?), what version of Firefox you're testing with, etc. etc.



(In reply to shellye from comment #46)
> another bug,
> restarting during this the tabs are discarded and auto-removed from the
> session and url is lost aka Data-loss
> use restart shortcut or any addon

This is bug 610357.

Comment 48

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I'd like to keep this uplift nom around and take this in ESR52.3. By then, the fix will have stabilized enough. This is a wontfix for ESR52.2.

Comment 49

[u595893]

(In reply to :Gijs from comment #47)
> (In reply to shellye from comment #44)
> > When the connection times-out the url is retained but cannot be reloaded or
> > refreshed
> 
> The screenshot is not helpful - I have no idea what happened there. At a
> guess, this issue pre-dates the regression this bug fixes - I can reproduce
> something vaguely similar (where about:blank remains loaded without an error
> page) on Firefox 51. The issue here is that the network request aborts
> without ever loading anything - it's supposed to load an error page instead.
> This is if you don't abort the request yourself. My understanding was that
> bug 1354796 should fix this. And indeed, on Linux at least, I can't
> reproduce on an up-to-date Nightly 55 when trying to connect to an
> unreachable site - I correctly get a "The connection has timed out" error
> page and the refresh button / try again works correctly.
> 
> If you're still seeing this issue on Nightly please file a new bug WITH
> DETAILED STEPS TO REPRODUCE, like what platform you're seeing this on,
> exactly how you're connecting / how the connection fails (are you manually
> hitting stop/esc?), what version of Firefox you're testing with, etc. etc.
> 
> 
> 
> (In reply to shellye from comment #46)
> > another bug,
> > restarting during this the tabs are discarded and auto-removed from the
> > session and url is lost aka Data-loss
> > use restart shortcut or any addon
> 
> This is bug 610357.

I tried in on win10 home with system dns cache disabled & on linux this does not happen afaik,
never seeing timeouts or network error pages.
No steps just started loading and the spinners spin but when they stop no error only the screenshot is what happens,
no clue whats going on.

Comment 50

[u595893]

(In reply to Ritu Kothari (:ritu) from comment #48)
> I'd like to keep this uplift nom around and take this in ESR52.3. By then,
> the fix will have stabilized enough. This is a wontfix for ESR52.2.

This fix partially fixes the url info lost when times out and works well except the problem which i mentioned above which already happens in ESR52 but even worse.
So this bug should be uplifted in ESR52 as user can still copy the url and paste & go which ESR52 does not have due to loss of information.

But you guys know better.

Hoping this also lands with 
> (In reply to shellye from comment #46)
> > another bug,
> > restarting during this the tabs are discarded and auto-removed from the
> > session and url is lost aka Data-loss
> > use restart shortcut or any addon
> 
> This is bug 610357.

Comment 51

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

(In reply to shellye from comment #49)
> I tried in on win10 home with system dns cache disabled & on linux this does
> not happen afaik,
> never seeing timeouts or network error pages.
> No steps just started loading and the spinners spin but when they stop no
> error only the screenshot is what happens,
> no clue whats going on.

File a NEW BUT WITH STEPS (what even is the 'system dns cache', how did you disable it, what site did you load and why was it not reachable, etc. etc.). STOP COMMENTING HERE. Also, don't mess with the tracking flags.

Re-requesting tracking. :-\

Comment 52

[Julien Cristau [:jcristau]]

Resetting tracking flag

Comment 53

[Gabi Cheta]

Verified as fixed using Fx 54.0 Build ID: 20170605134926 on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04.

Comment 54

[u595893]

(In reply to :Gijs from comment #51)
> (In reply to shellye from comment #49)
> > I tried in on win10 home with system dns cache disabled & on linux this does
> > not happen afaik,
> > never seeing timeouts or network error pages.
> > No steps just started loading and the spinners spin but when they stop no
> > error only the screenshot is what happens,
> > no clue whats going on.
> 
> File a NEW BUT WITH STEPS (what even is the 'system dns cache', how did you
> disable it, what site did you load and why was it not reachable, etc. etc.).
> STOP COMMENTING HERE. Also, don't mess with the tracking flags.
> 
> Re-requesting tracking. :-\

FYI i did not reset tracking flags see c48
Collision occurred - submit changes -yes 

Win10 services dns cache - disable

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Looks like it doesn't apply to esr:
grafting 402228:8d98a318d6ec "Bug 1366203 - revert about blank creation for URIs that don't inherit principals, r=mconley"
merging browser/base/content/tabbrowser.xml
merging browser/base/content/test/urlbar/browser_urlbar_stop_pending.js
merging browser/base/content/utilityOverlay.js
 warning: conflicts while merging browser/base/content/tabbrowser.xml! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/base/content/utilityOverlay.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 56

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

Attachment: Patch for ESR 52

The beta patch applied better, though that too had one (smaller, different) conflict.

Comment 57

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

https://wiki.mozilla.org/Release_Management/Release_owners doesn't list anyone for the 55 esr cycle, and this has missed the 54 one. Julien or Liz, can you make a decision on the approval request? I get release tracking emails for this bug every week but AFAICT there's nothing else I can do here.

Comment 58

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8873545 [details] [diff] [review]
Patch for beta

Looks like Ritu intends this for 52.3.0esr in comment 48. Fix for location bar regression in 52 caused by a security patch.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/08dd87b6bb8f

Comment 60

[Gabi Cheta [:Gabi] Desktop QA]

Issue verified as fixed in 55.0b6 BUILD ID: 20170629005143 also.