Last Comment Bug 647028 - pushState plus session restore doesn't quite work
: pushState plus session restore doesn't quite work
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 500328 675162
  Show dependency treegraph
 
Reported: 2011-03-31 16:04 PDT by Justin Lebar (not reading bugmail)
Modified: 2011-10-20 22:40 PDT (History)
2 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (6.32 KB, patch)
2011-04-09 23:31 PDT, Justin Lebar (not reading bugmail)
jonas: review+
Details | Diff | Splinter Review
Patch v3 (7.79 KB, patch)
2011-04-20 07:25 PDT, Justin Lebar (not reading bugmail)
paul: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-03-31 16:04:43 PDT
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) 2011-04-01 15:00:59 PDT
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) 2011-04-09 23:31:49 PDT
Created attachment 524911 [details] [diff] [review]
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 2011-04-14 05:14:41 PDT
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 2011-04-18 19:58:24 PDT
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) 2011-04-20 07:25:51 PDT
Created attachment 527260 [details] [diff] [review]
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) 2011-04-20 12:03:18 PDT
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 reading much bugmail, email directly) 2011-04-20 12:29:15 PDT
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)
Comment 8 Justin Lebar (not reading bugmail) 2011-04-20 13:41:15 PDT
Pushed updated patch to try.  http://tbpl.mozilla.org/?tree=MozillaTry&rev=68c4fe67eac6
Comment 9 Justin Lebar (not reading bugmail) 2011-04-20 15:25:22 PDT
And checked in:
http://hg.mozilla.org/mozilla-central/rev/2a551d43d984
Comment 10 Justin Lebar (not reading bugmail) 2011-04-20 22:00:15 PDT
Oops; I forgot the comment you asked for.  I'll push it with my next patch queue.
Comment 11 Justin Lebar (not reading bugmail) 2011-04-22 16:37:13 PDT
Comment added: http://hg.mozilla.org/mozilla-central/rev/4c54cf0d07c1

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