Bug 40027 - location.href and outgoing referrer not updated properly by pushState/replaceState
Summary: location.href and outgoing referrer not updated properly by pushState/replace...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
Keywords: InRadar
Depends on:
Reported: 2010-06-01 14:58 PDT by Darin Fisher (:fishd, Google)
Modified: 2010-08-19 10:35 PDT (History)
4 users (show)

See Also:

v1 patch (4.02 KB, patch)
2010-06-02 13:31 PDT, Darin Fisher (:fishd, Google)
beidson: review+
Details | Formatted Diff | Diff
v2 patch (10.80 KB, patch)
2010-06-02 16:59 PDT, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:
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.

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

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.
Comment 15 Brady Eidson 2010-08-19 10:35:04 PDT