Commands in driver.js miss checks for valid ChromeWindow (DOMwindow) - no NoSuchWindow failure thrown

RESOLVED FIXED in Firefox 53

Status

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks: 2 bugs)

Version 3
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 wontfix, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

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

Attachments

(7 attachments, 1 obsolete attachment)

A lot of chrome methods in driver.js do not check if the current window the operation is getting performed on is still valid. As result we get the following:

> MarionetteException: win is null

Instead each of the methods should throw a NoSuchWindowException.
I just noticed that getCurrentWindow() looks to be broken:

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/marionette/driver.js#257

This doesn't return a DOMWindow as noted in the docstring, but a ChromeWindow.

Andreas, what should it actually return?
Flags: needinfo?(ato)
Both Services.wm.getMostRecentWindow and this.curBrowser.window returns nsIDOMWindow.  this.curFrame is sometimes set to <xul:browser>’s DOMWindow: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/browser#p-contentWindow
Flags: needinfo?(ato)
This is not true. When I add a dump statement right next to the enumerator I always get `[object ChromeWindow]` returned. Also a check for `instanceof(Components.interfaces.nsIDOMWindow)` fails.
Blocks: 1288336
Created attachment 8822269 [details]
testcase

Here an example testcase which fails in Marionette server with 'browser is undefined' and 'this.browserForTab is undefined'. Reason is that the chrome window we want to operate on simply doesn't exist anymore.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
To fully support the spec for window checks we have two options:

1) Add a check in each of the methods in driver.js to ensure that the DOMWindow is still present

2) Have a single check in dispatcher.js right before we call the driver command, including a list of commands which do not need such a check.

The latter would make lesser work. Andreas, what would your preferred solution?
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #3)
> This is not true. When I add a dump statement right next to the enumerator I
> always get `[object ChromeWindow]` returned. Also a check for
> `instanceof(Components.interfaces.nsIDOMWindow)` fails.

This means the documentation on MDN is correct.  But please remember that an nsIDOMWindow is not quite the same as Window/WindowProxy.
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> (In reply to Henrik Skupin (:whimboo) from comment #3)
> > This is not true. When I add a dump statement right next to the enumerator I
> > always get `[object ChromeWindow]` returned. Also a check for
> > `instanceof(Components.interfaces.nsIDOMWindow)` fails.
> 
> This means the documentation on MDN is correct.  But please remember that an
> nsIDOMWindow is not quite the same as Window/WindowProxy.

Is _incorrect_.
(In reply to Henrik Skupin (:whimboo) from comment #5)
> To fully support the spec for window checks we have two options:
> 
> 1) Add a check in each of the methods in driver.js to ensure that the
> DOMWindow is still present
> 
> 2) Have a single check in dispatcher.js right before we call the driver
> command, including a list of commands which do not need such a check.
> 
> The latter would make lesser work. Andreas, what would your preferred
> solution?

I think we would need to do the first, as a certain subclass of commands are meant to work without a window.

