Add assert.contentBrowser and use it with commands which access tab related features

RESOLVED FIXED in Firefox 55

Status

Testing
Marionette
P2
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks: 1 bug)

Version 3
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [spec][17/06])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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?
Flags: needinfo?(ato)
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.
(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.
Flags: needinfo?(ato)
(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.
Flags: needinfo?(ato)
Assignee: nobody → hskupin
Blocks: 721859, 1368965
Status: NEW → ASSIGNED
Whiteboard: [spec][17/05]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8873032 - Flags: review?(ato)
Attachment #8873033 - Flags: review?(ato)
(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.
Flags: needinfo?(ato)

Comment 7

6 months 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 }.
Attachment #8873032 - Flags: review?(ato) → review-

Comment 8

6 months 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().
Attachment #8873033 - Flags: review?(ato) → review-
(Assignee)

Comment 9

6 months 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.
(Assignee)

Comment 10

6 months 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.
(Assignee)

Comment 11

6 months 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`.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

6 months 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`.
Similar to bug 1349861 this should be a P2.
Priority: -- → P2

Comment 16

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

Comment 17

6 months ago
mozreview-review
Comment on attachment 8873032 [details]
Bug 1368492 - Add assert.browsingContext for parent process.

https://reviewboard.mozilla.org/r/144516/#review148772
Attachment #8873032 - Flags: review?(ato) → review+

Comment 18

6 months 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
Attachment #8873033 - Flags: review?(ato) → review-
(Assignee)

Comment 19

6 months 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.
Andreas, can you please give feedback? Thanks.
Flags: needinfo?(ato)

Comment 21

6 months 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;
>   }
> }
Flags: needinfo?(ato)
(Assignee)

Comment 22

6 months 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.
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8873033 - Flags: review?(ato)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

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

Comment 28

6 months 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 ‘:’.
Attachment #8873033 - Flags: review?(ato) → review+
(Assignee)

Comment 29

6 months 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.
(Assignee)

Comment 30

6 months 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.
Comment hidden (mozreview-request)
Andreas, can you please check again? It should be in a state now which we both want to have.
Flags: needinfo?(ato)
(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.
Flags: needinfo?(ato)

Comment 34

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

Comment 35

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/674c1c263e87
https://hg.mozilla.org/mozilla-central/rev/bb7169659cd2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Whiteboard: [spec][17/05] → [spec][17/06]

Comment 36

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/ebab6ae00a55
https://hg.mozilla.org/releases/mozilla-beta/rev/d18618f51c5e
status-firefox55: --- → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.