Closed Bug 1368965 Opened 2 years ago Closed 2 years ago

For non browser windows don't inject chrome window handles into window handles

Categories

(Testing :: Marionette, defect)

Version 3
defect
Not set

Tracking

(firefox-esr52 wontfix, firefox55 fixed, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug, )

Details

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

Attachments

(2 files)

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)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
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.
Attachment #8877201 - Flags: review?(ato)
Attachment #8877202 - Flags: review?(ato)
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.
Attachment #8877201 - Flags: review?(ato) → review+
Comment on attachment 8877202 [details]
Bug 1368965 - Don't inject chrome window handles into window handles.

https://reviewboard.mozilla.org/r/148578/#review153468
Attachment #8877202 - Flags: review?(ato) → review+
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.
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.
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.
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.
Whiteboard: [spec][17/06]
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
https://hg.mozilla.org/mozilla-central/rev/603cd9b8ff99
https://hg.mozilla.org/mozilla-central/rev/5343d6a9bc64
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
Whiteboard: [spec][17/06] → [spec][17/06][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/adebfb4988c3
https://hg.mozilla.org/releases/mozilla-beta/rev/437cebf87c00
Flags: in-testsuite+
Whiteboard: [spec][17/06][checkin-needed-beta] → [spec][17/06]
You need to log in before you can comment on or make changes to this bug.