Closed Bug 1259029 Opened 8 years ago Closed 8 years ago

chrome_window_handles do not list non-browser chrome windows in content scope

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox47 fixed, firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: pi-marionette-server)

Attachments

(2 files)

Attached file Minimized testcase
When a test opens several chrome windows and some of those are not a browser window, they are not listed as chrome window handles when Marionette is in content scope. You have to explicitly switch into chrome scope. All normal browser windows are listed.

I do not see that we should make a difference here. The list as returned by `chrome_window_handles` should always be equal whether you are in chrome or in content scope. In case of `window_handles` we already check for a `gBrowser` object to iterate through tabs (content windows). So the proposed change should not make a difference.

Attached you can find a test case.

Andreas, can you please tell me your thoughts? Thanks.
Flags: needinfo?(ato)
Blocks: 1256425
What is the difference between a browser and a chrome window?

As far as I can tell the getChromeWindowHandles function in chrome space (https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/driver.js#L1426) is always called, irregardless of whether the current context is content.
Flags: needinfo?(ato)
The problem here is not the `getChromeWindowHandles` function but `getWinEnumerator()`:
http://hg.mozilla.org/mozilla-central/annotate/efe7d026ac64/testing/marionette/driver.js#l273

Here we differentiate between chrome and content.
So if we are in content scope the enumerator ONLY cares about browser windows. Other chrome windows like the about window, the library etc are not taking care of. When I use null in all the cases it works fine.
(In reply to Henrik Skupin (:whimboo) from comment #3)
> So if we are in content scope the enumerator ONLY cares about browser
> windows. Other chrome windows like the about window, the library etc are not
> taking care of. When I use null in all the cases it works fine.

OK, so the items returned from getChromeWindowHandles varies depending on whether the current context is content or chrome because `typ` is set to "navigator:browser" when trying to get the chrome window handles.

I would guess that the this.appName != "B2G" test is a workaround for this bug.
The change we would need here is:

   if (this.appName != "B2G" && this.context == Context.CONTENT) {
-    typ = "navigator:browser";
+    // typ = "navigator:browser";
   }

Means that we always work with a window enumerator which iterates over all existing chrome windows and not only navigator:browser. Removing the this.context check will not help.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Comment on attachment 8733893 [details]
MozReview Request: Bug 1259029 - Ensure that getChromeWindowHandles() returns the same window list for chrome and content scope. r?ato

https://reviewboard.mozilla.org/r/41989/#review38475

::: testing/marionette/driver.js:1370
(Diff revision 1)
>   * @return {Array.<string>}
>   *     Unique window handles.
>   */
>  GeckoDriver.prototype.getWindowHandles = function(cmd, resp) {
>    let hs = [];
> -  let winEn = this.getWinEnumerator();
> +  let winEn = Services.wm.getEnumerator(null);

According to API documentation for [nsIWindowMediator](https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWindowMediator#getEnumerator()) we should pass `null` here to get all windows, like your patch does.

However, is it possible this code will work if we pass `undefined` as well, or that is, passing no arguments?

The reason I bring this up is because I have a dislike for “magic” arguments like this, where it’s hard to infer from the context what a value signifies.

::: testing/marionette/driver.js:1373
(Diff revision 1)
>  GeckoDriver.prototype.getWindowHandles = function(cmd, resp) {
>    let hs = [];
> -  let winEn = this.getWinEnumerator();
> +  let winEn = Services.wm.getEnumerator(null);
>    while (winEn.hasMoreElements()) {
>      let win = winEn.getNext();
>      if (win.gBrowser && this.appName != "B2G") {

So just to be clear: Is this check sufficient for no longer passing `"navigator:browser"` to `getEnumerator`?

::: testing/marionette/harness/marionette/tests/unit/test_window_handles.py:101
(Diff revision 1)
> +        with self.marionette.using_context('chrome'):
> +            orig_chrome_handle = self.marionette.current_chrome_window_handle
> +            chrome_handles = self.marionette.chrome_window_handles
> +
> +            # Open a browser and a non-browser (about window) chrome window
> +            self.marionette.execute_script(""" window.open(); """)

Unnecessary triple quotes and semicolon.

::: testing/marionette/harness/marionette/tests/unit/test_window_handles.py:112
(Diff revision 1)
> +            for handle in handles_chrome_scope:
> +                if handle != orig_chrome_handle:
> +                    self.marionette.switch_to_window(handle)
> +                    self.marionette.close_chrome_window()

Assert that we returned to the expected number of open windows (1?)
Attachment #8733893 - Flags: review?(ato)
https://reviewboard.mozilla.org/r/41989/#review38475

> According to API documentation for [nsIWindowMediator](https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWindowMediator#getEnumerator()) we should pass `null` here to get all windows, like your patch does.
> 
> However, is it possible this code will work if we pass `undefined` as well, or that is, passing no arguments?
> 
> The reason I bring this up is because I have a dislike for “magic” arguments like this, where it’s hard to infer from the context what a value signifies.

`undefined` also works but isn't that also magic? Maybe we should keep the `getWinEnumerator()` method and add a comment which explains it? But that's up to you. For now I made the required changes.

> So just to be clear: Is this check sufficient for no longer passing `"navigator:browser"` to `getEnumerator`?

Here we want to iterate the list of tabs. Those are only available for browser windows, which are the only window type with a `gBrowser` property. So this should be fine, and also confirmed that all unit tests are passing.

> Assert that we returned to the expected number of open windows (1?)

I will fix that. Thanks.
Attachment #8733893 - Flags: review?(ato)
Comment on attachment 8733893 [details]
MozReview Request: Bug 1259029 - Ensure that getChromeWindowHandles() returns the same window list for chrome and content scope. r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41989/diff/1-2/
While for a failure in takeScreenshot which I see locally when I miss to switch to the start tab, I found that getCurrentWindow also makes use of the above assumption:

http://hg.mozilla.org/mozilla-central/annotate/efe7d026ac64/testing/marionette/driver.js#l252

Maybe we should this get fixed too so the method will also work for non-browser windows.
Whereby I think that the last comment should be covered by another bug.
Comment on attachment 8733893 [details]
MozReview Request: Bug 1259029 - Ensure that getChromeWindowHandles() returns the same window list for chrome and content scope. r?ato

https://reviewboard.mozilla.org/r/41989/#review38763

LGTM.

::: testing/marionette/driver.js:1370
(Diff revision 2)
>   * @return {Array.<string>}
>   *     Unique window handles.
>   */
>  GeckoDriver.prototype.getWindowHandles = function(cmd, resp) {
>    let hs = [];
> -  let winEn = this.getWinEnumerator();
> +  let winEn = Services.wm.getEnumerator(undefined);

`undefined` is implicit so we can drop the argument entirely.

::: testing/marionette/driver.js:1422
(Diff revision 2)
>   * @return {Array.<string>}
>   *     Unique window handles.
>   */
>  GeckoDriver.prototype.getChromeWindowHandles = function(cmd, resp) {
>    let hs = [];
> -  let winEn = this.getWinEnumerator();
> +  let winEn = Services.wm.getEnumerator(undefined);

Drop argument

::: testing/marionette/driver.js:1497
(Diff revision 2)
>      return switchTo == name ||
>          switchTo == contentWindowId ||
>          switchTo == outerId;
>    };
>  
> -  let winEn = this.getWinEnumerator();
> +  let winEn = Services.wm.getEnumerator(undefined);

Drop argument

::: testing/marionette/driver.js:2327
(Diff revision 2)
>    if (this.appName == "B2G") {
>      return;
>    }
>  
>    let nwins = 0;
> -  let winEn = this.getWinEnumerator();
> +  let winEn = Services.wm.getEnumerator(undefined);

Drop argument

::: testing/marionette/driver.js:2374
(Diff revision 2)
>      return;
>    }
>  
>    // Get the total number of windows
>    let nwins = 0;
> -  let winEn = this.getWinEnumerator();
> +  let winEn = Services.wm.getEnumerator(undefined);

Drop argument

::: testing/marionette/driver.js:2425
(Diff revision 2)
>          globalMessageManager.broadcastAsyncMessage(
>              "Marionette:deleteSession" + browser.knownFrames[i], {});
>        }
>      }
>  
> -    let winEn = this.getWinEnumerator();
> +    let winEn = Services.wm.getEnumerator(undefined);

Drop argument

::: testing/marionette/harness/marionette/tests/unit/test_window_handles.py:100
(Diff revision 2)
> +        start_tab = self.marionette.current_window_handle
> +        start_chrome_window = self.marionette.current_chrome_window_handle
> +        start_chrome_windows = self.marionette.chrome_window_handles
> +
> +        # Ensure that we work in chrome scope so we don't have any limitations
> +        with self.marionette.using_context('chrome'):

Please use double quotes

::: testing/marionette/harness/marionette/tests/unit/test_window_handles.py:102
(Diff revision 2)
> +        start_chrome_windows = self.marionette.chrome_window_handles
> +
> +        # Ensure that we work in chrome scope so we don't have any limitations
> +        with self.marionette.using_context('chrome'):
> +            # Open a browser and a non-browser (about window) chrome window
> +            self.marionette.execute_script('window.open()')

Please use double quotes
Attachment #8733893 - Flags: review?(ato) → review+
(In reply to Henrik Skupin (:whimboo) from comment #10)
> While for a failure in takeScreenshot which I see locally when I miss to
> switch to the start tab, I found that getCurrentWindow also makes use of the
> above assumption:
> 
> http://hg.mozilla.org/mozilla-central/annotate/efe7d026ac64/testing/
> marionette/driver.js#l252
> 
> Maybe we should this get fixed too so the method will also work for
> non-browser windows.

Yes.  Please file a separate bug when you have time.
https://reviewboard.mozilla.org/r/41989/#review38763

> `undefined` is implicit so we can drop the argument entirely.

No, you have to specify an argument. If you don't do it it will raise an exception. So we have the option between `undefined` or `null`.

> Please use double quotes

Wow, first time I have to use double quotes in Python code. Will be fixed.
Comment on attachment 8733893 [details]
MozReview Request: Bug 1259029 - Ensure that getChromeWindowHandles() returns the same window list for chrome and content scope. r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41989/diff/2-3/
Comment on attachment 8733893 [details]
MozReview Request: Bug 1259029 - Ensure that getChromeWindowHandles() returns the same window list for chrome and content scope. r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41989/diff/3-4/
https://hg.mozilla.org/mozilla-central/rev/26e9ebbb6441
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.