1077738 - Session restored about:newtab pages are retained in session history

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch

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.

Comment 3

[Steven MacLeod [:smacleod]]

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

Comment 4

[Steven MacLeod [:smacleod]]

(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));
> }

Comment 5

[Dave Townsend [:mossop]]

The test in here depends on bug 1075658 to pass in e10s.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 7

[Dave Townsend [:mossop]]

(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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 10

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/1ca37db5609f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ca37db5609f