1530303 - New window doesn't have the correct COOP

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Valentin Gosu [:valentin] (he/him)]

https://arturjanc.com/cgi-bin/coop/index-with-coop.py?coop=same-origin&openee_host=arturjanc.com
Desired behavior: Allow (pop-up is same-origin and both the main window and pop-up set COOP: same-origin)
Actual: Blocked

It seems the new window has policy OPENER_POLICY_NULL, instead of OPENER_POLICY_SAME_ORIGIN.

Comment 1

[Anne (:annevk)]

I wonder if https://gist.github.com/annevk/6f2dd8c79c77123f39797f6bdac43f3e is wrong here. Currently if unsafe-allow-outgoing is false, it'll always create a new browsing context group.

I guess this would argue instead for always normally creating an auxiliary browsing context and then only after navigating deciding whether to replace.

Comment 2

[Anne (:annevk)]

Having studied the problem space again I think the only change I need to make (and now have made) is to remove the specification-patching of "creating an auxiliary browsing context". This lets the navigation algorithm make all the decisions and the way that specification-patch is currently written would do the correct thing for Artur's tests as far as I can tell.

I'd love it if people could give the algorithm another review however as the logic is rather tricky. (Rewrite suggestions very much welcome too!)

Comment 3

[arturjanc]

Friendly ping -- any word on the fix for this issue? (it would unblock us from testing COOP across our sites, hopefully generating some more feedback :)

Comment 4

[Valentin Gosu [:valentin] (he/him)]

The problem here is that opening the new window doesn't pass the opener policy to the newly created about:blank.
I have a patch that moves the openerPolicy on the browsing context, but I'm still struggling to find a way to pass the policy over to a new window. The comment in ContentParent::CreateBrowser points to bug 1523636.
I'm still looking for a way to work around this issue.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1530303 - Add test for multiple COOP header navigations r=nika

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1530303 - Put CrossOriginOpenerPolicy in BrowsingContext r=nika

Depends on D23934

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1530303 - Use CanonicalBrowsingContext->CurrentWindowGlobal->DocumentPrincipal to get the topLevelWindowPrincipal r=nika

The principal we got from the child process was always a null principal, which was wrong.
This approach seems to give you the actual principal of the current document.

Depends on D23935

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1530303 - Remove nsIHttpChannelInternal.setTopWindowPrincipal r=nika

Since it wasn't reliable, we remove the topWindowPrincipal in favor of WindowGlobalParent->DocumentPrincipal()

Depends on D24895

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1530303 - Remove xpcshell-test for crossOriginOpenerPolicy since it can't work with browsingContexts r=nika

Depends on D24897

Comment 10

[Valentin Gosu [:valentin] (he/him)]

An update on the (lack of) progress here

There are a few problems here:

about:blank documents inherit cross-origin opener-policy from their creator.

1. This doesn't happen right now.
   I moved the COOP from being passed via nsILoadInfo to being a field on the BrowsingContext, so that it's propagated via opener/parent.

However, the BC that we get in nsHttpChannel doesn't have the proper policy. That's because right after a process switch we create a new browsing context in ContentParent::CreateBrowser with null parent and opener, so it gets a null policy.
I tried to follow Nika's suggestion of trying to set it in nsDocShell::CreateContentViewer, but I couldn't get a handle to the old browsing context. Maybe I wasn't looking in the right place? Nika, could you give me a hand with this?

2. The document origin was always a NullPrincipal. We passed it from the child process via nsIHttpChannelInternal.setTopWindowPrincipal, and while that worked in xpcshell-tests (because we set it manually), it didn't really work in real life.
   I put up a patch to get it from the CurrentWindowGlobal

3. A new window will never be same-site or same-origin
   also, A (same-site;a.com) -> new window goes to about:blank (same-site;nullprincipal) -> B (same-site;sub.a.com)
   Regardless of inherited policy, the about:blank window will never be same origin or same-site with B.

Not sure exactly how we're supposed to handle this situation. Anne?

Comment 11

[Anne (:annevk)]

I chatted with Valentin and there appears to be some problem with the origin for about:blank documents. At least the way it's obtained doesn't give the correct result. This will need input from Nika and perhaps bz.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1530303 - Add WPT test for CrossOriginOpenerPolicy checking that a same-site policy window.open doesn't end up in a different process r=annevk

Depends on D24898

Comment 13

[Valentin Gosu [:valentin] (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #10)

3. A new window will never be same-site or same-origin
   also, A (same-site;a.com) -> new window goes to about:blank (same-site;nullprincipal) -> B (same-site;sub.a.com)
   Regardless of inherited policy, the about:blank window will never be same origin or same-site with B.

I converted the test to a WPT. Right now it times out because of the browsing context issue, but the origin of the new window is the correct one, not a null principal.

Comment 14

[Neha Kochar [:neha]]

(In reply to Valentin Gosu [:valentin] from comment #13)

I converted the test to a WPT. Right now it times out because of the browsing context issue, but the origin of the new window is the correct one, not a null principal.

Valentin, can you try with the patch from bug 1523636 which is still undergoing review to confirm if that solves the problem or if you see more issues even with that fix?

Comment 15

[Valentin Gosu [:valentin] (he/him)]

(In reply to Neha Kochar [:neha] from comment #14)

(In reply to Valentin Gosu [:valentin] from comment #13)

I converted the test to a WPT. Right now it times out because of the browsing context issue, but the origin of the new window is the correct one, not a null principal.

Valentin, can you try with the patch from bug 1523636 which is still undergoing review to confirm if that solves the problem or if you see more issues even with that fix?

The test cases still don't work, even with the patches from bug 1523636 applied.
It's not clear to me if I would also require a fix for bug 1540839, or if it's a totally different issue.

I'm currently looking into Nika's suggestion of putting the COOP policy on WindowGlobalParent.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72cd1518eddf
Add test for multiple COOP header navigations r=nika
https://hg.mozilla.org/integration/autoland/rev/646b72163f00
Put CrossOriginOpenerPolicy in BrowsingContext r=nika
https://hg.mozilla.org/integration/autoland/rev/e63fb502dc78
Use CanonicalBrowsingContext->CurrentWindowGlobal->DocumentPrincipal to get the topLevelWindowPrincipal r=nika
https://hg.mozilla.org/integration/autoland/rev/390abfcb3b30
Remove nsIHttpChannelInternal.setTopWindowPrincipal r=nika
https://hg.mozilla.org/integration/autoland/rev/805db49d7a1b
Remove xpcshell-test for crossOriginOpenerPolicy since it can't work with browsingContexts r=nika
https://hg.mozilla.org/integration/autoland/rev/41bb6aa3a039
Add WPT test for CrossOriginOpenerPolicy checking that a same-site policy window.open doesn't end up in a different process r=annevk

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/72cd1518eddf
https://hg.mozilla.org/mozilla-central/rev/646b72163f00
https://hg.mozilla.org/mozilla-central/rev/e63fb502dc78
https://hg.mozilla.org/mozilla-central/rev/390abfcb3b30
https://hg.mozilla.org/mozilla-central/rev/805db49d7a1b
https://hg.mozilla.org/mozilla-central/rev/41bb6aa3a039

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16512 for changes under testing/web-platform/tests

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged