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

By moving commands from the content to the parent process (see bug 1368439) we need a new assertion to ensure that there is a valid contentBrowser available. 

We came across that for my patch on bug 1368101 which implements the `getCurrentUrl` processing purely in the parent process. So if we agree on that we would also have to update this command.

Andreas, any further feedback?

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

Adding the reply from Andreas on the other bug here, so we do not miss it.

(In reply to Andreas Tolfsen ‹:ato› from bug 1368101 comment #12)
> (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 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.

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

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

> By moving commands from the content to the parent process (see bug
> 1368439) we need a new assertion to ensure that there is a valid
> contentBrowser available.
>
> We came across that for my patch on bug 1368101 which implements the
> `getCurrentUrl` processing purely in the parent process. So if we
> agree on that we would also have to update this command.

Specifically, it needs to return a ‘no such window’ error, which
would normally be returned when driver.js calls listener.getCurrentUrl
and it can’t find the content frame.

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

(In reply to Henrik Skupin (:whimboo) from comment #1)
> > > 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.

So on the other I put outdated information. As you can see in the summary of this bug it should actually be contentBrowser and not contentWindow. From the parent process side we do not have access to the latter.

> > > > 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.

So if we don't want to have assertions in a `currentUrl` property we would still have to do checks if the current chrome window has a tabbrowser. Only then we can access contentBrowser. So we could raise an exception in case of a failure, or return eg. `null` in case of a non-browser window.

> > 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

Makes sense. So we can perfectly add a shared property. Thinking more about where to place it, I would suggest that we add it to the browser context class.

(In reply to Andreas Tolfsen ‹:ato› from comment #2)
> Specifically, it needs to return a ‘no such window’ error, which
> would normally be returned when driver.js calls listener.getCurrentUrl
> and it can’t find the content frame.

Correct. That's what we currently do.

Once we clarified the remaining parts I can get started to make this change. Thank you for the detailed answer.

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

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

> (In reply to Henrik Skupin (:whimboo) from comment #1)
> 
> So if we don't want to have assertions in a `currentUrl` property
> we would still have to do checks if the current chrome window has a
> tabbrowser. Only then we can access contentBrowser. So we could raise
> an exception in case of a failure, or return eg. `null` in case of a
> non-browser window.

Right, to be able to get a URL you need a content browser.  What your
patch is doing now looks right:

> assert.browsingContext(this.curBrowser);
> return this.curBrowser.contentBrowser.currentURI.spec;

I find this much better to read than the code it replaces:

> if (this.curBrowser.contentBrowser) {
>   return this.curBrowser.contentBrowser.currentURI.spec;
> } else {
>   throw new NoSuchWindowError(
>       "Not a browser window, or no tab currently selected");
> }

However, when you introduce a curBrowser.currentURL property, I don’t
understand why it must return null:

> get currentURL() {
>   if (this.contentBrowser) {
>     return this.contentBrowser.currentURI.spec;
>   }
>   return null;
> }

In my opinion, if a user tries to get the current URL and curBrowser
is not a content browser, would you not expect this to throw an error?
Failing silently and expecting the caller to check the return value
seems brittle and dangerous.

Because callers of curBrowser.currentURL are already using
assert.browsingContext, they would never get this far. I would argue
that with a caller that does not check this first, you would _expect_ an
error.

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

Comment on attachment 8873032 [details]
Bug 1368492 - Add assert.browsingContext for parent process.

https://reviewboard.mozilla.org/r/144516/#review148466

::: testing/marionette/assert.js:128
(Diff revision 1)
> +assert.browsingContext = function (context, msg = "") {
> +  msg = msg || "Not a valid top-level browsing context";
> +  assert.that(c => c && c.contentBrowser,
> +      msg,
> +      NoSuchWindowError)(context);
> +};

This implies that we also are in the top frame, yet the assertion only checks that a contentBrowser _is present_.  In other words, there is no test that the content frame script’s curContainer.frame is the top-level.

For this reason, I think that the error message here is probably wrong.  It should rather say “Current window does not have a content browser” or something similar.

::: testing/marionette/test_assert.js:122
(Diff revision 1)
>  
>    run_next_test();
>  });
>  
> +add_test(function test_browsingContext() {
> +  assert.browsingContext({ contentBrowser: 42});

Drop the space before contentBrowser.

::: testing/marionette/test_assert.js:124
(Diff revision 1)
>  });
>  
> +add_test(function test_browsingContext() {
> +  assert.browsingContext({ contentBrowser: 42});
> +
> +  for (let typ of [null, undefined, { contentBrowser: null }]) {

Similarly, drop spaces after { and before }.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

::: testing/marionette/browser.js:152
(Diff revision 1)
> +  get currentURL() {
> +    if (this.contentBrowser) {
> +      return this.contentBrowser.currentURI.spec;
> +    }
> +
> +    return null;
> +  }

See my comment on the bug that I wrote before the review.

::: testing/marionette/driver.js:1104
(Diff revision 1)
>   * @throws {UnexpectedAlertOpenError}
>   *     A modal dialog is open, blocking this operation.
>   */
>  GeckoDriver.prototype.goBack = function* (cmd, resp) {
>    assert.content(this.context);
> -  assert.window(this.getCurrentWindow());
> +  assert.browsingContext(this.curBrowser);

So I guess with this change and those similar to it, this.curBrowser is guaranteed to always be populated and that we don’t need to call this.getCurrentWindow().

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

Comment on attachment 8873032 [details]
Bug 1368492 - Add assert.browsingContext for parent process.

https://reviewboard.mozilla.org/r/144516/#review148466

> This implies that we also are in the top frame, yet the assertion only checks that a contentBrowser _is present_.  In other words, there is no test that the content frame script’s curContainer.frame is the top-level.
> 
> For this reason, I think that the error message here is probably wrong.  It should rather say “Current window does not have a content browser” or something similar.

Makes sense, and I had this wording before. I shouldn't have changed it in the last minute.

With that change we also have to revert the method name to `assert.contentBrowser`. Otherwise it will be confusing.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

> See my comment on the bug that I wrote before the review.

I see a disconnect here when we do not have this check in this class. Reason is that you imply that every code has to use `assert.contentBrowser` before it can call `.currentURL`. Otherwise it could fail with a plain JS error if no contentBrowser is available. This adds an extra level of complexity. Why shouldn't the class itself take care of it? 

Personally I have problems with such a logic.

> So I guess with this change and those similar to it, this.curBrowser is guaranteed to always be populated and that we don’t need to call this.getCurrentWindow().

We always have a chrome window registered. So `curBrowser` will indeed always be available. `assert.window` we would only have to use for those commands which are supported in chrome scope.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

> I see a disconnect here when we do not have this check in this class. Reason is that you imply that every code has to use `assert.contentBrowser` before it can call `.currentURL`. Otherwise it could fail with a plain JS error if no contentBrowser is available. This adds an extra level of complexity. Why shouldn't the class itself take care of it? 
> 
> Personally I have problems with such a logic.

Actually the `browser.Context` class is also used for non-browser chrome windows. Which means the `.currentURL` property should return a valid URL. We would check here if a `tabBrowser` is present. If yes, return the above, and otherwise `self.window.location.href`.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

> Actually the `browser.Context` class is also used for non-browser chrome windows. Which means the `.currentURL` property should return a valid URL. We would check here if a `tabBrowser` is present. If yes, return the above, and otherwise `self.window.location.href`.

Whereby this is not a good idea, because the Context class doesn't know about the current context. So we may need `contentURL`, and `chromeURL`.

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

Similar to bug 1349861 this should be a P2.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

> Whereby this is not a good idea, because the Context class doesn't know about the current context. So we may need `contentURL`, and `chromeURL`.

> I see a disconnect here when we do not have this check in this
> class. Reason is that you imply that every code has to use
> assert.contentBrowser before it can call .currentURL. Otherwise
> it could fail with a plain JS error if no contentBrowser is
> available. This adds an extra level of complexity. Why shouldn't the
> class itself take care of it?

On the contrary, I think _not failing_ is the problem.  If a consumer
forgets to use assert.contentBrowser before, then it is far worse to
return a known-bad value (null) than propagating an error that something
is wrong.

I know that browser.Context is used for non-content browser chrome
windows as well, so I like your idea of returning the chrome
URL for these.  This will make it more visible.  Maybe calling
ChromeWindow#document.location.href is an option?  In regular windows,
this returns "chrome://browser/content/browser.xul".

> We always have a chrome window registered. So `curBrowser` will indeed always be available. `assert.window` we would only have to use for those commands which are supported in chrome scope.

Right, so it goes without saying that you have a ChromeWindow when assert.browsingContext is used.  I think this is fine.

I was worried about the loss of error granularity, in the sense that it might be misleading to report that the current window is missing a content browser when the window itself is in fact not there.

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

Comment on attachment 8873032 [details]
Bug 1368492 - Add assert.browsingContext for parent process.

https://reviewboard.mozilla.org/r/144516/#review148772

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148774

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

> > I see a disconnect here when we do not have this check in this
> > class. Reason is that you imply that every code has to use
> > assert.contentBrowser before it can call .currentURL. Otherwise
> > it could fail with a plain JS error if no contentBrowser is
> > available. This adds an extra level of complexity. Why shouldn't the
> > class itself take care of it?
> 
> On the contrary, I think _not failing_ is the problem.  If a consumer
> forgets to use assert.contentBrowser before, then it is far worse to
> return a known-bad value (null) than propagating an error that something
> is wrong.
> 
> I know that browser.Context is used for non-content browser chrome
> windows as well, so I like your idea of returning the chrome
> URL for these.  This will make it more visible.  Maybe calling
> ChromeWindow#document.location.href is an option?  In regular windows,
> this returns "chrome://browser/content/browser.xul".

> On the contrary, I think _not failing_ is the problem.  If a consumer
forgets to use assert.contentBrowser before, then it is far worse to
return a known-bad value (null) than propagating an error that something
is wrong.

That's why I originally had the if condition which checked the `contentBrowser`, and raised an error if it's null. In that version the class method handled it correctly. So what do you propose now? Throwing a NoSuchWindowError inside the `currentURL` property?

> I know that browser.Context is used for non-content browser chrome
windows as well, so I like your idea of returning the chrome
URL for these.  This will make it more visible.  Maybe calling
ChromeWindow#document.location.href is an option?  In regular windows,
this returns "chrome://browser/content/browser.xul".

We currently use `document.location.href` when staying in chrome context to retrieve the URL from the ChromeWindow. That code is in driver.js. Really, the best would be to have a separate class for tabs beside the window. But this could be part of the big upcoming rewrite.

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

Andreas, can you please give feedback? Thanks.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

> > On the contrary, I think _not failing_ is the problem.  If a consumer
> forgets to use assert.contentBrowser before, then it is far worse to
> return a known-bad value (null) than propagating an error that something
> is wrong.
> 
> That's why I originally had the if condition which checked the `contentBrowser`, and raised an error if it's null. In that version the class method handled it correctly. So what do you propose now? Throwing a NoSuchWindowError inside the `currentURL` property?
> 
> > I know that browser.Context is used for non-content browser chrome
> windows as well, so I like your idea of returning the chrome
> URL for these.  This will make it more visible.  Maybe calling
> ChromeWindow#document.location.href is an option?  In regular windows,
> this returns "chrome://browser/content/browser.xul".
> 
> We currently use `document.location.href` when staying in chrome context to retrieve the URL from the ChromeWindow. That code is in driver.js. Really, the best would be to have a separate class for tabs beside the window. But this could be part of the big upcoming rewrite.

> That's why I originally had the if condition which checked the contentBrowser, and raised an error if it's null. In that version the class method handled it correctly. So what do you propose now? Throwing a NoSuchWindowError inside the currentURL property?

I support your earlier proposal to make currentURL not fail at all.  A possible implementation could look like this:

> get currentURL () {
>   if (this.contentBrowser) {
>     return this.contentBrowser.currentURI.spec;
>   } else {
>     let chromeWin = this.curBrowser.window;
>     return chromeWin.document.location.href;
>   }
> }

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

> > That's why I originally had the if condition which checked the contentBrowser, and raised an error if it's null. In that version the class method handled it correctly. So what do you propose now? Throwing a NoSuchWindowError inside the currentURL property?
> 
> I support your earlier proposal to make currentURL not fail at all.  A possible implementation could look like this:
> 
> > get currentURL () {
> >   if (this.contentBrowser) {
> >     return this.contentBrowser.currentURI.spec;
> >   } else {
> >     let chromeWin = this.curBrowser.window;
> >     return chromeWin.document.location.href;
> >   }
> > }

No, this will not work because if you are in the chrome process, you don't want to get the URL of the currently selected tab browser.

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

I spoke with Andreas on IRC and we are going to have the following implemented now:

1) Have a currentURL property on the driver, which allows to differentiate between chrome and content context
2) If chrome context is selected the chrome URL is returned
3) If content context is selected the current browser's URL is returned or a NoSuchWindow exception is raised

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review148468

> No, this will not work because if you are in the chrome process, you don't want to get the URL of the currently selected tab browser.

The update should hopefully fix your concerns.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review151852

::: testing/marionette/browser.js:157
(Diff revision 4)
> +  get currentURL() {
> +    if (this.contentBrowser) {
> +      return this.contentBrowser.currentURI.spec;
> +    }
> +
> +    return null;

I still think this should throw because APIs with null return values are pretty terrible to deal with, but I realise this patch is blocking other, more important work you have lined up, so I’m not going to create an issue on this.

::: testing/marionette/driver.js:149
(Diff revision 4)
>      return this.capabilities.get("moz:accessibilityChecks");
>    }
>  });
>  
> +Object.defineProperty(GeckoDriver.prototype, "currentURL", {
> +  get : function () {

Space before ‘:’.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review151852

> I still think this should throw because APIs with null return values are pretty terrible to deal with, but I realise this patch is blocking other, more important work you have lined up, so I’m not going to create an issue on this.

Ouch. I put all in driver.js instead of moving it here. Totally my fault. The next commit will fix it.

As try shows we now have a couple of test failures which I also have to fix. Looks like this change regressed something.

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

Comment on attachment 8873033 [details]
Bug 1368492 - Add currentURL property to the driver and context class.

https://reviewboard.mozilla.org/r/144518/#review151852

> Ouch. I put all in driver.js instead of moving it here. Totally my fault. The next commit will fix it.
> 
> As try shows we now have a couple of test failures which I also have to fix. Looks like this change regressed something.

The problem here is between those two lines:

>        return this.getCurrentWindow().location.href;
>        return this.curBrowser.window.location.href;

While the first works fine, the second fails. Reason is that getCurrentWindow() takes the current frame into account, while the latter doesn't do it. So we have to keep it.

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

Andreas, can you please check again? It should be in a state now which we both want to have.

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

(In reply to Henrik Skupin (:whimboo) from comment #32)
> Andreas, can you please check again? It should be in a state now which we
> both want to have.

Thanks, this looks good to me now.

	Pulsebot
	Comment 34 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/674c1c263e87
Add assert.browsingContext for parent process. r=ato
https://hg.mozilla.org/integration/autoland/rev/bb7169659cd2
Add currentURL property to the driver and context class. r=ato

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/674c1c263e87
https://hg.mozilla.org/mozilla-central/rev/bb7169659cd2

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/ebab6ae00a55
https://hg.mozilla.org/releases/mozilla-beta/rev/d18618f51c5e