Last Comment Bug 1330348 - Make forward- and backward commands synchronous
: Make forward- and backward commands synchronous
Status: ASSIGNED
: ateam-marionette-server, ateam-marionette-spec
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: Version 3
: All All
P1 major (vote)
: ---
Assigned To: Henrik Skupin (:whimboo) [away 02/18 - 02/27]
:
:
Mentors:
https://developer.mozilla.org/en-US/d...
Depends on: 1333458
Blocks: webdriver 1305659 1332998 1335778 1335952
  Show dependency treegraph
 
Reported: 2017-01-11 08:21 PST by Andreas Tolfsen
Modified: 2017-02-19 07:02 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Bug 1330348 - Make forward- and backward commands synchronous. (59 bytes, text/x-review-board-request)
2017-02-16 12:55 PST, Henrik Skupin (:whimboo) [away 02/18 - 02/27]
no flags Details | Review

Description User image Andreas Tolfsen 2017-01-11 08:21:32 PST
Tests using the forward- and backwards commands in Marionette are known to cause intermittents.  We suspect this is because they are not implemented in a synchronous manner, e.g. they don’t wait for the document to reach the expected state before returning.

Because navigating backwards and forwards may either reload a document from bfcache or navigate internally in a document (if push state or internal anchors have been used), we _do not_ expect a document load event to occur.  I’m not entirely sure what the right fix is, but suspect the fix might involve updating the WebDriver standard to be more specific.
Comment 2 User image Andreas Tolfsen 2017-01-11 08:24:46 PST
https://bugzilla.mozilla.org/show_bug.cgi?id=1291320 is for fixing the Refresh command.  The refresh command, in contrast to Back and Forward, always expect the document to reload and for the document load events to fire.
Comment 3 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-01-11 08:31:06 PST
Keep in mind that also JS code can trigger navigating back and forward. For those and like other commands (click, keypress) which cause a load of a page, it will be hard to check that in Marionette server with the current algorithm.

