Last Comment Bug 724239 - Loading a page in a new tab enables the back button
: Loading a page in a new tab enables the back button
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 16
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 751107 777782 (view as bug list)
Depends on: 729063 1077738 1084471
Blocks: 776167 455553 CVE-2012-3965 860084
  Show dependency treegraph
 
Reported: 2012-02-04 05:24 PST by Dão Gottwald [:dao]
Modified: 2014-10-17 09:52 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
verified
unaffected


Attachments
patch (1.37 KB, patch)
2012-02-09 08:02 PST, Dão Gottwald [:dao]
gavin.sharp: review-
Details | Diff | Splinter Review
do not add about:newtab to the session history (625 bytes, patch)
2012-07-02 16:39 PDT, Tim Taubert [:ttaubert]
bzbarsky: review-
gavin.sharp: feedback+
Details | Diff | Splinter Review
test to make sure about:newtab is not added to the session history (1.87 KB, patch)
2012-07-03 05:57 PDT, Tim Taubert [:ttaubert]
gavin.sharp: review+
Details | Diff | Splinter Review
do not add about:newtab (or any custom newtab url) to the session history (1.35 KB, patch)
2012-07-10 02:51 PDT, Tim Taubert [:ttaubert]
bzbarsky: review+
gavin.sharp: feedback+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-02-04 05:24:59 PST
STR:
- open a new tab
- load a page
Comment 1 Tim Taubert [:ttaubert] 2012-02-04 05:32:03 PST
That's not a regression but rather behavior worth talking about. If you look at Chrome, they do the same as we currently do. So we should figure out which behavior we think is best.

