1670811 - Extend browsingData.remove API to remove a subset of data with cookieStoreId

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Tom S]

See bug 1353726 for details.

In this bug I plan on adding support for removing cookies, indexedDB and localStorage by cookiesStoreId. We will only support next-gen storage for localStorage, so that means nightly/early-beta.

In my current implementation I also limited cookieStoreId to those used for contextualIdentities/containers (no private/default store).

Comment 1

[Tom S]

Attachment: Bug 1670811 - Basic browsingData.remove cookieStoreId support for clearing cookies

Comment 2

[Tom S]

Attachment: Bug 1670811 - Unify indexedDB and localStorage code

Depends on D93275

Comment 3

[Tom S]

Attachment: Bug 1670811 - browsingData.remove cookieStoreId support for next-gen localStorage and indexedDB

Depends on D93276

Comment 4

[Tom S]

I still need to write tests for this. There is also an open question about requiring additional permissions.

Comment 5

[Tom S]

Attachment: Bug 1670811 - Tests for browsingData.remove with cookieStoreId

Depends on D93277

Comment 6

[Tom S]

We decided to change cookieStoreId to a more usual behavior. It now also supports the values firefox-default and firefox-private. However indexedDB and localStorage don't support firefox-private, because it's not currently supported by the platform.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3175566381a6
Basic browsingData.remove cookieStoreId support for clearing cookies r=extension-reviewers,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/ef9da168cdfd
Unify indexedDB and localStorage code r=rpl
https://hg.mozilla.org/integration/autoland/rev/3de473631613
browsingData.remove cookieStoreId support for next-gen localStorage and indexedDB r=rpl
https://hg.mozilla.org/integration/autoland/rev/26b4582e47ae
Tests for browsingData.remove with cookieStoreId r=extension-reviewers,mixedpuppy,rpl

Comment 8

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest failures on test_ext_cookies_containers.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/43e06e876845cbc7bf3ce05565ac8516592a64e9
Log link: https://treeherder.mozilla.org/logviewer?job_id=320478832&repo=autoland&lineNumber=2857

Please also check failures on browser_ext_browsingData_localStorage.js -> https://treeherder.mozilla.org/logviewer?job_id=320483457&repo=autoland&lineNumber=2345

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Narcis Beleuzu [:NarcisB] from comment #8)

Backed out for mochitest failures on test_ext_cookies_containers.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/43e06e876845cbc7bf3ce05565ac8516592a64e9
Log link: https://treeherder.mozilla.org/logviewer?job_id=320478832&repo=autoland&lineNumber=2857

Hi Tom, I haven't digged into the failure logs yet, but the failure on test_ext_cookies_containers.html seems to be an android-only failure (I though to mention you it in case you didn't looked into it yet).

Please also check failures on browser_ext_browsingData_localStorage.js -> https://treeherder.mozilla.org/logviewer?job_id=320483457&repo=autoland&lineNumber=2345

