Closed Bug 635844 Opened 9 years ago Closed 9 years ago

Update pushState to latest spec. Again.

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0

People

(Reporter: sicking, Assigned: sicking)

References

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 3 obsolete files)

28.78 KB, patch
justin.lebar+bug
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
24.57 KB, patch
justin.lebar+bug
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
3.28 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
7.20 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
7.58 KB, patch
Details | Diff | Splinter Review
1.36 KB, patch
sicking
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → jonas
Attachment #514150 - Flags: review?(justin.lebar+bug)
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.
Attachment #514151 - Flags: review?(justin.lebar+bug)
Attachment #514150 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #514151 - Flags: review?(justin.lebar+bug) → review+
(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.
(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 on attachment 514150 [details] [diff] [review]
Back out bug 615501

a=beltzner for after b12 cutoff
Attachment #514150 - Flags: approval2.0? → approval2.0+
Attachment #514151 - Flags: approval2.0? → approval2.0+
(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).
I see, okay.  A comment would be appreciated.  :)
Attached patch Fix extension manager (obsolete) — Splinter Review
Of course the extension manager depend on the old behavior.
Attachment #514676 - Flags: review?(dtownsend)
Attached patch Fix more testsSplinter Review
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.
Attachment #514677 - Flags: review?(justin.lebar+bug)
Comment on attachment 514676 [details] [diff] [review]
Fix extension manager

Straightforward and low-risk, I like it!
Attachment #514676 - Flags: review?(dtownsend) → review+
Attachment #514676 - Flags: approval2.0+
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.
Attachment #514677 - Flags: review?(justin.lebar+bug) → review+
Attached patch Fix extension manager v2 (obsolete) — Splinter Review
This includes additional fixes to get tests passing
Attachment #514676 - Attachment is obsolete: true
Attachment #515366 - Flags: review?(dtownsend)
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.
Attachment #515367 - Flags: review?(justin.lebar+bug)
Comment on attachment 515366 [details] [diff] [review]
Fix extension manager v2

Nevermind, seems like more fixes are needed still :(
Attachment #515366 - Attachment is obsolete: true
Attachment #515366 - Flags: review?(dtownsend)
Attachment #515367 - Attachment is obsolete: true
Attachment #515367 - Flags: review?(justin.lebar+bug)
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.
Attachment #515367 - Attachment is obsolete: false
Attachment #515367 - Flags: review?(justin.lebar+bug)
Comment on attachment 515367 [details] [diff] [review]
Set history.state in more places

lgtm!
Attachment #515367 - Flags: review?(justin.lebar+bug) → review+
Attached patch final add-ons manager test fixes (obsolete) — Splinter Review
This should fix the final test failures in the extension manager.
Attachment #514677 - Flags: approval2.0+
Attachment #515367 - Flags: approval2.0+
Attachment #515793 - Flags: approval2.0? → approval2.0+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0
Mostly what needs documentation here is the history.state property.
Keywords: dev-doc-needed
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)?
Yes! Would you be able to attach a fix?
Blocks: 640165
No longer blocks: 640165
(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).
That is, I presume it's the state as set for the current page, but I want to be sure.
(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.
So would this be a way to peek at the history entry at the top of the stack without popping it then?
It's a way to peek at the history entry at the top of the stack without waiting for popstate to fire.
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.
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.
Depends on: 640519
Attachment #518043 - Flags: review?(jonas) → review+
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
Attachment #518043 - Attachment description: (AAv1) Fix (2) removeEventListener() 3rd param, from bug 500328 and this bug → (AAv1) Fix (2) removeEventListener() 3rd param, from bug 500328 and this bug [Checked in: Comment 32]
(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 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.
Attachment #518043 - Flags: approval2.0?
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
(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
Attachment #518043 - Flags: approval2.0?
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.