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

Whenever a new chrome window gets opened which is not a browser window, we include its outer window id in the window_handles list.

This is not appropriate for testing in content scope, and we should strongly avoid doing so. Exceptions for chrome windows I can think of now should be the following:

* New browser windows with an available tab browser
* Popup windows (which should browser windows but with all chrome stripped off)

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

I think I have fixed this as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1311041 but since that is some time away from landing, it makes sense to patch this in Marionette’s current implementation first.

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

Comment on attachment 8877201 [details]
Bug 1368965 - assert.contentBrowser has to check for closed chrome window.

https://reviewboard.mozilla.org/r/148538/#review153464

::: testing/marionette/assert.js:123
(Diff revision 1)
> +  // TODO: contentBrowser uses cached data and as such would need an
> +  // additional check that the chrome window is still open.
> +  assert.window(context && context.window);

Exactly what is it that is cached here?  Because we operate in chrome context, there is a chance that the window global is referenced and closed.  This is similar to a child window whose parent window (window.opener) has been closed.

The code here looks correct, but I’m not sure about the comment.

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

Comment on attachment 8877202 [details]
Bug 1368965 - Don't inject chrome window handles into window handles.

https://reviewboard.mozilla.org/r/148578/#review153468

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

Comment on attachment 8877201 [details]
Bug 1368965 - assert.contentBrowser has to check for closed chrome window.

https://reviewboard.mozilla.org/r/148538/#review153464

> Exactly what is it that is cached here?  Because we operate in chrome context, there is a chance that the window global is referenced and closed.  This is similar to a child window whose parent window (window.opener) has been closed.
> 
> The code here looks correct, but I’m not sure about the comment.

Ok, I should update the comment. What's cached here is the tab. We currently do not retrieve it each time we access the property, but set it when we call `switchToTab`. Maybe we don't necessarily need the refactoring and I can change that behavior by always accessing the real underlying tab.

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

Comment on attachment 8877201 [details]
Bug 1368965 - assert.contentBrowser has to check for closed chrome window.

https://reviewboard.mozilla.org/r/148538/#review153464

> Ok, I should update the comment. What's cached here is the tab. We currently do not retrieve it each time we access the property, but set it when we call `switchToTab`. Maybe we don't necessarily need the refactoring and I can change that behavior by always accessing the real underlying tab.

OK, I see!  Thanks for the clarification.  Makes sense now.

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

Comment on attachment 8877201 [details]
Bug 1368965 - assert.contentBrowser has to check for closed chrome window.

https://reviewboard.mozilla.org/r/148538/#review153464

> OK, I see!  Thanks for the clarification.  Makes sense now.

I dedicded to only update the comment for now. Changing the tab behavior could actually open another can of worms, and which should really be done in a separate bug.

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

I accidentally added the comment changes to the wrong commit, so I had to fix it. While doing it I also rebased against latest central. I'm going to land this patch now.

	Pulsebot
	Comment 15 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/603cd9b8ff99
assert.contentBrowser has to check for closed chrome window. r=ato
https://hg.mozilla.org/integration/autoland/rev/5343d6a9bc64
Don't inject chrome window handles into window handles. r=ato

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/603cd9b8ff99
https://hg.mozilla.org/mozilla-central/rev/5343d6a9bc64

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

Please uplift this test-only patch to beta. It's low risk and has good unit test coverage. No regressions have been seen yet through the whole last week. Thanks.

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

https://hg.mozilla.org/releases/mozilla-beta/rev/adebfb4988c3
https://hg.mozilla.org/releases/mozilla-beta/rev/437cebf87c00