Closed
Bug 615501
Opened 14 years ago
Closed 14 years ago
Update pushState's behavior when called before load to match spec
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
People
(Reporter: Ms2ger, Assigned: justin.lebar+bug)
References
()
Details
(Keywords: html5)
Attachments
(2 files, 8 obsolete files)
29.38 KB,
patch
|
jst
:
approval2.0+
|
Details | Diff | Splinter Review |
1022 bytes,
patch
|
Details | Diff | Splinter Review |
The have been a few changes to the spec for pushState. We should check if we need to update our implementation.
Reporter | ||
Comment 1•14 years ago
|
||
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.)
blocking2.0: --- → ?
Assignee | ||
Comment 2•14 years ago
|
||
I think we should decide whether we're blocking once we've figured out what the changes are.
blocking2.0: ? → ---
Assignee | ||
Comment 3•14 years ago
|
||
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.
Assignee | ||
Comment 4•14 years ago
|
||
... which actually means this is the same as bug 535999.
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Summary: Update pushState to specification → Update pushState's behavior when called before load to match spec
Comment 7•14 years ago
|
||
sync vs async isn't a small change, far from it.
Assignee | ||
Updated•14 years ago
|
Component: DOM: Other → Document Navigation
QA Contact: general → docshell
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.
Assignee | ||
Comment 9•14 years ago
|
||
Verified that calling pushState before onload results in popstate giving a null state object.
Assignee | ||
Updated•14 years ago
|
Attachment #497684 -
Attachment is obsolete: true
Assignee | ||
Comment 10•14 years ago
|
||
Assignee | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #497833 -
Attachment is obsolete: true
Assignee | ||
Comment 13•14 years ago
|
||
(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.
Attachment #497834 -
Flags: review?(jonas) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #497834 -
Flags: approval2.0?
Assignee | ||
Comment 15•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 497834 [details] [diff] [review] Patch v2 a=me
Attachment #497834 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 17•14 years ago
|
||
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
Assignee | ||
Comment 18•14 years ago
|
||
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.
Assignee | ||
Comment 19•14 years ago
|
||
Try results coming at http://tbpl.mozilla.org/?tree=MozillaTry&rev=15f4f907b2b6
Assignee | ||
Comment 20•14 years ago
|
||
This appears to be green with test_suspend disabled.
Assignee | ||
Comment 21•14 years ago
|
||
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
Assignee | ||
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 23•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #497834 -
Attachment is obsolete: true
Assignee | ||
Comment 24•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #508653 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•14 years ago
|
||
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
Assignee | ||
Comment 26•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #508653 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 27•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #510206 -
Flags: review?(jonas)
Assignee | ||
Updated•14 years ago
|
Attachment #510206 -
Attachment description: Patch v4 → Patch v4 - Don't send stale popstates.
Assignee | ||
Updated•14 years ago
|
Attachment #508653 -
Attachment description: Patch v3 → Patch v3 - Update pending state object
Assignee | ||
Comment 28•14 years ago
|
||
Adds PopStateEvent.initial property, as sicking and I discussed on IRC.
Attachment #512253 -
Flags: review?(jonas)
Assignee | ||
Updated•14 years ago
|
Attachment #510206 -
Attachment is obsolete: true
Attachment #510206 -
Flags: review?(jonas)
Assignee | ||
Updated•14 years ago
|
Attachment #508653 -
Attachment is obsolete: true
Reporter | ||
Comment 29•14 years ago
|
||
How about making the last parameter optional on the branch interface and mark the original init [noscript]? Would that work (maybe with [binaryname()])?
Assignee | ||
Comment 30•14 years ago
|
||
You mean for initPopStateEvent, right? I tried that and it didn't work, presumably because of the inheritance. But I'll try binaryname().
Assignee | ||
Comment 31•14 years ago
|
||
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.
Attachment #512253 -
Flags: review?(jonas) → review+
Reporter | ||
Comment 34•14 years ago
|
||
Reporter | ||
Comment 35•14 years ago
|
||
Comment on attachment 512622 [details] [diff] [review] Patch v5.1 Haven't been able to test it yet.
Attachment #512622 -
Attachment description: Make push/replaceState suppress the popstate-after-load event. → Patch v5.1
Attachment #512622 -
Flags: review?(jonas)
Comment on attachment 512622 [details] [diff] [review] Patch v5.1 r=me if you mark the isInitial as [optional] for backwards compat.
Attachment #512622 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 37•14 years ago
|
||
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?
Reporter | ||
Updated•14 years ago
|
Attachment #512622 -
Attachment is obsolete: true
Reporter | ||
Comment 38•14 years ago
|
||
Done
Attachment #512253 -
Attachment is obsolete: true
Attachment #512753 -
Flags: approval2.0?
Comment 39•14 years ago
|
||
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.
Assignee | ||
Comment 40•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 42•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 497834 [details] [diff] [review] Patch v2 That approval was granted a month ago and is no longer valid without new discussion.
Attachment #497834 -
Flags: approval2.0+
Assignee | ||
Comment 44•14 years ago
|
||
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 46•14 years ago
|
||
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.
Attachment #512753 -
Flags: approval2.0? → approval2.0+
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → justin.lebar+bug
Comment 48•14 years ago
|
||
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•14 years ago
|
||
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
Assignee | ||
Comment 51•14 years ago
|
||
Should the popstate event's |initial| property have a moz prefix?
Reporter | ||
Comment 52•14 years ago
|
||
Attachment #513186 -
Flags: review?(peterv)
Reporter | ||
Comment 53•14 years ago
|
||
r+a=sicking over IRC
Attachment #513186 -
Attachment is obsolete: true
Attachment #513186 -
Flags: review?(peterv)
Reporter | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 54•14 years ago
|
||
The follow-up landed as http://hg.mozilla.org/mozilla-central/rev/8812253737ce.
You need to log in
before you can comment on or make changes to this bug.
Description
•