615501 - Update pushState's behavior when called before load to match spec

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The have been a few changes to the spec for pushState. We should check if we need to update our implementation.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

As this is a new feature in 2.0, we might want to block. (I'm not particularly inclined one way or another, just want to put it in the queue.)

Comment 2

[Justin Lebar (not reading bugmail)]

I think we should decide whether we're blocking once we've figured out what the changes are.

Comment 3

[Justin Lebar (not reading bugmail)]

I think this is the relevant change to pushState:

> now, if you call pushState() or
> replaceState() before the first popstate has fired, it will update the
> state object so the first popstate will make sense.

Comment 4

[Justin Lebar (not reading bugmail)]

... which actually means this is the same as bug 535999.

Comment 6

[Justin Lebar (not reading bugmail)]

I'm not sure whether this should block.

On the one hand, bug 535999 clearly wasn't a high priority.  On the other hand, small changes to this interface make a difference to sites, as exemplified by bug 615061.

Comment 7

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

sync vs async isn't a small change, far from it.

Comment 8

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

I'd love to do this before Firefox 4. It's always really sad when we don't fix APIs while we have the opportunity to.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Tests.

Verified that calling pushState before onload results in popstate giving a null state object.

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 11

[Justin Lebar (not reading bugmail)]

This patch is an improvement over what we had, but sicking and I aren't sure that this solves the whole problem of calling pushState before onload.  See bug 618644 and http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-December/029476.html

Might be worth taking this patch for now, though.

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Fixing comment.

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to comment #8)
> I'd love to do this before Firefox 4. It's always really sad when we don't fix
> APIs while we have the opportunity to.

Jonas, any chance you'll be able to look at this soon?

Comment 14

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

For what it's worth, I think this behavior makes the most sense:

If pushState is called before the initial popstate, simply cancel firing the after-onload-popstate.

If the back button is pressed (or history.back() is called) after a pushState, but before onload, fire a popstate for the newly transitioned to state. Still leave the after-onload-popstate canceled.


I'm slightly less sure what to do if replaceState is called before onload, but I think treating it the same as when pushState is called makes the most sense. I.e. cancel the pending after-onload-popstate.

Comment 15

[Justin Lebar (not reading bugmail)]

Jonas, that sounds reasonable to me.  Do you have time to take this to the WhatWG list?

The thread is "Onpopstate is Flawed".

Also, since I don't imagine we'll get a quick, decisive response re the spec, do you think we should take this patch as-is, or do something else for FF4?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 497834 [details] [diff] [review]
Patch v2

a=me

Comment 17

[Justin Lebar (not reading bugmail)]

Looks like this tickles the intermittent orange in bug 612311 into a permaorange.  I'll have a look when I can.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=76f383813c0f

for comparison, the revision upon which my patch was based doesn't have this orange:

http://tbpl.mozilla.org/?tree=Firefox&rev=16bd82195df8

Comment 18

[Justin Lebar (not reading bugmail)]

Pushing to try with test_suspend disabled in an attempt to see if this patch causes other tests to fail or if the bad interaction is specifically with test_suspend.

Comment 19

[Justin Lebar (not reading bugmail)]

Try results coming at http://tbpl.mozilla.org/?tree=MozillaTry&rev=15f4f907b2b6

Comment 20

[Justin Lebar (not reading bugmail)]

This appears to be green with test_suspend disabled.

Comment 21

[Justin Lebar (not reading bugmail)]

I've disabled all the tests that this patch adds, and I'm still I'm getting persistent timeouts [1,2] on 

  content/html/content/test/test_bug277724.html

and

  content/html/document/test/test_bug172261.html

(I've re-enabled test_suspend, but no sign of that failure again.)

I can reproduce locally, but unfortunately only when running the entirety of mochitest-1.  Even

  $ TEST_PATH='content' make mochitest-plain

doesn't cut it.

Awfully strange.  I'll poke around and see what I can find.

[1] http://tbpl.mozilla.org/?tree=MozillaTry&rev=8e3ba18e7cfd
[2] http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1296439927.1296441355.29577.gz

Comment 22

[Justin Lebar (not reading bugmail)]

Maybe I'm hitting bug 602256!  The test for bug 622088 calls pushState on the inner window, and this messes up test_bug277724, which calls back() on the inner window.

Curious that I only see this problem with both this patch and the patch for bug 622088 applied.  But in any case, I'll make my test use a popup and see if that fixes the problem.

Comment 23

[Justin Lebar (not reading bugmail)]

Okay.  With the test fix to bug 622088, I'm now only hitting the test_suspend failure.

I think that failure is related to the tests in this bug, since I accidentally pushed with these tests disabled and it was green.

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3 - Update pending state object

The test issues I was having here were the same as bug 622088, actually -- I was calling pushState on the top-level window.

I fixed this by removing the offending test, _1.  It was testing that pushState before onload yields the correct object on popstate, but _3 does this as well.

The old _2 (now _1) calls replaceState on the top-level window, but this doesn't seem to be a problem.

bz, since you looked at a similar change in bug 622088, would you mind signing off on this one?

Comment 25

[Justin Lebar (not reading bugmail)]

Per the discussion on WhatWG [1], it looks like we should change this to make push/replaceState and navigation cancel popstate events.  There seems to be support from WebKit for this general approach.

I don't know if we want to try to do this for FF4, but FWIW, I can get Facebook to show me the wrong page by clicking around quickly enough -- presumably it's receiving a stale popstate.

[1] http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-February/030157.html

Comment 26

[Justin Lebar (not reading bugmail)]

Attachment: Patch v4 - Don't send stale popstates.

Comment 27

[Justin Lebar (not reading bugmail)]

Patch v4 attempts to get the behavior described in comment 25.

It's kind of a hack, though; to avoid changing interfaces, I added a code to nsError.h.  :)

Jonas, what do you think?

Comment 28

[Justin Lebar (not reading bugmail)]

Attachment: Patch v5

Adds PopStateEvent.initial property, as sicking and I discussed on IRC.

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

How about making the last parameter optional on the branch interface and mark the original init [noscript]? Would that work (maybe with [binaryname()])?

Comment 30

[Justin Lebar (not reading bugmail)]

You mean for initPopStateEvent, right?

I tried that and it didn't work, presumably because of the inheritance.  But I'll try binaryname().

Comment 31

[Justin Lebar (not reading bugmail)]

Ms2ger, is this what you meant?

> interface nsIDOMPopStateEvent : nsIDOMEvent
> {
>   [noscript, binaryname(initPopStateEventNoIsInitial)]
>   void initPopStateEvent(...);
> };
 
> interface nsIDOMPopStateEvent_MOZILLA_2_BRANCH : nsIDOMPopStateEvent
> {
>   readonly attribute nsIVariant state;
>   void initPopStateEvent(...);
> };
 
This fails with
 
> src/dom/interfaces/events/nsIDOMPopStateEvent_MOZILLA_2_BRANCH.idl:46: `initPopStateEvent' conflicts
> src/dom/interfaces/events/nsIDOMPopStateEvent.idl:52: with interface operation `initPopStateEvent'

Comment 32

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

If you make the two interfaces not inherit in any way, that error will go away.

Comment 33

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

Comment on attachment 512253 [details] [diff] [review]
Patch v5

This looks good to me. Even better would be to fix the init function. Ms2ger said he might step up and write a patch.

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v5.1

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 512622 [details] [diff] [review]
Patch v5.1

Haven't been able to test it yet.

Comment 36

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

Comment on attachment 512622 [details] [diff] [review]
Patch v5.1

r=me if you mark the isInitial as [optional] for backwards compat.

Comment 37

[Justin Lebar (not reading bugmail)]

Thanks, Ms2ger.

I'm really going to get away with adding a success code to nserror.h?  I thought there was no way that would fly.

Anyway, Jonas, can you double-check that the hunk in nsDocShell::EndPageLoad is right?

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v5.2

Done

Comment 39

[Benjamin Smedberg]

jst/bz, can you help make an approval call here? Also, the nomination doesn't come with a risk/reward analysis, but this seems like a significantly large patch to be taking this late in the game.

Comment 40

[Justin Lebar (not reading bugmail)]

FWIW, if I navigate around Facebook before the page has finished loading, I can get it to receive a stale popstate and show the wrong page.  This patch should fix this problem.

But I think we might want some indication from the WebKit people that they're onboard before we check this in?

Comment 41

[Boris Zbarsky [:bzbarsky]]

My gut feeling is that it's too late for this sort of change, unless we think we won't be able to fix our behavior after 2.0 ship for some reason.... and maybe even then.

Comment 42

[Justin Lebar (not reading bugmail)]

I wonder if we could take patch v3 at this point.  It's just test changes on top of v2, which has sicking's r+ and bz's a2.0.

Jonas, do you think it would be worth taking that?

Comment 43

[Benjamin Smedberg]

Comment on attachment 497834 [details] [diff] [review]
Patch v2

That approval was granted a month ago and is no longer valid without new discussion.

Comment 44

[Justin Lebar (not reading bugmail)]

Yes, absolutely!  We didn't check it in because we wanted to make a bigger change.  But that might have been a mistake.

Comment 45

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

So here's the full story:

Not long ago we realized that there are some serious problems with the pushState API. Serious enough that it's currently very hard to build a application which uses the 'state' feature but still works correctly.

Based on discussions on the list we have a proposal. I'm currently in talks with the webkit people to make sure that they too are happy with this proposal.

Webkit has already shipped pushState. If we do as well with the same bad API, I'm worried that we'll forever be locked into a API which results in buggy webpages and frustrated authors.


So I think the value of fixing this before shipping is fairly high.


On the risk side, this doesn't seem very risky to me. The flow changes in the patch are fairly small, most of the changes is mechanical XPCOM muckery since we're having to keep existing interface untouched. And this code is pretty well tested in mochitests.

On top of that we have one beta to ship still.

Comment 46

[Johnny Stenback (:jst)]

Comment on attachment 512753 [details] [diff] [review]
Patch v5.2

I think we should take this, but we need to make sure this gets into the beta, IOW, this needs to land ASAP. If this misses the beta, I don't think we can take it.

Comment 47

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

Checked in

http://hg.mozilla.org/mozilla-central/rev/2042b8f9756d

Comment 48

[Peter Van der Beken [:peterv]]

This triggers "###!!! ASSERTION: Class name and proto chain interface name mismatch!" on startup. In particular this change looks plain wrong:

-  DOM_CLASSINFO_MAP_BEGIN(PopStateEvent, nsIDOMPopStateEvent)
+  DOM_CLASSINFO_MAP_BEGIN(PopStateEvent, nsIDOMPopStateEvent_MOZILLA_2_BRANCH)

Why was it needed?

Comment 49

[Peter Van der Beken [:peterv]]

Er, not on startup, when going to about:addons.

Comment 50

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

Err.. yeah, that's wrong with the rejiggered interfaces. Maybe it shouldn't have been done no matter what actually

Comment 51

[Justin Lebar (not reading bugmail)]

Should the popstate event's |initial| property have a moz prefix?

Comment 52

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Assertion fix

Comment 53

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Assertion fix

r+a=sicking over IRC

Comment 54

[(no longer active)]

The follow-up landed as http://hg.mozilla.org/mozilla-central/rev/8812253737ce.