Closed Bug 1368101 Opened 2 years ago Closed 2 years ago

Change the getCurrentUrl command to retrieve the URL via the chrome process

Categories

(Testing :: Marionette, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: hang, perf)

Attachments

(1 file)

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.
I noticed that there are also other commands which will benefit from that. Those we can file as mentored follow-up bugs.
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.
Keywords: hang
On Monday I will have a detailed look at the performance improvements. But so far this patch looks promising.
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.
Attachment #8871922 - Flags: review?(ato) → review+
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.
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.
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.
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.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a025d46da10
getCurrentUrl can retrieve the URL via the chrome process. r=ato
We decided to land this because it should fix our OF #1 failure of the week in Firefox UI Tests.
(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
(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.
I copied the reply from Andreas to bug 1368492.
https://hg.mozilla.org/mozilla-central/rev/4a025d46da10
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1369083
You need to log in before you can comment on or make changes to this bug.