Closed Bug 1530303 Opened 8 months ago Closed 6 months ago

New window doesn't have the correct COOP

Categories

(Core :: Document Navigation, defect, P1)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Fission Milestone M2
Tracking Status
firefox68 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged][wptsync upstream])

Attachments

(6 files)

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.

Component: Networking → Document Navigation

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.

Status: NEW → ASSIGNED
Fission Milestone: --- → M2

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!)

Blocks: resab

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 :)

Depends on: 1523636

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.

Attachment #9051823 - Attachment is obsolete: true
Attachment #9051823 - Attachment description: Bug 1530303 - Add test for COOP header with new window → Bug 1530303 - Add test for COOP header with new window r=nika
Attachment #9051823 - Attachment is obsolete: false
Attachment #9051824 - Attachment description: Bug 1530303 - Put CrossOriginOpenerPolicy in BrowsingContext → Bug 1530303 - Put CrossOriginOpenerPolicy in BrowsingContext 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

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

Depends on D24895

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?

  1. 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

  2. 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?

Flags: needinfo?(nika)
Flags: needinfo?(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.

Flags: needinfo?(annevk)
Attachment #9051823 - Attachment description: Bug 1530303 - Add test for COOP header with new window r=nika → Bug 1530303 - Add test for multiple COOP header navigations r=nika

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

  1. 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.

(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?

Flags: needinfo?(valentin.gosu)

(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.

Flags: needinfo?(valentin.gosu)
Blocks: 1530329
Flags: needinfo?(nika)
Pushed by valentin.gosu@gmail.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16512 for changes under testing/web-platform/tests
Whiteboard: [necko-triaged] → [necko-triaged][wptsync upstream]
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.