Bug 36201 – hashchange event should be dispatched asynchronously

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

hashchange event should be dispatched asynchronously

see step #11 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

[Darin Fisher (:fishd, Google)]

Related to this, the HashChangeEvent now has additional fields:
http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#hashchangeevent

[Brady Eidson]

<rdar://problem/7761278>

[Brady Eidson]

There's 3 interesting new loading events in the spec where we don't match:  hashchange, popstate, and pageshow.

In exploring these 3 closely related events, there's 4 quite relevant bugzillas, including this one:

https://bugs.webkit.org/show_bug.cgi?id=36201
https://bugs.webkit.org/show_bug.cgi?id=36202
https://bugs.webkit.org/show_bug.cgi?id=36334
https://bugs.webkit.org/show_bug.cgi?id=36335

While I plan to resolve each of these with its own patch, I'll to lay the groundwork for all 4 to easily be resolved with a single zero-change-in-behavior patch here.

It will be touching Document.h, so its better to get it out of the way once.  ;)

Stay tuned here...

[Brady Eidson]

Created attachment 51114 [details]
Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners.

[Darin Adler]

Comment on attachment 51114 [details]
Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners.

>  void Document::enqueueStorageEvent(PassRefPtr<Event> storageEvent)

It seems that you could rename this function to be non-storage-event-specific as part of this "no behavior change" step. Is there a reason you didn't?

> +    void schedulePageshowEvent(bool persisted);
> +    void scheduleHashchangeEvent(const String& oldURL, const String& newURL);

> +    void schedulePopstateEvent(PassRefPtr<SerializedScriptValue> stateObject);

It's a little strange to use schedule for all these new functions, but enqueue for the old function.

> -    m_document->dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, true), m_document);
> +    m_document->schedulePageshowEvent(true);

Boolean arguments where we pass constnat valuesstink. And what in the world is "persisted"?

> +    KURL oldURL;
>      bool hashChange = equalIgnoringFragmentIdentifier(url, m_URL) && url.fragmentIdentifier() != m_URL.fragmentIdentifier();
> +    if (hashChange)
> +        oldURL = m_URL;

If all we need for the old URL is a string, it's more efficient to save it in a String local variable instead of KURL. I don't think we should bother with the "if" statement here -- it's OK to unconditionally save the old URL since it will just be a bump to a reference count.

Can you end up getting a lot of hashchange events all in a row? Is it OK to get a hashchange event, later, after the URL is no longer the same at all?

I’ll say r=me but there is room for improvement here.

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 51114 [details])
> >  void Document::enqueueStorageEvent(PassRefPtr<Event> storageEvent)
> 
> It seems that you could rename this function to be non-storage-event-specific
> as part of this "no behavior change" step. Is there a reason you didn't?

Good call - I'll just change it to "enqueueEvent"

> 
> > +    void schedulePageshowEvent(bool persisted);
> > +    void scheduleHashchangeEvent(const String& oldURL, const String& newURL);
> 
> > +    void schedulePopstateEvent(PassRefPtr<SerializedScriptValue> stateObject);
> 
> It's a little strange to use schedule for all these new functions, but enqueue
> for the old function.

You're right.

> 
> > -    m_document->dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, true), m_document);
> > +    m_document->schedulePageshowEvent(true);
> 
> Boolean arguments where we pass constnat valuesstink. And what in the world is
> "persisted"?

"persisted" is the official name of the field in the PageTransitionEvent that tells you whether this is a fresh page load or a page cache load, and it is defined to be a bool.  In this same patch I also call schedulePageShowEvent(false).

I'll make an enum for this for clarities sake, anyways.

> 
> > +    KURL oldURL;
> >      bool hashChange = equalIgnoringFragmentIdentifier(url, m_URL) && url.fragmentIdentifier() != m_URL.fragmentIdentifier();
> > +    if (hashChange)
> > +        oldURL = m_URL;
> 
> If all we need for the old URL is a string, it's more efficient to save it in a
> String local variable instead of KURL. I don't think we should bother with the
> "if" statement here -- it's OK to unconditionally save the old URL since it
> will just be a bump to a reference count.

Good point, will change.

> 
> Can you end up getting a lot of hashchange events all in a row? 

Currently, no.  Once they're asynchronous, yes.  

>Is it OK to get a hashchange event, later, after the URL is no longer the same at all?

That's what the new fields in the hashchange event are about - to make sure the event captures the before and after url so script knows what the change was even though it happened "in the past"

> 
> I’ll say r=me but there is room for improvement here.

I'll address your feedback first!

Thanks for the review.

[Brady Eidson]

> > 
> > > -    m_document->dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, true), m_document);
> > > +    m_document->schedulePageshowEvent(true);
> > 
> > Boolean arguments where we pass constnat valuesstink. And what in the world is
> > "persisted"?
> 
> "persisted" is the official name of the field in the PageTransitionEvent that
> tells you whether this is a fresh page load or a page cache load, and it is
> defined to be a bool.  In this same patch I also call
> schedulePageShowEvent(false).
> 
> I'll make an enum for this for clarities sake, anyways.
> 

