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

There's also https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browsingData.js :(

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.

Yeah good point, there's also "browser:purge-domain-data"...

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.

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.

Here the component integrated with Sanitizer.jsm and ForgetAboutSite.

Better approach.

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.

I don't know if it's possible to add a test here.

localStorage (bug 1286798), serviceWorkers (bug 1183245) are going to be merged in QuotaManager soon, no needs to have separate flags.

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.

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.

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.

deleteByHost is required by ForgetAboutSite. This patch should be green on try. The previous one had a couple of failing tests.

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?

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?

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(

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?

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

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.

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

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

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

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

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)

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.

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

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

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

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

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

> > + } > > + }); > > + } > > + > > + 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.

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

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.

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?

David, do you know whether it makes sense to have a separate cleaning operations for sss.clearAll() vs. clearing specifically HSTS and HPKP?

> .catch(() => { resultFlags |= c.flag; }); You are right. I reintroduce the promise here.

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

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

Comment on attachment 8981795 [details] [diff] [review] part 5 - passwords Review of attachment 8981795 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!

> 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?

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

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?

The last check here?

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!

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

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

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

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

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

https://hg.mozilla.org/mozilla-central/rev/97b440fff0b5 https://hg.mozilla.org/mozilla-central/rev/945c1f6e5c29 https://hg.mozilla.org/mozilla-central/rev/ef06c41bff1d https://hg.mozilla.org/mozilla-central/rev/bb116c8d9d2c https://hg.mozilla.org/mozilla-central/rev/7f2aed939175 https://hg.mozilla.org/mozilla-central/rev/9c6127c3f4a8 https://hg.mozilla.org/mozilla-central/rev/6652b69cb5d0 https://hg.mozilla.org/mozilla-central/rev/3b9b4cca1b33 https://hg.mozilla.org/mozilla-central/rev/470c748aeffe https://hg.mozilla.org/mozilla-central/rev/876254607268 https://hg.mozilla.org/mozilla-central/rev/59a46331ca79 https://hg.mozilla.org/mozilla-central/rev/22de53cb87c4 https://hg.mozilla.org/mozilla-central/rev/9be5e7c20ed3 https://hg.mozilla.org/mozilla-central/rev/ebdf4bc31e58 https://hg.mozilla.org/mozilla-central/rev/144e01c7cfc4 https://hg.mozilla.org/mozilla-central/rev/3725c0472caa https://hg.mozilla.org/mozilla-central/rev/9fa43598c248 https://hg.mozilla.org/mozilla-central/rev/ee1e13b50338 https://hg.mozilla.org/mozilla-central/rev/178ac5165152 https://hg.mozilla.org/mozilla-central/rev/52b383cf34cf

The landing of this may have increased FX_SANITIZE_COOKIES_2 and FX_SANITIZE_CACHE considerably, see the following evolution probes, it's particularly bad for cookies that in the 95th percentile now seem to take 5 seconds, that means some cases became very slow. https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median!5th-percentile!95th-percentile&cumulative=0&end_date=2018-05-16&keys=!__none__!__none__&max_channel_version=nightly%252F62&measure=FX_SANITIZE_COOKIES_2&min_channel_version=nightly%252F61&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2018-05-07&trim=1&use_submission_date=0 https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median!5th-percentile!95th-percentile&cumulative=0&end_date=2018-05-16&keys=!__none__!__none__&max_channel_version=nightly%252F62&measure=FX_SANITIZE_CACHE&min_channel_version=nightly%252F61&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2018-05-07&trim=1&use_submission_date=0 Any idea about why that happened?

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.

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