Closed
Bug 1368965
Opened 7 years ago
Closed 7 years ago
For non browser windows don't inject chrome window handles into window handles
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox-esr52 wontfix, firefox55 fixed, firefox56 fixed)
RESOLVED
FIXED
mozilla56
People
(Reporter: whimboo, Assigned: whimboo)
References
()
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 | ||
Updated•7 years ago
|
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8877201 -
Flags: review?(ato)
Attachment #8877202 -
Flags: review?(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.
Attachment #8877201 -
Flags: review?(ato) → review+
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
Attachment #8877202 -
Flags: review?(ato) → review+
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.
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.
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [spec][17/06]
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
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/603cd9b8ff99
https://hg.mozilla.org/mozilla-central/rev/5343d6a9bc64
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Updated•7 years ago
|
status-firefox55:
--- → affected
status-firefox-esr52:
--- → ?
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.
Whiteboard: [spec][17/06] → [spec][17/06][checkin-needed-beta]
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
Flags: in-testsuite+
Whiteboard: [spec][17/06][checkin-needed-beta] → [spec][17/06]
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•