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

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

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.

Comment 3

[(no longer active)]

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

Comment 4

[(no longer active)]

Attachment: Bug 1515343 - Emit the correct rejection code from the anti-tracking backend when a dynamic storage check fails with a doubly nested iframe;

Comment 5

[(no longer active)]

This fixes https://iframely.com/domains/facebook, www.reddit.com, as well as http://wot.wikia.com/wiki/The_Path_of_Daggers.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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 7

[(no longer active)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4f9b811b0f2a

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b4e4d44001f7

Comment 11

[Vlad Lucaci, Desktop QA (:vlucaci)]

Hello, 

Confirming this issue as verified fixed on 66.0a1 and 65.0b6. Verified on Windows 10x64, Ubuntu 16.04 and macOS 10.12.