Closed Bug 1670811 Opened 6 months ago Closed 5 months ago

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

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(firefox84 fixed)

RESOLVED FIXED
84 Branch
Tracking Status
firefox84 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 1 obsolete file)

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

Depends on D93275

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

Attachment #9181132 - Attachment description: Bug 1670811 - :Basic browsingData.remove cookieStoreId support for clearing cookies → Bug 1670811 - Basic browsingData.remove cookieStoreId support for clearing cookies

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.

Severity: -- → N/A
Priority: -- → P2
Attachment #9181259 - Attachment description: Bug 1670811 - Tests → Bug 1670811 - Tests for browsingData.remove with cookieStoreId
Pushed by evilpies@gmail.com:
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
Flags: needinfo?(evilpies)

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

(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:

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.

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.

Flags: needinfo?(evilpies)
Pushed by evilpies@gmail.com:
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

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.

Flags: needinfo?(evilpies)
Pushed by evilpies@gmail.com:
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

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

Attachment #9186059 - Attachment is obsolete: true

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

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

Regressions: 1675643
Summary: Extend browsingData.remove API to remove a subset of data with cookieStoreId for contextualIdentities → Extend browsingData.remove API to remove a subset of data with cookieStoreId
Keywords: dev-doc-needed

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.

Flags: needinfo?(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).

Flags: needinfo?(lgreco)
You need to log in before you can comment on or make changes to this bug.