Closed Bug 647028 Opened 9 years ago Closed 9 years ago

pushState plus session restore doesn't quite work

Categories

(Core :: DOM: Navigation, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 500328
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.
Attached patch Patch v1 (obsolete) — Splinter Review
I think this hack fixes things.

Paul, what do you think of the change to session restore?  Jonas, do the test changes look good?
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
Attachment #524911 - Flags: review?(paul)
Attachment #524911 - Flags: review?(jonas)
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.
Attachment #524911 - Flags: review?(jonas) → review+
Attached patch Patch v3Splinter Review
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.
Attachment #524911 - Attachment is obsolete: true
Attachment #524911 - Flags: review?(paul)
Comment on attachment 527260 [details] [diff] [review]
Patch v3

Yep; this fixed the problem.  Looks good on try now.
Attachment #527260 - Flags: review?(paul)
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)
Attachment #527260 - Flags: review?(paul) → review+
And checked in:
http://hg.mozilla.org/mozilla-central/rev/2a551d43d984
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Oops; I forgot the comment you asked for.  I'll push it with my next patch queue.
Blocks: 675162
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.