635844 - Update pushState to latest spec. Again.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Ok, so new round of spec updates:

We want to *never* fire popstate on the initial load.
We don't want to fire popstate when transitioning to a bfcached document.
Expose the current state on the History object rather than on the document.

The result of this is that popstate is always fired for transitions within a Document, but never otherwise.

Patches coming up to implement.

I played around with facebook since they are a high-profile user of pushState. Things appear to work fine. Both with and without these patches facebook seems to be able to get into confused states. I couldn't really tell a difference though, except possibly they got confused more often without these patches than with.

Comment 1

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

Attachment: Back out bug 615501

Comment 2

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

Attachment: Implement remaining changes

This does the bulk of the changes. Most of it is simply test changes. This was also where most of the time writing the patch was spent.

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 514151 [details] [diff] [review]
Implement remaining changes

> diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
> --- a/docshell/base/nsDocShell.cpp
> +++ b/docshell/base/nsDocShell.cpp
> @@ -8424,17 +8424,19 @@ nsDocShell::InternalLoad(nsIURI * aURI,
>                  doc->SetDocumentURI(newURI);
>              }
>  
>              SetDocCurrentStateObj(mOSHE);
>  
>              // Dispatch the popstate and hashchange events, as appropriate.
>              nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobal);
>              if (window) {
> -                window->DispatchSyncPopState();
> +                if (sameDocIdent || doHashchange) {
> +                  window->DispatchSyncPopState();
> +                }

Why was this change necessary?  Under what circumstances do we not want to fire
a popstate here?

>  NS_IMETHODIMP
> +nsHistory::GetState(nsIVariant **aState)
> +{
> +  *aState = nsnull;
> +
> +  // Check that ReplaceState hasn't been pref'ed off
> +  if (!nsContentUtils::GetBoolPref(sAllowReplaceStatePrefStr, PR_FALSE))
> +    return NS_OK;

Why check only for replacestate being pref'ed off?  Checking that both push and
replaceState are off seems better.

> +
> +  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mInnerWindow));
> +  if (!win)
> +    return NS_ERROR_NOT_AVAILABLE;
> +
> +  if (!nsContentUtils::CanCallerAccess(win->GetOuterWindow()))
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +
> +  // AddState might run scripts, so we need to hold a strong reference to the
> +  // docShell here to keep it from going away.
> +  nsCOMPtr<nsIDOMNSDocument_MOZILLA_2_0_BRANCH> doc =
> +    do_QueryInterface(win->GetExtantDocument());
> +  if (!doc)
> +    return NS_ERROR_NOT_AVAILABLE;
> +
> +  return doc->GetMozCurrentStateObject(aState);
> +}

Comment is out-of-date.  Can GetMozCurrentStateObject (i.e., deserializing
JSON) run scripts?  I hope not, since then we'd have bug 634834 all over again.

> diff --git a/dom/tests/mochitest/whatwg/file_bug500328_1.html b/dom/tests/mochitest/whatwg/file_bug500328_1.html

I'm sorry about the pain you went through here.

r=me if we can agree on what to do about the first hunk above.

Comment 4

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