After digging in to this one, the PageTransitionEvent itself is too hard-wired into using this bool, and has to remain as such for the purposes of the DOM/IDL/etc.

I'm adding an enum to Document.h specifically for the Pageshow event helper that will keep clarity there.

[Brady Eidson]

Landed the groundwork in http://trac.webkit.org/changeset/56249

Now resolving this bug, bug 36202, and bug 36334 should all be very simple, but I won't have time to tackle it for a few days.

[Alexey Proskuryakov]

We use enqueueEvent sometimes, and postTask other times. I talked to Brady about standardizing on one mechanism, and also about the fact that we needn't be using it whenever a specification says so. In most cases, events to be posted are triggered by network loading, which is itself asynchronous, so the behavior is indistinguishable.

Since these particular events can be dispatched during synchronous operations, we do need to post them in order to match the spec.

[Eric Seidel (no email)]

Looks like this broke the chromium compile?

[Eric Seidel (no email)]

sheriffbot> bradee-oh, darin: r56249 appears to have broken Chromium Mac Release, Chromium Linux Release

Not sure why the EWS didn't catch it.

[Darin Adler]

Just need to change callers to call enqueueEvent instead of enqueueStorageEvent.

[Dimitri Glazkov (Google)]

Chromium build fixed in http://trac.webkit.org/changeset/56258.

[Eric Seidel (no email)]

Attachment 51114 [details] was posted by a committer and has review+, assigning to Brady Eidson for commit.

[Eric Seidel (no email)]

Looks like this can be closed, no?

[Darin Fisher (:fishd, Google)]

No, this bug is not fixed.  Please see comment #8.  Brady only landed the groundwork for fixing this bug.  There is still a follow-up patch that is required.

[Eric Seidel (no email)]

Comment on attachment 51114 [details]
Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners.

Ok.  Clearing the flags and obsoleting this landed patch.

[Steven Lai]

Created attachment 54630 [details]
Patch

Revert hashchange to fire asynchronously

[Brady Eidson]

Comment on attachment 54630 [details]
Patch

Thanks for doing this!

One question though - I'm kind of surprised that no existing layouttest fails with this change.  Did we really not need to update any previous results?

r+ if that's the case.

[Steven Lai]

Just re-ran layout tests on r58441
The change didn't affect any existing layout tests.

[Brady Eidson]

Okay, I stand by my r+ then!

[Brady Eidson]

Okay, I stand by my r+ then!

[Darin Fisher (:fishd, Google)]

Comment on attachment 54630 [details]
Patch

WebCore/ChangeLog:5
 +          Reverted hashchange() event back to async.
Is this description of the change really correct?  I didn't think hashchange was ever async before.  I would have expected this to read:  "Change hashchange() event to be async." or something like that :)

[Steven Lai]

It used to be async on before r51644

Please see:
http://trac.webkit.org/changeset/51644

[Darin Fisher (:fishd, Google)]

(In reply to comment #24)
> It used to be async on before r51644
> 
> Please see:
> http://trac.webkit.org/changeset/51644

Ah, I see.  Sorry for the spam then!

[Steven Lai]

Hi,
Just want to ask why it's cq-?

[WebKit Commit Bot]

Comment on attachment 54630 [details]
Patch

Clearing flags on attachment: 54630

Committed r58736: <http://trac.webkit.org/changeset/58736>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58736 might have broken Qt Linux Release

[Steven Lai]

Should reopen this bug because the patch doesn't update HashChangeEvent to its new proposed interface (i.e the arguments for old hash and the new hash in the event callback).

[Brady Eidson]

(In reply to comment #30)
> Should reopen this bug because the patch doesn't update HashChangeEvent to its
> new proposed interface (i.e the arguments for old hash and the new hash in the
> event callback).

This bug is about the hash change event being dispatched asynchronously or not.

If other changes need to be made to bring the event up to spec, please file separate bugzillas for those changes, as they are logically separate tasks.

[Brady Eidson]

Also note that the FIXME in the code already references the bug for that  :)

https://bugs.webkit.org/show_bug.cgi?id=36335

[Steven Lai]

oops
i see!

[Eric Seidel (no email)]

I believe this caused:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r58943%20(5683)/fast/loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll-pretty-diff.html
to start being flaky on Gtk, and probably all bots.

The test probably doesn't want to print the hashChanged event?  Or certainly not depend on its firing order in relation to other events?

[Eric Seidel (no email)]

Filed bug 38754 about the new test failure.

[Steven Lai]

submitted patch to https://bugs.webkit.org/show_bug.cgi?id=36335 to fix the failure without disabling the test case.