Closed Bug 1077738 Opened 5 years ago Closed 5 years ago

Session restored about:newtab pages are retained in session history

Categories

(Firefox :: Session Restore, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.2

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file)

1. Open a new tab to the about:newtab page.
2. Restart your browser and restore the session.
3. Enter a new url in the tab or click one of the items.

You'll now be able to click back to get back to the new tab page. This is different to what happens if you don't session restore in between, the point of bug 724239.

Bug 999239 has made this more obvious in e10s where we always session restore the tab between the non-remote about:newtab and any remote page, unfortunately it also broke the test for this :(

Same thing happens for about:blank too.
Assignee: nobody → dtownsend+bugmail
Duplicate of this bug: 1077581
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Attached patch patchSplinter Review
Straightforward enough but does involve changing from iterating over the session history entries to walking the transaction linked list. This is what getEntryAtIndex does internally anyway so this shouldn't be a problem. Probably also faster but probably negligible unless we have quite a lot of history.
Attachment #8500650 - Flags: review?(ttaubert)
Attachment #8500650 - Flags: review?(ttaubert) → review?(smacleod)
Iteration: 35.3 → 36.1
Comment on attachment 8500650 [details] [diff] [review]
patch

Review of attachment 8500650 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, just a couple nits.

::: browser/components/sessionstore/SessionHistory.jsm
@@ +97,5 @@
> +
> +      while (txn && i <= newest) {
> +        let shEntry = txn.sHEntry;
> +        let entry = this.serializeEntry(shEntry, isPinned);
> +        entry.persist = txn.persist;

Can we add a comment before all the transaction looping just to say that we're doing it ourselves here because we need access to the transaction's persist value.

::: browser/components/sessionstore/test/browser_history_persist.js
@@ +3,5 @@
> +
> +"use strict";
> +
> +/**
> + * Ensure that history entries that should not be persisted are restored in the 

Trailing whitespace.

@@ +77,5 @@
> +  // Cleanup.
> +  gBrowser.removeTab(tab);
> +});
> +
> +function promiseTabRestoring(tab) {

In browser/components/sessionstore/test/head.js there is a |waitForTabState()| function, but not a matching |PromiseTabState()|. Lets convert this new function here into the missing |PromiseTabState()| and put it in head.js
Attachment #8500650 - Flags: review?(smacleod) → review+
(In reply to Steven MacLeod [:smacleod] from comment #3)
> > +function promiseTabRestoring(tab) {
> 
> In browser/components/sessionstore/test/head.js there is a
> |waitForTabState()| function, but not a matching |PromiseTabState()|. Lets
> convert this new function here into the missing |PromiseTabState()| and put
> it in head.js

meant to say, should be able to just do something simple like:
> function promiseTabState(tab, state) {
>   return new Promise(resolve => waitForTabState(tab, state, resolve));
> }
The test in here depends on bug 1075658 to pass in e10s.
Depends on: 1075658
So... I didn't really get to give feedback here in time (sorry) but we disabled going back to the newtab page purely for security reasons. AFAIK we're going to want it in the history and will enable going back to it as soon as we get around to make about:newtab unprivileged - which is quite important for e10s anyway.
(In reply to Tim Taubert [:ttaubert] from comment #6)
> So... I didn't really get to give feedback here in time (sorry) but we
> disabled going back to the newtab page purely for security reasons. AFAIK
> we're going to want it in the history and will enable going back to it as
> soon as we get around to make about:newtab unprivileged - which is quite
> important for e10s anyway.

Reading the original bug it seemed like there was active discussion that we gave up thinking about because the security issue trumped all. But regardless this patch just makes restored tab's history behave the same as the original tab's history both for about:newtab and about:blank. Even if we reverted about:newtab's behaviour we'd still need this for about:blank.
Right, about:blank is still an issue. The patch looks good to me, didn't want to imply I don't want to land it. Just wanted to make sure we're all on the same page.
Duplicate of this bug: 1084471
Iteration: 36.1 → ---
https://hg.mozilla.org/mozilla-central/rev/1ca37db5609f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.