Closed Bug 1479483 Opened Last year Closed Last year

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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?
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1479475
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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(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.
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.
Flags: needinfo?(mconley)
Assignee: nobody → bzbarsky
Flags: needinfo?(bzbarsky)
Priority: -- → P3
Attachment #8999995 - Flags: review?(mrbkap) → review+
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/3b64da678053
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.