The asan failure on browser_ext_browsingData_localStorage.js looks like a pre-existing intermittent, see Bug 1389349 (I haven't checked if there is any actual change in terms of frequency of the pre-existing intermittent failure).

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #9)

(In reply to Narcis Beleuzu [:NarcisB] from comment #8)

Backed out for mochitest failures on test_ext_cookies_containers.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/43e06e876845cbc7bf3ce05565ac8516592a64e9
Log link: https://treeherder.mozilla.org/logviewer?job_id=320478832&repo=autoland&lineNumber=2857

Hi Tom, I haven't digged into the failure logs yet, but the failure on test_ext_cookies_containers.html seems to be an android-only failure (I though to mention you it in case you didn't looked into it yet).

I'm pretty sure that the failure is due to browsingData not being supported on Android, to be fair we don't officially support containers on Fenix (and we didn't even support it on Fennec) and so the simplest option may be to just skip the entire test file on Android builds, you can see that error by searching for Warning processing permissions: Error processing permissions.1: Value "browsingData" must either: in the logcat logs here:

• https://firefoxci.taskcluster-artifacts.net/fugtU8UJQRWkymaWBjp-6g/0/public/test_info//logcat-emulator-5554.log

If we do want to keep the rest of the test to run on Android even if we don't officially support the containers feature on Fenix yet, the simplest option would be to split the test case for browsingData into a separate add_task and skip only that task on Android builds.

Comment 11

[Tom S]

I ported the tests to new test suite, with a lot of stuff copied over from browser/components/extensions/test/xpcshell/test_ext_browsingData_cookies_cache.js. While adding this test I uncovered a preexisting issue. When any option like for example hostnames is specified we would iterate through Services.cookies.cookies. This array doesn't contain private cookies.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e6562022528
Basic browsingData.remove cookieStoreId support for clearing cookies r=extension-reviewers,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/3ce25bc93634
Unify indexedDB and localStorage code r=rpl
https://hg.mozilla.org/integration/autoland/rev/edde0caa4fa2
browsingData.remove cookieStoreId support for next-gen localStorage and indexedDB r=rpl
https://hg.mozilla.org/integration/autoland/rev/d8c8748e2368
Tests for browsingData.remove with cookieStoreId r=extension-reviewers,mixedpuppy,rpl

Comment 13

[Dorel Luca [:dluca]]

Backed out 4 changesets (bug 1670811) for Browser-chrome failures in extensions/test/browser/browser_ext_browsingData_localStorage.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=320586540&repo=autoland&lineNumber=2389

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=d8c8748e23681296796be1bbba43fc5a56c4e566

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

Comment 14

[Tom S]

Try push with this fixed: https://treeherder.mozilla.org/jobs?repo=try&revision=f2ab22042ab5c3a3c769167195a805a8116e8538.

We think what is happening here is that all content-script load and the background script finishes before all three tabs are completely loaded. Thus we hang forever waiting for the background script.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73a97d32f0fd
Basic browsingData.remove cookieStoreId support for clearing cookies r=extension-reviewers,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6929e9831029
Unify indexedDB and localStorage code r=rpl
https://hg.mozilla.org/integration/autoland/rev/9ab0d5968405
browsingData.remove cookieStoreId support for next-gen localStorage and indexedDB r=rpl
https://hg.mozilla.org/integration/autoland/rev/12e386f1f541
Tests for browsingData.remove with cookieStoreId r=extension-reviewers,mixedpuppy,rpl

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/73a97d32f0fd
https://hg.mozilla.org/mozilla-central/rev/6929e9831029
https://hg.mozilla.org/mozilla-central/rev/9ab0d5968405
https://hg.mozilla.org/mozilla-central/rev/12e386f1f541

Comment 17

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Backed out 4 changesets (Bug 1670811) for Browser-chrome failures in extensions/test/browser/browser_ext_browsingData_localStorage.js. CLOSED TREE

Backed out changeset d8c8748e2368 (bug 1670811)
Backed out changeset edde0caa4fa2 (bug 1670811)
Backed out changeset 3ce25bc93634 (bug 1670811)
Backed out changeset 2e6562022528 (bug 1670811)

Comment 18

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Apologies, did a bad push from moz-phab, ignore the above.

Comment 19

[gwarser]

Is dom.storage.next_gen set to false still supported? Because after this bug landed Cookie AutoDelete add-on started to throw when cleaning localStorage - https://bugzilla.mozilla.org/show_bug.cgi?id=1675643

Comment 20

[Tom S]

First draft:
https://github.com/mdn/browser-compat-data/pull/7313
https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browsingData/RemovalOptions$revision/1652552

Comment 21

[Caitlin Neiman [:caitmuenster]]

Thanks, Tom!

Luca, can you take a quick look at https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browsingData/RemovalOptions$revision/1652552 to make sure it looks ok?

I'll check in with the MDN folks about the next push for the browser compat tables.

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Caitlin Neiman [:caitmuenster] from comment #21)

Thanks, Tom!

Luca, can you take a quick look at https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browsingData/RemovalOptions$revision/1652552 to make sure it looks ok?

Looks good to me too.

(as a side note I'm wondering how we could remind our future selves to remove that note about nightly when the LSNG feature will be eventually enabled on nightly, it may be reasonable to file a bugzilla issue just for updated that in the docs and make it blocked on Bug 1599979).