Closed Bug 1386977 Opened 7 years ago Closed 7 years ago

No page load detected for pages manipulating the browser history (pushstate/popstate)

Categories

(Remote Protocol :: Marionette, enhancement, P1)

enhancement

Tracking

(firefox56 fixed, firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [spec][17/08])

Attachments

(1 file)

Originally reported as: https://github.com/mozilla/geckodriver/issues/815

In some cases Marionette doesn't see any page load events and as such waits until the page load times out. Here a test case:

Steps:
1) Open https://www.nrk.no/valg/2017/valgomat/
2) Click one of the smilies
3) Click on "Neste" which will appear as overlay of the image
4) Click the Back button

With step 3 we have a navigation to a new site, but none of the usual page load events we have registered for (beforeunload, pagehide, DOMContentLoaded, pageshow) are caught. As such the click command aborts early.

With step 4 we would also expect those events to fire but there aren't any neither.

Olli, when we talked lately about those events you did some testing. Mind telling me how I can easily see which events are fired in those cases? Is there a section for that in the developer tools? Or how are you doing that? Thanks.
Flags: needinfo?(bugs)
The page uses React (https://facebook.github.io/react/), so maybe the framework is doing something nifty with the location? When I check via the network inspector of devtools, I also do not see a page load including no DOMContentLoaded or pageshow events.

So how does it work when the location changes that no events are fired? Please note that we listen for the events in a frame script, so inside the content process. Interesting is that with the location change we also add a new item to the bfcache.
How are the event listeners registered? Using which phase and which group?
Flags: needinfo?(bugs)
(if you're using bubble phase in default group, page can call stopPropagation() and your listeners won't get called.)
(In reply to Olli Pettay [:smaug] from comment #2)
> How are the event listeners registered? Using which phase and which group?

It's still the same as we do in bug 1368434 and I'm waiting for a ni? from you. Maybe we can move it here now? So I have no idea what group means, so maybe can shed more details on it?

The registering for relevant events is happening here:
https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/testing/marionette/listener.js#148-200

We do that before we trigger the navigation, so all the listeners are attached.
Flags: needinfo?(bugs)
So the listeners are added to bubble phase in the default group. Any page calling stopPropagation() would
prevent marionette listeners to get the events.
Flags: needinfo?(bugs)
What I would have to change to make it working? Is it by using mozSystemGroup=true when registering the event listeners, or are some more things necessary?
Flags: needinfo?(bugs)
Depends on how they should work. If it is ok to to get the listeners called before the web page, then just use capture phase.
If after web page is better, then system group.
Flags: needinfo?(bugs)
Ideally we want to have those listeners called afterward I think. But I tried both to check the behavior but none of them are working for me for this specific website. No events are coming through.

When I use the developer tools and check in the inspector for a possible registered "onbeforeunload", "onpageshow" etc event, I always get back null. So does the page really stop the propagation of events? Is there an easy way to figure that out?
Flags: needinfo?(bugs)
Hmm, then it is about something else. (Marionette definitely adds listeners wrong way. )

Are you sure a new page is loaded when Neste is clicked? Could the page just use history.pushState() or history.replaceState() ?
Based on devtools I don't see a new document being loaded.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #9)
> Are you sure a new page is loaded when Neste is clicked? Could the page just
> use history.pushState() or history.replaceState() ?
> Based on devtools I don't see a new document being loaded.

Oh, that's actually a good hint! And indeed as I said in comment 1 I also don't see a full page load. I haven't known about the fact that history manipulation can be done this way. Are there some events we could listen for in such cases? Also when pushState() is used, does it mean that when we are going backward/forward in the history no events will be fired too?
Flags: needinfo?(bugs)
when going back/forward in history, popstate event should fire. But calling pushState doesn't fire events.
I guess you could add a nsIWebProgressListener and get it notified
http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/docshell/base/nsDocShell.cpp#12334
Flags: needinfo?(bugs)
Ok, so that's indeed the case here:

1502203294767	Marionette	TRACE	6 -> [0,15,"goBack",{}]
1502203294896	Marionette	DEBUG	Received DOM event "popstate" for "https://www.nrk.no/valg/2017/valgomat/"
1502203294902	Marionette	TRACE	6 <- [1,15,null,{}]
1502203294909	Marionette	TRACE	6 -> [0,16,"goForward",{}]
1502203295127	Marionette	DEBUG	Received DOM event "popstate" for "https://www.nrk.no/valg/2017/valgomat/paastand/1701"

(In reply to Olli Pettay [:smaug] from comment #11)
> when going back/forward in history, popstate event should fire. But calling
> pushState doesn't fire events.

So given the definition on MDN (https://developer.mozilla.org/en-US/docs/Web/API/History_API#The_pushState()_method) we might not have to wait for an event here, given that Firefox isn't going to load the URI at all:

> URL — The new history entry's URL is given by this parameter. Note that the browser
> won't attempt to load this URL after a call to pushState(), but it might attempt to
> load the URL later, for instance after the user restarts the browser. 

Which means it should not hurt us when Marionette doesn't wait. Also when the page added with pushState() gets navigated to later from a different URL we get the usual page load events:

1502203788346	Marionette	TRACE	6 -> [0,18,"goBack",{}]
1502203788350	Marionette	DEBUG	Received DOM event "beforeunload" for "http://example.org/"
1502203788570	Marionette	DEBUG	Received DOM event "pagehide" for "http://example.org/"
1502203788571	Marionette	DEBUG	Received DOM event "unload" for "http://example.org/"
1502203789059	Marionette	DEBUG	Received DOM event "DOMContentLoaded" for "https://www.nrk.no/valg/2017/valgomat/paastand/1701"
1502203789321	Marionette	DEBUG	Received DOM event "pageshow" for "https://www.nrk.no/valg/2017/valgomat/paastand/1701"
1502203789329	Marionette	TRACE	6 <- [1,18,null,{}]
1502203789348	Marionette	TRACE	6 -> [0,19,"goBack",{}]
1502203789468	Marionette	DEBUG	Received DOM event "popstate" for "https://www.nrk.no/valg/2017/valgomat/"
1502203789492	Marionette	TRACE	6 <- [1,19,null,{}]

Thank you Olli for the help!
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: For some websites no page loads are detected because of missing page load events → No page load detected for pages manipulating the browser history (pushstate/popstate)
Attachment #8895009 - Flags: review?(ato)
Are you sure WebDriver should take into account history
manipulation/popstate when navigating?
If we don't take this into account we experience page load timeouts all the time when tests are navigating back and forward. So supporting this is mandatory. Also this is an official HTML5 feature which will be used by a lot of sites.

Also the spec only talks about document ready state. We handle all those steps via page load events. So nothing is in conflict with the spec here. We still return when the page as loading has the correct state as given by the page load strategy.
Attachment #8895009 - Flags: review?(ato) → review?(dburns)
Comment on attachment 8895009 [details]
Bug 1386977 - Handle popstate events for page loads.

https://reviewboard.mozilla.org/r/166138/#review171832

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:225
(Diff revision 1)
> +
> +        # By using pushState() the target page is not loaded
> +        with self.assertRaises(errors.NoSuchElementException):
> +            self.marionette.find_element(By.ID, "target")
> +
> +        self.marionette.go_back()

Can you check that the element that shouldnt be there after clicking on forward is there when you go back
Attachment #8895009 - Flags: review?(dburns) → review+
Comment on attachment 8895009 [details]
Bug 1386977 - Handle popstate events for page loads.

https://reviewboard.mozilla.org/r/166138/#review171832

> Can you check that the element that shouldnt be there after clicking on forward is there when you go back

When navigating back in history we reach the originally loaded web page which has the link with the id "forward". The element with the id "target" we will first see when navigation_pushstate_target.html is reloaded.

I will add URL checks to make sure we navigate correctly.
Comment on attachment 8895009 [details]
Bug 1386977 - Handle popstate events for page loads.

https://reviewboard.mozilla.org/r/166138/#review171832

> When navigating back in history we reach the originally loaded web page which has the link with the id "forward". The element with the id "target" we will first see when navigation_pushstate_target.html is reloaded.
> 
> I will add URL checks to make sure we navigate correctly.

I also added a new test for refresh() with the last update which I missed before.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74d0c75abca5
Handle popstate events for page loads. r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/74d0c75abca5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Minimal changes with a very low risk of regressions. Would be great to get this test-only change uplifted to beta. Thanks.
Whiteboard: [checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/b6b61f8221fb
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta]
Whiteboard: [spec][17/08]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.