I would suggest adding a new assertion to testing/marionette/assert.js.
Also, if we have time before 23 January, it would be great to get a fix for this into Aurora (next ESR).
(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> This means the documentation on MDN is incorrect.  But please remember that an
> nsIDOMWindow is not quite the same as Window/WindowProxy.

Ok, so in that case we can simply use window.document.defaultView to check if the window is correct. If it is null we can assert the failure.

(In reply to Andreas Tolfsen ‹:ato› from comment #8)
> I think we would need to do the first, as a certain subclass of commands are
> meant to work without a window.

Because of that I said we would need a blacklist where we can add those commands which do not need a check. This list is way shorter as adding an assert to each of the command methods. So you are still behind the addition to each method which needs to access a window? Please let me know so I can get started.

> I would suggest adding a new assertion to testing/marionette/assert.js.

Yep, that's what I have locally already as a prototype.

(In reply to Andreas Tolfsen ‹:ato› from comment #9)
> Also, if we have time before 23 January, it would be great to get a fix for
> this into Aurora (next ESR).

January the 23rd is not a hard cut-off for us. We still have time during the beta period to uplift patches for 52. But my goal is to have this patch ready early next week.
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #10)
> (In reply to Andreas Tolfsen ‹:ato› from comment #8)
> > I think we would need to do the first, as a certain subclass of commands are
> > meant to work without a window.
> 
> Because of that I said we would need a blacklist where we can add those
> commands which do not need a check. This list is way shorter as adding an
> assert to each of the command methods. So you are still behind the addition
> to each method which needs to access a window? Please let me know so I can
> get started.

I’m still in favour of a least-surprises approach, and an extra `assert.window(container)` alongside the other assertions made for each command is not invasive and it follows the pattern used for other assertions.

One thing we could consider in the future is to “annotate” individual commands, but my problem with preconditions is that it doesn’t exactly define in _which order or _when_ precondition steps are run:

    GeckoDriver.prototype.takeScreenshot.preconditions = [hasWindow, windowID];

Since the WebDriver specification is rather exact about the order steps should be taken in (e.g. a top-level browsing context present test, before parameter present before, before type check, before bounds check, &c.), it is good to know we can control, and more importantly review, this aspect easily by reading a command implementation line-by-line.

While generalising checks for multiple commands with a blacklist of commands not to check might save us a few lines of code, it makes the code more surprising and harder to reason about for newcomers who are oblivious to the fact that a command’s implementation is influenced by the dispatcher.

> (In reply to Andreas Tolfsen ‹:ato› from comment #9)
> > Also, if we have time before 23 January, it would be great to get a fix for
> > this into Aurora (next ESR).
> 
> January the 23rd is not a hard cut-off for us. We still have time during the
> beta period to uplift patches for 52. But my goal is to have this patch
> ready early next week.

I see; this is useful to know.

This is off-topic for this thread, but I think we should do a meeting sometime early in January and coordinate what we need to go into the next ESR and prioritise a bit.
Flags: needinfo?(ato)
Thank you for the feedback. So I'm going to work on this now.
status-firefox52: --- → affected
status-firefox53: --- → affected
Andreas, I wonder how much time I should afford in setting up tests for this change. Would it be enough for you to simply ensure the assert method is working, or should we really test each webdriver command for the correct handling? I'm happy to do the latter.
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #13)
> Andreas, I wonder how much time I should afford in setting up tests for this
> change. Would it be enough for you to simply ensure the assert method is
> working, or should we really test each webdriver command for the correct
> handling? I'm happy to do the latter.

No, that sounds fine to me.
Flags: needinfo?(ato)
With the added test a bunch of problems started to appear when it comes to the definition of a browsing context. When seeing the current code including getCurrentWindow() from a chrome context all is fine, but once you look at this method from a content context you see inappropriate behavior:

1. getCurrentWindow() ONLY returns chrome windows in case of curFrame == null. This is fine if you only work with chrome windows but totally fails when you work with tabs. In such a case you need the content window, similar what we do with curFrame.

2. We cannot simply return curBrowser.browserForTab in case of curBrowser != null, because if chrome scope is active and this method called, it doesn't matter if the tab has been closed or not. 

3. We have different implementations for getting the window handles and current handle for chrome and content: getChromeWindowHandles vs getWindowHandles, getCurrentChromeWindowHandle vs. getCurrentWindowHandle. If we would have been nice to have only the latter, and which themselves determine the correct list based on the selected context. Changing this would be a mess (especially for backward compatibility) today, so no viable option.

The only option I might see now is to give getCurrentWindow() a parameter for the requested context. By default it's the current one, but could be overwritten to force the method to work in the other scope. Here an example:

1. Open tab
2. Switch to tab
3. Close tab

The following two options should always work independent which context is currently set:

* getWindowHandle => Calls getCurrentWindow("content"), and returns `null` (window is closed)
* getChromeWindowHandle => Calls getCurrentWindow("chrome"), and returns the chrome window handle

Maybe something similar is necessary for curFrame != null, but that I haven't yet investigated.
It looks like that bug 1311350 is blocking my work in the way that a check for a valid window for a following server command is still reporting the chrome window as open. As such we fail in the tests. I have to get bug 1311350 fixed first.
Depends on: 1311350
Whiteboard: [blocked by bug 1311350]
whimboo, you might also be blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=1311041.  We’ll see.
I don't think so given that this check works on the plain window and doesn't need the handles.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Whiteboard: [blocked by bug 1311350] → [spec][17/02]
Duplicate of this bug: 1279207
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8826689 - Flags: review?(ato)
(Assignee)

Comment 26

2 years ago
mozreview-review
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review110506

Firefox ui tests still show failures for window handles in the tabbar tests. I will have to take an extra look. Maybe with the rebase I got rid of some formerly made changes.
The handling of tabs in Firefox Puppeteer is somewhat silly. As I noticed we switch automatically to the newly opened tab if it's done in foreground. I propose that I fix the test failures for now, which are kinda obvious, and as a follow-up bug we can remove the auto-switch.
Well, I have to revert that. Looks like it actually hit an edge case when switching between windows. So it has the second tab selected, but when switching to the chrome window handle, the active tab changes to the first one. This is definitely a regression and should not happen in Marionette.
Wow, kinda bad bug here where we accidentally swap outerId and contentWindowId:
https://hg.mozilla.org/mozilla-central/annotate/tip/testing/marionette/driver.js#l1220

But that's not the only problem... continuing with investigation.
Ok, the real underlying bug is in the same code. We do not check if the target is a chrome window first, and as such set the tabIndex to 0 because for the first tab byNameOrId() returns true.

Maybe this is related to the other problem I have seen while working on bug 1124604 and I filed as bug 1335085. I will check that soon.

Comment 31

2 years ago
mozreview-review
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

::: testing/marionette/assert.js:114
(Diff revision 3)
> + *     |win| is returned unaltered.
> + *
> + * @throws {NoSuchWindowError}
> + *     If |win| has been closed.
> + */
> +assert.hasWindow = function (win, msg = "") {

What do you think about renaming this `assert.window`?

::: testing/marionette/assert.js:116
(Diff revision 3)
> + * @throws {NoSuchWindowError}
> + *     If |win| has been closed.
> + */
> +assert.hasWindow = function (win, msg = "") {
> +  msg = msg || "Unable to locate window";
> +  return assert.that(w => w && w.document.defaultView, msg, NoSuchWindowError)(win);

This assertion is wrong.  The specification says:

> If the current top-level browsing context is no longer open, return error with error code no such window.

As far as I can tell, `getCurrentWindow` returns whatever window is active in Firefox if `this.curBrowser` is not set.  What we need to do is to check if the current _WebDriver-selected window_ is still open.

::: testing/marionette/driver.js:314
(Diff revision 3)
> + * @param {Context=} forcedContext
> + *     Optional name of the context to use for the checks.
> + *     Defaults to the current context.
> + *

What does this imply?  From just reading the API documentation I don’t understand what the function is doing if given this argument.

::: testing/marionette/driver.js:320
(Diff revision 3)
> + *     Optional name of the context to use for the checks.
> + *     Defaults to the current context.
> + *
>   * @return {nsIDOMWindow}
>   */
> -GeckoDriver.prototype.getCurrentWindow = function() {
> +GeckoDriver.prototype.getCurrentWindow = function (forcedContext) {

`forcedContext = undefined`

::: testing/marionette/driver.js:322
(Diff revision 3)
> +  let win = null;
> +

Remove.

::: testing/marionette/driver.js:327
(Diff revision 3)
> +  let win = null;
> +
>    if (this.curFrame === null) {
>      if (this.curBrowser === null) {
> -      if (this.context == Context.CONTENT) {
> -        typ = "navigator:browser";
> +      let typ = (context === Context.CONTENT) ? "navigator:browser" : null;
> +      win = Services.wm.getMostRecentWindow(typ);

Return directly.

::: testing/marionette/driver.js:329
(Diff revision 3)
> -      if (this.context == Context.CONTENT) {
> -        typ = "navigator:browser";
> +      let typ = (context === Context.CONTENT) ? "navigator:browser" : null;
> +      win = Services.wm.getMostRecentWindow(typ);
> -      }
> -      return Services.wm.getMostRecentWindow(typ);
>      } else {
> -      return this.curBrowser.window;
> +      if (context == Context.CHROME){

Space before {.

::: testing/marionette/driver.js:330
(Diff revision 3)
> -        typ = "navigator:browser";
> +      win = Services.wm.getMostRecentWindow(typ);
> -      }
> -      return Services.wm.getMostRecentWindow(typ);
>      } else {
> -      return this.curBrowser.window;
> +      if (context == Context.CHROME){
> +        win = this.curBrowser.window;

Return directly.

::: testing/marionette/driver.js:333
(Diff revision 3)
>      } else {
> -      return this.curBrowser.window;
> +      if (context == Context.CHROME){
> +        win = this.curBrowser.window;
> +      } else if (context === Context.CONTENT) {
> +        if (this.curBrowser.tab && browser.getBrowserForTab(this.curBrowser.tab)) {
> +          win = this.curBrowser.window;

Return directly.

::: testing/marionette/driver.js:338
(Diff revision 3)
> +          win = this.curBrowser.window;
> +        }
> +      }
>      }
>    } else {
> -    return this.curFrame;
> +    win = this.curFrame;

Return directly.

::: testing/marionette/driver.js:340
(Diff revision 3)
> +
> +  return win;

Remove.
Attachment #8826689 - Flags: review?(ato) → review+
Depends on: 1336445
(Assignee)

Comment 32

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> What do you think about renaming this `assert.window`?

Sure. Works for me too.

> This assertion is wrong.  The specification says:
> 
> > If the current top-level browsing context is no longer open, return error with error code no such window.
> 
> As far as I can tell, `getCurrentWindow` returns whatever window is active in Firefox if `this.curBrowser` is not set.  What we need to do is to check if the current _WebDriver-selected window_ is still open.

We discussed this on IRC and I will work on splitting this method up into two distinct ones. The current method is nearly perfect but needs the fallback to `getMostRecentWindow()` removed, because of the reasons Andreas told above. this.curBrowser gets set very early during setup, so it should not be necessary here.

> What does this imply?  From just reading the API documentation I don’t understand what the function is doing if given this argument.

When you call this method in chrome scope you expect to get the window for all kinds of windows, and also if the currently selected tab doesn't exist anymore. Only in content scope we have to make sure, to also check the tabs themselves for a still valid content browser window.

> Remove.

I used this to not have to list each and every else case when the window will be null. This gives a way better workflow as a dozen of different return calls. It also eases debugging capabilities, so you only have to enter a single dump() line right before the final return statement. I would propose that we keep it here. Why are you against it?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> When you call this method in chrome scope you expect to get the window for all kinds of windows, and also if the currently selected tab doesn't exist anymore. Only in content scope we have to make sure, to also check the tabs themselves for a still valid content browser window.

This also affects close() vs closeChromeWindow() and other context specific methods.
Attachment #8826690 - Flags: review?(mjzffr)

Comment 36

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> I used this to not have to list each and every else case when the window will be null. This gives a way better workflow as a dozen of different return calls. It also eases debugging capabilities, so you only have to enter a single dump() line right before the final return statement. I would propose that we keep it here. Why are you against it?

That sounds reasonable.  I was under the impression that one branch would match and that it would eventually end up calling an API that returned null.

Comment 37

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> This also affects close() vs closeChromeWindow() and other context specific methods.

If we need different behaviour based on the context, maybe it would be clearer to rewrite this function using a switch statement like we do elsewhere, even if this means there will be some code duplication.  The inline ternaries are not great for understand what the function does.

Anyway, can you fix the API documentation?
(Assignee)

Comment 38

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> If we need different behaviour based on the context, maybe it would be clearer to rewrite this function using a switch statement like we do elsewhere, even if this means there will be some code duplication.  The inline ternaries are not great for understand what the function does.
> 
> Anyway, can you fix the API documentation?

I can rewrite it with a switch based on the forced context for sure. And this indeed makes more sense. Thanks!

Comment 39

2 years ago
mozreview-review
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review110806
Attachment #8826690 - Flags: review?(mjzffr) → review+
(Assignee)

Comment 40

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> I can rewrite it with a switch based on the forced context for sure. And this indeed makes more sense. Thanks!

I splitted this up between content and chrome and as it looks like we don't need the call to Services.wm.getMostRecentWindow() for chrome scope. We always do it for content scope. But anyway I will leave the code in for now, just in case that it is a special code path we do not hit with our unit tests.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> We discussed this on IRC and I will work on splitting this method up into two distinct ones. The current method is nearly perfect but needs the fallback to `getMostRecentWindow()` removed, because of the reasons Andreas told above. this.curBrowser gets set very early during setup, so it should not be necessary here.

This is done now. Please have a look again. Thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I missed to rebase against current central before pushing the last try build. So here an updated one which should fix the listed firefox ui tests failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=452cfa50e4d333e782c00396d616ac07b4b1eb34

Comment 49

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> This is done now. Please have a look again. Thanks.

Did you split `getCurrentWindow`?

> I splitted this up between content and chrome and as it looks like we don't need the call to Services.wm.getMostRecentWindow() for chrome scope. We always do it for content scope. But anyway I will leave the code in for now, just in case that it is a special code path we do not hit with our unit tests.

I still only find a version of this function using ternaries?

Comment 50

2 years ago
mozreview-review
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review111566

::: testing/marionette/driver.js:311
(Diff revision 6)
>  /**
> - * Gets the current active window.
> + * Get the current active window.
>   *
>   * @param {Context=} forcedContext
> - *     Optional name of the context to use for the checks.
> - *     Defaults to the current context.
> + *     Optional name of the context to use for the checks. It will be required
> + *     if a command always needs a specific context, whether which context is
> + *     currently set. Defaults to the current context.
>   *
>   * @return {nsIDOMWindow}
> + *     The currently active window.
>   */
> -GeckoDriver.prototype.getCurrentWindow = function (forcedContext = undefined) {
> +GeckoDriver.prototype.getActiveWindow = function (forcedContext = undefined) {

It’s not clear to me what a “current active window” is.  Can you be more specific?

From what I can tell, this rewritten function will only return the WebDriver-selected current top-level browsing context, and not the window/tab that is currently active?
Attachment #8826690 - Flags: review?(ato) → review+
(Assignee)

Comment 51

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> Did you split `getCurrentWindow`?

Yes, I had to do it as another commit given that I completely busted my local commit series with hg. :( I hope that this is fine.

Comment 52

2 years ago
mozreview-review-reply
Comment on attachment 8826689 [details]
Bug 1322383 - Add missing checks for valid chrome window.

https://reviewboard.mozilla.org/r/104580/#review110568

> Yes, I had to do it as another commit given that I completely busted my local commit series with hg. :( I hope that this is fine.

Nevermind, I found this in the next patch.

> I still only find a version of this function using ternaries?

Found it in the next patch.
(Assignee)

Comment 53

2 years ago
mozreview-review-reply
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review111566

> It’s not clear to me what a “current active window” is.  Can you be more specific?
> 
> From what I can tell, this rewritten function will only return the WebDriver-selected current top-level browsing context, and not the window/tab that is currently active?

This is correct. Do you have a better name for the function? Should we use getSelectedWindow(), or shall I revert the rename?

Comment 54

2 years ago
mozreview-review-reply
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review111566

> This is correct. Do you have a better name for the function? Should we use getSelectedWindow(), or shall I revert the rename?

I think the old name (`getCurrentWindow`) was better, but I’d be happy to also make this into a getter on the `GeckoDriver` object so it could be accessed as `this.win` or `this.window`.

Regardless, I would like to see the API documentation improved before we land this.  I came up with this:

> Get the session's current top-level browsing context, represented by {@code ChromeWindow}.
> 
> This will return the outer chrome window previously selected by window handle through {@code #switchToWindow}, or the first window that was registered.
(Assignee)

Comment 55

2 years ago
mozreview-review-reply
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review111566

> I think the old name (`getCurrentWindow`) was better, but I’d be happy to also make this into a getter on the `GeckoDriver` object so it could be accessed as `this.win` or `this.window`.
> 
> Regardless, I would like to see the API documentation improved before we land this.  I came up with this:
> 
> > Get the session's current top-level browsing context, represented by {@code ChromeWindow}.
> > 
> > This will return the outer chrome window previously selected by window handle through {@code #switchToWindow}, or the first window that was registered.

We cannot make it a getter because we need the extra parameter to force a specific context. So it has to remain as a method.

I will do the other changes for sure.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

Please have a look again. Your suggested changes have been implemented. Thanks.
Attachment #8826690 - Flags: review+ → review?(ato)

Comment 61

2 years ago
mozreview-review
Comment on attachment 8834333 [details]
Bug 1322383 - Update Puppeteer and firefox-ui tests for valid window checks.

https://reviewboard.mozilla.org/r/110296/#review111976
Attachment #8834333 - Flags: review?(mjzffr) → review+

Comment 62

2 years ago
mozreview-review
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review112386

::: testing/marionette/driver.js:636
(Diff revision 8)
> -      if (clickToStart && (this.appName != "B2G")) {
> -        let pService = Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +      if (clickToStart) {
> +        Services.prompt.alert(win, "", "Click to start execution of marionette tests");
> -            .getService(Ci.nsIPromptService);
> -        pService.alert(win, "", "Click to start execution of marionette tests");
>        }

Nice, although this really does belong in a separate commit.
Attachment #8826690 - Flags: review?(ato) → review+

Comment 63

2 years ago
mozreview-review-reply
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review111566

> We cannot make it a getter because we need the extra parameter to force a specific context. So it has to remain as a method.
> 
> I will do the other changes for sure.

The recent fixups look good.

I indicated before that I’m not a great fan of the `forcedContext` argument and would like to see it go away.  A future alternative is to have two handlers; one specialised for each context.  Or to check `this.context` internally to determine which context to force, if that is even possible.

Anyway, I’m happy for this patch to land in its current state.
(Assignee)

Comment 64

2 years ago
mozreview-review-reply
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review111566

> The recent fixups look good.
> 
> I indicated before that I’m not a great fan of the `forcedContext` argument and would like to see it go away.  A future alternative is to have two handlers; one specialised for each context.  Or to check `this.context` internally to determine which context to force, if that is even possible.
> 
> Anyway, I’m happy for this patch to land in its current state.

This won't work given that methods like close(), closeChromeWindow(), getWindowHandles(), and getChromeWindowHandles have to work in both contexts, but always need the list of window handles returned in their own required context. This is different to when you only work on content context with commands outside of chrome.
(Assignee)

Comment 65

2 years ago
mozreview-review
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review112400

::: testing/marionette/driver.js:636
(Diff revision 8)
> -      if (clickToStart && (this.appName != "B2G")) {
> -        let pService = Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +      if (clickToStart) {
> +        Services.prompt.alert(win, "", "Click to start execution of marionette tests");
> -            .getService(Ci.nsIPromptService);
> -        pService.alert(win, "", "Click to start execution of marionette tests");
>        }

I will obey that for the future.

Comment 66

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/deb5b08545fd
Add missing checks for valid window r=ato
https://hg.mozilla.org/integration/autoland/rev/f5e6da82e68d
getCurrentWindow() has to only return the currently selected window. r=ato,maja_zf
https://hg.mozilla.org/integration/autoland/rev/be39185b68b0
Update Puppeteer and firefox-ui tests for valid window checks. r=maja_zf
Backed out for frequently failing to find windows in wpt tests on Windows:

https://hg.mozilla.org/integration/autoland/rev/7617c2a06e181b42ec99a6c7b7ddfa7d2d7c45e5
https://hg.mozilla.org/integration/autoland/rev/614014c33b90e105b0ded75a26d5e45ffb74bb7b
https://hg.mozilla.org/integration/autoland/rev/aa6970ec8561cbd309406d887ac1446f68427811

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=be39185b68b08eb5037fc592ba99f769dfb67089
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=75963277&repo=autoland

07:35:09     INFO - TEST-START | /html/browsers/history/the-location-interface/location-protocol-setter.html
07:35:09     INFO - TEST-UNEXPECTED-ERROR | /html/browsers/history/the-location-interface/location-protocol-setter.html | Unable to locate window
07:35:09     INFO - stacktrace:
07:35:09     INFO - 	this.WebDriverError@chrome://marionette/content/error.js:214:17
07:35:09     INFO - 	this.NoSuchWindowError@chrome://marionette/content/error.js:351:3
07:35:09     INFO - 	assert.that/<@chrome://marionette/content/assert.js:318:13
07:35:09     INFO - 	assert.window@chrome://marionette/content/assert.js:136:10
07:35:09     INFO - 	GeckoDriver.prototype.close@chrome://marionette/content/driver.js:2185:3
07:35:09     INFO - 	Dispatcher.prototype.execute/req<@chrome://marionette/content/dispatcher.js:128:20
07:35:09     INFO - 	TaskImpl_run@resource://gre/modules/Task.jsm:319:42
07:35:09     INFO - 	TaskImpl@resource://gre/modules/Task.jsm:277:3
07:35:09     INFO - 	asyncFunction@resource://gre/modules/Task.jsm:252:14
07:35:09     INFO - 	Task_spawn@resource://gre/modules/Task.jsm:166:12
07:35:09     INFO - 	Dispatcher.prototype.execute@chrome://marionette/content/dispatcher.js:118:13
07:35:09     INFO - 	Dispatcher.prototype.onPacket@chrome://marionette/content/dispatcher.js:89:5
07:35:09     INFO - 	_onJSONObjectReady/<@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:483:11
07:35:09     INFO - 	exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
07:35:09     INFO - 	exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Flags: needinfo?(hskupin)
After some trying I can reproduce this locally now with the following command and my patch applied:

./mach wpt /html/browsers/history/the-location-interface/location-protocol-setter-non-broken-weird.html /html/browsers/history/the-location-interface/location-protocol-setter.html --no-pause

I will have to check what's going on, but I believe something in WPT side closes a window when it shouldn't do it. So the next test cannot find a proper window to execute on.

The problematic test might be location-protocol-setter-non-broken-weird.html here.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Hm, I cannot reproduce this problem with WPT anymore since the latest rebase. Maybe there was another issue with those tests which got fixed over the weekend. I will do another try build for just the webplatform-tests.
Depends on: 1339347
The Windows slave loaner didn't help me at all. I ran tests all day long and I haven't seen this particular failure neither with mach nor mozharness.
The only indication so far is that we hit this problem with webplatform tests when some code in wptrunner tries to close a tab when it doesn't exist anymore:

> 07:52:40     INFO - 	this.WebDriverError@chrome://marionette/content/error.js:214:17
> 07:52:40     INFO - 	this.NoSuchWindowError@chrome://marionette/content/error.js:351:3
> 07:52:40     INFO - 	assert.that/<@chrome://marionette/content/assert.js:318:13
> 07:52:40     INFO - 	assert.window@chrome://marionette/content/assert.js:136:10
> 07:52:40     INFO - 	GeckoDriver.prototype.close@chrome://marionette/content/driver.js:2185:3

Sadly there is no stacktrace available for the Python code, which I find kinda limiting. Is there any reason for this James?

Can I assume that this is one of the calls to close() in this file only?

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/harness/wptrunner/executors/executormarionette.py

If yes, then the only line of code which is using self.marionette.close() is:

https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/testing/web-platform/harness/wptrunner/executors/executormarionette.py#168

Something strange I find the following line:

https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/testing/web-platform/harness/wptrunner/executors/executormarionette.py#430

What kind of window are we going to close here? Why can't this be done via self.marionette.close()?

I feel that we have a race condition here and we might be able to easily fix it by ignoring the NoSuchWindow exception in close_old_windows().
Flags: needinfo?(james)
Something interesting is going on here. I put a couple of debug statements in executormarionette.py and got the following details:

> 11:29:08 CRITICAL - ** close old windows *****

This is getting called at the beginning of MarionetteTestharnessExecutor::do_testharness() when self.close_after_done is set to True.

> 11:29:08 CRITICAL - *** window handles: [u'1', u'4294967297']
> 11:29:08 CRITICAL - *** chrome window handles: [u'1', u'5']

Here we have an overlap of a window handle with the outerID == 1. It is listed as a tab and as a window at the same time. So far there is only one known window which gets opened as very first window on Windows, which is the graphics test window. But that is getting closed immediately. See bug 1315611 for details.

> 11:29:08 CRITICAL - *** last runner handle: 4294967297
> 11:29:08 CRITICAL - *** new runner handle: 4294967297
> 11:29:08 CRITICAL - *** remaining handles: [u'1']
> 11:29:08 CRITICAL - *** about to close handle: 1
> 11:29:08 CRITICAL - *** window handles: [u'1', u'4294967297']
> 11:29:08 CRITICAL - *** chrome window handles: [u'1', u'5']
> 11:29:08 CRITICAL - **** exception by closing window: Unable to locate window

So we indeed want to close the window with the id 1. And I'm fairly sure that the above described situation is the problem here. We are simply running into a race condition by assuming that winID=1 is the browser window to work with.

I see two solutions here:

1) Getting bug 1315611 fixed first so that Marionette waits for the first real browser window and doesn't simply pick the very first opened chrome window.

2) Ensure that wpt-runner works with a browser window. Something similar to what we actually do for Firefox UI tests.

I'm more in favor of option 1, which is the clean way even option 2 is quicker.
Depends on: 1315611
Yes, option 1 seems like the only reasonable fix here.
Flags: needinfo?(james)
I pushed a try build with a patch for the above issue and I cannot see the problem anymore. But sadly there is another situation now with the unknown content type dialog, which seem to stay open for WPT chunks 1 and 4. I'm fairly sure this is a problem with the Marionette executor of those test jobs not handling it correctly. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62c6bdd7e0167ddb24babe4f0d0f259ac7885b9d&selectedJob=80499422
Blocks: 1315611
No longer depends on: 1315611
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Henrik Skupin (:whimboo) from comment #77)
> problem anymore. But sadly there is another situation now with the unknown
> content type dialog, which seem to stay open for WPT chunks 1 and 4. I'm
> fairly sure this is a problem with the Marionette executor of those test
> jobs not handling it correctly. 

I can reproduce this locally on my OS X machine with the following commands:

mach wpt --no-pause /html/semantics/embedded-content/the-area-element/area-download-click.html /html/semantics/embedded-content/the-area-element/area-processing.html

mach wpt --no-pause /html/semantics/text-level-semantics/the-a-element/a-download-click.html /html/semantics/text-level-semantics/the-a-element/a-stringifier.html

I haven't seen this failure before so I assume it came in with the recent sync of web-platform-tests?
Depends on: 1343164
Blocks: 1343164
No longer depends on: 1343164
As it turned out on bug 1343164 this would actually be a regression caused by my changes for `getCurrentWindow()`. The current patch only returns a chrome window in content scope when a tabbrowser is available. Given that we create browser instances of all type of chrome windows we also have to return the chrome window in those cases. I will get this fixed and some more tests added.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

Andreas, with the latest changes I would like to see another review for this commit. Thanks.
Attachment #8826690 - Flags: review?(ato)

Comment 92

2 years ago
mozreview-review
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review117360

Comment 93

2 years ago
mozreview-review
Comment on attachment 8841896 [details]
Bug 1322383 - Ignore GFX sanity check window during startup of Firefox.

https://reviewboard.mozilla.org/r/115960/#review117364

::: testing/marionette/driver.js:662
(Diff revision 4)
> +      // Bug 1315611 - On Windows the GFX sanity test window might be open. Marionette
> +      // has to wait for it getting closed before starting the session. So poll wait for
> +      // that happening.
> +      checkTimer.initWithCallback(waitForWindow.bind(this), 100,
> +          Ci.nsITimer.TYPE_ONE_SHOT);

This is the fix for bug 1315611, so no need to reference the bug.

Can you also rephrase the comment?  I would suggest:

> When Firefox starts on Windows, an additional GFX sanity test window may appear off-screen.  Marionette should wait for it to close.

Thirdly, this check is made when a new session is requested by the client.  It should probably be made when Marionette initialises, before we start accepting new connections.
Attachment #8841896 - Flags: review?(ato) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 97

2 years ago
mozreview-review-reply
Comment on attachment 8841896 [details]
Bug 1322383 - Ignore GFX sanity check window during startup of Firefox.

https://reviewboard.mozilla.org/r/115960/#review117364

> This is the fix for bug 1315611, so no need to reference the bug.
> 
> Can you also rephrase the comment?  I would suggest:
> 
> > When Firefox starts on Windows, an additional GFX sanity test window may appear off-screen.  Marionette should wait for it to close.
> 
> Thirdly, this check is made when a new session is requested by the client.  It should probably be made when Marionette initialises, before we start accepting new connections.

> This is the fix for bug 1315611, so no need to reference the bug.

Not sure what you mean. If that refers to the bug # in the commit message, I cannot use bug 1315611 there because a push will be rejected. If we really want this landed with the correct bug id, I would have to extract the commit and push it as part of the other bug. If you want that please let me know.

> Thirdly, this check is made when a new session is requested by the client.  It should probably be made when Marionette initialises, before we start accepting new connections.

What would be your favorite location in the code?
(Assignee)

Comment 98

2 years ago
mozreview-review-reply
Comment on attachment 8826690 [details]
Bug 1322383 - getCurrentWindow() has to only return the currently selected window.

https://reviewboard.mozilla.org/r/104582/#review117360

Ups, I actually missed to add the new HTML test page for blob downloads, which caused permfailures for Mn jobs. The most recent push for this commit fixed it.

Comment 99

2 years ago
mozreview-review-reply
Comment on attachment 8841896 [details]
Bug 1322383 - Ignore GFX sanity check window during startup of Firefox.

https://reviewboard.mozilla.org/r/115960/#review117364

> > This is the fix for bug 1315611, so no need to reference the bug.
> 
> Not sure what you mean. If that refers to the bug # in the commit message, I cannot use bug 1315611 there because a push will be rejected. If we really want this landed with the correct bug id, I would have to extract the commit and push it as part of the other bug. If you want that please let me know.
> 
> > Thirdly, this check is made when a new session is requested by the client.  It should probably be made when Marionette initialises, before we start accepting new connections.
> 
> What would be your favorite location in the code?

> Not sure what you mean. If that refers to the bug # in the commit message, I cannot use bug 1315611 there because a push will be rejected. If we really want this landed with the correct bug id, I would have to extract the commit and push it as part of the other bug. If you want that please let me know.

I don’t think every comment in the source code should be linked to a bug detailing why we made a change.  Links to bugs are OK for known issues we need to address in the future, but not so useful if they provide additional context to what the comment explains.  The source code should be authorative source, so if we feel the comment doesn’t properly surmise all the details, we should expand it.

> What would be your favorite location in the code?

I don’t know, possibly in [`MarionetteServer#start`](http://searchfox.org/mozilla-central/source/testing/marionette/server.js#87)?
(Assignee)

Comment 100

2 years ago
mozreview-review-reply
Comment on attachment 8841896 [details]
Bug 1322383 - Ignore GFX sanity check window during startup of Firefox.

https://reviewboard.mozilla.org/r/115960/#review117364

> > Not sure what you mean. If that refers to the bug # in the commit message, I cannot use bug 1315611 there because a push will be rejected. If we really want this landed with the correct bug id, I would have to extract the commit and push it as part of the other bug. If you want that please let me know.
> 
> I don’t think every comment in the source code should be linked to a bug detailing why we made a change.  Links to bugs are OK for known issues we need to address in the future, but not so useful if they provide additional context to what the comment explains.  The source code should be authorative source, so if we feel the comment doesn’t properly surmise all the details, we should expand it.
> 
> > What would be your favorite location in the code?
> 
> I don’t know, possibly in [`MarionetteServer#start`](http://searchfox.org/mozilla-central/source/testing/marionette/server.js#87)?

Ok, I can clearly remove this bug # from the comment. No problem at all. And as I read you are fine with including the patch into this commit series.

> I don’t know, possibly in MarionetteServer#start?

I don't think this is the right place because it only contains socket related code. An option would be marionette.js, but all the initialization code is run on `final-ui-startup` which means "Triggered just before the first window for the application is displayed". So we would miss the opening of the GFX sanity window, which will happen at a later time.

An option might be to wait for the `sessionstore-windows-restored` observer notification, which is sent when all browser windows have been opened. We could then run the GFX window code and allow Marionette to initialize itself once the window is not open anymore. Doing that we would also have a ride-along patch for bug 1300709.
(Assignee)

Comment 101

2 years ago
mozreview-review-reply
Comment on attachment 8841896 [details]
Bug 1322383 - Ignore GFX sanity check window during startup of Firefox.

https://reviewboard.mozilla.org/r/115960/#review117364

> Ok, I can clearly remove this bug # from the comment. No problem at all. And as I read you are fine with including the patch into this commit series.
> 
> > I don’t know, possibly in MarionetteServer#start?
> 
> I don't think this is the right place because it only contains socket related code. An option would be marionette.js, but all the initialization code is run on `final-ui-startup` which means "Triggered just before the first window for the application is displayed". So we would miss the opening of the GFX sanity window, which will happen at a later time.
> 
> An option might be to wait for the `sessionstore-windows-restored` observer notification, which is sent when all browser windows have been opened. We could then run the GFX window code and allow Marionette to initialize itself once the window is not open anymore. Doing that we would also have a ride-along patch for bug 1300709.

I can verify that this works. Here an excerpt from the observer notifications caught and the existence of a GFX window:

* topic 'xul-window-visible' - gfx window: 
* topic 'xul-window-visible' - gfx window: [object ChromeWindow]
* topic 'xul-window-destroyed' - gfx window: null
* topic 'xul-window-visible' - gfx window: null
* topic 'sessionstore-windows-restored' - gfx window: null

We would still have to look for a GFX window with using the `sessionstore-windows-restored` observer notification because it could still be closed after the browser window is ready.
(Assignee)

Comment 102

2 years ago
mozreview-review-reply
Comment on attachment 8841896 [details]
Bug 1322383 - Ignore GFX sanity check window during startup of Firefox.

https://reviewboard.mozilla.org/r/115960/#review117364

> I can verify that this works. Here an excerpt from the observer notifications caught and the existence of a GFX window:
> 
> * topic 'xul-window-visible' - gfx window: 
> * topic 'xul-window-visible' - gfx window: [object ChromeWindow]
> * topic 'xul-window-destroyed' - gfx window: null
> * topic 'xul-window-visible' - gfx window: null
> * topic 'sessionstore-windows-restored' - gfx window: null
> 
> We would still have to look for a GFX window with using the `sessionstore-windows-restored` observer notification because it could still be closed after the browser window is ready.

Actually this should have been:

> topic 'final-ui-startup' - gfx window:
> topic 'xul-window-visible' - gfx window: [object ChromeWindow]
> topic 'xul-window-destroyed' - gfx window: null
> topic 'xul-window-visible' - gfx window: null
> topic 'sessionstore-windows-restored' - gfx window: null

Comment 103

2 years ago
mozreview-review-reply
Comment on attachment 8841896 [details]
Bug 1322383 - Ignore GFX sanity check window during startup of Firefox.

https://reviewboard.mozilla.org/r/115960/#review117364

> Actually this should have been:
> 
> > topic 'final-ui-startup' - gfx window:
> > topic 'xul-window-visible' - gfx window: [object ChromeWindow]
> > topic 'xul-window-destroyed' - gfx window: null
> > topic 'xul-window-visible' - gfx window: null
> > topic 'sessionstore-windows-restored' - gfx window: null

That sounds good to me.
Attachment #8841896 - Flags: review?(ato) → review-
Comment hidden (mozreview-request)

Comment 105

2 years ago
mozreview-review
Comment on attachment 8841896 [details]
Bug 1322383 - Ignore GFX sanity check window during startup of Firefox.

https://reviewboard.mozilla.org/r/115960/#review117466

::: testing/marionette/components/marionette.js:169
(Diff revision 6)
> +      if (this.gfxWindow) {
> +        this.observerService.addObserver(this, "domwindowclosed", false);
> +      } else {
> +        this.observerService.addObserver(this, "xpcom-shutdown", false);
> +        this.finalUiStartup = true;
> +        this.init();
> +      }

This is clever.  Thanks for making this change.
Attachment #8841896 - Flags: review?(ato) → review+

Comment 106

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74b0c928a23e
Add missing checks for valid window r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/890130185456
getCurrentWindow() has to only return the currently selected window. r=ato+446296,maja_zf
https://hg.mozilla.org/integration/autoland/rev/20f6edaec61e
Update Puppeteer and firefox-ui tests for valid window checks. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/d080b72ec241
Ignore GFX sanity check window during startup of Firefox. r=ato+446296
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=80814605&repo=autoland
Flags: needinfo?(hskupin)

Comment 108

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af67c447c17b
Backed out changeset d080b72ec241 
https://hg.mozilla.org/integration/autoland/rev/b4ce1f38c9f2
Backed out changeset 20f6edaec61e 
https://hg.mozilla.org/integration/autoland/rev/c4bbcb3d7737
Backed out changeset 890130185456 
https://hg.mozilla.org/integration/autoland/rev/b43926a87603
Backed out changeset 74b0c928a23e for frequent failure rate in marionette tests
The problem we are facing here is related to bug 1331198, which I'm investigating now. Due to the change from `final-ui-startup` to `sessionstore-windows-restored` we seem to add a couple of more seconds which makes this intermittent failure occurring more often. There seems to be something wrong with the Linux TC workers within the first minutes, which is causing a very slow Firefox startup, so that we exceed the 120s startup timeout.
Flags: needinfo?(hskupin)
When talking with Andreas on Vidyo today I noticed that the patch is not complete yet. As I have seen when working on the back/forward patch we do not correctly check for valid windows in listener.js when context has been switched to a frame. In those cases we still throw strange failure messages.

Given that the timeout during start of Firefox will be fixed soon via bug 1345105, I will move out the GFX patch to bug 1315611. It would be great to get this landed sooner than later, also because it basically hasn't something to do with this bug.
No longer blocks: 1300709
No longer blocks: 1315611
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8841896 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Henrik Skupin (:whimboo) from comment #110)
> When talking with Andreas on Vidyo today I noticed that the patch is not
> complete yet. As I have seen when working on the back/forward patch we do
> not correctly check for valid windows in listener.js when context has been
> switched to a frame. In those cases we still throw strange failure messages.

I would like to keep this bug for chrome only and file a follow-up for necessary work on the listener.js side. This would most likely only affect handling frames, so any work for this should not block landing this overall change.

> Given that the timeout during start of Firefox will be fixed soon via bug
> 1345105, I will move out the GFX patch to bug 1315611. It would be great to
> get this landed sooner than later, also because it basically hasn't
> something to do with this bug.

The updated patch series is without the gfx patch now.
Summary: Chrome context methods (driver.js) miss checks for valid window - no NoSuchWindow failure thrown → Chrome context methods (driver.js) miss checks for valid chrome window - no NoSuchWindow failure thrown
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Now we have a different test failure for wpt webdriver tests here. Specifically for navigate.py where we fail in cleanup() to close left-over windows:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=abe3ee655050&selectedJob=85154702

Failing test is 'test_get_current_url_nested_browsing_context', and the `Unable to locate window` failure is thrown for:

current_window = session.window_handle


Those tests are new, so I think we have a maybe incorrect behaving test here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8849513 - Flags: review?(james)

Comment 125

2 years ago
mozreview-review
Comment on attachment 8849513 [details]
Bug 1322383 - Ensure that finalizers for webdriver tests always work on a valid window.

https://reviewboard.mozilla.org/r/122308/#review124410
Attachment #8849513 - Flags: review?(james) → review+

Comment 126

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f04e497a574
Ensure that finalizers always work on a valid window. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/2da5d0c57ecd
Add missing checks for valid chrome window. r=ato
https://hg.mozilla.org/integration/autoland/rev/e010bd839cf7
getCurrentWindow() has to only return the currently selected window. r=ato,maja_zf
https://hg.mozilla.org/integration/autoland/rev/a892984bb20c
Update Puppeteer and firefox-ui tests for valid window checks. r=maja_zf
Backed out for frequently failing marionette test test_screenshot.py TestScreenCaptureChrome.test_capture_full_area on Linux x64 opt with e10s disabled:

https://hg.mozilla.org/integration/autoland/rev/72afbe24d43e7dd433f45ba372f1992047b49017
https://hg.mozilla.org/integration/autoland/rev/061a193c62079fdd15c4af428b34a36ee12308b5
https://hg.mozilla.org/integration/autoland/rev/5e1afe4b3d87b7ea2a0e8c1ea5a018f1e1b95c0c
https://hg.mozilla.org/integration/autoland/rev/ca006aa6abf0200ede96ec9abe146e9f06a6e6ed

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a892984bb20cb7ed4db09f497b22959be8f39d30&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=85393729&repo=autoland

TEST-UNEXPECTED-FAIL | test_screenshot.py TestScreenCaptureChrome.test_capture_full_area | AssertionError: u'iVBORw0KGgoAAAANSUhEUgAABQAAAAQQCAYAAAC9RfbYAAAgAElEQVR4nOzd/XfU9Z3/f/+O7x/w/Z [truncated]... != u'iVBORw0KGgoAAAANSUhEUgAABQAAAAQQCAYAAAC9RfbYAAAgAElEQVR4nOzd/XfU9Z3/f/+O7x/w/Z [truncated]...

One screenshot shows the chrome window, the other only the content.
Flags: needinfo?(hskupin)
Depends on: 1349305
Failing tests while my patch was on autoland:

https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=mn%20linux64%20opt&fromchange=a892984bb20c&tochange=72afbe24d43e

I will retrigger some of them to see if this is a permanent failure. If yes I will most likely have to use a one click loaner for investigation because I cannot see it locally.
Flags: needinfo?(hskupin)
Test failure happens in this line:

https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py#198

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #127)
> One screenshot shows the chrome window, the other only the content.

This is not true. Both screenshots are covering the full chrome part of Firefox. Visually inspecting them doesn't show any difference. So maybe some subtle pixel differences are causing this failure. I will have to find a tool, to visualize those.
Created attachment 8850029 [details]
screenshot_diff.png

This image shows a diff between both taken screenshots. As you can see the subtle difference is in the lower left of the window, which is actually used to display the page load status.

Checking one of the screenshots in detail now, I clearly see the URL to `127.0.0.1:43217/empty.html` displayed. It's close to be faded out. I have no idea why this got triggered with my changes in this patch series.
Ok, so a test as run before this failing screenshot test loaded this locally hosted HTML page, and clicked one of the links. As such the link was highlighted and it's URL was shown in the lower left corner of Firefox for a couple of seconds. Given that the tests are running that fast I assume that when we started this screenshot test the link was about to fade out. But we still got it into the first screenshot.

And here I blame my newly added test file test_window_status_content.py, which is getting run before the screenshot tests. To fix this I pushed a try build now where we always load `about:blank` in setUp for the screenshot tests. With that we should be able to remove any remaining traces from formerly running tests, which could interfere in taking screenshots.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8850074 - Flags: review?(dburns)

Comment 138

2 years ago
mozreview-review
Comment on attachment 8850074 [details]
Bug 1322383 - Hardening screenshot tests by forcing about:blank to be loaded by default.

https://reviewboard.mozilla.org/r/122790/#review124988
Attachment #8850074 - Flags: review?(dburns) → review+

Comment 139

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ef6b1153784
Hardening screenshot tests by forcing about:blank to be loaded by default. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/30793b4a8c2b
Ensure that finalizers for webdriver tests always work on a valid window. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/ff17e8bc0756
Add missing checks for valid chrome window. r=ato
https://hg.mozilla.org/integration/autoland/rev/9588802b1409
getCurrentWindow() has to only return the currently selected window. r=ato,maja_zf
https://hg.mozilla.org/integration/autoland/rev/edf016cb9c55
Update Puppeteer and firefox-ui tests for valid window checks. r=maja_zf
Depends on: 1315611
Whiteboard: [spec][17/02] → [spec][17/02][needs bug 1315611 uplifted before beta uplift]
https://superuser.com/questions/431268/firefox-show-link-urls-in-status-bar has some more information on the ‘status bar’, as it is called.  Perhaps there is a pref we can flip to disable it when running Marionette tests?
It's not really a status bar. It was it before we had the add-on bar, which also get removed. It's just something like a panel right now. 

I don't think we should disable it. In case of failures it gives some helpful information in screenshots. In other Marionette tests we wouldn't have a situation as what we had here.
Summary: Chrome context methods (driver.js) miss checks for valid chrome window - no NoSuchWindow failure thrown → Commands in driver.js miss checks for valid ChromeWindow (DOMwindow) - no NoSuchWindow failure thrown
I just filed bug 1349861 for the remaining checks in listener.js.
Please uplift this test-only patch to aurora for now. Uplifts to beta are still blocked.
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Whiteboard: [spec][17/02][needs bug 1315611 uplifted before beta uplift] → [spec][17/02][needs bug 1315611 uplifted before beta uplift][checkin-needed-aurora]

Comment 145

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/1156fbdde40c
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d5124bd2bcb
https://hg.mozilla.org/releases/mozilla-aurora/rev/c64597242ffd
https://hg.mozilla.org/releases/mozilla-aurora/rev/041eba73027e
https://hg.mozilla.org/releases/mozilla-aurora/rev/23c1e33165b4
status-firefox54: affected → fixed
Whiteboard: [spec][17/02][needs bug 1315611 uplifted before beta uplift][checkin-needed-aurora] → [spec][17/02][needs bug 1315611 uplifted before beta uplift]
Depends on: 1351335
We clearly need the patch on bug 1351335 uplifted first too.
Whiteboard: [spec][17/02][needs bug 1315611 uplifted before beta uplift] → [spec][17/02][needs bug 1315611 and bug 1351335 uplifted first]
Flags: needinfo?(ato)
Whiteboard: [spec][17/02][needs bug 1315611 and bug 1351335 uplifted first] → [spec][17/02][needs bug 1315611 and bug 1351335 uplifted first][checkin-needed-beta][checkin-needed-esr52]
For the uplift to beta and esr52 we do not need https://hg.mozilla.org/releases/mozilla-central/rev/30793b4a8c2b, which patches a feature which is not available on beta.

I pushed a build with all the patches applied to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f485dca86ea901b660a1f2728d74aae493314fbe
Flags: needinfo?(ato)
Linux and Windows (7 VM) tests are looking fine. For Windows 8 we seem to have a backlog, so not sure if you want to wait for those results to appear, Ryan.  If not, I think we can get this all uplifted. Thanks.
Flags: needinfo?(ryanvm)

Comment 149

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c2a69c08a669
https://hg.mozilla.org/releases/mozilla-beta/rev/375ed9cda59f
https://hg.mozilla.org/releases/mozilla-beta/rev/740e8aeb2e22
https://hg.mozilla.org/releases/mozilla-beta/rev/127a8c319327

This has conflicts on esr52. If you want to sort out the dependencies and do another Try push, that worked well for the Beta uplifts.
status-firefox53: affected → fixed
Whiteboard: [spec][17/02][needs bug 1315611 and bug 1351335 uplifted first][checkin-needed-beta][checkin-needed-esr52] → [spec][17/02]
I will have a look if an uplift to esr52 is easily doable.
Flags: needinfo?(ryanvm)
To be able to uplift this patch series we would need the patches on bug 1319237, bug 1333014, and bug 1150527 uplifted first. The latter two apply cleanly but for bug 1319237 Andreas has to take a look.
Today we decided to no longer uplift feature additions for Marionette to esr52. Only fixes for regressions or other critical issues will be uplifted.
status-firefox-esr52: affected → wontfix
status-firefox52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.