1479483 - Can we remove the chrome-only ability to set window.opener to some random window

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

nsGlobalWindowInner::SetOpener has a codepath for the following case:

1)  The passed-in value is a JS object.
2)  Caller is chrome.

In this case we check for that object being an actual DOM window, and if so set it as our opener.  Code coverage results seem to suggest that this codepath is not being exercised at all.

Any objections to me just removing this codepath?

Comment 1

[:Gijs (he/him)]

https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%27%5C.opener+%3D%5B%5E%3D%5D%27+-file%3A.py&redirect=false shows a dom/base test and some other stuff that's clearly not window-related or unprivileged. Is the dom/base thing a unit test for this case? Or is that a plain mochitest only and therefore doesn't meet:

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #0)
> 2)  Caller is chrome.

?

Is there an alternative unprivileged codepath that'll be followed by the privileged code if we remove the existing one this bug is talking about? What would the difference be?



Off-hand, I don't know of any reason to keep this, though mconley is probably a slightly better person to ask given his knowledge of our window mediator stuff and chrome window lifecycles - but he's on PTO until the start of next week.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Or is that a plain mochitest only and therefore doesn't meet:

Yep.  The dom/base thing is not a chrome caller.  So when it does:

  win.opener = $("x").contentWindow;

that just redefines the "opener" property on the Window object to the new value, without changing the window.opener that things over Xrays see or the "opener window" spec concept.

The other things at that link are either setting the value to null, are web platform tests (so not chrome) or not operating on a DOM window (LoginManagerParent.jsm).

> Is there an alternative unprivileged codepath that'll be followed by the privileged code if we remove
> the existing one this bug is talking about? What would the difference be?

The unprivileged codepath looks like this:

1)  If the given value is null, set the internal "opener window" concept to null and return.
2)  Otherwise redefine the property to have the given value, without changing the underlying "opener window".

In particular, step 1 persists across navigations, but step 2 does not.

I'll try waiting for mconley, just in case; this is not urgent.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I can't think of a good reason for chrome code to overwrite the opener like this - and even if there was such a use-case, I water we'd want to have a separate ChromeOnly function to do it.

I think it's fine to drop the privileged opener setting codepath.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

*wager

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the ability of chrome code to permanently set window.opener to a non-null value

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b64da678053
Remove the ability of chrome code to permanently set window.opener to a non-null value.  r=mrbkap

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/3b64da678053