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.

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.

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.