Closed Bug 1641905 Opened 5 years ago Closed 5 years ago

ThirdPartyUtil::IsThirdPartyWindow returns different values in fission

Categories

(Core :: DOM: Security, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla79
Fission Milestone M6b
Tracking Status
firefox79 --- disabled
firefox80 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 2 obsolete files)

https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/netwerk/base/mozIThirdPartyUtil.idl#43-59

By documentation, http://example.com, https://example.com and https://example.com:1234 should be considered not-3rd-parties. But using mozIThirdPartyUtil::isThirdPartyWindow(), the result is different in fission because we use the process boundary:

https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/dom/base/ThirdPartyUtil.cpp#249-254

This has several consequences:

  1. loadInfo has a wrong isInThirdPartyContext value:
    https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/netwerk/base/nsILoadInfo.idl#486
  2. windowcontext has a similar bug here: https://searchfox.org/mozilla-central/source/docshell/base/WindowContext.h#26
  3. document.cookie has a similar issue. See bug 1641459.

Anny said she will investigate this bug. She worked on IsThirdPartyGlobal, which may be related.

M6b

Assignee: nobody → agakhokidze
Severity: -- → S3
Fission Milestone: --- → M6b
Priority: -- → P3
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
Assignee: agakhokidze → amarchesini
See Also: → 1617784
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1c9058277ddd ThirdPartyUtil::IsThirdPartyWindow for fission, r=dimi,necko-reviewers,dragana
Flags: needinfo?(amarchesini)
Attachment #9157365 - Attachment description: Bug 1641905 - ThirdPartyUtil::IsThirdPartyWindow for fission - fix webextension third-party tests, r?robwu → Bug 1641905 - ThirdPartyUtil::IsThirdPartyWindow for fission - webExtension, r?robwu
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7a7f2357ab75 ThirdPartyUtil::IsThirdPartyWindow for fission, r=dimi,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/3ffee9957222 ThirdPartyUtil::IsThirdPartyWindow for fission - webExtension, r=robwu,ckerschb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
Regressions: 1648163

This seems to have broken a WPT test, specifically if you have the pref privacy.partition.network_state set to false (which is the default value of that pref on all channels except for Nightly -- that's why the test failure didn't come up in infra yet).

This means this is going to cause perma-orange in that test when the trains shift this coming week. (And this represents a change in behavior for users, too; it's hard to know how substantial of a change, but it seems like there's a decent chance it could break some sites/content given that it's an observable & I-think-unintentional change.) See bug 1648163 comment 9 for more details & for a command that you can use to reproduce the failure locally.

baku, are you OK with backing this out, to avoid shipping that unexpected behavior-change (and perma-orange) on beta? (After the branch, you can hopefully sort out the pref-dependent test failure, and re-land on Nightly in a week or whenever's convenient.)

Flags: needinfo?(amarchesini)

If one of the reviewers (e.g. ckerschb) can weigh on on comment 8 sooner than baku can, that would be great, too. (I know it's still the weekend in my timezone, but time is of the essence since the beta merge is hours away; so I'm hoping someone sees this near the start of their day on Monday.)

(tl;dr: please confirm that it's fine for sheriffs/release-managers/whoever to back this bug out for now, to avoid introducing a perma-orange on beta during the merge.)

If we don't hear anything and have to make a call before the merge, I think my recommendation to sheriffs/release managers would be to proceed & back this out from mozilla-central, since as far as I can tell this is a fission-specific thing and doesn't seem to have any urgency to it needing to stay in. But it would be helpful to have confirmation from someone involved with the patch confirm that it's fine, given that this is in the DOM:Security component & hypothetically maybe there are subtle downstream effects of this patch that we're implicitly depending on.

Flags: needinfo?(ckerschb)
Flags: needinfo?(dlee)

I think it should be ok to back this out. If I understand correctly, this is to make partition network state work in Fission. So probably this is not super urgent for 79. If we don't get any feedback from baku soon, let's back this out first.

Flags: needinfo?(dlee)

This means this is going to cause perma-orange in that test when the trains shift this coming week. (And this represents a change in behavior for users, too; it's hard to know how substantial of a change, but it seems like there's a decent chance it could break some sites/content given that it's an observable & I-think-unintentional change.) See bug 1648163 comment 9 for more details & for a command that you can use to reproduce the failure locally.

I would like to spend a bit of time understanding what the real issue is because backing out this patch would probably hide a bug elsewhere.
Network state isolation should not impact the way the browser works: it's about having separate cache items, separate HTTP connection pools, and so on. Nothing of this should be visible from the page content.

This is worrisome because, in the broken WPT, it seems that, something relies on the network/image cache. This is wrong because the main scope of the cache is to reduce networking, without changing the final result.

If this is OK with you and RyanVM, I would like to spend a few hours on this issue and dig on what the real issue is. Maybe the fix is trivial and we can uplift a patch to beta.

Flags: needinfo?(amarchesini)

(In reply to Daniel Holbert [:dholbert] from comment #9)

If one of the reviewers (e.g. ckerschb) can weigh on on comment 8 sooner than baku can, that would be great, too. (I know it's still the weekend in my timezone, but time is of the essence since the beta merge is hours away; so I'm hoping someone sees this near the start of their day on Monday.)

I think a back out would be fine so we can avoid a perma orange and a change in behavior on beta (at least for now). Since baku already replied and apparently is on it investigating, I guess we could allow him some time and then make a final decision. Personally I would feel more comfortable with a backout and try to reland on central in the new cycle.

Flags: needinfo?(ckerschb)
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e4ff1079d9e9 ThirdPartyUtil::IsThirdPartyWindow returns different values in fission - use the principal to inherit for about:blank channels, r=dimi
Attachment #9155956 - Attachment is obsolete: true
Attachment #9157365 - Attachment is obsolete: true

We got a regression reported on webcompat here because of this issue.
see https://github.com/webcompat/web-bugs/issues/56383

baku, About this last comment, dennis mentioned that he is looking at it to understand if it's a real regression.

Flags: needinfo?(dschubert)
Flags: needinfo?(amarchesini)

Fission implemented the 3rd-party check differently so yes, to me this was a fission-regression.

Flags: needinfo?(amarchesini)
Regressions: 1682208
Blocks: 1701800

So this was a bit low on my priority list because the site where this first popped up was doing very quirky stuff and we've only ever seen that one specific case.

In the end, yeah, this is bug 1701800, so we already have a bug for that on file.

Flags: needinfo?(dschubert)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: