Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

There is no value to let ´getCurrentUrl()´ send a synchronous request to the listener script just to retrieve the current URL. The content browser should all know about this.

It has been shown that the current behavior also has a performance issue and can delay Marionette by a couple of seconds especially for debug builds.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 1 • 7 years ago

I noticed that there are also other commands which will benefit from that. Those we can file as mentored follow-up bugs.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago

And the main reason why to do this change is still a possible hang in case of a reload of the framescript happening when calling this command. We have already seen this a lot in the past.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 7 years ago

On Monday I will have a detailed look at the performance improvements. But so far this patch looks promising.

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8871922 [details]
Bug 1368101 - getCurrentUrl can retrieve the URL via the chrome process.

https://reviewboard.mozilla.org/r/143434/#review147288

::: testing/marionette/driver.js:1027
(Diff revision 1)
> +      } else {
> +        throw new NoSuchWindowError(
> +          "Not a browser window, or no tab currently selected");
> +      }

Would this not be caught by assert.window(this.getCurrentWindow()) above?  If not, should an existence check on this.curBrowser.contentBrowser be part of it?

::: testing/marionette/driver.js:1120
(Diff revision 1)
>  
>    if (!this.curBrowser.contentBrowser.webNavigation.canGoBack) {
>      return;
>    }
>  
> -  let currentURL = yield this.listener.getCurrentUrl();
> +  let currentURL = this.getCurrentUrl();

I think probably we could define a property on the GeckoDriver prototype called this.currentURL that would return the current URL, without running the assertion checks in this.getCurrentUrl.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 7 years ago
mozreview-review-reply

Comment on attachment 8871922 [details]
Bug 1368101 - getCurrentUrl can retrieve the URL via the chrome process.

https://reviewboard.mozilla.org/r/143434/#review147288

> Would this not be caught by assert.window(this.getCurrentWindow()) above?  If not, should an existence check on this.curBrowser.contentBrowser be part of it?

Both times no. Reason is that we still want to work with chrome windows which do not have tabs and as such no content browsers. It means we cannot make this a general check in `assert.window`. But what we could do is to have a new assertion like `assert.contentWindow` which we could call separately here in case of the content scope being active. Does that sound fine to you?

> I think probably we could define a property on the GeckoDriver prototype called this.currentURL that would return the current URL, without running the assertion checks in this.getCurrentUrl.

What harms us from doing the assertion checks? If we do this change, then we should at least get rid of the chrome window check, but the `contentBrowser` check should be left in the property code.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 7 years ago

I had a look at Android test jobs like Mn3 and I have seen initially we have times between 200 and 500ms to get a reply. The worst case was the following after a timeout in loading the page for a refresh command:

05-29 01:42:46.834 I/Gecko   (  723): 1496047366836	Marionette	TRACE	193 -> [0,24,"getCurrentUrl",{}]
05-29 01:42:57.344 I/Gecko   (  723): 1496047377347	Marionette	TRACE	193 <- [1,24,null,{"value":"http://172.17.0.5:51606/slow?delay=3"}]

This took 11s only to return the current URL. 

With my changes the following is visible for the same method:

05-26 14:56:39.682 I/Gecko   (  719): 1495835799686	Marionette	TRACE	253 -> [0,24,"getCurrentUrl",{}]
05-26 14:56:39.732 I/Gecko   (  719): 1495835799732	Marionette	TRACE	253 <- [1,24,null,{"value":"http://172.17.0.5:51449/slow?delay=3"}]

It was run only once and we would need more data to better compare it, but for that single time it shows a time saving of 10s. For all the other instance of `getCurrentUrl` it takes about 30 - 60ms now.

	David Burns :automatedtester
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8871922 [details]
Bug 1368101 - getCurrentUrl can retrieve the URL via the chrome process.

https://reviewboard.mozilla.org/r/143434/#review147288

> Both times no. Reason is that we still want to work with chrome windows which do not have tabs and as such no content browsers. It means we cannot make this a general check in `assert.window`. But what we could do is to have a new assertion like `assert.contentWindow` which we could call separately here in case of the content scope being active. Does that sound fine to you?

If we are looking to move things to the parent process to cut down on traffic between the processes then we should make a `assert.contentWindow` method. This can be done in a follow up.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

Comment on attachment 8871922 [details]
Bug 1368101 - getCurrentUrl can retrieve the URL via the chrome process.

https://reviewboard.mozilla.org/r/143434/#review147288

> If we are looking to move things to the parent process to cut down on traffic between the processes then we should make a `assert.contentWindow` method. This can be done in a follow up.

Thanks David. I will file a follow-up bug for that which will block further work on bug 1368439.

	Pulsebot
	Comment 10 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a025d46da10
getCurrentUrl can retrieve the URL via the chrome process. r=ato

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 11 • 7 years ago

We decided to land this because it should fix our OF #1 failure of the week in Firefox UI Tests.

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #6)

> > Would this not be caught by assert.window(this.getCurrentWindow())
> > above?  If not, should an existence check on
> > this.curBrowser.contentBrowser be part of it?
>
> Both times no. Reason is that we still want to work with chrome
> windows which do not have tabs and as such no content browsers. It
> means we cannot make this a general check in `assert.window`. But what
> we could do is to have a new assertion like `assert.contentWindow`
> which we could call separately here in case of the content scope being
> active. Does that sound fine to you?

Yes, as long as we have a clear definition of ‘content window’.  If
we are actually talking about a ChromeWindows which have a <xul:browser>
present, then could be more consistent to reuse existing Gecko
terminology.

> > I think probably we could define a property on the GeckoDriver
> > prototype called this.currentURL that would return the current URL,
> > without running the assertion checks in this.getCurrentUrl.
>
> What harms us from doing the assertion checks? If we do this change,
> then we should at least get rid of the chrome window check, but the
> `contentBrowser` check should be left in the property code.

There should be no reprecussions from running the assertions multiple
times, but it is both unnecessary and means we run the steps outlined in
the specification in a different order.

Whilst it is not a firm requirement that an implementation’s
algorithms follows the algorithmic steps in succession, it is not hard
to think of cases where running the assertions as part of another
command could cause problems.

More importantly, it increases the toll on the reader to understand what
errors might be returned when examining the command that calls another
command.  For example, when goBack calls the getCurrentUrl command
again, it is not immediately obvious that the window- and user prompt
assertions are run again.

There should be a separation between the session state and the entry
points for commands, and commands should not be allowed to call
other commands directly.  Whilst it is certainly possible to call
GeckoDriver#getCurrentUrl when you need the URL, it introduces the
danger that one of its assertions may interfere with the calling code,
perhaps throwing an unwanted error.  Now, getting the URL is not the
best example of a command that might induce this situation, but the
principle still holds: interdependency between commands

	Andreas Tolfsen ❲:ato❳
	Comment 13 • 7 years ago

(Got cut off there.)

… leads to unclear delegation of who is responsible for running
preconditions, makes it unnecessarily hard to review whether a command
implements the algorithmic steps correctly thereby also posing race
condition risks, and is computationally pointless.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 14 • 7 years ago

I copied the reply from Andreas to bug 1368492.

	Carsten Book [:Tomcat]
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4a025d46da10