1618557 - It seems that LoadInfo's top-level document constructor sets up the wrong top-level anti-tracking principal

Status: NEW

(Core :: Privacy: Anti-Tracking, defect, P3)

Description

[(no longer active)]

See this principal. Here innerWindow is the old inner window that the top-level navigation is replacing. It seems wrong to use its principal as the top-level anti-tracking principal.

baku, what do you think?

Comment 1

[(no longer active)]

This is related to https://hg.mozilla.org/mozilla-central/rev/d496a070135c. It's possible that stopAtOurLevel is mostly false so we always bail our early here, but it could be that the fix to bug 1577298 wasn't completely right...

Comment 2

[(no longer active)]

baku said he'll take this.

Comment 3

[Matt Woodrow (:mattwoodrow)]

I tried dropping this write in this try push: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=290729119&revision=9fd39f404f603d7cb668853a20013d7b661600d9

It seems to be fine, but it'd be nice to have a test.

Comment 4

[Andrea Marchesini [:baku]]

Matt, if you want to work on this, it's great. I can review the code. Let me know.

Comment 5

[Matt Woodrow (:mattwoodrow)]

I'm not really sure where to start for adding a test sorry.

I can upload a patch to just remove the offending call if that's useful!

Comment 6

[Andrea Marchesini [:baku]]

Dimi, do we still need to fix this issue? Asking because the fission-porting has changed the code a lot lately.

Comment 7

[Dimi Lee [:dimi]]

We still use topLevelPrincipal from LoadInfo to get partitionPrincipal at here. But it is only used when we can't partitionKey from CookieJarSetting in some special cases, so in most of the cases, the value of topLevelPrincipal doesn't matter.

I think the right solution now is to find the cases we don't set partitionKey, and then remove TopLevelPrincipal from LoadInfo entirely to prevent using it in the future. Because this value doesn't work in Fission anyway.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: Bug 1618557 - Remove top-level principal from LoadInfo, r?dimi

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:baku, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D79098	Bug 1618557 - Remove top-level principal from LoadInfo, r?dimi	baku	dimi: Resigned from review

:baku, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Comment 11

[Tim Huang[:timhuang]]

Dimi, do you think this is still relevant? Thanks.

Comment 12

[Dimi Lee [:dimi]]

(In reply to Tim Huang[:timhuang] from comment #11)

Dimi, do you think this is still relevant? Thanks.

As per discussion with :tim offline, we think removing toplevelprincipal from loadinfo is still the right thing to do, so yes, this is still relevant.

Comment 13

[Abhishek Madan[:abhishekmadan]]

Attachment: WIP: Bug 1618557 - Remove top-level principal from LoadInfo. r?#anti-tracking

Comment 14

[Abhishek Madan[:abhishekmadan]]

Attachment: WIP: Bug 1618557 - Remove top-level principal from LoadInfo. r=anti-tracking-reviewers