In Mozmill I solved this problem by assigning a uuid for each page load. Once a page gets unloaded it gets reset (and the current value backup'ed), and once loaded a new value is set. With that we were able to see that a page load was happening for a tab. So this was able to also handle external page loads triggered by whatever the user is doing.
Comment 4 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-02 04:33:00 PST
I feel this should wait for a solution on bug 1288336, so we know better what could be done here. Andreas, David, what do you think?
Comment 5 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-02 04:42:37 PST
Well, the bug number I wanted to use should have been bug 1333458.
Comment 6 User image Andreas Tolfsen 2017-02-02 05:08:21 PST
I don’t think we should wait.  This is a P1.
Comment 7 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-07 03:04:33 PST
I will give this a shot.
Comment 8 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-07 04:00:50 PST
Information about how to work with bfcache can be found here:
https://developer.mozilla.org/en-US/docs/Working_with_BFCache
Comment 9 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-07 06:17:37 PST
Experimenting with events while using go_back() in a Marionette test shows the following:

> ******** Caught unload event: pagehide
> ******** Caught load event: DOMContentLoaded
> ******** Caught load event: pageshow

This means that we also get a `DOMContentLoaded` event fired we could make use of. There shouldn't be a need to wait for the full page being loaded. This is just similar to what we do for the `get()` command.

For about: pages I do not see any event being fired, and I think that this might be covered by the progress listener as part of the `get()` command.

My proposal would really be to improve the generic page load behavior, at least the part for splitting out the waitForPageLoad method so that this method can be also used by other commands.
Comment 10 User image Andreas Tolfsen 2017-02-07 07:21:26 PST
(In reply to Henrik Skupin (:whimboo) from comment #9)
> Experimenting with events while using go_back() in a Marionette test shows
> the following:
> 
> > ******** Caught unload event: pagehide
> > ******** Caught load event: DOMContentLoaded
> > ******** Caught load event: pageshow
> 
> This means that we also get a `DOMContentLoaded` event fired we could make
> use of. There shouldn't be a need to wait for the full page being loaded.
> This is just similar to what we do for the `get()` command.

Bear in mind that not all traversing of the browsing context’s history by delta +/- 1 cause DOMContentLoaded to fire.  This is the case if the document by default does not fire DOMContentLoaded events (ImageDocument) as well as for a few ‘special’ schemes (see https://url.spec.whatwg.org/#is-special), or when navigating _within_ one document by fragment.

> For about: pages I do not see any event being fired, and I think that this
> might be covered by the progress listener as part of the `get()` command.

This is also true for other ‘special’ local schemes, including "blob", "data", and "filesystem".

> My proposal would really be to improve the generic page load behavior, at
> least the part for splitting out the waitForPageLoad method so that this
> method can be also used by other commands.

Yes, that is a good starting point.  I would suggest taking the existing implementation with its current behaviour and make that work independently before trying to improve on it.
Comment 11 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-09 04:17:38 PST
(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> I don’t think we should wait.  This is a P1.

Given that the page load detection code will completely change via bug 1333458, I don't think it make sense to work on that code now, which gets thrown away soon again.
Comment 12 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-16 05:06:13 PST
I spoke with David yesterday and we decided to not wait for bug 1333458 but go ahead right now to get it actually fixed.

So while working on the implementation I noticed a situation when the spec is not that clear and causes a huge delay in running the tests. This happens when back/forward gets called if no history is actually available. In such cases we would try to navigate but then fail after 300s with a timeout. The browser should always know if such a navigation is possible, so bailing out early without loosing 300s would be great.

I filed the following issue for the webdriver spec: https://github.com/w3c/webdriver/issues/776
Comment 13 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-16 12:55:23 PST Comment hidden (mozreview-request)
Comment 14 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-16 12:58:54 PST
Comment on attachment 8838209 [details]
Bug 1330348 - Make forward- and backward commands synchronous.

This is an early WIP which just makes it working for the couple of examples. I would like to know if this is the right direction I'm heading. 

Short note... Similar to get() this code runs all in content. When I try to attach the event listeners in chrome, I do not see any events coming through. Also I made a unload method, which could generally be used. It returns if a page load is expected, and if so the calling code can execute the waitForPageLoad method. Both we could also use for get() later, if it turns out my idea from running all in chrome doesn't work.
Comment 15 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-17 04:00:11 PST
Also as noticed the post navigation steps for backward and forward are exactly identical to get(). So it's not enough to only wait for pageshow. Take into account that the former page could have been an error page, local file URL, and all that.

The open webdriver spec issue for that is: https://github.com/w3c/webdriver/issues/777
Comment 16 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-17 08:29:03 PST Comment hidden (mozreview-request)
Comment 17 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-02-17 08:32:35 PST
So I just uploaded the latest state of the patch which works for back and forward except for remoteness changes. Here I have the problem in how I have to specify the parameters for the waitForPageLoaded method. Maybe you can give me a hint, Andreas?

Otherwise I have a problem now with the following:

> **** rejecting promise for timeout
> JavaScript error: chrome://marionette/content/error.js, line 160: Error: Error loading page, timed out (checkLoad)

As it looks like the new code in error.js doesn't correctly forward the rejected promise, or I didn't update something in my patch in regards to those changes.

Andreas, I would appreciate if you can have a look over this WIP patch and tell me your feedback. Thanks.

Please be aware that I will not be around all next week.
Comment 18 User image Andreas Tolfsen 2017-02-19 06:53:22 PST
Comment on attachment 8838209 [details]
Bug 1330348 - Make forward- and backward commands synchronous.

https://reviewboard.mozilla.org/r/113166/#review115248

::: testing/marionette/listener.js:1159
(Diff revision 2)
> +function waitForPageLoaded(msg) {
> +  let {pageTimeout, startTime} = msg;

Changing `msg` to `msg.json` should let you extract `pageTimeout` and `startTime` correctly.
Comment 19 User image Andreas Tolfsen 2017-02-19 07:00:30 PST
Comment on attachment 8838209 [details]
Bug 1330348 - Make forward- and backward commands synchronous.

https://reviewboard.mozilla.org/r/113166/#review115250

::: testing/marionette/driver.js:1034
(Diff revision 2)
> -  yield this.listener.goBack();
> +  if (!this.curBrowser.tab) {
> +    return;
> +  }

What do these test for?

::: testing/marionette/driver.js:1046
(Diff revision 2)
> +  // If a remoteness update interrupts our page load, this will never return
> +  // We need to re-issue this request to correctly poll for readyState and
> +  // send errors.
> +  dump('*** current message id: ' + this.listener.activeMessageId + '\n');
> +  this.curBrowser.pendingCommands.push(() => {
> +    cmd.parameters.pageTimeout = this.timeouts.pageLoad;
> +    cmd.parameters.startTime = new Date().getTime();
> +    this.mm.broadcastAsyncMessage(
> +        "Marionette:waitForPageLoaded" + this.curBrowser.curFrameId,
> +        cmd.parameters);
> +  });

I’m not a great fan of the `pendingCommands.push(…)` API, but I guess this works fine for now?

In a more ideal world we would make _all_ calls through the `AsyncProxyChannel` (`this.listener`) repeat if they failed to execute so that we wouldn’t have to special case `waitForPageLoaded`.

I don’t know off hand if that is feasible or if we can even detect if a command failed, but it would certainly be possible to assume that all listener messages are prone to failure because the content frame script might disappear at any time.  We could then clear `pendingCommands` or the equivalent every time a response comes through from the listener.

Anyway, I’m not expecting you to fix that here.  Just explaining that `pendingCommands` is an abomination and we would do well by getting rid of it.
Comment 20 User image Andreas Tolfsen 2017-02-19 07:02:20 PST
Just curious about waitForPageLoaded/waitForPageUnloaded: I thought you said you would do this waiting entirely in chrome space?  That the web progress listener would only send the right events if it was attached to the <xul:browser> in chrome?

If we were to move this code to chrome, the only thing we would have to contact the listener about would be to get the document’s readyState.

Note You need to log in before you can comment on or make changes to this bug.