Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Description • 6 years ago

With the changes on bug 1430109 we lost the window handle in the failure message for `assert.open()` as it can be seen on bug 1383686:

Before: NoSuchWindowException: Unable to locate window: 2147483684
After:  NoSuchWindowException: Browsing context has been discarded

For debugging purposes I would request to get this handle back.

Andreas, can you please have a look?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 1 • 6 years ago

This makes sense.  I will take care of it.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 6 years ago

It is worth noting that I can’t see that https://bugzil.la/1430109
actually regressed anything.  The “Unable to locate window”
string is used in [1] and the patch [2] didn’t actually remove any
in-string variables.

In any case, I’m happy to fix this, because it is a good idea!
In any case, the patch is going to be slightly less pretty that it
could have been if https://bugzil.la/marionette-window-tracking
would’ve been in place.

  [1] https://searchfox.org/mozilla-central/source/testing/marionette/driver.js#1516
  [2] https://hg.mozilla.org/mozilla-central/rev/b67c5b1e033b

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 4 • 6 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #3)
> It is worth noting that I can’t see that https://bugzil.la/1430109
> actually regressed anything.  The “Unable to locate window”
> string is used in [1] and the patch [2] didn’t actually remove any
> in-string variables.

Please note that this instance is not related to `switchToWindow` because this method isn't doing any kind of assertion, but `close` [1], which now has the changed assertion in place. And this regression happened lately. I don't see anything else which could have caused it.

[1] https://searchfox.org/mozilla-central/source/testing/marionette/driver.js#2702

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8943326 [details]
Bug 1431151 - Include outerWindowID in error message for assert.open.

https://reviewboard.mozilla.org/r/213650/#review219440

::: commit-message-4e429:8
(Diff revision 1)
> +When given a browser.Context assert.open will include its curFrameId
> +property, which is effectively the outerWindowID, when the assertion
> +fails.
> +
> +Because GeckoDriver#getCurrentWindow() always returns null we can't
> +look up the ChromeWindow outerWindowID.

I don't understand this sentence. Why does this method always return null for ChromeWindows? That is not true, because otherwise the assert would always fail. We clearly need to check for the id.

::: testing/marionette/assert.js:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -const {utils: Cu} = Components;
> +const {interfaces: Ci, utils: Cu} = Components;

This is never used. Please remove.

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

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

