Bug 42940 - popstate events are lost when network connection is in progress
: popstate events are lost when network connection is in progress
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: History
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Mihai Parparita
http://www.bcherry.net/playground/pus...
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-24 18:28 PDT by Ben Cherry
Modified: 2010-11-19 00:07 PST (History)
10 users (show)

See Also:


Attachments
Patch (6.68 KB, patch)
2010-10-08 11:58 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 1 Brady Eidson 2010-07-28 09:53:10 PDT
<rdar://problem/8245251>
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 4 Mihai Parparita 2010-10-08 11:58:36 PDT
Created attachment 70273 [details]
Patch
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!