Use a single module for history and site data cleanup/sanitization

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: johannh, Assigned: baku)

Tracking

(Blocks 2 bugs)

58 Branch
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [storage-v2])

Attachments

(18 attachments, 29 obsolete attachments)

3.95 KB, patch
johannh
: review+
Details | Diff | Splinter Review
4.78 KB, patch
johannh
: review+
Details | Diff | Splinter Review
3.66 KB, patch
johannh
: review+
Details | Diff | Splinter Review
3.50 KB, patch
johannh
: review+
Details | Diff | Splinter Review
10.62 KB, patch
johannh
: review+
Details | Diff | Splinter Review
19.75 KB, patch
johannh
: review+
Details | Diff | Splinter Review
13.12 KB, patch
johannh
: review+
Details | Diff | Splinter Review
14.48 KB, patch
johannh
: review+
Details | Diff | Splinter Review
6.40 KB, patch
johannh
: review+
Details | Diff | Splinter Review
6.67 KB, patch
johannh
: review+
Details | Diff | Splinter Review
7.90 KB, patch
johannh
: review+
Details | Diff | Splinter Review
13.54 KB, patch
johannh
: review+
Details | Diff | Splinter Review
4.69 KB, patch
johannh
: review+
Details | Diff | Splinter Review
2.38 KB, patch
johannh
: review+
Details | Diff | Splinter Review
7.36 KB, patch
johannh
: review+
Details | Diff | Splinter Review
19.58 KB, patch
johannh
: review+
Details | Diff | Splinter Review
3.78 KB, patch
johannh
: review+
Details | Diff | Splinter Review
7.48 KB, patch
johannh
: review+
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
We currently use at least three larger modules that are primarily concerned with cleaning up user data in Desktop Firefox alone:

https://searchfox.org/mozilla-central/source/browser/base/content/sanitize.js
https://searchfox.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm
https://searchfox.org/mozilla-central/source/browser/components/preferences/SiteDataManager.jsm

Additionally there are smaller pieces of code that do this independently, without calling into either module.

Though there is a distinction that e.g. ForgetAboutSite.jsm is about clearing data specific to a certain site while sanitize.js is about clearing all data or clearing by time range, I believe it's counterproductive and dangerous to duplicate this sort of code in general.

Ideally we could have one module live in Toolkit that takes care of both tasks (clearing by time and clearing by site) and is well tested and complete. This would hopefully help reduce issues such as bug 1047098.
Ideally it would also be possible to use this for Android as well to avoid things like bug 1415342, although I've got no idea how feasible that would be.

While large parts of the sanitization implementations are more or less identical (the fix for bug 1415342 e.g. consists of basically pasting the current desktop implementation for clearing offlineApps into the Android sanitizer), there are some areas with possibly genuine differences between Desktop and Android
(Reporter)

Updated

a year ago
Blocks: storage-v2
Priority: -- → P3
Whiteboard: [storage-v2][triage] → [storage-v2]
(Reporter)

Updated

a year ago
See Also: → 1152448
(Reporter)

Updated

a year ago
Depends on: 1435996
Likely could be out of scope however "clear-origin-attributes-data" appears to be everywhere too and this tries to achieve similar goals of forgetting a site.
(Reporter)

Comment 4

a year ago
Yeah good point, there's also "browser:purge-domain-data"...
(Reporter)

Updated

a year ago
Component: General → Data Sanitization
Product: Firefox → Toolkit
(Assignee)

Comment 5

11 months ago
Posted patch part 1 - nsIClearDataService (obsolete) — Splinter Review
The first patch is the just about the interface and a JS file that will contain the implementation. I just realized that, using C++ for the implementation will require a lot of effort and not big benefits.
But having a xpcom interface will allow us to call this interface from C++. This is needed for the Clear-Site-Data project.
Assignee: nobody → amarchesini
Attachment #8976236 - Flags: review?(jhofmann)
Attachment #8976236 - Flags: feedback?(mak77)
(Assignee)

Comment 6

11 months ago
This is what I have. Nothing more than cache and cookies.
There are a couple of tests for cookies and for the interface.

If we like the current approach, I'm going to replace cookies and cache in ForgetAboutSite and Sanitizer.jsm.
Attachment #8976237 - Flags: review?(jhofmann)
(Assignee)

Comment 7

11 months ago
Here the component integrated with Sanitizer.jsm and ForgetAboutSite.
Attachment #8976237 - Attachment is obsolete: true
Attachment #8976237 - Flags: review?(jhofmann)
Attachment #8976245 - Flags: review?(jhofmann)
(Assignee)

Comment 8

11 months ago
Better approach.
Attachment #8976245 - Attachment is obsolete: true
Attachment #8976245 - Flags: review?(jhofmann)
Attachment #8976414 - Flags: review?(jhofmann)
(Assignee)

Comment 9

11 months ago
Posted patch part 3 - plugin data (obsolete) — Splinter Review
Attachment #8976415 - Flags: review?(jhofmann)
(Assignee)

Comment 10

11 months ago
Posted patch part 4 - downloads (obsolete) — Splinter Review
A follow up would be to fix this component (and probably forgetaboutsite + sanitizer.jsm) for file URLs: principal.URL.host is an empty string for file URLs.
Attachment #8976472 - Flags: review?(jhofmann)
(Assignee)

Comment 11

11 months ago
Posted patch part 5 - passwords (obsolete) — Splinter Review
Attachment #8976474 - Flags: review?(jhofmann)
(Assignee)

Comment 12

11 months ago
Posted patch part 6 - Media devices (obsolete) — Splinter Review
I don't know if it's possible to add a test here.
Attachment #8976475 - Flags: review?(jhofmann)
(Assignee)

Comment 13

11 months ago
Posted patch part 7 - appCache (obsolete) — Splinter Review
Attachment #8976486 - Flags: review?(jhofmann)
(Assignee)

Comment 14

11 months ago
Posted patch part 8 - DOM Quota (obsolete) — Splinter Review
localStorage (bug 1286798), serviceWorkers (bug 1183245) are going to be merged in QuotaManager soon, no needs to have separate flags.
Attachment #8976487 - Flags: review?(jhofmann)

Comment 15

11 months ago
Er, where does that leave Fennec's sanitizer? As far as I can see, this means that desktop and mobile Sanitizer.jsm are drifting out of sync again.
(Assignee)

Comment 16

