ThirdPartyUtil::IsThirdPartyWindow returns different values in fission
Categories
(Core :: DOM: Security, defect, P3)
Tracking
()
People
(Reporter: baku, Assigned: baku)
References
(Blocks 3 open bugs)
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 2 obsolete files)
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:
This has several consequences:
- loadInfo has a wrong isInThirdPartyContext value:
https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/netwerk/base/nsILoadInfo.idl#486 - windowcontext has a similar bug here: https://searchfox.org/mozilla-central/source/docshell/base/WindowContext.h#26
- document.cookie has a similar issue. See bug 1641459.
Comment 1•5 years ago
|
||
Anny said she will investigate this bug. She worked on IsThirdPartyGlobal, which may be related.
M6b
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
![]() |
||
Comment 4•5 years ago
•
|
||
Backed out for perma failures.
Logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306522410&repo=autoland&lineNumber=5463
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306524355&repo=autoland&lineNumber=2356
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306522335&repo=autoland&lineNumber=3844
Backout: https://hg.mozilla.org/integration/autoland/rev/532902052359239ea7c8a2525f3d3b9b4a9b5bef
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D79307
![]() |
||
Updated•5 years ago
|
Updated•5 years ago
|
![]() |
||
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7a7f2357ab75
https://hg.mozilla.org/mozilla-central/rev/3ffee9957222
Comment 8•5 years ago
|
||
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.)
Comment 9•5 years ago
•
|
||
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.
Updated•5 years ago
|
Comment 10•5 years ago
|
||
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.
Assignee | ||
Comment 11•5 years ago
|
||
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.
Comment 12•5 years ago
|
||
(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.
Comment 13•5 years ago
|
||
backout |
Backed out from Beta79 per discussion with Andrea and Christoph.
https://hg.mozilla.org/releases/mozilla-beta/rev/595f0e492a780194c23dc3a855e41278d9508d1b
Assignee | ||
Comment 14•5 years ago
|
||
Comment 15•5 years ago
|
||
Comment 16•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•5 years ago
|
![]() |
||
Comment 17•5 years ago
|
||
We got a regression reported on webcompat here because of this issue.
see https://github.com/webcompat/web-bugs/issues/56383
![]() |
||
Comment 18•5 years ago
|
||
baku, About this last comment, dennis mentioned that he is looking at it to understand if it's a real regression.
Assignee | ||
Comment 19•5 years ago
|
||
Fission implemented the 3rd-party check differently so yes, to me this was a fission-regression.
Comment 20•4 years ago
|
||
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.
Description
•