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

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.)

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

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.

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

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.

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

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.

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

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.

Fixing comment.

(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?

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.

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 on attachment 497834 [details] [diff] [review] Patch v2 a=me

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

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.

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

This appears to be green with test_suspend disabled.

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

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.

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.

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?

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

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?

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

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

You mean for initPopStateEvent, right? I tried that and it didn't work, presumably because of the inheritance. But I'll try binaryname().

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'

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

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 on attachment 512622 [details] [diff] [review] Patch v5.1 Haven't been able to test it yet.

Comment on attachment 512622 [details] [diff] [review] Patch v5.1 r=me if you mark the isInitial as [optional] for backwards compat.

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?

Done

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.

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?

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.

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 on attachment 497834 [details] [diff] [review] Patch v2 That approval was granted a month ago and is no longer valid without new discussion.

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

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 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.

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

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?

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

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

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

r+a=sicking over IRC

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