1226781 - When entering an invalid URI with keyword.enabled=false, the error page doesn't go into the tab's history

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[dagger.bugzilla]

STR:

1) Set keyword.enabled = false
2) Open a new tab and load, say, https://hg.mozilla.org/
3) Type "2600::" into the address bar and hit enter

I'd expect to be able to go Back from the resulting error page, but instead Back is disabled.

If you do this instead:

1) Set keyword.enabled = false
2) Open a new tab and load https://hg.mozilla.org/
3) Click on "mozilla-central"
3) Type "2600::" into the address bar and hit enter

then Back will be enabled, but it'll take you to https://hg.mozilla.org/ (the first page) rather than https://hg.mozilla.org/mozilla-central (the second one). So it looks like the error page just replaces the currently-loaded page without interacting with the tab's history at all.

If you enable e10s on a current Nightly then the problem seems to go away (although e10s has other issues with loading these error pages). I can't test with e10s on the builds where this started showing up though, because on those builds it refuses to activate for me.

Regression range: between 2014-11-17 and 2014-11-18's nightlies.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a52bf59965a0&tochange=084441e904d1

I'm not sure which bug regressed it; none of them look particularly likely. Last year's hourly builds are of course long gone, so I can't narrow it down any further either.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Local bisection narrowed it down to https://hg.mozilla.org/integration/fx-team/rev/dd555a81cbba. Guess that sheds some light on when it only reproduces with non-e10s as well.

Tim, I know you're not working on Fx front end anymore, but can you suggest someone else who might be able to investigate?

Comment 2

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> Local bisection narrowed it down to
> https://hg.mozilla.org/integration/fx-team/rev/dd555a81cbba. Guess that
> sheds some light on when it only reproduces with non-e10s as well.
> 
> Tim, I know you're not working on Fx front end anymore, but can you suggest
> someone else who might be able to investigate?

Purely based on that diff it seems like only the LoadInOtherProcess call's exceptions should be triggering the catch... code, and the fact that the same-process-load ones (the only kind, in non-e10s) are also doing that is what's wrong here. That said, not sure why the catch statement's work there causes us to lose a history entry...

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> Tim, I know you're not working on Fx front end anymore, but can you suggest
> someone else who might be able to investigate?

Mike has been doing quite some e10s/sessionstore stuff lately. Hope he has a few free cycles to investigate further.

Comment 4

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

(In reply to Tim Taubert [:ttaubert] from comment #3)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> > Tim, I know you're not working on Fx front end anymore, but can you suggest
> > someone else who might be able to investigate?
> 
> Mike has been doing quite some e10s/sessionstore stuff lately. Hope he has a
> few free cycles to investigate further.

I'm afraid that's not the case. :/

Comment 5

[:Gijs (he/him)]

I'll try to take a look either later this week or next week.

Comment 6

[:Gijs (he/him)]

So the regression identified is pretty easy to fix. The issue is just that there must be some other regression, because if I fix this particular bug, and I do:

1) load https://hg.mozilla.org/
2) load ::2600 (results in error page)
3) go back
4) go forward

