popstate events are lost when network connection is in progress

[Ben Cherry]

User history navigation (back/forward buttons) while there is network traffic does not always fire a popstate event.  These events are completely lost, and can get an application into the wrong state, where the user navigated through their history, but the app was never notified.  Seen in Chrome 5 and 6 and Safari 5 on OS X 10.6.  Mozilla's Firefox 4 implementation of popstate does not show this behavior.

Reproduced here by downloading an image for 1s in the popstate handler, which prevents further popstates from firing while the image downloads: http://www.bcherry.net/playground/pushstate

[Brady Eidson]

<rdar://problem/8245251>

[Zach Rait]

We're experiencing a similar error at Facebook with our implementation of HTML5 history. We have a workaround that verifies that the current location actually matches the most recently fired popstate event, but that's an ugly hack and we'd greatly prefer that multiple presses of "Back" actually generated the correct number of popstate events.

[Mihai Parparita]

Looking into this.

[Mihai Parparita]

Created attachment 70273 [details]
Patch

[Mihai Parparita]

Darin or Brady, could you take a look at this?

[Adam Barth]

Comment on attachment 70273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70273&action=review

> WebCore/loader/FrameLoader.cpp:661
> +    m_frame->document()->setReadyState(Document::Loading);

Is this change detectable by reading document.readyState?  If so, we should add a test that shows the change.

[Mihai Parparita]

(In reply to comment #6)
> (From update of attachment 70273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70273&action=review
> 
> > WebCore/loader/FrameLoader.cpp:661
> > +    m_frame->document()->setReadyState(Document::Loading);
> 
> Is this change detectable by reading document.readyState?  If so, we should add a test that shows the change.

Without this change a couple of tests in fast/loader/stateobjects fail (document-destroyed-navigate-back-with-fragment-scroll.html and document-destroyed-navigate-back.html) since they navigate back to a page that had a state object attached to its history entry. I don't think it's otherwise directly testable from layout tests though, since no script runs between FrameLoader::didBeginDocument and Document::implicitOpen.

[Adam Barth]

Comment on attachment 70273 [details]
Patch

Okiedokes.  This looks good to me, but it wouldn't hurt to have a pushState/popState expert look it over.

[Darin Fisher (:fishd, Google)]

Comment on attachment 70273 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 70273 [details]
Patch

Clearing flags on attachment: 70273

Committed r69432: <http://trac.webkit.org/changeset/69432>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zach Rait]

Confirmed fixed for FB, thanks Mihai.

[Ben Cherry]

Fixed on my test case as well.  Thanks to everyone involved!