1421628 - Page load listener returns too early due to `popstate` event

Status: NEW

(Remote Protocol :: Marionette, enhancement, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As noticed on bug 1411264 the Marionette page load listener returns too early from a page load action because a `popstate` event is received in a lot of cases like the following:

> <a href="javascript:window.location.hash = 'foo'">hash change</a>

The events as fired are:

1511962904870	Marionette	TRACE	3 -> [0,8,"clickElement",{"id":"39f23b43-8de9-bd4e-8b7c-c40dcb77f9ce"}]
1511962904916	Marionette	DEBUG	Received DOM event "popstate" for "http://127.0.0.1:61185/clicks.html#foo"
1511962904917	Marionette	DEBUG	Received DOM event "beforeunload" for "http://127.0.0.1:61185/clicks.html#foo"
1511962904926	Marionette	DEBUG	Received DOM event "hashchange" for "http://127.0.0.1:61185/clicks.html#foo"

So when Marionette sees `popstate` we assume that the page load is done and return. But that is wrong in this case because the page load hasn't even be begun. 

Also in case of history manipulation there are page loads which ONLY fire a `popstate` event, and nothing else:

1511963401840	Marionette	DEBUG	Received DOM event "popstate" for "http://127.0.0.1:61489/navigation_pushstate.html"


I wonder how we can figure out if receiving a `popstate` event means the page listener can be stopped, or that we have to wait for follow-up events. 

Olli, I would kinda appreciate your feedback here. Thanks!

Comment 1

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

Check page's readyState ?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Check page's readyState ?

So it looks like that we should only check the `readyState` of the document when the `popstate` event wasn't fired based on history manipulation means the `event.state` is undefined. If history manipulation is involved, I assume we can take that event for granted that navigation has ended?

I have a patch which works locally and fixes the above mentioned problem.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Just to add for a `popstate` event based on history manipulation I get `undefined` for `readyState`, and then no other event is fired.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

What's interesting is the part of the spec: https://html.spec.whatwg.org/#event-popstate

> Fired at the Window when the user navigates the session history

My example from comment 0 clearly doesn't navigate the history but clicks a Javascript link which changes the URL's hash.

Comment 5

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

document.readyState is never undefined.

It isn't quite clear to me what this bug is about. popstate isn't something to be used for page load indication, I'd say.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Olli Pettay [:smaug] from comment #5)
> document.readyState is never undefined.

Just to make sure that I make use of the correct property. When testing it I was using `event.target.readyState`, whereby the event listener is attached to the frame scripts global.

When I print its value from a `popstate` handler, I get:

> ready state: [object Undefined] undefined

For `DOMContentLoaded` and `pageshow` I get the correct values:

> ready state: [object String] "interactive"
> ready state: [object String] "complete"

> It isn't quite clear to me what this bug is about. popstate isn't something
> to be used for page load indication, I'd say.

I had to add listening for `popstate` because clicking on the back or forward button, which navigates to a page as injected with history manipulation seems to only fire the `popstate` event. There is nothing else to listening for, or do I miss something?

In regards of this bug the problem is which events should we expect when just navigating from a URL to the same with a hash (fragment) appended, eg. https://example.org => https://example.org#foo. In case for Marionette navigation we see the following events when clicking a link that uses `window.location.hash` to set #foo (see comment 0):

> 1511962904870	Marionette	TRACE	3 -> [0,8,"clickElement",{"id":"39f23b43-8de9-bd4e-8b7c-c40dcb77f9ce"}]
> 1511962904916	Marionette	DEBUG	Received DOM event "popstate" for "http://127.0.0.1:61185/clicks.html#foo"
> 1511962904917	Marionette	DEBUG	Received DOM event "beforeunload" for "http://127.0.0.1:61185/clicks.html#foo"
> 1511962904926	Marionette	DEBUG	Received DOM event "hashchange" for "http://127.0.0.1:61185/clicks.html#foo"

Comment 7

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

(In reply to Henrik Skupin (:whimboo) from comment #6)
> (In reply to Olli Pettay [:smaug] from comment #5)
> > document.readyState is never undefined.
> 
> Just to make sure that I make use of the correct property. When testing it I
> was using `event.target.readyState`, whereby the event listener is attached
> to the frame scripts global.
> 
> When I print its value from a `popstate` handler, I get:
> 
> > ready state: [object Undefined] undefined

Then you're using some event which isn't dispatched to document object. document object has a getter for readyState, and that returns a string.
https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/dom/base/nsDocument.cpp#8818-8833


> I had to add listening for `popstate` because clicking on the back or
> forward button, which navigates to a page as injected with history
> manipulation seems to only fire the `popstate` event. There is nothing else
> to listening for, or do I miss something?
pageshow is fired always when loading a new page, even when the page is coming back from session history's bfcache.

popstate is needed if you need to know about fragment navigation within session history,
but those are page loads.

 
> In regards of this bug the problem is which events should we expect when
> just navigating from a URL to the same with a hash (fragment) appended, eg.
ok, so not page loading

> https://example.org => https://example.org#foo. In case for Marionette
> navigation we see the following events when clicking a link that uses
> `window.location.hash` to set #foo (see comment 0):
> 
> > 1511962904870	Marionette	TRACE	3 -> [0,8,"clickElement",{"id":"39f23b43-8de9-bd4e-8b7c-c40dcb77f9ce"}]
> > 1511962904916	Marionette	DEBUG	Received DOM event "popstate" for "http://127.0.0.1:61185/clicks.html#foo"
> > 1511962904917	Marionette	DEBUG	Received DOM event "beforeunload" for "http://127.0.0.1:61185/clicks.html#foo"
I don't understand where this beforeunload is coming from

Comment 8

[Alexei Barantsev]

Another sample that could be related to this issue:

driver = webdriver.Firefox()
driver.get("http://php-addressbook.sourceforge.net/demo/")
driver.find_element_by_css_selector("form[name=logout] a").click()
driver.get("http://php-addressbook.sourceforge.net/demo/")

Trace log will be attached belog.

Comment 9

[Alexei Barantsev]

Attachment: trace.log

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I sat down with Olli yesterday to go over this problem and get it described. As it turned out it's more complicated and Olli needs to take a deeper look at the specification for bfcache. So for now here a manual testcase to get it reproduced:

1) Open any website like https://www.mozilla.org/en-US/
2) Open the JS console and execute `history.pushState({}, "page 2", "bar.html");`
3) Reload the page (you should see a 404 page)
4) Go back in history

With step 4) only a `popstate` event is emitted, but nothing else. The question now is if this is the correct behavior.

If it is, then I would like to know how to detect that a bfcache navigation is taking place, because stopping the page load listener for `popstate` means we return too early from waiting for a `hashchange` navigation, due to `popstate` is getting emitted before.

Comment 11

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

or spec for the session history, since Chrome and Safari had weird behavior too, and Chrome doesn't have bfcache.

Comment 12

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

one should use pageshow/hide to detect bfcache navigations.
But do you need that information somehow in popstate?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No, if checking the persisted property of the pagehide event will work in those above cases, then we will be fine. Note that bug 1673059 covers all the bfcache related work that we will have to do.