1281952 - Fire the popstate event asynchronously

Status: NEW

(Core :: DOM: Navigation, defect, P3)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

At http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/docshell/base/nsDocShell.cpp#10308 we synchronously fire popstate during a navigation.  The spec does not appear to require this.  All history traversals are either done off of an event or have the non-blocking events flag set, in which case the popstate event is going to be dispatched asynchronously.  And in my simple testcase, Chrome dispatches the event asynchronously: http://people.mozilla.org/~khuey/popstate-test/container.html

I think we can do what FireAsyncHashChange does here without any problem.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This part of the spec changed couple of times, IIRC.

One thing to remember is that we must do scrolling after firing popstate.
That is a recent change from bug 1186774.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(Note that it would be ok, for my purposes, to do this only for cross origin frames)

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: container.html

I'll attach the two files in khuey's directory here in case his people account goes pop (hey-o). You probably have to download locally for them to do whatever they are supposed to do.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: test.html

Comment 5

[Andrew McCreight [:mccr8]]

This doesn't sound too tricky, so I'll take a crack at it. I need to read up on session history first, though.

Comment 6

[Andrew McCreight [:mccr8]]

I looked through the spec. 7.8.10 History traversal says that this can be done with a "non-blocking events flag" set. If that flag is set, then the hash change and popstate events are to be done async. Otherwise, they are supposed to be done sync. There seem to be two cases for history traversal: "Navigating across documents" and "Navigating to a fragment". Only the latter mentions having the flag set, which I guess implies it is not set for the former?

Of course, our implementation does something else entirely: AFAICT hash change is always async, and popstate is always sync, so maybe I'm not reading the spec correctly.

Bug 515190 made hashchange sync, but that broke Domino's pizza website (bug 615061) so it was reverted. This change and backout corresponded to some spec changes, so I'll try to figure out how the spec got to the current state which seems to imply sync hashchange under some conditions.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Bug 1281952 - Fire the popstate event asynchronously.

Review commit: https://reviewboard.mozilla.org/r/71030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71030/

Comment 8

[Andrew McCreight [:mccr8]]

I had to change docshell/test/navigation/file_scrollRestoration.html to make it pass. I had to basically split up case 6 so that after each navigation we return and wait for a popstate event, like the current state waits for pageshow. Does that sound reasonable, Olli?  The test for bug 1186774 passes. (Don't look too closely at the C++, as that part of the patch is still hacky.)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Sounds reasonable. Scrolling happens still after popstate, right?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

It would be good to test a bit what other browsers do here. At least blink supports scroll restoration stuff too.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #9)
> Sounds reasonable. Scrolling happens still after popstate, right?

Yes. Before I implemented that part of it, the test in bug 1186774 failed on with my patch, as you would expect. With my modifications to your test, we still check all the scrolling stuff, just inside the event handler for popstate, instead of immediately after.

I should probably write some tests for how this interacts with hashchange, too, if we don't have them already.

(In reply to Olli Pettay [:smaug] from comment #10)
> It would be good to test a bit what other browsers do here. At least blink
> supports scroll restoration stuff too.

Alright. I should also write a test that checks that we are actually doing the popstate asynchronously.

Comment 12

[Andrew McCreight [:mccr8]]

It looks like Safari behaves the same as Chrome here: the popstate is not run immediately after either history.back() call. I'll try to check Edge when I get home.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

FWIW, often testing webkit and blink might be enough, but their session history implementation happens to be the most broken one so testing also Edge it good.

Comment 14

[Andrew McCreight [:mccr8]]

Edge looks similar to Firefox, running the popstate synchronously. However, it seems to restore the scroll state before the popstate ran.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

aha, its scroll state restoration is still based on the older spec.

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: Bug 1281952 - Basic test for sync-ness of popstate event.

Review commit: https://reviewboard.mozilla.org/r/71304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71304/

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 8780311 [details]
Bug 1281952 - Fire the popstate event asynchronously.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71030/diff/1-2/

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 8780311 [details]
Bug 1281952 - Fire the popstate event asynchronously.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71030/diff/2-3/

Comment 19

[Andrew McCreight [:mccr8]]

I cleaned up my code a bit, fixed some docshell tests, and removed failures for a number of web platform tests that pass now. There's another docshell test I need to fix, and three tests in toolkit/mozapps/extensions/test/browser/ (browser_bug562797.js, browser_types.js and browser_uninstalling.js) fail. The addons manager code itself seems to involve navigation, so I guess I have to figure out how that works.

Comment 20

[Andrew McCreight [:mccr8]]

I manually checked that the navigation in the addons manager, as described in bug 562797 comment 0, still works with my patch.

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: Bug 1281952 - Fix addon manager tests to work with async popstate.

Make the FakeHistory forward/back equivalents async to match the new
popstate behavior.

Also, wait_for_view_load normally immediately checks if
aManagerWindow.gViewController.isLoading has been set, but with async
popstate it won't have been set until return to the event loop again,
so pass in aForceWait = true in a bunch of places that do navigation.

Review commit: https://reviewboard.mozilla.org/r/75422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75422/

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8780311 [details]
Bug 1281952 - Fire the popstate event asynchronously.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71030/diff/3-4/

Comment 23

[Andrew McCreight [:mccr8]]

The second patch fixes addon manager tests. I did get one failure in one of these tests in about 15 retriggers. I'm not sure why. https://treeherder.mozilla.org/#/jobs?repo=try&revision=64b8aae384b5 I'll ask mossop for review on those once my retriggers finish.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8780311 [details]
Bug 1281952 - Fire the popstate event asynchronously.

https://reviewboard.mozilla.org/r/71030/#review73538

::: docshell/base/nsDocShell.cpp:10362
(Diff revision 4)
> +            thisHolder->SetCurScrollPosEx(bx, by);
> +          };
> +        }
> +
>          if (historyNavBetweenSameDoc || doHashchange) {
> -          win->DispatchSyncPopState();
> +          win->DispatchAsyncPopState(Move(scrollCallback));

In fragment navigations popstate should happen sync per spec, and that is what blink seems to do too.
(but weirdly enough, it does async hashchange in that case. It is possible that we should always do async hashchange as we do now and just fix the spec)

::: docshell/base/nsDocShell.cpp:10372
(Diff revision 4)
>          }
>  
>          if (doHashchange) {
>            // Note that currentURI hasn't changed because it's on the
>            // stack, so we can just use it directly as the old URI.
>            win->DispatchAsyncHashchange(currentURI, aURI);

Hmm, so now we end up dispatching two separate tasks. Per spec popstate and hashchange should happen within the same task.
So could you merge the popstate and hashchange dispatching runnables and fire whatever events are needed in different cases.

::: docshell/test/navigation/file_popstate_sync.html:4
(Diff revision 4)
> +<html>
> +  <head>
> +    <script>
> +      let correctOrder = ["start-onload", "end-onload", "onpopstate", "onhashchange"];

nit, the events are "popstate" and "hashchange". not on***

::: dom/base/nsGlobalWindow.cpp:10261
(Diff revision 4)
> +  }
> +
> +private:
> +  JS::PersistentRooted<JS::Value> mStateJSValue;
> +  RefPtr<nsGlobalWindow> mWindow;
> +  mozilla::function<void ()> mRunAfter;

