Closed Bug 1226781 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- verified

People

(Reporter: dagger.bugzilla, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
Status: UNCONFIRMED → NEW
Component: Location Bar → Tabbed Browser
Ever confirmed: true
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)
(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)
I'll try to take a look either later this week or next week.
Flags: needinfo?(gijskruitbosch+bugs)
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.
(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: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(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+
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
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+
(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)
https://hg.mozilla.org/mozilla-central/rev/b6fc3f88058a
https://hg.mozilla.org/mozilla-central/rev/eebe1132e3eb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
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
Depends on: 1336335
You need to log in before you can comment on or make changes to this bug.