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.

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.

I think this hack fixes things. Paul, what do you think of the change to session restore? Jonas, do the test changes look good?

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

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 on attachment 527260 [details] [diff] [review] Patch v3 Yep; this fixed the problem. Looks good on try now.

Comment on attachment 527260 [details] [diff] [review] Patch v3 ># HG changeset patch ># User Justin Lebar <justin.lebar@gmail.com> ># 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)

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

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

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

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