I end up with about:blank loading. That is strictly better than the current situation, but still hardly ideal... In Firefox 35/36 before the regression, at least, these steps "just work". Presumably the first regression hid the next one. :-(

Poking some more.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> So the regression identified is pretty easy to fix. The issue is just that
> there must be some other regression, because if I fix this particular bug,
> and I do:
> 
> 1) load https://hg.mozilla.org/
> 2) load ::2600 (results in error page)
> 3) go back
> 4) go forward
> 
> I end up with about:blank loading. That is strictly better than the current
> situation, but still hardly ideal... In Firefox 35/36 before the regression,
> at least, these steps "just work". Presumably the first regression hid the
> next one. :-(
> 
> Poking some more.

OK, finally figured it out (manually regression-tested with local builds with a patch for the bug as described in comment #0, and then confirmed with local backout). The second bug here is caused by the code we landed as part of bug 1199430. I believe this is now throwing an error earlier, because it isn't a valid IPv6 url, and that triggers a different codepath. I would find out through debugging exactly how it used to work, but it seems that browser debugging in that timeframe was hosed (bug 1214663).

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1226781 - only manually switch remoteness if we were trying to switch remoteness to begin with, r=mconley

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

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > So the regression identified is pretty easy to fix. The issue is just that
> > there must be some other regression, because if I fix this particular bug,
> > and I do:
> > 
> > 1) load https://hg.mozilla.org/
> > 2) load ::2600 (results in error page)
> > 3) go back
> > 4) go forward
> > 
> > I end up with about:blank loading. That is strictly better than the current
> > situation, but still hardly ideal... In Firefox 35/36 before the regression,
> > at least, these steps "just work". Presumably the first regression hid the
> > next one. :-(
> > 
> > Poking some more.
> 
> OK, finally figured it out (manually regression-tested with local builds
> with a patch for the bug as described in comment #0, and then confirmed with
> local backout). The second bug here is caused by the code we landed as part
> of bug 1199430. I believe this is now throwing an error earlier, because it
> isn't a valid IPv6 url, and that triggers a different codepath. I would find
> out through debugging exactly how it used to work, but it seems that browser
> debugging in that timeframe was hosed (bug 1214663).

It seems that the basic issue here is that the URL used to be a valid URI (but with an invalid host, so you used to get the "can't reach this host" error page) and could be stored correctly in session history, and now it can't be stored at all, so we store about:blank instead... I don't know how to fix that, so I've just fixed the original issue as filed - we no longer overwrite other session history entries. If we want to investigate the remaining issue, let's do that in a separate bug, but I'm not sure it can sanely be fixed...

Comment 10

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

Comment on attachment 8711862 [details]
MozReview Request: Bug 1226781 - only manually switch remoteness if we were trying to switch remoteness to begin with, r=mconley

https://reviewboard.mozilla.org/r/32359/#review29045

LGTM. How hard do you think it'd be to write a quick regression test?

Comment 11

[:Gijs (he/him)]

Comment on attachment 8711862 [details]
MozReview Request: Bug 1226781 - only manually switch remoteness if we were trying to switch remoteness to begin with, r=mconley

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

Comment 12

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1226781 - add a regression test for going back from pages that cause about:neterror to be shown, r?mconley

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

Comment 13

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

Comment on attachment 8712207 [details]
MozReview Request: Bug 1226781 - add a regression test for going back from pages that cause about:neterror to be shown, r?mconley

https://reviewboard.mozilla.org/r/32475/#review29177

This looks fine to me. Just a few minor suggestions. Thanks Gijs!

::: browser/base/content/test/general/browser_invalid_uri_back_forward_manipulation.js:5
(Diff revision 1)
> +  Services.prefs.clearUserPref("keyword.enabled");

Pro-tip! You can skip this step by using:

```JavaScript
yield pushPrefs(["keyword.enabled", false]);
```

in your test. Once the test completes, the preference will automatically be reset.

pushPrefs is from https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/browser/base/content/test/general/head.js#198

::: browser/base/content/test/general/browser_invalid_uri_back_forward_manipulation.js:7
(Diff revision 1)
> +

Can you add a docstring to document what is being tested here exactly?

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/627ac298f230
https://hg.mozilla.org/integration/fx-team/rev/3ae6880a94e4

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for breaking the test it added in e10s mochitest runs: https://treeherder.mozilla.org/logviewer.html#?job_id=6855857&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/8696e9783a9f

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b6fc3f88058a
https://hg.mozilla.org/integration/fx-team/rev/eebe1132e3eb

Comment 17

[:Gijs (he/him)]

(In reply to Wes Kocher (:KWierso) from comment #15)
> Backed out for breaking the test it added in e10s mochitest runs:
> https://treeherder.mozilla.org/logviewer.html#?job_id=6855857&repo=fx-team
> 
> https://hg.mozilla.org/integration/fx-team/rev/8696e9783a9f

Fixed by using an event listener that will fire for e10s (passing wantsUntrusted=true to addEventListener) like the ssl error report tests.

remote:   https://hg.mozilla.org/integration/fx-team/rev/b6fc3f88058a
remote:   https://hg.mozilla.org/integration/fx-team/rev/eebe1132e3eb

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6fc3f88058a
https://hg.mozilla.org/mozilla-central/rev/eebe1132e3eb

Comment 19

[Vuyisile Ndlovu [:terrameijar]]

Clicking back now takes you back to the correct page. Marking as VERIFIED FIXED