|Summary:||popstate events are lost when network connection is in progress|
|Product:||WebKit||Reporter:||Ben Cherry <bcherry>|
|Component:||History||Assignee:||Mihai Parparita <mihaip>|
|Severity:||Normal||CC:||abarth, ap, beidson, commit-queue, fishd, mathias, mihaip, rik, tonyg, zr|
|Version:||528+ (Nightly build)|
Description Ben Cherry 2010-07-24 18:28:38 PDT
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
Comment 2 Zach Rait 2010-09-28 18:51:41 PDT
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.
Comment 3 Mihai Parparita 2010-10-08 09:22:24 PDT
Looking into this.
Comment 5 Mihai Parparita 2010-10-08 12:01:16 PDT
Darin or Brady, could you take a look at this?
Comment 6 Adam Barth 2010-10-08 12:23:35 PDT
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.
Comment 7 Mihai Parparita 2010-10-08 12:39:35 PDT
(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.
Comment 8 Adam Barth 2010-10-08 12:44:21 PDT
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.
Comment 9 Darin Fisher (:fishd, Google) 2010-10-08 16:35:20 PDT
Comment on attachment 70273 [details] Patch LGTM
Comment 10 WebKit Commit Bot 2010-10-08 17:02:09 PDT
Comment on attachment 70273 [details] Patch Clearing flags on attachment: 70273 Committed r69432: <http://trac.webkit.org/changeset/69432>
Comment 11 WebKit Commit Bot 2010-10-08 17:02:15 PDT
All reviewed patches have been landed. Closing bug.
Comment 12 Zach Rait 2010-10-11 14:17:00 PDT
Confirmed fixed for FB, thanks Mihai.
Comment 13 Ben Cherry 2010-10-11 16:19:42 PDT
Fixed on my test case as well. Thanks to everyone involved!