647028 - pushState plus session restore doesn't quite work

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Justin Lebar (not reading bugmail)]

Jonas's STR follow.  I suspect this might be related to bug 646641.

1. Load page A
2. Pushstate to url B
3. Save session history
4. Close tab
5. Restore session history
6. Check that you are on B (works)
7. Go back()
8. Check that you only did a popstate to A, rather than a load (fails!)
9. Go forward()
10. Check that you only did a popstate to B, rather than a load (succeeds)

Sicking further says:

> The test we currently have, browser_500328.js, doesn't do the check in
> 8 which is why it succeeds. Once we fix this that test can also be
> somewhat simplified as it now listens to both "load" and "popstate"
> events.

Comment 1

[Justin Lebar (not reading bugmail)]

Looks like Jonas was right, and this doesn't have to do with bug 646641.

I think the problem stems from sss_restoreTab in sessionStore.js.  It does:

> browser.webNavigation.setCurrentURI(this._getURIFromString("about:blank"));

which creates a new history entry, thus overwriting the current one, and overwriting its document identifier.  Thus when we go back(), we do a load.

Still working on understanding the guts of this thing, though.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

I think this hack fixes things.

Paul, what do you think of the change to session restore?  Jonas, do the test changes look good?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please also add the step-8 test which the current test doesn't do. You're somewhat testing that by only listening to "popstate", but it'd be nice to have an explicit test for it as well.

I.e. after having restored the tab state and waited for "load" to fire, set some global variable in the page and make sure it's there after the *first* "popstate" has fired.

(Optional nit: You can remove the |handler| temporary since it's now used in just once place)

r=me on the test changes with those fixes.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 524911 [details] [diff] [review]
Patch v1

Err.. apparently I forgot to mark this. See comment 3 for review comments and for which parts this r+ covers.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

This was causing a test failure I'd misattributed to another patch.  I think the change I made fixes the problem, but I'll push to try before asking for another review.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 527260 [details] [diff] [review]
Patch v3

Yep; this fixed the problem.  Looks good on try now.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 527260 [details] [diff] [review]
Patch v3

># HG changeset patch
># User Justin Lebar <[e-mail]>
># Date 1303309427 14400
># Node ID 9ee64308a022dcae42b7c4e1fc16af74d53e58d1
># Parent  e6c9568389da68623d31d5f425ac15da3ca8be0e
>Bug 647028 - Fix pushstate's interaction with session restore.
>
>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js
>--- a/browser/components/sessionstore/src/nsSessionStore.js
>+++ b/browser/components/sessionstore/src/nsSessionStore.js
>@@ -2855,27 +2855,35 @@ SessionStoreService.prototype = {
>     if (activeIndex >= tabData.entries.length)
>       activeIndex = tabData.entries.length - 1;
> 
>     // Reset currentURI.
>     browser.webNavigation.setCurrentURI(this._getURIFromString("about:blank"));
> 
>     // Attach data that will be restored on "load" event, after tab is restored.
>     if (activeIndex > -1) {
>+      let curSHEntry = browser.webNavigation.sessionHistory.
>+                       getEntryAtIndex(activeIndex, false).
>+                       QueryInterface(Ci.nsISHEntry);
>+      let docIdentifier = curSHEntry.docIdentifier;
>+
>       // restore those aspects of the currently active documents which are not
>       // preserved in the plain history entries (mainly scroll state and text data)
>       browser.__SS_restore_data = tabData.entries[activeIndex] || {};
>       browser.__SS_restore_pageStyle = tabData.pageStyle || "";
>       browser.__SS_restore_tab = aTab;
>+      browser.__SS_restore_docIdentifierFunc = function() {
>+        curSHEntry.docIdentifier = docIdentifier;
>+      };

I don't really want to hold onto curSHEntry in there - that's no good if for some reason we don't get into restoreDocument (I fixed it so that we *should* now though). The other option would be to get the SHEntry again from there, which I think I'd prefer. (we might turn on bartab-like by default sometime and I don't think holding onto the shentry like that for a long time is a good move).

Also, please add a comment saying that setting currentURI resets docIdentifier which is why we need to do this.

r+ with that (and assuming it still passes)

Comment 8

[Justin Lebar (not reading bugmail)]

Pushed updated patch to try.  http://tbpl.mozilla.org/?tree=MozillaTry&rev=68c4fe67eac6

Comment 9

[Justin Lebar (not reading bugmail)]

And checked in:
http://hg.mozilla.org/mozilla-central/rev/2a551d43d984

Comment 10

[Justin Lebar (not reading bugmail)]

Oops; I forgot the comment you asked for.  I'll push it with my next patch queue.

Comment 11

[Justin Lebar (not reading bugmail)]

Comment added: http://hg.mozilla.org/mozilla-central/rev/4c54cf0d07c1