1253538 - Window.open() should not reuse an existing window if it's running a different container

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: test

I wrote a test to check what bz suggested in bug 1249224 comment 9. Here the scenario I want to test:

. tabA: userContextId=1 window.name='tab-1'
. tabB: userContextId=2 window.name='tab-2'
. tabA does: window.open(something, 'tab-2');

What I want to see is that we create a new tabC with userContextId=1 and window.name='tab-2'.
In our current code we are actually reusing tabB because it's called 'tab-2' ignoring the fact that its userContextId is different than 1.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: fix.patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8726620 [details] [diff] [review]
test

I was trying to figure out why this test is failing, even though my manual tests pass.

The answer is that this test is doing the window.open() call from system code.  So when we end up in nsDocShell::ValidateOrigin (called from nsDocShell::CanAccessItem) we hit this code block:

  if (nsContentUtils::GetCurrentJSContext() &&
      nsContentUtils::IsCallerChrome()) {
    return true;
  }

and the targeting is allowed.  If this line in the test:

>+      let w = content.window.open(opts.url, 'tab-2');

is changed to:

      let w = content.window.wrappedJSObject.open(opts.url, 'tab-2');

then the test passes.

So I guess an important question here is: do we want chrome code to be able to trigger linking from one user context to another one (in which case the existing setup is fine), or do we want to forbid that, in which case we probably need the actual patch?

>+    Cu.import("resource://gre/modules/PromiseUtils.jsm");

Why not just use normal Promises?

>+      w.onload = function() { resolve(); }

This is a bit annoying because the test failing manifests as a test timeout, but I guess we can live with that...  

>+      is(browser.contentDocument.nodePrincipal.userContextId, 1, "Tab3 UCI must be 1");

Can we also assert that "browser" is neither "browser1" nor "browser2"?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8726629 [details] [diff] [review]
fix.patch

r=me assuming we want to prevent the "chrome can allow targeting across user context ids" thing.

That said, it looks like now we're checking that GetIsInIsolatedMozBrowserElement() matches, GetAppId() matches, mUserContextId matches.  The only parts of OriginAttributes::operator== we're not checking are mAddonId and mSignedPkg.  Please file a followup bug to see whether we should be checking those too (and probably needinfo bholley).

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8511fae8a1cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2cf128ceae

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9ef8acccce

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9b726495d1

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8511fae8a1cf
https://hg.mozilla.org/mozilla-central/rev/0c2cf128ceae
https://hg.mozilla.org/mozilla-central/rev/7b9ef8acccce
https://hg.mozilla.org/mozilla-central/rev/2f9b726495d1