11 months ago
Posted patch part 1 - nsIClearDataService (obsolete) — Splinter Review
As discussed on IRC, I added 'aIsUserRequest' parameter to decide if use deleteData() as fallback in case the cleaner doesn't support deleting data by principal nor time range.
Attachment #8976236 - Attachment is obsolete: true
Attachment #8976236 - Flags: review?(jhofmann)
Attachment #8976236 - Flags: feedback?(mak77)
Attachment #8976605 - Flags: review?(jhofmann)
Attachment #8976605 - Flags: feedback?(mak77)
(Assignee)

Comment 17

11 months ago
Attachment #8976414 - Attachment is obsolete: true
Attachment #8976414 - Flags: review?(jhofmann)
Attachment #8976606 - Flags: review?(jhofmann)
(Assignee)

Comment 18

11 months ago
Posted patch part 3 - plugin data (obsolete) — Splinter Review
Attachment #8976415 - Attachment is obsolete: true
Attachment #8976415 - Flags: review?(jhofmann)
Attachment #8976607 - Flags: review?(jhofmann)
(Assignee)

Comment 19

11 months ago
Posted patch part 4 - downloads (obsolete) — Splinter Review
Attachment #8976472 - Attachment is obsolete: true
Attachment #8976472 - Flags: review?(jhofmann)
Attachment #8976608 - Flags: review?(jhofmann)
(Assignee)

Comment 20

11 months ago
Posted patch part 5 - passwords (obsolete) — Splinter Review
Attachment #8976474 - Attachment is obsolete: true
Attachment #8976474 - Flags: review?(jhofmann)
Attachment #8976609 - Flags: review?(jhofmann)
(Assignee)

Comment 21

11 months ago
Attachment #8976475 - Attachment is obsolete: true
Attachment #8976475 - Flags: review?(jhofmann)
Attachment #8976610 - Flags: review?(jhofmann)
(Assignee)

Comment 22

11 months ago
Posted patch part 7 - appCache (obsolete) — Splinter Review
Attachment #8976486 - Attachment is obsolete: true
Attachment #8976486 - Flags: review?(jhofmann)
Attachment #8976611 - Flags: review?(jhofmann)
(Assignee)

Comment 23

11 months ago
Posted patch part 8 - DOM Quota (obsolete) — Splinter Review
Attachment #8976487 - Attachment is obsolete: true
Attachment #8976487 - Flags: review?(jhofmann)
Attachment #8976612 - Flags: review?(jhofmann)
(Assignee)

Comment 24

11 months ago
Attachment #8976615 - Flags: review?(jhofmann)
(Assignee)

Comment 25

11 months ago
Posted patch part A - push notification (obsolete) — Splinter Review
Attachment #8976616 - Flags: review?(jhofmann)
(Assignee)

Comment 26

11 months ago
Posted patch part B - history (obsolete) — Splinter Review
Attachment #8976618 - Flags: review?(jhofmann)
(Assignee)

Comment 27

11 months ago
Attachment #8976619 - Flags: review?(jhofmann)
(Assignee)

Comment 28

11 months ago
Attachment #8976620 - Flags: review?(jhofmann)
(Assignee)

Comment 29

11 months ago
Posted patch part E - logins (obsolete) — Splinter Review
Attachment #8976621 - Flags: review?(jhofmann)
(Assignee)

Comment 30

11 months ago
Posted patch part F - HSTS and HPKP (obsolete) — Splinter Review
Attachment #8976622 - Flags: review?(jhofmann)
(Assignee)

Comment 31

11 months ago
Attachment #8976624 - Flags: review?(jhofmann)
(Assignee)

Comment 32

11 months ago
Posted patch part H - Security settings (obsolete) — Splinter Review
Attachment #8976625 - Flags: review?(jhofmann)
(Assignee)

Comment 33

11 months ago
Posted patch part I - EME (obsolete) — Splinter Review
Attachment #8976627 - Flags: review?(jhofmann)
(Assignee)

Comment 34

11 months ago
ForgetAboutSite doesn't delete all. I don't want to change the current behavior with this set of patches. We can discuss if we want to delete more or less in a follow ups.
Attachment #8976628 - Flags: review?(jhofmann)
(Assignee)

Comment 35

11 months ago
deleteByHost is required by ForgetAboutSite. This patch should be green on try. The previous one had a couple of failing tests.
Attachment #8976605 - Attachment is obsolete: true
Attachment #8976605 - Flags: review?(jhofmann)
Attachment #8976605 - Flags: feedback?(mak77)
Attachment #8976917 - Flags: review?(jhofmann)
Attachment #8976917 - Flags: feedback?(mak77)
(Assignee)

Comment 36

11 months ago
Attachment #8976606 - Attachment is obsolete: true
Attachment #8976606 - Flags: review?(jhofmann)
Attachment #8976918 - Flags: review?(jhofmann)
(Assignee)

Comment 37

11 months ago
Attachment #8976607 - Attachment is obsolete: true
Attachment #8976607 - Flags: review?(jhofmann)
Attachment #8976919 - Flags: review?(jhofmann)
(Assignee)

Comment 38

11 months ago
Attachment #8976608 - Attachment is obsolete: true
Attachment #8976608 - Flags: review?(jhofmann)
Attachment #8976920 - Flags: review?(jhofmann)
(Assignee)

Comment 39

11 months ago
Posted patch part 5 - passwords (obsolete) — Splinter Review
Attachment #8976609 - Attachment is obsolete: true
Attachment #8976609 - Flags: review?(jhofmann)
Attachment #8976921 - Flags: review?(jhofmann)
(Assignee)

Comment 40

11 months ago
Attachment #8976611 - Attachment is obsolete: true
Attachment #8976611 - Flags: review?(jhofmann)
Attachment #8976923 - Flags: review?(jhofmann)
(Assignee)

Comment 41

11 months ago
Posted patch part 8 - DOM Quota (obsolete) — Splinter Review
Attachment #8976612 - Attachment is obsolete: true
Attachment #8976612 - Flags: review?(jhofmann)
Attachment #8976924 - Flags: review?(jhofmann)
(Assignee)

Comment 42

11 months ago
Attachment #8976616 - Attachment is obsolete: true
Attachment #8976616 - Flags: review?(jhofmann)
Attachment #8976925 - Flags: review?(jhofmann)
(Assignee)

Comment 43

