Bug 36202 – popstate event should be dispatched asynchronously

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

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

[Brady Eidson]

<rdar://problem/7761279>

[Mihai Parparita]

Created attachment 81647 [details]
Patch

[Mihai Parparita]

Darin, let me know if the change to FrameLoaderClientImpl::pluginLoadObserver can be tested.

[Darin Fisher (:fishd, Google)]

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.

[Eric Seidel (no email)]

Attachment 81647 [details] was posted by a committer and has review+, assigning to Mihai Parparita for commit.

[Jon Honeycutt]

Created attachment 265347 [details]
Patch

[Brent Fulgham]

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 <[e-mail]>.

Please reference the CRBugs entry, if available.

[Jon Honeycutt]

(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 <[e-mail]>.
> 
> 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.

[Jon Honeycutt]

Committed r192369: <http://trac.webkit.org/changeset/192369>

[Andy Estes]

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

[Brady Eidson]

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

[Andy Estes]

(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