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

VERIFIED FIXED in Firefox 47

Status

()

VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: dagger.bugzilla, Assigned: Gijs)

Tracking

({regression})

Trunk
Firefox 47
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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.

Updated

3 years ago
Status: UNCONFIRMED → NEW
Component: Location Bar → Tabbed Browser
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
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?
Blocks: 1099857
Flags: needinfo?(ttaubert)
Keywords: regressionwindow-wanted
(Assignee)

Comment 2

3 years ago
(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...
(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.
Flags: needinfo?(ttaubert) → needinfo?(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. :/
Flags: needinfo?(mconley)
(Assignee)

Comment 5

3 years ago
I'll try to take a look either later this week or next week.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
(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).
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 8

3 years ago
Created attachment 8711862 [details]
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/
Attachment #8711862 - Flags: review?(mconley)
(Assignee)

Comment 9

3 years ago
(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 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?
Attachment #8711862 - Flags: review?(mconley) → review+
(Assignee)

Comment 11

3 years ago
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/
Attachment #8711862 - Attachment description: MozReview Request: Bug 1226781 - only manually switch remoteness if we were trying to switch remoteness to begin with, r?mconley → MozReview Request: Bug 1226781 - only manually switch remoteness if we were trying to switch remoteness to begin with, r=mconley
(Assignee)

Comment 12

3 years ago
Created 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

Review commit: https://reviewboard.mozilla.org/r/32475/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32475/
Attachment #8712207 - Flags: review?(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?
Attachment #8712207 - Flags: review?(mconley) → review+
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
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 17

3 years ago
(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
Flags: needinfo?(gijskruitbosch+bugs)

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6fc3f88058a
https://hg.mozilla.org/mozilla-central/rev/eebe1132e3eb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Clicking back now takes you back to the correct page. Marking as VERIFIED FIXED
Status: RESOLVED → VERIFIED
QA Whiteboard: bugday-20160203
status-firefox47: fixed → verified

Updated

2 years ago
Depends on: 1336335
You need to log in before you can comment on or make changes to this bug.