1566836 - Tracking cookies are not disabled from the doorhanger

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Eliza Balazs]

Environment:
Device: OnePlus 5T (Android 9);
Build: ESR 68.1b2;

Precondition:
Have Tracking Protection set to "Enabled"
Have Cookies set to "Enabled, excluding tracking cookies"

Steps to reproduce:

1. Launch Fennec and go to https://senglehardt.com/test/cookie_restrictions/simple_iframe.html
2. Tap the shield icon and choose "Disable protection" from the doorhanger
3. Observe the information about the Tracking Cookies

Expected result:
A value for the disabled Tracking Cookie
"Your current non-HTTPOnly cookies are:
testPageCookie="
is displayed.

Actual result:
The value for the disabled Tracking Cookie is missing, so this is not disabled.

Note:

• On Desktop the Tracking Cookies are disabled.

Comment 1

[Chris Peterson [:cpeterson]]

The “Disable protection” button on Fennec’s site permissions door hanger should disable both ETP and standard TP for the site. The current implementation only disables regular TP.

Andrei, is this bug in the Fennec front-end code or in the Gecko Android shared with GeckoView?

We should fix this bug before shipping Fennec ETP. If a user needed to disable Tracking Protection to make a site work, they would have to disable ETP for all sites (in the Fennec settings) if the "Disable protection" button doesn't disable both ETP and standard TP.

Comment 2

[Chris Peterson [:cpeterson]]

The site permissions door hanger's "Enable protection" button should restore the site to the user's current settings for standard TP and ETP. "Disable protection" should make sure both standard TP and ETP are disabled for the site, but "Enable protection" might re-enable only standard TP, only ETP, both, or neither.

Comment 3

[Andrei Lazar]

Should probably update the gdoc as well with these new specs.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Andrei Lazar from comment #3)

Should probably update the gdoc as well with these new specs.

I see that the doc already says:

The “Disable protection” button on Fennec’s site permissions door hanger should disable both ETP and standard TP for the site.

I will update the doc to describe how the door hanger's "Enable protection" button should work (copying comment 2 from above).

https://docs.google.com/document/d/1YV-BECjbWviACyr4BRfMSiwP1mahbUAr2as5GyBYh3I/edit

Comment 5

[Chris Peterson [:cpeterson]]

I'm lowering the priority from P1 to P2. We might not be able to fix this bug in Fennec 68 without a lot of work. Fenix has the same problem because GV does not yet support site exceptions for TP (bug 1557009).

Comment 6

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 7

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

:andrei.a.lazar, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 8

[Andrei Lazar]

This is not a regression, it was not implemented in the first time.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

ETP is scheduled to be enabled by default for Fennec 68.1, so setting the tracking flag accordingly.

Comment 10

[(no longer active)]

It looks like we have a pref that isn't set on mobile, which is preventing the content blocking allow list from being respected for ETP interventions (e.g. when ETP decides to block cookies from a third-party tracker). I have a patch which I believe should fix the bug.

Comment 11

[(no longer active)]

Attachment: Bug 1566836 - Respect the Content Blocking allow list for ETP interventions on all platforms;

Comment 12

[(no longer active)]

I confirmed myself that this patch indeed fixes the bug.

Comment 13

[(no longer active)]

Attachment: Bug 1566836 - Respect the Content Blocking allow list for ETP interventions on all platforms;

Comment 14

[(no longer active)]

Comment on attachment 9082347 [details]
Bug 1566836 - Respect the Content Blocking allow list for ETP interventions on all platforms;

Beta/Release Uplift Approval Request

• User impact if declined: See comment 0.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0.
• List of other uplifts needed: None.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The code that gets enabled by this pref has been shipped to users on desktop...
• String changes made/needed: None.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See comment 0. This is required for ETP on Fennec.
• User impact if declined: See comment 0.
• Fix Landed on Version: Nightly
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See above.
• String or UUID changes made by this patch: None

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df552dfeb2d3
Respect the Content Blocking allow list for ETP interventions on all platforms; r=baku

Comment 16

[(no longer active)]

Dylan, this bug would break GV based apps using the content blocking allow list as well.

Comment 17

[Razvan Maries]

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

Comment 18

[Eliza Balazs]

Hi!
I tested this on Nightly 70.0a1 (2019-08-02) with OnePlus 5T (Android 9) and the issue is fixed.
I will mark this as verified on Firefox 70. Thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9082347 [details]
Bug 1566836 - Respect the Content Blocking allow list for ETP interventions on all platforms;

Fix for ETP due to ship in 68.1. Approved for Fennec 68.1b5. Note that this needs the rebased patch rather than a graft from m-c.

Comment 20

[Oana Pop-Rus]

This patch fail to apply:

Hunk #1 FAILED at 1565
1 out of 1 hunks FAILED -- saving rejects to file browser/app/profile/firefox.js.rej
patching file modules/libpref/init/StaticPrefList.h
Hunk #1 FAILED at 723
1 out of 1 hunks FAILED -- saving rejects to file modules/libpref/init/StaticPrefList.h.rej

Thank you.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out https://hg.mozilla.org/releases/mozilla-beta/rev/a0a695337bed3db31edaa1a38d36ab4828d46385 for containing both Bug 1570434 and Bug 1566836:
https://hg.mozilla.org/releases/mozilla-beta/rev/a0a695337bed3db31edaa1a38d36ab4828d46385

Relanded: https://hg.mozilla.org/releases/mozilla-beta/rev/507c6138ec8e

Comment 22

[Chris Peterson [:cpeterson]]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #21)

Relanded: https://hg.mozilla.org/releases/mozilla-beta/rev/507c6138ec8e

Aryx, are you landing patches to ESR? We still need to uplift this fix to mozilla-esr68 for Fennec ESR 68.1.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/1f1fbfaa4d84

Comment 24

[Eliza Balazs]

Hi! Verified as fixed on Beta 69.0b11 with OnePlus 5T (Android 9), Motorola Nexus 6 (Android 7.1.1).
I will mark this as verified on Firefox 69. Thanks!

Comment 25

[Eliza Balazs]

Hello! Verified as fixed on ESR 68.1b5 with Nexus 9 (Android 7.1.1; tablet), Samsung Galaxy S8 (Android 9).
I will mark this as verified on Firefox esr68. Thanks!