I actually think that it provides benefit to users that use the back button to go back to the new tab page and visit a different site from there again.
Comment 2 Dão Gottwald [:dao] 2012-02-04 06:32:02 PST
(In reply to Tim Taubert [:ttaubert] from comment #1)
> I actually think that it provides benefit to users that use the back button
> to go back to the new tab page and visit a different site from there again.

I don't really think this makes much sense. Accel+t and the new tab button are the obvious ways to get that page. Being able to go back is especially unexpected if you consider that the loaded page may be opened from the location bar, a bookmark, etc., so the new tab page may not exist as a step at all in the users mind. This is exactly what happened to me and made me believe I accidentally replaced some page rather than opening a new tab.
Comment 3 Dão Gottwald [:dao] 2012-02-05 12:34:35 PST
And of course this also happens with about:newtab's grid disabled.
Comment 4 Tiger.711 2012-02-05 22:57:38 PST
I also think that about: newtab should be processed as blank tab
Comment 5 Dão Gottwald [:dao] 2012-02-09 08:02:22 PST
Created attachment 595746 [details] [diff] [review]
patch
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-09 14:42:46 PST
Can we instead just change nsDocShell::ShouldAddToSessionHistory to return false for about:newtab, in addition to about:blank?
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-09 14:44:37 PST
(or maybe have it check an app-specific pref or something)
Comment 8 Alex Limi (:limi) — Firefox UX Team 2012-02-10 15:50:48 PST
Back button should work to get back to the starting point.

A very obvious case is if you click the wrong thumbnail, and end up on the wrong site. You shouldn't have to close the tab, open a new tab, and click again. Back should always work when there's content there.

There's also more links and content coming to the new tab page, so the current behavior is correct, and should be kept.
Comment 9 Dão Gottwald [:dao] 2012-02-11 05:10:25 PST
See comment 3.
Comment 10 Tim Taubert [:ttaubert] 2012-05-09 11:02:19 PDT
*** Bug 751107 has been marked as a duplicate of this bug. ***
Comment 11 Willy_ Foo_Foo 2012-05-20 21:18:39 PDT
when using small icons(theme) the back button should be hidden too
Comment 12 Sam 2012-06-10 16:05:15 PDT
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #8)
> Back button should work to get back to the starting point.
> 
> A very obvious case is if you click the wrong thumbnail, and end up on the
> wrong site. You shouldn't have to close the tab, open a new tab, and click
> again. Back should always work when there's content there.
> 
> There's also more links and content coming to the new tab page, so the
> current behavior is correct, and should be kept.

when you enter your own URL you still have NTP in the tab history/back button/ which - by your own logic shouldn't be - as I didn't use NTP's tiles/contents to navigate to my target page.

Right now users have to tweak about:config stettings to (partially) fix this broken/lacking tabflow.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-29 13:45:33 PDT
The "click wrong thumbnail, need to try again" scenario from comment 8 is compelling, but I'm not sure how it weighs against the scenarios described in comment 2.

UX considerations aside, there are security implications to allowing a web site to trigger the loading of a chrome-privileged page, so until we can make about:newtab not be privileged, I think we're going to need to fix this (per comment 6).
Comment 14 Tim Taubert [:ttaubert] 2012-07-02 16:39:14 PDT
Created attachment 638535 [details] [diff] [review]
do not add about:newtab to the session history
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-03 04:59:07 PDT
Comment on attachment 638535 [details] [diff] [review]
do not add about:newtab to the session history

Looks good to me, but bz should r+. Can you add a test that checks that the back button is appropriately disabled when navigating away?
Comment 16 Tim Taubert [:ttaubert] 2012-07-03 05:57:48 PDT
Created attachment 638676 [details] [diff] [review]
test to make sure about:newtab is not added to the session history
Comment 17 Boris Zbarsky [:bz] 2012-07-06 13:09:20 PDT
Comment on attachment 638535 [details] [diff] [review]
do not add about:newtab to the session history

I'm not happy with this hardcoding, because apps other than Firefox might use other things than about:newtab.  And even for Firefox this is pref-controllable.

Should this instead be testing for the initial tab URI, whatever that is?  If we don't want it for random values of that pref, then we should add an API for docshell to ask the UI whether the given URI should be added.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-06 13:18:00 PDT
(In reply to Boris Zbarsky (:bz) from comment #17)
> Should this instead be testing for the initial tab URI, whatever that is? 
> If we don't want it for random values of that pref, then we should add an
> API for docshell to ask the UI whether the given URI should be added.

I don't think we want this to kick in for arbitrary values of that pref, since someone might want a normal website URL to be their new tab URL, and doing that shouldn't affect session history for other loads of that URL.

Adding an API kind of seems like overkill. The odds of someone using about:newtab for something (and additionally also caring about the session history behavior for that URL) seem rather low - can you reconsider just taking the simple hardcoding patch?
Comment 19 Boris Zbarsky [:bz] 2012-07-06 16:52:54 PDT
> The odds of someone using about:newtab for something

I'm more worried about a different embedder using some other URI for the default page.  about:newtab is part of Firefox, not toolkit or Gecko....

Would it make sense to compare to the value of the pref on the default branch?
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-07 10:04:45 PDT
I'm not sure that saying "browser.newtab.url is the default pref you need to set for docshell to be aware of your new tab page" is much better than saying "about:newtab is the URI you must use for docshell to be aware of your new tab page", from an embedder point of view, but I guess it is more flexible. Let's just do that then.
Comment 21 Tim Taubert [:ttaubert] 2012-07-10 02:51:12 PDT
Created attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

The patch does now read the value of 'browser.newtab.url' and prevents matching pages from getting added to the session history.
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-10 08:07:13 PDT
Comment on attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

You'll need to use GetDefaultCString per comment 18/comment 19, but otherwise looks good to me.

(I don't know whether we need to worry about the performance implications of calling GetSpec here for large (e.g. data:) URIs.)
Comment 23 Boris Zbarsky [:bz] 2012-07-10 08:17:55 PDT
Comment on attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

Should failure rv return true instead?

With that and the GetDefaultCString, should be ok.

And yes, this kinda sucks for data: URIs...  I don't think the suck is too terrible, though.  This is not hot code.
Comment 24 Tim Taubert [:ttaubert] 2012-07-10 09:01:21 PDT
(In reply to Boris Zbarsky (:bz) from comment #23)
> Should failure rv return true instead?

Done.

> With that and the GetDefaultCString, should be ok.

Done.

Pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/c9c8ecd605b5
Comment 25 Tim Taubert [:ttaubert] 2012-07-10 14:34:54 PDT
Comment on attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New Tab Page
User impact if declined: security implications, possible privilege escalation
Risk to taking this patch (and alternatives if risky): risk is very low
String or UUID changes made by this patch: None.
Comment 26 Tim Taubert [:ttaubert] 2012-07-10 23:46:00 PDT
https://hg.mozilla.org/mozilla-central/rev/c9c8ecd605b5
Comment 27 Alex Keybl [:akeybl] 2012-07-11 16:34:00 PDT
Comment on attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

[Triage Comment]
This missed our last planned Beta 14 build, so only approving for Aurora 15.

Including Dan and Al as well in case they want to protest based upon security implications.
Comment 28 Tim Taubert [:ttaubert] 2012-07-12 03:25:37 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/16379f3e5b13
Comment 29 Ioana (away) 2012-08-06 07:57:49 PDT
Verified as fixed on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0
Build identifier: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0
Comment 30 Tim Taubert [:ttaubert] 2013-02-15 02:33:09 PST
*** Bug 777782 has been marked as a duplicate of this bug. ***
Comment 31 Sid 2013-06-18 15:45:56 PDT
> do not add about:newtab (or any custom newtab url) to the session history

I don't see it on my Nightly with custom newtab url. Here are STR.
1. Set browser.newtab.url = http://google.com/
2. Open new tab
3. Go to some other page
4. Back button is enabled

Note You need to log in before you can comment on or make changes to this bug.