Bug 36202

Summary: popstate event should be dispatched asynchronously
Product: WebKit Reporter: Darin Fisher (:fishd, Google) <fishd>
Component: Page LoadingAssignee: Mihai Parparita <mihai>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ap, beidson, commit-queue, eric, esprehn+autocc, jhoneycutt, kangil.han, mihai
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=150202
Bug Depends on:    
Bug Blocks: 153686    
Attachments:
Description Flags
Patch
none
Patch bfulgham: review+

Description Darin Fisher (:fishd, Google) 2010-03-16 16:51:24 PDT
popstate event should be dispatched asynchronously

see step #10.2 of the history traversal algorithm (section 6.5.9):
http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#traverse-the-history

this is a recent change to the spec
Comment 1 Brady Eidson 2010-03-16 16:53:19 PDT
<rdar://problem/7761279>
Comment 2 Mihai Parparita 2011-02-08 09:18:38 PST
Created attachment 81647 [details]
Patch
Comment 3 Mihai Parparita 2011-02-08 09:19:16 PST
Darin, let me know if the change to FrameLoaderClientImpl::pluginLoadObserver can be tested.
Comment 4 Darin Fisher (:fishd, Google) 2011-02-08 09:42:15 PST
Comment on attachment 81647 [details]
Patch

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

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:-1548
> -        // We can arrive here if a popstate event handler detaches this frame.

To test this, I think you'd need to modify the NPAPI test plugin.  This patch
actually fixes the bug where we would fail to notify a plugin of when a frame
finishes navigating.

You could have a test, where a plugin uses NPN_GetURLNotify w/ target equal to
the name of a frame.  It expects to receive a NPP_URLNotify call once the frame
navigation completes.

I'm not sure if TestNetscapePlugin has any tests like this already.
Comment 5 Eric Seidel (no email) 2011-06-18 13:42:48 PDT
Attachment 81647 [details] was posted by a committer and has review+, assigning to Mihai Parparita for commit.
Comment 6 Jon Honeycutt 2015-11-11 18:55:53 PST
Created attachment 265347 [details]
Patch
Comment 7 Brent Fulgham 2015-11-11 19:13:30 PST
Comment on attachment 265347 [details]
Patch

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

Looks great! R=me.

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=36202

I think there might be a radar for this. Can you add it, please?

> Source/WebCore/ChangeLog:6
> +        Based on an original patch by Mihai Parparita <mihaip@chromium.org>.

Please reference the CRBugs entry, if available.
Comment 8 Jon Honeycutt 2015-11-11 23:00:01 PST
(In reply to comment #7)
> Comment on attachment 265347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265347&action=review
> 
> Looks great! R=me.
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=36202
> 
> I think there might be a radar for this. Can you add it, please?

Fixed.

> 
> > Source/WebCore/ChangeLog:6
> > +        Based on an original patch by Mihai Parparita <mihaip@chromium.org>.
> 
> Please reference the CRBugs entry, if available.

The original patch came from this bug, not a CR.

Thanks for the review! I've pinged Brady to take a look at this as well before I land it.
Comment 9 Jon Honeycutt 2015-11-12 10:39:08 PST
Committed r192369: <http://trac.webkit.org/changeset/192369>
Comment 10 Andy Estes 2016-01-25 17:10:32 PST
I don't think this fix is right. The spec says to dispatch the event asynchronously only when the "asynchronous events" flag is set. This flag is only set is during navigation to a fragment identifier, afaict. For cases like history.back(), we still want to dispatch the event synchronously (which makes sense, because the history navigation itself happens asynchronously).
Comment 11 Brady Eidson 2016-01-25 18:53:45 PST
(In reply to comment #10)
> I don't think this fix is right. The spec says to dispatch the event
> asynchronously only when the "asynchronous events" flag is set. This flag is
> only set is during navigation to a fragment identifier, afaict. For cases
> like history.back(), we still want to dispatch the event synchronously
> (which makes sense, because the history navigation itself happens
> asynchronously).

Reading the spec, yup you're right.
Comment 12 Andy Estes 2016-01-29 16:22:32 PST
(In reply to comment #11)
> (In reply to comment #10)
> > I don't think this fix is right. The spec says to dispatch the event
> > asynchronously only when the "asynchronous events" flag is set. This flag is
> > only set is during navigation to a fragment identifier, afaict. For cases
> > like history.back(), we still want to dispatch the event synchronously
> > (which makes sense, because the history navigation itself happens
> > asynchronously).
> 
> Reading the spec, yup you're right.

I filed https://bugs.webkit.org/show_bug.cgi?id=153686