Ok, so you use the function stuff to avoid having to have ->SetCurScrollPosEx(bx, by); call in two places. Not sure the function makes this any easier to read, but not so it makes it harder to read it either, so fine.

Comment 25

[Andrew McCreight [:mccr8]]

I'll just wait until the main CPP changes are fixed before I request review on the addon manager test changes, in case something ends up breaking.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #24)
> In fragment navigations popstate should happen sync per spec
And I was so wrong with that.

Comment 27

[Andrew McCreight [:mccr8]]

Olli and I looked at the spec a bit. For fragment navigation, popstate and hashchange should be run within the same task, but async. For regular forward and backward navigation, the spec says that the navigation itself should happen async ("To traverse the history by a delta delta, the user agent must append a task to this top-level browsing context's session history traversal queue"), and then the popstate and hashchange should run sync within that same task.

Right now in Gecko, it looks like we run the navigation itself sync (whether it is fragment or otherwise), but then dispatch a new task for the hashchange. (With the popstate always run sync, as per this bug.)

Comment 28

[Andrew McCreight [:mccr8]]

Olli filed a spec bug, because Blink apparently also always does sync popstate on fragment navigation and async hashchange, like Gecko: https://github.com/whatwg/html/issues/1792

Comment 29

[Andrew McCreight [:mccr8]]

I'm not working on this. I got bogged down in trying to figure out what the spec actually says, and what people actually do, and it hasn't come up as a particular issue for Quantum DOM yet so I think it is okay to leave it alone.

Comment 30

[Andrew Overholt [:overholt]]

Maybe this is something that is of interest to Samael?

Comment 31

[Samael Wang [:freesamael] (away for now)]

There doesn't seem to be a conclusion in the spec issue, but I'm happy to take a look and re-initiate the discussion to find out what we want to do, either at the spec or the implementation.

Comment 32

[Samael Wang [:freesamael] (away for now)]

Webkit decided not to do it async https://bugs.webkit.org/show_bug.cgi?id=153686#c2

Comment 33

[Samael Wang [:freesamael] (away for now)]

Sorry I can no longer work on this. I still think we should just change the spec, tho.