1627220 - First-party cookies are not removed after a daily cleanup

Status: VERIFIED FIXED

(Core :: Privacy: Anti-Tracking, defect, P1)

Description

[Simona Badau, Desktop QA]

Affected versions

• Nightly 76.0a1

Affected platforms

• All

Prerequisites:

• Use a new profile.

• In about:config set the preferences
  -- privacy.purge_trackers.enabled=true.
  -- privacy.userInteraction.expiration =1.

• Make sure that in the Settings of the Web Console, "Enabled browser chrome and add-on debugging toolboxes" is checked.

Steps to reproduce

1. Launch Firefox
2. Navigate to one or two popular websites to create some cookies and site storage
   (interacted with Amazon, YouTube, Google.com, Yandex.ru, BBC and USAToday)
3. Go to about:preferences#privacy and observe the cookies from the "Manage Cookies and Site Data" dialog.
4. Open the Browser Console, type and enter `Services.obs.notifyObservers(null, "idle-daily");' to send the idle-daily notification manually.
5. Check again the cookies from the "Manage Cookies and Site Data" dialog.
6. Open the Browser Console and to send the 30 days passed interaction timeout manually - type and enter:
   await Components.classes["@mozilla.org/purge-tracker-service;1"].getService(Components.interfaces.nsIPurgeTrackerService).purgeTrackingCookieJars()
7. Check again the cookies from the "Manage Cookies and Site Data" dialog.

Expected result

• in step 5 - the third-party cookies from the Disconnect Tracking Protection list should be removed.
• in step 7 - the first-party cookies from the Disconnect Tracking Protection list should be removed.

Actual result

• in step 7 - the first-party cookies are not removed.
  For more details, please look at the screencast:
  https://drive.google.com/file/d/1-gbji5YaiV2wwawEKEuLFKQELvw23L7I/view

Additional notes

• in step 5 - mc.yandex.ru as a third-party cookie should also be removed. Should I file a new bug for this particular case?

Comment 1

[Steven Englehardt [:englehardt]]

(In reply to Simona Badau from comment #0)

Expected result

• in step 5 - the third-party cookies from the Disconnect Tracking Protection list should be removed.
• in step 7 - the first-party cookies from the Disconnect Tracking Protection list should be removed.

I expect both tracking cookies that have received user interaction in the first-party context and those that have not to be purged when you run Services.obs.notifyObservers(null, "idle-daily"); (i.e., after step 5). This is the notification that will trigger purging during normal operation.

Additional notes

• in step 5 - mc.yandex.ru as a third-party cookie should also be removed. Should I file a new bug for this particular case?

Yes please! As long as you didn't interact with mc.yandex.ru then it's likely a separate issue. That origin is on the disconnect list and your screencast does have a cookie set so it should be cleared. I suspect this might happen because yandex.ru has interaction.

Comment 2

[Steven Englehardt [:englehardt]]

If I set the prefs as described in Comment 0 and visit google.com, perform a search, close the tab, and then open the browser console and run await Components.classes["@mozilla.org/purge-tracker-service;1"].getService(Components.interfaces.nsIPurgeTrackerService).purgeTrackingCookieJars() I see:

22:23:53.955 *** PurgeTrackerService:: Purging trackers enabled, beginning batch. PurgeTrackerService.jsm:283:16
22:23:53.965 *** PurgeTrackerService:: tracking cookie found with no interaction permission, deleting related data. doubleclick.net PurgeTrackerService.jsm:283:16
22:23:53.973 *** PurgeTrackerService:: Data deleted from:  doubleclick.net PurgeTrackerService.jsm:283:16
22:23:53.974 *** PurgeTrackerService:: All cookie purging finished, resetting list until tomorrow.

if I then restart the browser and re-run the purgeTrackingCookieJars(); command I see:

22:26:18.947 *** PurgeTrackerService:: Purging trackers enabled, beginning batch. PurgeTrackerService.jsm:283:16
22:26:18.957 *** PurgeTrackerService:: tracking cookie found with no interaction permission, deleting related data. google.com PurgeTrackerService.jsm:283:16
22:26:18.967 *** PurgeTrackerService:: Data deleted from:  google.com PurgeTrackerService.jsm:283:16
22:26:18.968 *** PurgeTrackerService:: tracking cookie found with no interaction permission, deleting related data. google.com PurgeTrackerService.jsm:283:16
22:26:18.974 *** PurgeTrackerService:: Data deleted from:  google.com PurgeTrackerService.jsm:283:16
22:26:18.975 *** PurgeTrackerService:: tracking cookie found with no interaction permission, deleting related data. google.com PurgeTrackerService.jsm:283:16
22:26:18.980 *** PurgeTrackerService:: Data deleted from:  google.com PurgeTrackerService.jsm:283:16
22:26:18.980 *** PurgeTrackerService:: tracking cookie found with no interaction permission, deleting related data. google.com PurgeTrackerService.jsm:283:16
22:26:18.985 *** PurgeTrackerService:: Data deleted from:  google.com PurgeTrackerService.jsm:283:16
22:26:18.987 *** PurgeTrackerService:: tracking cookie found with no interaction permission, deleting related data. google.com PurgeTrackerService.jsm:283:16
22:26:18.991 *** PurgeTrackerService:: Data deleted from:  google.com PurgeTrackerService.jsm:283:16
22:26:18.992 *** PurgeTrackerService:: tracking cookie found with no interaction permission, deleting related data. www.google.com PurgeTrackerService.jsm:283:16
22:26:19.035 undefined
22:26:19.033 *** PurgeTrackerService:: Data deleted from:  www.google.com PurgeTrackerService.jsm:283:16
22:26:19.035 *** PurgeTrackerService:: All cookie purging finished, resetting list until tomorrow. PurgeTrackerService.jsm:283:16

Comment 3

[Steven Englehardt [:englehardt]]

Ni Johann to get this on his radar.

Comment 4

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

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

Comment 5

[Johann Hofmann [:johannh]]

Yup, it looks like the permission manager doesn't do expiration checks for getAllWithTypePrefix and getAllForPrincipal 🤦‍♂️

Comment 6

[Johann Hofmann [:johannh]]

Baku, is this fixed with your recent refactorings?

Comment 8

[Andrea Marchesini [:baku]]

No, it's not. This bug is all yours if you want.

Comment 9

[Johann Hofmann [:johannh]]

Attachment: Bug 1627220 - Consider quota manager activity for cookie purging. r=ewright

We're using nsIStorageActivityService to get only principals that were using storage in the last 3 days.

A few notes on that:

• 3 days is based on the assumption that it's very unlikely that a client would miss idle daily for 3 days in a row.

• We're currently only persisting activity for up to 24 hours. Bug 1630598 tracks extension to 3 days.

• We're currently not persisting storage activity service across restarts. This is bug 1459974 which we're aiming to resolve.

• This will not immediately clear all old tracking storage, only when it is used another time. In the same vein
  there's a chance the we miss clearing if the user manages to persistently have Firefox sessions that are so short
  that idle-daily is rarely triggered. This would be problematic for cookie purging in general, though.

• This produces a significantly lower number of principals to check. We could consider switching cookies
  to the same approach (only get the last x days of activity).

I talked to Steve Englehardt and we're generally okay with these caveats in favor of the simplified implementation.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9cb0695cee1f
Consider quota manager activity for cookie purging. r=ewright

Comment 11

[Johann Hofmann [:johannh]]

Ok, duh, I submitted the patch for bug 1619625 on this bug instead :(

I'll rename this bug and file a new one, and mark bug 1619625 as dupe 😬

Comment 12

[Johann Hofmann [:johannh]]

Or ask for a backout, maybe that's better to avoid confusing QA here...

Comment 13

[Dorel Luca [:dluca]]

Backed out changeset 9cb0695cee1f (bug 1627220) on dev's request

Backout:
https://hg.mozilla.org/integration/autoland/rev/ddeeb9ce1712782fe5acc81ef4b0e20cfae15728

Comment 14

[Phabricator Automation]

Comment on attachment 9140992 [details]
Bug 1627220 - Consider quota manager activity for cookie purging. r=ewright

Revision D71173 was moved to bug 1619625. Setting attachment 9140992 [details] to obsolete.

Comment 15

[Johann Hofmann [:johannh]]

Attachment: Bug 1627220 - Skip expired permissions in getAllWithTypePrefix and getAllForPrincipal. r=baku

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1f668b65e69
Skip expired permissions in getAllWithTypePrefix and getAllForPrincipal. r=baku

Comment 17

[Daniel Varga [:dvarga]]

Backed out changeset b1f668b65e69 (Bug 1627220) for causing xpcshell failures at extensions/permissions/test/unit/test_permmanager_expiration.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=299570179&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=b1f668b65e691fa492b2a73f2b505a8f78a83fd3

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299568461&repo=autoland&lineNumber=4294

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=dc41492033491f409344d26684c2de16db767900

[task 2020-04-27T14:26:22.648Z] 14:26:22     INFO -  TEST-START | extensions/permissions/test/unit/test_permmanager_expiration.js
[task 2020-04-27T14:26:22.938Z] 14:26:22  WARNING -  TEST-UNEXPECTED-FAIL | extensions/permissions/test/unit/test_permmanager_expiration.js | xpcshell return code: 0
[task 2020-04-27T14:26:22.939Z] 14:26:22     INFO -  TEST-INFO took 308ms
[task 2020-04-27T14:26:22.939Z] 14:26:22     INFO -  >>>>>>>
[task 2020-04-27T14:26:22.939Z] 14:26:22     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2020-04-27T14:26:22.939Z] 14:26:22     INFO -  (xpcshell/head.js) | test pending (2)
[task 2020-04-27T14:26:22.939Z] 14:26:22     INFO -  TEST-PASS | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 84] 1 == 1
[task 2020-04-27T14:26:22.939Z] 14:26:22     INFO -  TEST-PASS | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 88] 1 == 1
[task 2020-04-27T14:26:22.939Z] 14:26:22     INFO -  TEST-PASS | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 92] 1 == 1
[task 2020-04-27T14:26:22.939Z] 14:26:22     INFO -  TEST-PASS | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 96] 1 == 1
[task 2020-04-27T14:26:22.940Z] 14:26:22     INFO -  TEST-PASS | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 97] 1 == 1
[task 2020-04-27T14:26:22.940Z] 14:26:22     INFO -  TEST-PASS | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 101] 1 == 1
[task 2020-04-27T14:26:22.940Z] 14:26:22     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2020-04-27T14:26:22.940Z] 14:26:22     INFO -  running event loop
[task 2020-04-27T14:26:22.940Z] 14:26:22     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2020-04-27T14:26:22.943Z] 14:26:22     INFO -  TEST-PASS | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 106] 0 == 0
[task 2020-04-27T14:26:22.944Z] 14:26:22     INFO -  TEST-PASS | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 110] 0 == 0
[task 2020-04-27T14:26:22.944Z] 14:26:22  WARNING -  TEST-UNEXPECTED-FAIL | extensions/permissions/test/unit/test_permmanager_expiration.js | do_run_test - [do_run_test : 115] 5 == 3
[task 2020-04-27T14:26:22.944Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/tests/extensions/permissions/test/unit/test_permmanager_expiration.js:do_run_test:115
[task 2020-04-27T14:26:22.944Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/tests/extensions/permissions/test/unit/head.js:do_run_generator:11
[task 2020-04-27T14:26:22.944Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/tests/extensions/permissions/test/unit/test_permmanager_expiration.js:continue_test:13
[task 2020-04-27T14:26:22.944Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/head.js:notify:215
[task 2020-04-27T14:26:22.944Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/head.js:_do_main:242
[task 2020-04-27T14:26:22.944Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/head.js:_execute_test:569
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  -e:null:1
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  exiting test
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  caught exception [Exception... "Abort"  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "JS frame :: /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/head.js :: _abort_failed_test :: line 787"  data: no]
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/tests/extensions/permissions/test/unit/test_permmanager_expiration.js:continue_test:13
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/head.js:notify:215
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/head.js:_do_main:242
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  /Users/cltbld/tasks/task_1587995910/build/tests/xpcshell/head.js:_execute_test:569
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  -e:null:1
[task 2020-04-27T14:26:22.945Z] 14:26:22     INFO -  exiting test

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9525dffbe6e
Backed out changeset b1f668b65e69 for causing xpcshell failures at extensions/permissions/test/unit/test_permmanager_expiration.js

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3a579876ab7
Skip expired permissions in getAllWithTypePrefix and getAllForPrincipal. r=baku

Comment 20

[Stefan Hindli [:stefan_hindli]]

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

Comment 21

[Simona Badau, Desktop QA]

Verified as fixed on the latest Nightly 77.0a1 - on Windows 10 x64, Ubuntu 18.04, and Mac OS X 10.15 - I can confirm that both tracking cookies that have received user interaction in the first-party context and those that have not are being purged.