|Summary:||location.href and outgoing referrer not updated properly by pushState/replaceState|
|Product:||WebKit||Reporter:||Darin Fisher (:fishd, Google) <fishd>|
|Component:||Page Loading||Assignee:||Darin Fisher (:fishd, Google) <fishd>|
|Severity:||Normal||CC:||abarth, beidson, commit-queue, hurst|
|Version:||528+ (Nightly build)|
Description Darin Fisher (:fishd, Google) 2010-06-01 14:58:51 PDT
location.href not updated properly by pushState/replaceState location.href (FrameLoader::m_url) and FrameLoader::m_outgoingReferrer remain set to whatever the URL of the original document was. This seems undesirable. From http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#dom-location-href: "The href attribute must return the current address of the associated Document object, as an absolute URL." From http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#dom-history-pushstate: "If the third argument is present, set the document's current address to the absolute URL that was found earlier in this algorithm." This to me implies that location.href should according to the spec be updated just as the document's URL and documentURI properties are updated as a result of pushState/replaceState being called with a valid third parameter.
Comment 1 Darin Fisher (:fishd, Google) 2010-06-01 15:00:09 PDT
Firefox nightlies seem to update location.href and send a HTTP-Referer corresponding to the updated URL.
Comment 2 Darin Fisher (:fishd, Google) 2010-06-01 15:00:34 PDT
Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=45361
Comment 3 Brady Eidson 2010-06-01 15:09:46 PDT
I agree. We update the "current address of the associated Document object" by... updating the URL of the associated Document*. Sadly FrameLoader has all sorts of crazy notions of URLs of its own. We really should unravel all of that and have one authoritative source on what URL is current, and FrameLoader should ask that authoritative source when it needs to know. I think Document/DocumentLoader is the correct place for that. Adam, thoughts?
Comment 4 Darin Fisher (:fishd, Google) 2010-06-01 22:09:33 PDT
BTW, the simple fix for this bug is to call FrameLoader::setURL() from Document::updateURLForPushOrReplaceState(). I have a patch for that, but I haven't posted it yet because I haven't finished the tests for it.
Comment 5 Darin Fisher (:fishd, Google) 2010-06-02 13:31:13 PDT
Created attachment 57692 [details] v1 patch
Comment 6 Brady Eidson 2010-06-02 15:21:02 PDT
Comment on attachment 57692 [details] v1 patch I suppose I'm kind of surprised that all the layouttests pass with this change. That's actually good news for nuking the redundancy in the future. r+!
Comment 7 Darin Fisher (:fishd, Google) 2010-06-02 15:36:54 PDT
Comment on attachment 57692 [details] v1 patch whoops, i left out expected results.
Comment 8 Brady Eidson 2010-06-02 15:42:21 PDT
Comment 9 Joey Hurst 2010-06-02 16:15:44 PDT
Thanks Darin for the quick follow-up on this bug. I thought of another problem this could cause for developers using pushState. Assume the following scenario: 1) Visit http://www.foobar.com/ 2) http://www.foobar.com/one (via pushState) 3) http://www.foobar.com/two (via pushState) 4) Visit http://www.someothersite.com/ 5) Hit the back button twice. Logically, this should land the user at /one. However, if the page isn't in the back-forward cache and the user rapidly clicks the back button, it is possible that page script will miss the popstate event from /two to /one, and the user will be stuck looking at state /two when the URL indicates /one. This could be fixed by inspecting the location object at some point and updating the page state to match, but as Darin pointed out, the location object isn't reliable. I haven't been able to find an alternative way to grab the actual location in this scenario. Once could try placing a popstate handler near the top of the document and hoping for the best, but the error case is rather bad and I can't think of a way that one could track the issue (since the problem stems from the developer missing an event). It looks like you've already started on a fix, so this comment is intended more for other web developers trying to use pushState in Webkit... Thanks!
Comment 10 Darin Fisher (:fishd, Google) 2010-06-02 16:59:33 PDT
Created attachment 57715 [details] v2 patch Now with test results, more tests, and a fix for an existing test :-/
Comment 11 Darin Fisher (:fishd, Google) 2010-06-02 17:07:19 PDT
(In reply to comment #9) Hi Joey, The patch in this bug makes the location update properly. If you are trying to workaround the current behavior of WebKit, please try looking at document.URL instead. This patch just makes location get updated when we update document.URL. I don't understand the issue with clicking back rapidly. We should always dispatch popstate event for the final back navigation.
Comment 12 Brady Eidson 2010-06-02 17:24:15 PDT
Comment on attachment 57715 [details] v2 patch This one makes me sadder, but is more what I was expecting.
Comment 13 WebKit Commit Bot 2010-06-02 22:38:28 PDT
Comment on attachment 57715 [details] v2 patch Clearing flags on attachment: 57715 Committed r60608: <http://trac.webkit.org/changeset/60608>
Comment 14 WebKit Commit Bot 2010-06-02 22:38:34 PDT
All reviewed patches have been landed. Closing bug.