11 months ago
Attachment #8976618 - Attachment is obsolete: true
Attachment #8976618 - Flags: review?(jhofmann)
Attachment #8976926 - Flags: review?(jhofmann)
(Assignee)

Comment 44

11 months ago
Posted patch part F - HSTS and HPKP (obsolete) — Splinter Review
Attachment #8976622 - Attachment is obsolete: true
Attachment #8976622 - Flags: review?(jhofmann)
Attachment #8976927 - Flags: review?(jhofmann)
(Assignee)

Comment 45

11 months ago
Attachment #8976624 - Attachment is obsolete: true
Attachment #8976624 - Flags: review?(jhofmann)
Attachment #8976928 - Flags: review?(jhofmann)
(Assignee)

Comment 46

11 months ago
Posted patch part I - EMESplinter Review
Attachment #8976627 - Attachment is obsolete: true
Attachment #8976627 - Flags: review?(jhofmann)
Attachment #8976929 - Flags: review?(jhofmann)
(Assignee)

Comment 47

11 months ago
Attachment #8976628 - Attachment is obsolete: true
Attachment #8976628 - Flags: review?(jhofmann)
Attachment #8976931 - Flags: review?(jhofmann)
(Assignee)

Updated

11 months ago
Attachment #8976929 - Attachment description: cleanup_I.patch → part I - EME
(Assignee)

Comment 48

11 months ago
Posted patch part 8 - DOM Quota (obsolete) — Splinter Review
Attachment #8976924 - Attachment is obsolete: true
Attachment #8976924 - Flags: review?(jhofmann)
Attachment #8976987 - Flags: review?(jhofmann)
(Assignee)

Updated

11 months ago
Blocks: 1268889
(Reporter)

Comment 49

11 months ago
Comment on attachment 8976917 [details] [diff] [review]
part 1 - nsIClearDataService

Review of attachment 8976917 [details] [diff] [review]:
-----------------------------------------------------------------

The interface looks very good to me, though I'd really count on mak to give his input here as well.

A couple of notes:

- I was hoping we could return Promises instead of using callbacks, but talking about this with baku we attested that if we want this to get adopted by both JS and C++ modules, using callbacks might be better.

- While it has a very similar API, ClearDataService probably doesn't need to entirely replace Sanitizer.jsm ever. Sanitizer can still keep track of sanitizeOnShutdown and the other pref mechanics that this doesn't need to know about and just defer to this module for the actual clearing. That sounds like a clean separation of concerns to me but again, mak may have some thoughts on it.

- I would count on this module to replace all other uses of Sanitizer and ForgetAboutSite (e.g. the Forget button or Right Click -> Forget About Site)

- We probably want to define this on Services.jsm to enable easier usage.

I'll take a look at the other patches and will probably defer some of them to domain experts.

Thank you for all this work!

::: toolkit/components/cleardata/nsIClearDataService.idl
@@ +26,5 @@
> +   *        to remove more than less, for privacy reason. If false (e.g.
> +   *        Clear-Site-Data header), we don't want to delete more than what is
> +   *        strictly required.
> +   * @param aFlags List of flags. See below the accepted values.
> +   * @param aCallback ths callback will be executed when the operation is

spelling: *this* callback (also below)

@@ +128,5 @@
> +   */
> +
> +  /**
> +   * Delete all the possible caches.
> +   * TODO: add CLEAR_PREDICTOR_CACHE ?

I'm not a Necko expert but that sounds reasonable to me.

@@ +133,5 @@
> +   */
> +  const uint32_t CLEAR_ALL_CACHES = CLEAR_NETWORK_CACHE | CLEAR_IMAGE_CACHE;
> +
> +  /*
> +  const uint32_t CLEAR_DOM_STORAGES = CLEAR_DOM_QUOTA | CLEAR_DOM_PUSH_NOTIFICATIONS | CLEAR_FORMDATA | CLEAR_SESSION_HISTORY;

I don't really understand why FormData is in here :)

@@ +137,5 @@
> +  const uint32_t CLEAR_DOM_STORAGES = CLEAR_DOM_QUOTA | CLEAR_DOM_PUSH_NOTIFICATIONS | CLEAR_FORMDATA | CLEAR_SESSION_HISTORY;
> +  */
> +
> +  /*
> +  const uint32_t CLEAR_BROWSER_DATA = CLEAR_DOWNLOADS | CLEAR_PASSWORDS | CLEAR_PERMISSIONS | CLEAR_CONTENT_PREFERENCES | CLEAR_HISTORY | CLEAR_LOGINS;

Hm, what use case are you trying to cover here? We might want to give this a better name (or remove it). Something like CLEAR_USER_DATA? CLEAR_USER_DEFINED_DATA? CLEAR_PERSISTENT_USER_DATA?

Also, might CLEAR_AUTH_TOKENS also be part of this?
Attachment #8976917 - Flags: review?(jhofmann) → review+
Comment on attachment 8976917 [details] [diff] [review]
part 1 - nsIClearDataService

Review of attachment 8976917 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/cleardata/ClearDataService.js
@@ +7,5 @@
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", this);
> +
> +this.ClearDataService = function() {};
> +
> +ClearDataService.prototype = Object.freeze({

If this is supposed to work as a service (singleton), I'd suggest to add _xpcom_factory: XPCOMUtils.generateSingletonFactory(ClearDataService),

::: toolkit/components/cleardata/nsIClearDataService.idl
@@ +29,5 @@
> +   * @param aFlags List of flags. See below the accepted values.
> +   * @param aCallback ths callback will be executed when the operation is
> +   *                  completed.
> +   */
> +  void deleteDataFromHost(in AUTF8String aHost,

How is host defined here? For example, will passing "mozilla.org" clear also "fancysubdomain.mozilla.org"? What if I pass ".mozilla.org"? Are we supposed to be able to clear only a domain but not its subdomains (for example, history allows to clear "mozilla.org" but also ".mozilla.org" and only in the latter case it also clears subdomains)
It's probably worth documenting the host definition or expected format.

@@ +54,5 @@
> +
> +  /**
> +   * Delete all data in a time range
> +   * @param aFrom timestamp
> +   * @param aTo timestamp

The documentation should specify whether these limits are included or excluded

This says timestamp, but then takes a PRTime, maybe it should say microseconds from the epoch? I find timestamp confusing since it's commonly used for milliseconds.

@@ +117,5 @@
> +  const uint32_t CLEAR_LOGINS = 1 << 18;
> +  */
> +
> +  /**
> +   * Use this value to delete all the data.

This is a bit confusing since there is also a "delete all data" API that is DeleteData.
Maybe "Delete data from all of the above categories." or similar

@@ +133,5 @@
> +   */
> +  const uint32_t CLEAR_ALL_CACHES = CLEAR_NETWORK_CACHE | CLEAR_IMAGE_CACHE;
> +
> +  /*
> +  const uint32_t CLEAR_DOM_STORAGES = CLEAR_DOM_QUOTA | CLEAR_DOM_PUSH_NOTIFICATIONS | CLEAR_FORMDATA | CLEAR_SESSION_HISTORY;

Yeah, DOM Storage is a bit confusing since there is also a feature named like that, and form data and session history look extraneous then. 
Maybe CLEAR_WEBCONTENT_STORAGES?
does DOM_QUOTA include indexedDB?

@@ +153,5 @@
> +   * Called to indicate that the data cleaning is completed.
> +   * @param aFailedFlags this value contains the flags that failed during the
> +   *                     cleanup. If nothing failed, aFailedFlags will be 0.
> +   */
> +  void onDataDeleted(in uint32_t aFailedFlags);

There still one unclear case here, if I pass CLEAR_BROWSER_DATA and only CLEAR_HISTORY fails, what's in these flags, the former or the latter?
Attachment #8976917 - Flags: feedback?(mak77) → feedback+
(Reporter)

Comment 51

11 months ago
Comment on attachment 8976918 [details] [diff] [review]
part 2 - cookies/network cache/image cache

Review of attachment 8976918 [details] [diff] [review]:
-----------------------------------------------------------------

I have a couple of comments but I don't think I need to see this again. Thanks!

Was testing clearing cache too finicky?

Note that for cache you can easily replace the fennec version as well: https://searchfox.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm#79

Even cookies should work, since we treat plugin data separately in the new service and the comment notes that the only difference should be that we're not clearing plugin data: https://searchfox.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm#106

I wouldn't strictly require this to happen in this bug. I'll just make sure to note all the Fennec equivalents or differences in my review so that we'll have an easier time in a follow-up bug.

::: toolkit/components/cleardata/ClearDataService.js
@@ +17,5 @@
> +//                    not implemented, deleteAll() will be used as fallback.
> +// *deleteByRange() - this method is implemented only if the cleaner knows how
> +//                    to delete data by time range. It receives 2 timestamp
> +//                    parameters: aFrom/aTo. If not implemented, deleteAll() is
> +//                    used as fallback.

this seems like a reasonable way to structure the file. We could consider moving these cleaners into separate files in their own subdirectory at some point...

@@ +29,5 @@
> +    });
> +  },
> +
> +  deleteByRange(aFrom, aTo) {
> +    const enumerator = Services.cookies.enumerator;

nit: we generally use `let` for all regular inline variable declarations in frontend code, while const is reserved for actual "constants", so either

const ENUMERATOR = 

or

let enumerator = 

in which case the latter sounds more appropriate here :)

