Closed Bug 1515343 Opened Last year Closed Last year

Embedded Facebook Videos show two non-blocked facebook entries in the cookies sub-panel

Categories

(Firefox :: Protections UI, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 66
Tracking Status
firefox64 --- unaffected
firefox65 + verified
firefox66 + verified

People

(Reporter: ehsan, Assigned: ehsan)

References

()

Details

(Whiteboard: [privacy65])

Attachments

(1 file)

Go to the address in the URL field and look at the cookies subpanel.

Under Tracking Cookies you will see two non-blocked items for:

  * https://www.facebook.com
  * https://staticxx.facebook.com
A couple of things are going wrong here.

First and foremost, we mistakenly don't set a rejection reason here:

<https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/toolkit/components/antitracking/AntiTrackingCommon.cpp#1184>

This means that when the storage check for staticxx.facebook.com for example fails, we don't raise a content blocking error correctly.  But even worse, then we get to this code: <https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/toolkit/components/antitracking/AntiTrackingCommon.cpp#1496> and since we have no idea that an error really occurred previously (there we have a rejection code of 0) we add insult to injury and send a STATE_COOKIES_LOADED notification.

As a result, not only do we not show what we do to these two origins correctly, but also we show the exact opposite of the truth in the UI.
Assignee: nobody → ehsan
Duplicate of this bug: 1512141
Whiteboard: [privacy65]
(In reply to :Ehsan Akhgari from comment #1)
> But even worse,
> then we get to this code:
> <https://searchfox.org/mozilla-central/rev/
> 13788edbabb04d004e4a1ceff41d4de68a8320a2/toolkit/components/antitracking/
> AntiTrackingCommon.cpp#1496> and since we have no idea that an error really
> occurred previously (there we have a rejection code of 0) we add insult to
> injury and send a STATE_COOKIES_LOADED notification.

Note that we can't yet change this part of things before bug 1509174 is fixed, because if we don't send the STATE_COOKIES_LOADED notification with a rejection code of 0, then for cases where a tracker has previously received the 3rdPartyStorage permission, no notification code will be sent, and the Cookies subpanel will not be updated.

(The browser_trackingUI_cookies_subview.js test catches this case.)
Blocks: 1510860
No longer blocks: 1510860
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4e4d44001f7
Emit the correct rejection code from the anti-tracking backend when a dynamic storage check fails with a doubly nested iframe; r=baku
Comment on attachment 9032470 [details]
Bug 1515343 - Emit the correct rejection code from the anti-tracking backend when a dynamic storage check fails with a doubly nested iframe;

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1501992

User impact if declined: (Not really a regression from bug 1501992, but that bug makes the impact of this visible on the 65 release.)

See comment 0.  Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1512141#c0 as well as https://bugzilla.mozilla.org/show_bug.cgi?id=1512141#c11.  It is very likely this impacts many others sites similarly.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Super low risk.  We were returning an error without recording the reason why the error was happening.  Our UI uses this reason in order to indicate that something was blocked, and the lack of this reason being recorded was the source of this bug.  The code change here is just adding this recording, and is possible to prove that it's 100% safe in terms of the possibility of introducing regressions and/or stability issues.

String changes made/needed: None
Attachment #9032470 - Flags: approval-mozilla-beta?
Comment on attachment 9032470 [details]
Bug 1515343 - Emit the correct rejection code from the anti-tracking backend when a dynamic storage check fails with a doubly nested iframe;

[Triage Comment]
Looks like a pretty simple patch with new tests included. Tests on autoland are looking solid, so approving this for 65.0b6 now so it can go in with the other control center fixes going into that build. We'll want QA to look it over too.
Attachment #9032470 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
https://hg.mozilla.org/mozilla-central/rev/b4e4d44001f7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Hello, 

Confirming this issue as verified fixed on 66.0a1 and 65.0b6. Verified on Windows 10x64, Ubuntu 16.04 and macOS 10.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.