(In reply to comment #3)
> Comment on attachment 514151 [details] [diff] [review]
> Implement remaining changes
> 
> > diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
> > --- a/docshell/base/nsDocShell.cpp
> > +++ b/docshell/base/nsDocShell.cpp
> > @@ -8424,17 +8424,19 @@ nsDocShell::InternalLoad(nsIURI * aURI,
> >                  doc->SetDocumentURI(newURI);
> >              }
> >  
> >              SetDocCurrentStateObj(mOSHE);
> >  
> >              // Dispatch the popstate and hashchange events, as appropriate.
> >              nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobal);
> >              if (window) {
> > -                window->DispatchSyncPopState();
> > +                if (sameDocIdent || doHashchange) {
> > +                  window->DispatchSyncPopState();
> > +                }
> 
> Why was this change necessary?  Under what circumstances do we not want to
> fire a popstate here?

When going back (or forward) to a bfcached page.

> >  NS_IMETHODIMP
> > +nsHistory::GetState(nsIVariant **aState)
> > +{
> > +  *aState = nsnull;
> > +
> > +  // Check that ReplaceState hasn't been pref'ed off
> > +  if (!nsContentUtils::GetBoolPref(sAllowReplaceStatePrefStr, PR_FALSE))
> > +    return NS_OK;
> 
> Why check only for replacestate being pref'ed off?  Checking that both push and
> replaceState are off seems better.

Oh, didn't realize that they are separate prefs (why are they?). Should we maybe remove this check completely? There shouldn't be any risky code in this path, especially if both pushState and replaceState is disabled.

You fine with removing this check?

> > +  // AddState might run scripts, so we need to hold a strong reference to the
> > +  // docShell here to keep it from going away.
> 
> Comment is out-of-date.  Can GetMozCurrentStateObject (i.e., deserializing
> JSON) run scripts?  I hope not, since then we'd have bug 634834 all over again.

Copy-paste error. Removing.

Comment 5

[Justin Lebar (not reading bugmail)]

(In reply to comment #4)
> > Why was this change necessary?  Under what circumstances do we not want to
> > fire a popstate here?
> 
> When going back (or forward) to a bfcached page.

Ah, right!  So should it be just if (isSameDocIdent) plus an assert that doHashchange implies isSameDocIdent?  A comment would be helpful, too.

> Oh, didn't realize that they are separate prefs (why are they?). Should we
> maybe remove this check completely? There shouldn't be any risky code in this
> path, especially if both pushState and replaceState is disabled.
> 
> You fine with removing this check?

Yes.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 514150 [details] [diff] [review]
Back out bug 615501

a=beltzner for after b12 cutoff

Comment 7

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

(In reply to comment #5)
> (In reply to comment #4)
> > > Why was this change necessary?  Under what circumstances do we not want to
> > > fire a popstate here?
> > 
> > When going back (or forward) to a bfcached page.
> 
> Ah, right!  So should it be just if (isSameDocIdent) plus an assert that
> doHashchange implies isSameDocIdent?  A comment would be helpful, too.

sameDocIdent is false if you click a link like <a href="#foo">. IIRC because aSHEntry is null. (By the way, this was the one bug I had during development, fortunately tests caught it).

Comment 8

[Justin Lebar (not reading bugmail)]

I see, okay.  A comment would be appreciated.  :)

Comment 9

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

Attachment: Fix extension manager

Of course the extension manager depend on the old behavior.

Comment 10

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

Attachment: Fix more tests

More test fixes. Something seems fishy with the session restore behavior. But equally so with and without this patch. I'll file a separate bug on that.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 514676 [details] [diff] [review]
Fix extension manager

Straightforward and low-risk, I like it!

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 514677 [details] [diff] [review]
Fix more tests

>-      ok(aEvent.state, "Event should have a state property.");
>-      is(JSON.stringify(aEvent.state), JSON.stringify({obj1:1}),
>+      //ok(aEvent.state, "Event should have a state property.");
>+      is(JSON.stringify(tab.linkedBrowser.contentWindow.history.state), JSON.stringify({obj1:1}),
>          "first popstate object.");

Not to be too picky about a test, but is there a reason that line was commented out rather than deleted?

>       // After these push/replaceState calls, the window should have three
>       // history entries:
>       //   testURL (state object: null)      <-- oldest
>       //   testURL (state object: {obj1:1})
>       //   page2   (state object: {obj3:3})  <-- newest
>       let contentWindow = tab.linkedBrowser.contentWindow;
>       let history = contentWindow.history;
>       history.pushState({obj1:1}, "title-obj1");
>-      history.pushState({obj2:2}, "title-obj2", "page2");
>+      history.pushState({obj2:2}, "title-obj2", "?foo");
>       history.replaceState({obj3:3}, "title-obj3");

Was this change required to un-bust the test?  That's awfully strange.  Anyway, the comment above needs to be updated.

Comment 13

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

Attachment: Fix extension manager v2

This includes additional fixes to get tests passing

Comment 14

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

Attachment: Set history.state in more places

So migrating the extension manager to use history.state uncovered a bug in this new property. We were only setting it right before firing the load event.

This patch now makes sure to set it asap after creating the Document object, and also update it every time pushState/replaceState is called.

Tests included.

Comment 15

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

Comment on attachment 515366 [details] [diff] [review]
Fix extension manager v2

Nevermind, seems like more fixes are needed still :(

Comment 16

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

Comment on attachment 515367 [details] [diff] [review]
Set history.state in more places

Ok, I'm fairly certain this is correct and enough to fix the gecko side of things.

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 515367 [details] [diff] [review]
Set history.state in more places

lgtm!

Comment 18

[Dave Townsend [:mossop]]

Attachment: final add-ons manager test fixes

This should fix the final test failures in the extension manager.

Comment 19

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

Attachment: Rolled up extension manager patch

Comment 20

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

Checked in:

http://hg.mozilla.org/mozilla-central/rev/d34e0e81327a
http://hg.mozilla.org/mozilla-central/rev/9e139c2c0c25
http://hg.mozilla.org/mozilla-central/rev/a7b9ace8b3a3

Thanks for all the help, especially Mossop who did some awesome debugging today!

Comment 21

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

Mostly what needs documentation here is the history.state property.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 514677 [details] [diff] [review]
Fix more tests

>       // Clean up after ourselves and finish the test.
>       tab.linkedBrowser.removeEventListener("popstate", arguments.callee, false);
>+      tab.linkedBrowser.removeEventListener("load", arguments.callee, false);
>       gBrowser.removeTab(tab);
>       finish();
>     }
>-  }, true);
>+  };
>+
>+  tab.linkedBrowser.addEventListener("load", handler, true);
>+  tab.linkedBrowser.addEventListener("popstate", handler, true);

Shouldn't the 3rd parameter be the same instead of add(true) + remove(false)?

Comment 23

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

Yes! Would you be able to attach a fix?

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (AAv1) Fix (2) removeEventListener() 3rd param, from bug 500328 and this bug [Checked in: Comment 32]

Comment 25

[Eric Shepherd [:sheppy]]

(In reply to comment #21)
> Mostly what needs documentation here is the history.state property.

What does the history.state property get used for? There are no comments explaining what its value is used for (the fact that its type is nsIVariant makes it extra hard to be sure I can make a good educated guess).

Comment 26

[Eric Shepherd [:sheppy]]

That is, I presume it's the state as set for the current page, but I want to be sure.

Comment 27

[Justin Lebar (not reading bugmail)]

(In reply to comment #26)
> That is, I presume it's the state as set for the current page, but I want to be
> sure.

You presume correctly!

But I'd say that it's the state set for the current *history entry*, since a given document might have many states.

Comment 28

[Eric Shepherd [:sheppy]]

So would this be a way to peek at the history entry at the top of the stack without popping it then?

Comment 29

[Justin Lebar (not reading bugmail)]

It's a way to peek at the history entry at the top of the stack without waiting for popstate to fire.

Comment 30

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/DOM/window.history (see the state row in the history properties table).
https://developer.mozilla.org/en/DOM/Manipulating_the_browser_history#Peeking_at_the_current_state

Question: what is document.mozCurrentStateObject? I tried using it and I get "undefined" back in places where history.state is valid, so I'm unclear on what it's for.

Comment 31

[Justin Lebar (not reading bugmail)]

I updated the docs.  Like I should have explicitly said in comment 29, it doesn't make sense to talk about "peeking without popping".  When you're reading a state object, you've already popped it.

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 518043 [details] [diff] [review]
(AAv1) Fix (2) removeEventListener() 3rd param, from bug 500328 and this bug
[Checked in: Comment 32]

http://hg.mozilla.org/mozilla-central/rev/4866be78732f

Comment 33

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #32)
> http://hg.mozilla.org/mozilla-central/rev/4866be78732f

In the future, please restrict commit messages to descriptions of the actual commit. Quoting the entire bug's summary is not useful, and is in fact misleading (that commit has nothing to do with "Update pushstate to latest spec").

Comment 34

[Serge Gautherie (:sgautherie)]

Comment on attachment 518043 [details] [diff] [review]
(AAv1) Fix (2) removeEventListener() 3rd param, from bug 500328 and this bug
[Checked in: Comment 32]

"approval2.0=?":
test-only, just to go along the rest of this bug, fwiw.

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 518043 [details] [diff] [review]
(AAv1) Fix (2) removeEventListener() 3rd param, from bug 500328 and this bug
[Checked in: Comment 32]

are we getting test failures from this? If not I'm inclined to leave it alone

Comment 36

[Justin Lebar (not reading bugmail)]

(In reply to comment #35)
> Comment on attachment 518043 [details] [diff] [review]
> (AAv1) Fix (2) removeEventListener() 3rd param, from bug 500328 and this bug
> [Checked in: Comment 32]
> 
> are we getting test failures from this? If not I'm inclined to leave it alone

It's already been checked in.

> http://hg.mozilla.org/mozilla-central/rev/4866be78732f

Comment 37

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)
> are we getting test failures from this? If not I'm inclined to leave it alone

No failure that I'm aware of. Just wanted to be safer.

(In reply to comment #36)
> It's already been checked in.

On m-c, but not on m-2.0.