I can see that there are some advantages of using const over let (though it's also a bit confusing that const variables are still very much mutable), this is mostly to be consistent in our usage of const and let throughout the codebase.

@@ +47,5 @@
> +
> +    return new Promise((aResolve, aReject) => {
> +      let count = 0;
> +      while (aEnumerator.hasMoreElements()) {
> +        const cookie = aEnumerator.getNext().QueryInterface(Ci.nsICookie2);

let cookie

@@ +57,5 @@
> +            new Promise((aResolve, aReject) => {
> +              setTimeout(() => {
> +                this._deleteInternal(aEnumerator, aCb).then(aResolve, aReject);
> +              }, 0);
> +            }).then(aResolve, aReject);

Maybe I'm wrong but I think you can simplify this to 

setTimeout(() => {
  this._deleteInternal(aEnumerator, aCb).then(aResolve, aReject);
}, 0);

without the promise wrapper.

@@ +81,5 @@
> +
> +const ImageCacheCleaner = {
> +  deleteAll() {
> +    return new Promise(aResolve => {
> +      const imageCache = Cc["@mozilla.org/image/tools;1"]

let imageCache

@@ +116,5 @@
> +
> +    return this._deleteInternal(aFlags, aCallback, aCleaner => {
> +      // Some of the 'Cleaners' do not support to delete by principal. Let's
> +      // use deleteAll() as fallback.
> +      if ("deleteByHost" in aCleaner && aCleaner.deleteByHost) {

nit: is there any practical advantage of this over just 

if (aCleaner.deleteByHost) {

?

@@ +186,5 @@
> +  // promise object. All these promise objects are resolved before calling
> +  // onDataDeleted.
> +  _deleteInternal(aFlags, aCallback, aHelper) {
> +    let resultFlags = 0;
> +    const promises = FLAGS_MAP.filter(c => aFlags & c.flag).map(c => {

let promises

::: toolkit/components/cleardata/tests/unit/test_basic.js
@@ +1,1 @@
> +/**

nit: Last I remember we're putting license headers on all new tests, e.g.

/* Any copyright is dedicated to the Public Domain.
   http://creativecommons.org/publicdomain/zero/1.0/ */

::: toolkit/components/cleardata/tests/unit/test_cookies.js
@@ +1,1 @@
> +/**

License header

@@ +21,5 @@
> +    service.deleteData(Ci.nsIClearDataService.CLEAR_COOKIES, value => {
> +      Assert.equal(value, 0);
> +      aResolve();
> +    });
> +  });

Considering that we'll see a lot more of this, you should consider putting these promise wrappers as helper functions in a head.js file.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm
@@ +48,5 @@
>  
> +    ["http://", "https://"].forEach(scheme => {
> +      promises.push(new Promise(resolve => {
> +        const service = Cc["@mozilla.org/clear-data-service;1"]
> +                          .getService(Ci.nsIClearDataService);

I think this shows that it would be nice to have this defined on Services.jsm, so that we could do Services.clearData.deleteDataFromHost(
Attachment #8976918 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 52

11 months ago
Comment on attachment 8976920 [details] [diff] [review]
part 4 - downloads

Review of attachment 8976920 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but please see the comments (particularly the last one)

Thank you for writing tests as you go along!

::: toolkit/components/cleardata/ClearDataService.js
@@ +178,5 @@
> +const DownloadsCleaner = {
> +  deleteByHost(aHost, aOriginAttributes) {
> +    return Downloads.getList(Downloads.ALL).then(aList => {
> +      aList.removeFinished(aDownload => hasRootDomain(
> +        NetUtil.newURI(aDownload.source.url).host, aHost));

please use Services.io.newURI instead (also saves the netutil import)

@@ +319,5 @@
> + * current string.  For example, if this is "www.mozilla.org", and we pass in
> + * "mozilla.org", this will return true.  It would return false the other way
> + * around.
> + */
> +function hasRootDomain(str, aDomain) {

We are using this in a bunch of places now, it might be worth filing a bug for including this functionality in nsIEffectiveTLDService (which already has a method for getting the base domain).

::: toolkit/components/cleardata/nsIClearDataService.idl
@@ +111,2 @@
>    /* TODO
>    const uint32_t CLEAR_EME = 1 << 4;

Shouldn't this be 5 now?
Attachment #8976920 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 53

11 months ago
Comment on attachment 8976919 [details] [diff] [review]
part 3 - plugin data

Review of attachment 8976919 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/cleardata/ClearDataService.js
@@ +108,5 @@
> +    });
> +  },
> +
> +  deleteByRange(aFrom, aTo) {
> +    const age = Date.now() / 1000 - aFrom / 1000000;

let age

@@ +131,5 @@
> +            }
> +          });
> +        }
> +
> +        return true;

Why is this necessary?

@@ +155,5 @@
> +
> +    const promises = [];
> +    const tags = ph.getPluginTags();
> +    for (const tag of tags) {
> +      promises.push(aCb(ph, tag));

please "let" all variables above
Attachment #8976919 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 54

11 months ago
Comment on attachment 8976921 [details] [diff] [review]
part 5 - passwords

Review of attachment 8976921 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine but please implement clearing all passwords, otherwise usability of this module is significantly weakened IMO (since callers can not trust that the deleteAll fallback will be used).

::: toolkit/components/cleardata/ClearDataService.js
@@ +205,5 @@
> +  deleteByHost(aHost, aOriginAttributes) {
> +    return new Promise(aResolve => {
> +      try {
> +        // Clear all passwords for domain
> +        const logins = Services.logins.getAllLogins();

let logins

@@ +206,5 @@
> +    return new Promise(aResolve => {
> +      try {
> +        // Clear all passwords for domain
> +        const logins = Services.logins.getAllLogins();
> +        for (const login of logins) {

let login

@@ +214,5 @@
> +        }
> +      } catch (ex) {
> +        // XXXehsan: is there a better way to do this rather than this
> +        // hacky comparison?
> +        if (!ex.message.includes("User canceled Master Password entry")) {

gah

@@ +230,5 @@
> +  },
> +
> +  deleteAll() {
> +    // Not implemented.
> +    return Promise.resolve();

I think we should avoid breaking our own promise here (that this is always implemented) and just make a version that removes all logins, even if it's currently not used. This would also allow us to remove the empty deleteByRange definition.

::: toolkit/components/cleardata/nsIClearDataService.idl
@@ +116,2 @@
>    /* TODO
>    const uint32_t CLEAR_EME = 1 << 4;

I suspect the end result of this patch series will make sense here, right? Otherwise this should be 6.
Attachment #8976921 - Flags: review?(jhofmann) → review-
(Reporter)

Comment 55

11 months ago
Comment on attachment 8976610 [details] [diff] [review]
part 6 - Media devices

Review of attachment 8976610 [details] [diff] [review]:
-----------------------------------------------------------------

This in itself is fine, please see my comment about patch 2.

::: toolkit/components/cleardata/ClearDataService.js
@@ +234,5 @@
>  };
>  
> +const MediaDevicesCleaner = {
> +  deleteByRange(aFrom, aTo) {
> +    const mediaMgr = Cc["@mozilla.org/mediaManagerService;1"]

let

@@ +236,5 @@
> +const MediaDevicesCleaner = {
> +  deleteByRange(aFrom, aTo) {
> +    const mediaMgr = Cc["@mozilla.org/mediaManagerService;1"]
> +                       .getService(Ci.nsIMediaManagerService);
> +    mediaMgr.sanitizeDeviceIds(aFrom);

I think I missed something in the review for part 2.

Previously we had a try...catch {} block around this block that we're removing now (for cookies as well):

https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/browser/modules/Sanitizer.jsm#360-367

Since this is sync, assuming it throws an exception, if I'm reading the code correctly we're not actually ending up in this catch callback (because no promise gets rejected):

.catch(() => { resultFlags |= c.flag; });

I may be wrong, however. Can you try to verify my assumption, please? It may make sense to enclose all aCleaner.deleteByFoo calls with try..catch and reject when caught.

Another solution could be to force all Cleaner methods to have the async keyword by convention, which would also make returning with Promise.resolve() unnecessary.

@@ +241,5 @@
> +    return Promise.resolve();
> +  },
> +
> +  deleteAll() {
> +    const mediaMgr = Cc["@mozilla.org/mediaManagerService;1"]

let
Attachment #8976610 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 56

11 months ago
Comment on attachment 8976923 [details] [diff] [review]
part 7 - appCache

Review of attachment 8976923 [details] [diff] [review]:
-----------------------------------------------------------------

ForgetAboutSite doesn't clear AppCache? Ugh, that sounds familiar. We should probably file a bug for it...
Attachment #8976923 - Flags: review?(jhofmann) → review+

Comment 57

11 months ago
> ForgetAboutSite doesn't clear AppCache? Ugh, that sounds familiar. We should
> probably file a bug for it...

Bug 862465
(Reporter)

Comment 58

11 months ago
Comment on attachment 8976987 [details] [diff] [review]
part 8 - DOM Quota

Review of attachment 8976987 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine but I'd like to take another quick look at the patch after we resolved that catching part. 

This code would really look much nicer if written in async-await style :)

::: toolkit/components/cleardata/ClearDataService.js
@@ +278,5 @@
> +                                 aPrincipal.URI.host);
> +
> +    // ServiceWorkers: they must be removed before cleaning QuotaManager.
> +    return ServiceWorkerCleanUp.removeFromPrincipal(aPrincipal).catch(() => {})
> +      .catch(() => {})

There's an unneeded catch here. I'm also not sure it's a good idea to catch at all.

Looking at ServiceWorkerCleanup it doesn't seem to propagate failure downstream anyway. Why do we still need to catch then? We're running the risk of silencing e.g. unrelated JS errors. We could either:

1) Add a comment why we're catching this, or
2) Not catch at all, or
3) Catch with a temporary variable and reject that after quota removal is done.

I'd probably prefer 3)

@@ +295,5 @@
> +
> +    // ServiceWorkers: they must be removed before cleaning QuotaManager.
> +    return Promise.all([
> +      ServiceWorkerCleanUp.removeFromHost("http://" + aHost).catch(() => {}),
> +      ServiceWorkerCleanUp.removeFromHost("https://" + aHost).catch(() => {}),

See above.

@@ +296,5 @@
> +    // ServiceWorkers: they must be removed before cleaning QuotaManager.
> +    return Promise.all([
> +      ServiceWorkerCleanUp.removeFromHost("http://" + aHost).catch(() => {}),
> +      ServiceWorkerCleanUp.removeFromHost("https://" + aHost).catch(() => {}),
> +    ]).catch(() => {})

That would be an unnecessary catch.

@@ +304,5 @@
> +        let httpURI = NetUtil.newURI("http://" + aHost);
> +        let httpsURI = NetUtil.newURI("https://" + aHost);
> +        // Following code section has been reverted to the state before Bug 1238183,
> +        // but added a new argument to clearStoragesForPrincipal() for indicating
> +        // clear all storages under a given origin.

nit: this comment feels a little superfluous to me, we might as well remove it.

@@ +308,5 @@
> +        // clear all storages under a given origin.
> +        let httpPrincipal = Services.scriptSecurityManager
> +                                     .createCodebasePrincipal(httpURI, aOriginAttributes);
> +        let httpsPrincipal = Services.scriptSecurityManager
> +                                     .createCodebasePrincipal(httpsURI, aOriginAttributes);

nit: could consider not passing origin attributes (passing {} instead) to make it explicit that we don't intend to use them for deletion

@@ +323,5 @@
> +      });
> +  },
> +
> +  deleteByRange(aFrom, aTo) {
> +    const principals = sas.getActiveOrigins(aFrom, aTo)

let principals

@@ +326,5 @@
> +  deleteByRange(aFrom, aTo) {
> +    const principals = sas.getActiveOrigins(aFrom, aTo)
> +                          .QueryInterface(Ci.nsIArray);
> +
> +    const promises = [];

let

@@ +328,5 @@
> +                          .QueryInterface(Ci.nsIArray);
> +
> +    const promises = [];
> +    for (let i = 0; i < principals.length; ++i) {
> +      const principal = principals.queryElementAt(i, Ci.nsIPrincipal);

let

@@ +348,5 @@
> +    Services.obs.notifyObservers(null, "extension:purge-localStorage");
> +
> +    // ServiceWorkers
> +    return ServiceWorkerCleanUp.removeAll()
> +      .catch(() => {})

see notes above

@@ +355,5 @@
> +        return new Promise((aResolve, aReject) => {
> +          Services.qms.getUsage(aRequest => {
> +            if (aRequest.resultCode != Cr.NS_OK) {
> +              // We are probably shutting down. We don't want to propagate the
> +              // error, rejecting the promise.

aren't we resolving the promise?

@@ +362,5 @@
> +            }
> +
> +            const promises = [];
> +            for (let item of aRequest.result) {
> +              const principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(item.origin);

let

@@ +367,5 @@
> +              if (principal.URI.scheme == "http" ||
> +                  principal.URI.scheme == "https" ||
> +                  principal.URI.scheme == "file") {
> +                promises.push(new Promise(aResolve => {
> +                  const req = Services.qms.clearStoragesForPrincipal(principal, null, false);

let
Attachment #8976987 - Flags: review?(jhofmann) → review-
(Reporter)

Updated

11 months ago
Attachment #8976615 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 59

11 months ago
Comment on attachment 8976615 [details] [diff] [review]
part 9 - network predictor

Review of attachment 8976615 [details] [diff] [review]:
-----------------------------------------------------------------

(r=me assuming this is cleared in Sanitizer through CLEAR_ALL_CACHES)
(Reporter)

Comment 60

11 months ago
Comment on attachment 8976925 [details] [diff] [review]
part A - push notification

Review of attachment 8976925 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine considering it's just copy-paste from the other code, though you could consider making an _deleteInternal(hostName) function that de-dupes the logic in the two functions.

::: toolkit/components/cleardata/ClearDataService.js
@@ +374,5 @@
>  
> +const PushNotificationsCleaner = {
> +  deleteByHost(aHost, aOriginAttributes) {
> +    return new Promise(aResolve => {
> +      var push = Cc["@mozilla.org/push/Service;1"].

let

@@ +378,5 @@
> +      var push = Cc["@mozilla.org/push/Service;1"].
> +                 getService(Ci.nsIPushService);
> +      push.clearForDomain(aHost, aStatus => {
> +        if (!Components.isSuccessCode(aStatus)) {
> +          throw new Error("Exception occured while clearing push notifications: " + aStatus);

Should probably just call reject directly instead of throwing here.

@@ +387,5 @@
> +  },
> +
> +  deleteAll() {
> +    return new Promise(aResolve => {
> +      var push = Cc["@mozilla.org/push/Service;1"].

let

@@ +391,5 @@
> +      var push = Cc["@mozilla.org/push/Service;1"].
> +                 getService(Ci.nsIPushService);
> +      push.clearForDomain("*", aStatus => {
> +        if (!Components.isSuccessCode(aStatus)) {
> +          throw new Error("Exception occured while clearing push notifications: " + aStatus);

Should probably just call reject directly instead of throwing here.
Attachment #8976925 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 61

11 months ago
Comment on attachment 8976926 [details] [diff] [review]
part B - history

Review of attachment 8976926 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the understanding that the responsibility for managing the Places shutdown client [0] lies with the user of this API (might be worth a comment somewhere). 

https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/modules/Sanitizer.jsm#243
Attachment #8976926 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 62

11 months ago
Comment on attachment 8976619 [details] [diff] [review]
part C - session history

Review of attachment 8976619 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/cleardata/ClearDataService.js
@@ +395,5 @@
>  };
>  
> +const SessionHistoryCleaner = {
> +  deleteByRange(aFrom, aTo) {
> +    Services.obs.notifyObservers(null, "browser:purge-session-history", String(aFrom));

might still want to resolve here
Attachment #8976619 - Flags: review?(jhofmann) → review+
(Reporter)

Updated

11 months ago
Attachment #8976620 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 63

11 months ago
Comment on attachment 8976621 [details] [diff] [review]
part E - logins

Review of attachment 8976621 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/Sanitizer.jsm
@@ +396,5 @@
>        async clear(range) {
>          let refObj = {};
>          TelemetryStopwatch.start("FX_SANITIZE_SESSIONS", refObj);
> +        await clearData(range, Ci.nsIClearDataService.CLEAR_AUTH_TOKENS |
> +                               Ci.nsIClearDataService.CLEAR_LOGINS);

need to add

TelemetryStopwatch.finish("FX_SANITIZE_SESSIONS", refObj);

back in here...

::: toolkit/components/cleardata/nsIClearDataService.idl
@@ +125,5 @@
>  
> +  /**
> +   * Logins
> +   */
> +  const uint32_t CLEAR_LOGINS = 1 << 14;

I think CLEAR_LOGINS as a name is too similar to CLEAR_PASSWORDS, we should change this to CLEAR_AUTH_SESSIONS or CLEAR_AUTH_CACHE
Attachment #8976621 - Flags: review?(jhofmann) → review-
(Reporter)

Updated

11 months ago
Attachment #8976625 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 64

11 months ago
Comment on attachment 8976928 [details] [diff] [review]
part G - Permissions/Preferences

Review of attachment 8976928 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/cleardata/ClearDataService.js
@@ +525,5 @@
> +
> +const PreferencesCleaner = {
> +  deleteByHost(aHost, aOriginAttributes) {
> +    return new Promise(aResolve => {
> +      const cps2 = Cc["@mozilla.org/content-pref/service;1"]

let

@@ +530,5 @@
> +                     .getService(Ci.nsIContentPrefService2);
> +      cps2.removeBySubdomain(aHost, null, {
> +        handleCompletion: aReason => {
> +          // Notify other consumers, including extensions
> +          Services.obs.notifyObservers(null, "browser:purge-domain-data",

I know you didn't write this code, but isn't that the same notification that clears localStorage? This seems super arbitrary :|

@@ +543,5 @@
> +    });
> +  },
> +
> +  deleteByRange(aFrom, aTo) {
> +    const cps2 = Cc["@mozilla.org/content-pref/service;1"]

let

@@ +550,5 @@
> +    return Promise.resolve();
> +  },
> +
> +  deleteAll() {
> +    const cps2 = Cc["@mozilla.org/content-pref/service;1"]

let
Attachment #8976928 - Flags: review?(jhofmann) → review+
(Assignee)

Comment 65

11 months ago
> > +  const uint32_t CLEAR_DOM_STORAGES = CLEAR_DOM_QUOTA | CLEAR_DOM_PUSH_NOTIFICATIONS | CLEAR_FORMDATA | CLEAR_SESSION_HISTORY;

I prefer to add documentation here. DOM Storage is used in specs to define localStorage/indexedDB/etc.

> does DOM_QUOTA include indexedDB?

Yes. I'll add documentation here in following patches.

All the other comments, applied.
(Assignee)

Comment 66

11 months ago
> > +            }
> > +          });
> > +        }
> > +
> > +        return true;
> 
> Why is this necessary?

to make eslint happy. Just because there is a previous return <value>, I must add a return <value> here as well.
(Assignee)

Comment 67

11 months ago
> > +function hasRootDomain(str, aDomain) {
> 
> We are using this in a bunch of places now, it might be worth filing a bug
> for including this functionality in nsIEffectiveTLDService (which already
> has a method for getting the base domain).

Follow up?

> Shouldn't this be 5 now?

Yeah... all the commented flags will change value in the following patches.
(Reporter)

Updated

11 months ago
Attachment #8976931 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 68

11 months ago
Comment on attachment 8976929 [details] [diff] [review]
part I - EME

Review of attachment 8976929 [details] [diff] [review]:
-----------------------------------------------------------------

Please see notes

::: toolkit/components/cleardata/ClearDataService.js
@@ +579,5 @@
> +    });
> +  },
> +
> +  deleteAll() {
> +    // Not implemented.

Hm. If there's no way to remove EME data for all sites, we should file a bug about it and reference it here.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm
@@ +23,5 @@
>                                     Ci.nsIClearDataService.CLEAR_ALL,
> +                                   value => {
> +          // https://blogs.msdn.microsoft.com/jeuge/2005/06/08/bit-fiddling-3/
> +          const count = value - ((value >> 1) & 0o33333333333) - ((value >> 2) & 0o11111111111);
> +          errorCount += ((count + (count >> 3)) & 0o30707070707) % 63;

I hope this is fine for JS numeric values (which are what? 64 bit doubles?) but assuming it is, please put this in a helper function and give it an appropriate name and some more commentary.
Attachment #8976929 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 69

11 months ago
Comment on attachment 8976927 [details] [diff] [review]
part F - HSTS and HPKP

Review of attachment 8976927 [details] [diff] [review]:
-----------------------------------------------------------------

So I'm not really sure about this one, isn't this just security settings? Would it make sense to merge this into the security settings cleaner?
Attachment #8976927 - Flags: review?(jhofmann)
(Reporter)

Comment 70

11 months ago
David, do you know whether it makes sense to have a separate cleaning operations for sss.clearAll() vs. clearing specifically HSTS and HPKP?
Flags: needinfo?(dkeeler)
(Assignee)

Comment 71

11 months ago
Attachment #8976921 - Attachment is obsolete: true
Attachment #8981795 - Flags: review?(jhofmann)
(Assignee)

Comment 72

11 months ago
> .catch(() => { resultFlags |= c.flag; });

You are right. I reintroduce the promise here.
(Assignee)

Comment 73

11 months ago
> nit: could consider not passing origin attributes (passing {} instead) to
> make it explicit that we don't intend to use them for deletion

deleteByHost receives the originAttributes dictionary to be used. We should use it.
(Assignee)

Comment 74

11 months ago
Attachment #8976987 - Attachment is obsolete: true
Attachment #8981799 - Flags: review?(jhofmann)
(Assignee)

Comment 75

11 months ago
Attachment #8976621 - Attachment is obsolete: true
Attachment #8981800 - Flags: review?(jhofmann)
(In reply to Johann Hofmann [:johannh] from comment #70)
> David, do you know whether it makes sense to have a separate cleaning
> operations for sss.clearAll() vs. clearing specifically HSTS and HPKP?

The only cases I can come up with for why we would want to separate the two would be:

a) developers wanting to clear one or the other for a specific site they're working on (in which case this feature really isn't the best for them anyway since it clobbers state for every site)
b) users who have encountered a problem (broken pin, no https support) on a site and need to clear state to get it to work (in which case I think the majority of users wouldn't be familiar with the differences between the two settings, so offering to clear only one or the other might not be meaningful, and again this clears it for every site not just the site that's broken, so again this isn't the feature for them)

So I would say just lump them together.
Flags: needinfo?(dkeeler)
(Reporter)

Comment 77

11 months ago
Comment on attachment 8981795 [details] [diff] [review]
part 5 - passwords

Review of attachment 8981795 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!
Attachment #8981795 - Flags: review?(jhofmann) → review+
(Reporter)

Updated

11 months ago
Attachment #8981800 - Flags: review?(jhofmann) → review+
(Assignee)

Comment 78

11 months ago
> So I would say just lump them together.

Do we want to proceed with this patch? Johann, do you want to review this patch again?
Flags: needinfo?(jhofmann)
(Reporter)

Comment 79

11 months ago
(In reply to Andrea Marchesini [:baku] from comment #78)
> > So I would say just lump them together.
> 
> Do we want to proceed with this patch? Johann, do you want to review this
> patch again?

I thought the idea was to merge Part F and H now :)
Flags: needinfo?(jhofmann)
(Reporter)

Comment 80

11 months ago
Comment on attachment 8981799 [details] [diff] [review]
part 8 - DOM Quota

Review of attachment 8981799 [details] [diff] [review]:
-----------------------------------------------------------------

The general approach should work (so I don't think I need to review this again), but please take a look at the comment below.

::: toolkit/components/cleardata/ClearDataService.js
@@ +276,5 @@
> +                                 aPrincipal.URI.host);
> +
> +    // ServiceWorkers: they must be removed before cleaning QuotaManager.
> +    return ServiceWorkerCleanUp.removeFromPrincipal(aPrincipal)
> +      .then(_ => true, _ => false)

Maybe I'm reading this wrong, but shouldn't this be the other way around?

.then(_ => /* exceptionThrown = */ false, _ => /* exceptionThrown = */ true)

@@ +352,5 @@
> +    Services.obs.notifyObservers(null, "extension:purge-localStorage");
> +
> +    // ServiceWorkers
> +    return ServiceWorkerCleanUp.removeAll()
> +      .then(_ => true, _ => false)

see above

@@ +380,5 @@
> +                }));
> +              }
> +            }
> +
> +            Promise.all(promises).then(exceptionThrown ? aReject : aResolve, aReject);

nit: technically you don't need the last aReject argument, right?
Attachment #8981799 - Flags: review?(jhofmann) → review+
(Assignee)

Comment 81

11 months ago
The last check here?
Attachment #8976625 - Attachment is obsolete: true
Attachment #8976927 - Attachment is obsolete: true
Attachment #8982459 - Flags: review?(jhofmann)
(Reporter)

Comment 82

11 months ago
Comment on attachment 8982459 [details] [diff] [review]
part H - Security settings

Review of attachment 8982459 [details] [diff] [review]:
-----------------------------------------------------------------

You also need to update the CLEAR_FORGET_ABOUT_SITE flag in nsIClearDataService.idl, apart from that r=me

Thank you for all the great patches!
Attachment #8982459 - Flags: review?(jhofmann) → review+
(Assignee)

Comment 83

11 months ago
> You also need to update the CLEAR_FORGET_ABOUT_SITE flag in
> nsIClearDataService.idl, apart from that r=me

Done in part J. But I don't ask a review again.

Comment 84

11 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97b440fff0b5
Introduce nsIClearDataService - part 1 - IDL, r=johannh, r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/945c1f6e5c29
Introduce nsIClearDataService - part 2 - cookies/network cache/image cache, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef06c41bff1d
Introduce nsIClearDataService - part 3 - plugin data, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb116c8d9d2c
Introduce nsIClearDataService - part 4 - download, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2aed939175
Introduce nsIClearDataService - part 5 - passwords, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6127c3f4a8
Introduce nsIClearDataService - part 6 - Media devices, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/6652b69cb5d0
Introduce nsIClearDataService - part 7 - appCache, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9b4cca1b33
Introduce nsIClearDataService - part 8 - DOM Quota, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/470c748aeffe
Introduce nsIClearDataService - part 9 - network predictor, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/876254607268
Introduce nsIClearDataService - part 10 - push notification, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a46331ca79
Introduce nsIClearDataService - part 11 - history, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/22de53cb87c4
Introduce nsIClearDataService - part 12 - session history, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be5e7c20ed3
Introduce nsIClearDataService - part 13 - auth tokens, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebdf4bc31e58
Introduce nsIClearDataService - part 14 - logins, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/144e01c7cfc4
Introduce nsIClearDataService - part 15 - permissions and preferences, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/3725c0472caa
Introduce nsIClearDataService - part 16 - security settings, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa43598c248
Introduce nsIClearDataService - part 17 - EME data, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1e13b50338
Introduce nsIClearDataService - part 18 - custom flags for ForgetAboutSite, r=johannh

Comment 85

11 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/178ac5165152
Introduce nsIClearDataService - part 19 - android package, r=me CLOSED TREE
(Assignee)

Updated

11 months ago
Blocks: 1466130

Comment 86

11 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52b383cf34cf
Introduce nsIClearDataService - part 20 - disable android xpcshell, r=me CLOSED TREE

Comment 87

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/af42c1a9e157
Port bug 1422365 to TB/SM: Add ClearDataService to package manifests. rs=bustage-fix
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED

Updated

11 months ago
Target Milestone: --- → mozilla62
Flags: needinfo?(jhofmann)
(Assignee)

Comment 90

10 months ago
FX_SANITIZE_COOKIES_2 now covers more than just cookies: cookies, plugin data, media device.

There are only 2 main changes:
a. nsIClearDataService is a xpcom interface,
b. all is promise-based. This means that we have an extra step via event-loop.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #90)
> FX_SANITIZE_COOKIES_2 now covers more than just cookies: cookies, plugin
> data, media device.

Ok, I see that also CLEAR_CACHE now includes the images cache.
In practice we have a new baseline.
Flags: needinfo?(jhofmann)

Updated

9 months ago
Depends on: 1477539
You need to log in before you can comment on or make changes to this bug.