> (In reply to Andreas Tolfsen ‹:ato› from comment #3)
> 
> > It is worth noting that I can’t see that
> > https://bugzil.la/1430109 actually regressed anything.  The
> > “Unable to locate window” string is used in [1] and the patch
> > [2] didn’t actually remove any in-string variables.
> 
> Please note that this instance is not related to `switchToWindow`
> because this method isn't doing any kind of assertion, but `close`
> [1], which now has the changed assertion in place. And this
> regression happened lately. I don't see anything else which could
> have caused it.

So what is it related to?

The only reference I could find of “Unable to switch to window
<window ID>” is in GeckoDriver#switchToWindow.  As you can see from
[1], assert.window originally had “Unable to switch to window” as
its default message, which means it either got overriden by the
caller, which #close does not, or the message you refer to actually
comes from #switchToWindow.

Can you tell me where “Unable to locate window: 2147483684”
originates from, if not from switchToWindow?

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

(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> The only reference I could find of “Unable to switch to window
> <window ID>” is in GeckoDriver#switchToWindow.  As you can see from

You are mixing up different errors here. As mentioned earlier, this error is not raised by switchToWindow, because this method is not doing any assert checks. But the failure is "Unable to locate window", which was coming from `assert.window`, which you migrated now to `assert.open`. 

> [1], assert.window originally had “Unable to switch to window” as
> its default message, which means it either got overriden by the

No it didn't had. See https://hg.mozilla.org/mozilla-central/rev/b67c5b1e033b#l1.62

> Can you tell me where “Unable to locate window: 2147483684”
> originates from, if not from switchToWindow?

Check the traceback in the log for that failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=156788992&repo=autoland&lineNumber=4543

Btw. this is the reason why I want to keep the traceback even for WebDriverError types for debug and trace logs.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 8 • 6 years ago

So I mixed up “Unable to switch to window” with “Unable to
locate window” in the first two mentions of it in my previous
comment.

GeckoDriver#close calls assert.open, which prior to bug 1430109
called assert.window [1]. assert.window had a default message
of “Unable to locate window” [2].  The only reference to
the string “Unable to locate window: <outerWindowID>” is in
GeckoDriver#switchToWindow [3], hence why I brought it up.

In other words, I can’t find a reference to
assert.window/assert.contentBrowser ever having included the
outerWindowID.

  [1] https://hg.mozilla.org/mozilla-central/rev/b67c5b1e033b#l3.800
  [2] https://hg.mozilla.org/mozilla-central/rev/b67c5b1e033b#l1.62
  [3] https://searchfox.org/mozilla-central/source/testing/marionette/driver.js#1516

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 9 • 6 years ago

So I had another look at the patch and also applied it locally. I think we both got a bit confused here. When checking a couple of the latest test failures I have seen that we actually fail at two different places. In `switchToWindow` as you were thinking AND `close`. With that you are correct, that the handle was only shown formerly for the former but not the latter. So that change for content context is fine.

Also regarding the other issue regarding ChromeWindow I better understand the situation, and yes in cases of no window present which raises the exception null will always be passed in. AFAIR your window tracking patch will wrap also ChromeWindows, right? If yes, we should be able to tell the handle when not passing in real window objects into this assert function but the wrappers. For now it's indeed not possible.

Also what I have seen when glancing over the new method, the `closed` getter in browser.js looks wrong to me because `this.contentBrowser === null` does not mean that the tab is closed. It simply doesn't have an active browser yet. That's one of the issues we have and is logged as bug 1363368. Maybe this is fine for now until we can tackle this bug.

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

Comment on attachment 8943326 [details]
Bug 1431151 - Include outerWindowID in error message for assert.open.

https://reviewboard.mozilla.org/r/213650/#review219440

> I don't understand this sentence. Why does this method always return null for ChromeWindows? That is not true, because otherwise the assert would always fail. We clearly need to check for the id.

As mentioned in my last comment we might be able to do so in the future when we could pass in the ChromeWindow wrapper, if that is what we get with your window tracking refactoring patch.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 11 • 6 years ago

Thanks for checking out the patch and verifying all of this locally.

ChromeWindows will indeed have a ‘browsing context’ abstraction
just like <xul:browser>s.  They will share the same interface and
both have a ‘closed’ attribute that we can use in assert.open.
That is partly why I want to make this change to begin with, so that
we can share the same assertion for both.  The code in this patch
speficially works around the API differences between ChromeWindow
and browser.Context for the time being, but you are correct that
with the window tracking refactoring the workaround will go away.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 12 • 6 years ago

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

> Also what I have seen when glancing over the new method,
> the `closed` getter in browser.js looks wrong to me because
> `this.contentBrowser === null` does not mean that the tab is
> closed. It simply doesn't have an active browser yet. That's one
> of the issues we have and is logged as bug 1363368. Maybe this is
> fine for now until we can tackle this bug.

The contentBrowser getter [1] also checks this.tab, which if I read
this code correctly, does check that the tab hasn’t been closed.
If this.tab is null, this.contentBrowser returns null and the code
in this.closed will return true.

I can add an additional if (this.tab) condition in this.closed to
make it clearer if you wish?

  [1] https://searchfox.org/mozilla-central/rev/e9a067a401e41017766f4ab90bc3906603273d51/testing/marionette/browser.js#157-170

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

this.tab is basically mood in current code given that it doesn't check the real underlying tab but the cashed value only. This gives all sort of problems as we know nowadays. As long as your window refactoring patch hasn't landed, we cannot improve anything here.

(In reply to Andreas Tolfsen ‹:ato› from comment #12)
> I can add an additional if (this.tab) condition in this.closed to
> make it clearer if you wish?

We may not wanna do that now, given that would change the logic a bit for those tabs which are still open but don't have a content browser yet (see bug 1363368, bug 1373191).

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 16 • 6 years ago
mozreview-review

Comment on attachment 8943326 [details]
Bug 1431151 - Include outerWindowID in error message for assert.open.

https://reviewboard.mozilla.org/r/213650/#review220550

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 17 • 6 years ago

OK, maybe it is safer to apply this change as part of
https://bugzil.la/marionette-window-tracking.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 18 • 6 years ago

It would help us in the meantime to identify the window, so I wouldn't be worried to get it landed. But fine with me given that the window tracking patch might not take that long anymore.