Once user confirms to clear all sites data the following items should be removed for all sites: - persistent permission - cookies - HTTP Cache - appcache - IndexedDB - localStorage

Comment on attachment 8813075 [details] Bug 1312361 - Clear all sites data from the Site Data section, @Gijs, This patch implements the "Clear All Data" button [1]. - Please turn on "browser.storageManager.enabled" - Because, like cookies.js, it touches cookies as well, some shared utility functions are pulled out into CookiesUtil.jsm [1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179637920

Comment on attachment 8813075 [details] Bug 1312361 - Clear all sites data from the Site Data section, https://reviewboard.mozilla.org/r/94602/#review95206 This has some serious performance issues and needs tests. ::: browser/components/preferences/CookiesUtil.jsm:1 (Diff revision 2) > +"use strict"; > + > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "ContextualIdentityService", > + "resource://gre/modules/ContextualIdentityService.jsm"); This file needs a license header. Convention is "<whatever>Utils.jsm", not "Util.jsm". I would also find "CookieUtils" more natural in English than "CookiesUtils". ::: browser/components/preferences/CookiesUtil.jsm:13 (Diff revision 2) > +this.EXPORTED_SYMBOLS = [ > + "CookiesUtil" > +]; > + > +this.CookiesUtil = { > + Nit: please remove this blank line ::: browser/components/preferences/CookiesUtil.jsm:15 (Diff revision 2) > +]; > + > +this.CookiesUtil = { > + > + isPrivateCookie(cookie) { > + var { userContextId } = cookie.originAttributes; let ::: browser/components/preferences/CookiesUtil.jsm:24 (Diff revision 2) > + } > + return !ContextualIdentityService.getIdentityFromId(userContextId).public; > + }, > + > + makeStrippedHost(host) { > + var formattedHost = host.charAt(0) == "." ? host.substring(1, host.length) : host; Nit: let Nit: `host.startsWith(".")`, and leave out the `host.length` param from str.substring (it's optional). ::: browser/components/preferences/CookiesUtil.jsm:25 (Diff revision 2) > + return !ContextualIdentityService.getIdentityFromId(userContextId).public; > + }, > + > + makeStrippedHost(host) { > + var formattedHost = host.charAt(0) == "." ? host.substring(1, host.length) : host; > + return formattedHost.substring(0, 4) == "www." ? formattedHost.substring(4, formattedHost.length) : formattedHost; Same nits as the previous line. ::: browser/components/preferences/SiteDataManager.jsm:187 (Diff revision 2) > + } > + }, > + > + _removeCookie(site) { > + let target = Services.eTLD.getBaseDomainFromHost(site.perm.principal.URI.host); > + let e = Services.cookies.enumerator; Why not use getCookiesFromHost() ? ::: browser/components/preferences/SiteDataManager.jsm:189 (Diff revision 2) > + > + _removeCookie(site) { > + let target = Services.eTLD.getBaseDomainFromHost(site.perm.principal.URI.host); > + let e = Services.cookies.enumerator; > + while (e.hasMoreElements()) { > + Nit: remove empty line ::: browser/components/preferences/SiteDataManager.jsm:215 (Diff revision 2) > + for (let [key, site] of this._sites) { > + this._removePermission(site); > + this._removeDiskCache(site); > + this._removeQuotaUsage(site); > + this._removeAppCache(site); > + this._removeCookie(site); This is very inefficient. If we have N sites, we will iterate over all the cookies N times looking only for the cookies of 1 site, and removing those, so now this is O(N^2) and it should really be pretty much O(1) given that we're basically just wiping the entire cookie db. Similar problems likely exist for the other removal calls. It's also not clear to me whether the cookie enumerator is resistant to cookies being removed while iterating, so you might be skipping cookies by removing them this way. Please just implement `removeAll()` by wiping all the relevant storage items, rather than iterating over them on a site-by-site basis. That will also be a much smaller patch. You can add the 'remove items by site' stuff when working on that patch. I would also expect that a lot of that code already exists in the page info dialog code, to name an example - perhaps we can share code from there? ::: browser/components/preferences/moz.build:22 (Diff revision 2) > DEFINES['HAVE_SHELL_SERVICE'] = 1 > > JAR_MANIFESTS += ['jar.mn'] > > EXTRA_JS_MODULES += [ > + 'CookiesUtil.jsm', The two methods here look like they should live in toolkit. Or really, the two methods could just live on the SiteDataManager, I guess, which avoids creating a new module and all the impact that creates. ::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:174 (Diff revision 2) > # e.g., "The total usage is currently using 200 MB" > # %1$S = size > # %2$S = unit (MB, KB, etc.) > totalSiteDataSize=Your stored site data is currently using %1$S %2$S of disk space > +clearSiteDataPromptTitle=Clear all cookies and site data > +clearSiteDataPromptText=Selecting Clear Now will clear all cookies and site data stored by Firefox. This may sign you out of websites and remove offline web content. Nit: quotes around 'clear now'. Did this string come from the (finalized) design?

Comment on attachment 8813075 [details] Bug 1312361 - Clear all sites data from the Site Data section, https://reviewboard.mozilla.org/r/94602/#review95206 > This is very inefficient. If we have N sites, we will iterate over all the cookies N times looking only for the cookies of 1 site, and removing those, so now this is O(N^2) and it should really be pretty much O(1) given that we're basically just wiping the entire cookie db. Similar problems likely exist for the other removal calls. > > It's also not clear to me whether the cookie enumerator is resistant to cookies being removed while iterating, so you might be skipping cookies by removing them this way. > > Please just implement `removeAll()` by wiping all the relevant storage items, rather than iterating over them on a site-by-site basis. That will also be a much smaller patch. You can add the 'remove items by site' stuff when working on that patch. I would also expect that a lot of that code already exists in the page info dialog code, to name an example - perhaps we can share code from there? To be clear, with: > You can add the 'remove items by site' stuff when working on that patch. I would also expect that a lot of that code already exists in the page info dialog code, to name an example - perhaps we can share code from there? I meant that presumably we will add functionality to remove items on a site-by-site basis. We can add those methods when we need them, rather than implementing 'remove everything' on top of them now.

Comment on attachment 8813075 [details] Bug 1312361 - Clear all sites data from the Site Data section, https://reviewboard.mozilla.org/r/94602/#review95460 ::: browser/components/preferences/SiteDataManager.jsm:215 (Diff revision 2) > + for (let [key, site] of this._sites) { > + this._removePermission(site); > + this._removeDiskCache(site); > + this._removeQuotaUsage(site); > + this._removeAppCache(site); > + this._removeCookie(site); Will modify to clear all the relevant storage items. Originally it was intended to remov data of sites which "had requested" persistent permission before. Just like your comment, this is inefficient. So discussing with DOM: Shawn and UX: Tina, to clear all data is fine and better since this function is clearly called "Clear All Data". ::: browser/components/preferences/moz.build:22 (Diff revision 2) > DEFINES['HAVE_SHELL_SERVICE'] = 1 > > JAR_MANIFESTS += ['jar.mn'] > > EXTRA_JS_MODULES += [ > + 'CookiesUtil.jsm', Will put the two methods in SiteDataManager, thanks. ::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:174 (Diff revision 2) > # e.g., "The total usage is currently using 200 MB" > # %1$S = size > # %2$S = unit (MB, KB, etc.) > totalSiteDataSize=Your stored site data is currently using %1$S %2$S of disk space > +clearSiteDataPromptTitle=Clear all cookies and site data > +clearSiteDataPromptText=Selecting Clear Now will clear all cookies and site data stored by Firefox. This may sign you out of websites and remove offline web content. Will add quotes, thanks. This is from UX spec. However, it is still under the final review. Since the review schedule is uncertain and not to block the project progress, we implement the spec first. Then, update strings if required.

Comment on attachment 8813075 [details] Bug 1312361 - Clear all sites data from the Site Data section, https://reviewboard.mozilla.org/r/94602/#review95460 > Will put the two methods in SiteDataManager, thanks. To be clear. Since this patch is going to clear all data (including all cookies), this patch does not require these two methods. I will leave these two back to the original place. Move them into SiteDataManager when it is required in another patch of removing site by site.

@Jan, We are implementing the "Clear All Data" function in about:preferences for the Storage Management project. The "Clear All Data" function needs to clear all indexedDB, http cache, appcache, cookies. However, it would raise NS_ERROR_UNEXPECTED exception when calling nsIQuotaManagerService::clear[1] to clear indexedDB. This is because the "dom.quotaManager.testing" pref is not turned on[2]. This nsIQuotaManagerService::clear API had been done in 961049 1 year ago. Is this API ready to use? [1] https://dxr.mozilla.org/mozilla-central/source/dom/quota/nsIQuotaManagerService.idl#45 [2] https://dxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManagerService.cpp#536

The current implementation of clear() can't be used because it deletes everything, including data used by chrome, about:home, moz extensions, etc. We need to carefully specify what needs to be removed.

Comment on attachment 8813075 [details] Bug 1312361 - Clear all sites data from the Site Data section, @Gijs, Updated the patch. According to the comment 9 from Jan, we need to remove quota usage one specific site by one specific site. Other removal of cookies, http cache and app cache is updated to remove once for all. About testing approach, - Permission: To add and check permission is simple so it simply add permission and check if permission is removed as expected. - App cache: Register a spy function on OfflineAppCacheHelper.clear and check if the clear function is called as expected. - Quota usage: Because nsIQuotaManagerService is a ‘builtinclass’ interface [1], cannot mock it in JS. A test page is loaded and saves to indexedDB. The test will check if quota usage is removed as expected. - Cookie: Not sure if this is because the interface carries ‘notxpcom’ methods [1][2]. Cannot mock it in JS either. Alternatively, the test tries to observe the cookies-cleared event to see if clearing cookies as expected. - Http cache: Can mock it in JS and spy on the clear method. However, the intermittent failure of `Assertion failure: !sSelf` would appear in the constructor[4] after mocking. Since a test page was loaded already and created cache, the test checks if cache usage is cleared as expected. [1] https://dxr.mozilla.org/mozilla-central/source/dom/quota/nsIQuotaManagerService.idl#14 [2] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsICookieManager.idl#71 [3] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsICookieManager2.idl#64 [4] https://dxr.mozilla.org/mozilla-central/source/netwerk/cache2/CacheStorageService.cpp#122

Here is the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ade1e6b553891f3ecf05329b1595d89268db7e0a

Comment on attachment 8813075 [details] Bug 1312361 - Clear all sites data from the Site Data section, https://reviewboard.mozilla.org/r/94602/#review97470 While I have a lot of comments, they should be easy to fix, so r=me with the below addressed. Feel free to needinfo me if you want me to have another look for the next rev. ::: browser/components/preferences/SiteDataManager.jsm:172 (Diff revision 3) > + _removeQuotaUsage(site) { > + this._qms.clearStoragesForPrincipal(site.perm.principal, null, true); > + }, > + > + removeAll() { > + for (let [key, site] of this._sites) { // eslint-disable-line no-unused-vars Nit: `let site of this._sites.values()` and get rid of the eslint comment. Might as well do the same for all the other uses in this file... ::: browser/components/preferences/in-content/advanced.js:496 (Diff revision 3) > + let text = prefStrBundle.getString("clearSiteDataPromptText"); > + let btn0Label = prefStrBundle.getString("clearSiteDataNow"); > + > + let result = Services.prompt.confirmEx( > + window, title, text, flags, btn0Label, null, null, null, {}); > + if (result === 0) { Nit: Prevailing style in browser/ code is loose comparison ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:6 (Diff revision 3) > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +const { classes: Cc, interfaces: Ci, manager: Cm, utils: Cu, results: Cr } = Components; Nit: remove Cm ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:9 (Diff revision 3) > +"use strict"; > + > +const { classes: Cc, interfaces: Ci, manager: Cm, utils: Cu, results: Cr } = Components; > + > +Cu.import('resource://gre/modules/XPCOMUtils.jsm'); > +Services.scriptloader.loadSubScript("resource://testing-common/sinon-1.16.1.js"); If you load this on a scope object you don't have to clean up so much afterwards. ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:51 (Diff revision 3) > + let callback = { > + onUsageResult: function(request) { > + resolve(request.usage); > + } > + }; > + Services.qms.getUsageForPrincipal(principal, callback); nsIQuotaUsageCallback has the `function` idl annotation (https://dxr.mozilla.org/mozilla-central/source/dom/quota/nsIQuotaCallbacks.idl), so you don't need any of this: ```js Services.qms.getUsageForPrincipal(principal, r => resolve(r.usage)); ``` would work. ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:69 (Diff revision 3) > + resolve(usage); > + }, > + QueryInterface: XPCOMUtils.generateQI([ > + Components.interfaces.nsICacheStorageConsumptionObserver, > + Components.interfaces.nsISupportsWeakReference > + ]) Nit: should have trailing comma. ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:83 (Diff revision 3) > + observe: function() { > + Services.obs.removeObserver(obs, "sitedatamanager:sites-updated"); > + resolve(); > + } > + }; > + Services.obs.addObserver(obs, "sitedatamanager:sites-updated", false); You don't need this helper - just use TestUtils.topicObserved() ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:97 (Diff revision 3) > + Services.obs.removeObserver(obs, "cookie-changed"); > + resolve(); > + } > + } > + }; > + Services.obs.addObserver(obs, "cookie-changed", false); Ditto. `topicObserved` takes a `checkFn` as a second argument that you can use for the `cookie-changed` and `cleared` checks. ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:103 (Diff revision 3) > + delete window.setImmediate; > + delete window.clearImmediate; Where do these come from? ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:105 (Diff revision 3) > + > +registerCleanupFunction(function() { > + delete window.sinon; > + delete window.setImmediate; > + delete window.clearImmediate; > + Services.prefs.setBoolPref("browser.storageManager.enabled", false); Don't do this, use pushPrefEnv instead. ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:112 (Diff revision 3) > + yield BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_BASE_URL + "site_data_test.html"); > + gBrowser.removeCurrentTab(); You should wait for the async indexeddb code in this page to have actually run, or this will go intermittent. I also think these tasks should just be 1 task - there isn't much point separating out the 'setup' bit. ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:121 (Diff revision 3) > + yield getCacheUsage().then(usage => { > + ok(usage > 0, "The cahce usage should not be 0"); Here and below, it'd be more idiomatic task jsm usage to just take the return value and use it without the `.then` callback: ```js let cacheUsage = yield getCacheUsage(); Assert.greater(cacheUsage, 0, ...); ``` ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:122 (Diff revision 3) > +add_task(function*() { > + yield openPreferencesViaOpenPreferencesAPI("advanced", "networkTab", { leaveOpen: true }); > + > + // Test the initial states > + yield getCacheUsage().then(usage => { > + ok(usage > 0, "The cahce usage should not be 0"); Nit: cache ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:122 (Diff revision 3) > +add_task(function*() { > + yield openPreferencesViaOpenPreferencesAPI("advanced", "networkTab", { leaveOpen: true }); > + > + // Test the initial states > + yield getCacheUsage().then(usage => { > + ok(usage > 0, "The cahce usage should not be 0"); `ok(comparison` is a code smell. Use: ```js Assert.greater(x, y, description) ``` here and below. ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:179 (Diff revision 3) > + let doc = gBrowser.selectedBrowser.contentDocument; > + let clearBtn = doc.getElementById("clearSiteDataButton"); > + > + mockOfflineAppCacheHelper.register(); > + clearBtn.doCommand(); > + mockOfflineAppCacheHelper.unregister(); This relis on it being sync. I'd remove it after the promises have been resolved. Also, please make the `unregister` function deal with being called more than once (I think it does already) and then add it to the `registerCleanupFunction` code so that if the test fails, we won't break subsequent tests. ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:187 (Diff revision 3) > + yield updatePromise; > + > + // Test all the items are removed > + yield cookiesClearedPromise; > + > + ok(mockOfflineAppCacheHelper.clear.calledOnce, "Not clearing app cache"); Nit: Use "should have cleared" rather than a message that only makes sense when the test fails. :-) (here and below) ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:190 (Diff revision 3) > + isnot(status, Ci.nsIPermissionManager.ALLOW_ACTION, "Not removing permission"); > + isnot(status, Ci.nsIPermissionManager.DENY_ACTION, "Not removing permission"); Just use `is` with the status we *do* expect? ::: browser/components/preferences/in-content/tests/site_data_test.html:24 (Diff revision 3) > + for (let i = 1000; i > 0; --i) { > + store.put({ id: i, description: "Site Data Test"}); > + } Is testing with 1000 items better than testing with just 1? :-) ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:63 (Diff revision 3) > <!ENTITY offlineStorage2.label "Offline Web Content and User Data"> > > <!-- Site Data section manages sites using Storage API and is under Network --> > -<!ENTITY siteData.label "Site Data"> > +<!ENTITY siteData.label "Site Data"> > +<!ENTITY clearSiteData.label "Clear All Data"> > +<!ENTITY clearSiteData.accesskey "l"> It looks like this conflicts with "limit cache to".

(In reply to :Gijs Kruitbosch from comment #13) > Comment on attachment 8813075 [details] > Bug 1312361 - Clear all sites data from the Site Data section > > https://reviewboard.mozilla.org/r/94602/#review97470 > > While I have a lot of comments, they should be easy to fix, > so r=me with the below addressed. Feel free to needinfo me if you want me to have another look for the next rev. Thanks for the suggestions. The patch is updated accordingly. Below are some illustrations. > browser/components/preferences/in-content/tests/browser_advanced_siteData.js:9 > (Diff revision 3) > > +Services.scriptloader.loadSubScript("resource://testing-common/sinon-1.16.1.js"); > > If you load this on a scope object you don't have to clean up so much > afterwards. Even with a scope, like ``` let global = {}; Services.scriptloader.loadSubScript("resource://testing-common/sinon-1.16.1.js", global); ``` Sinon lib still hooks setImmediate and clearImmediate onto window. Deleting setImmediate and clearImmediate inside registerCleanupFunction is required still. So just make sinon hooked onto window too, then deleting all these together. > ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:63 > (Diff revision 3) > > +<!ENTITY clearSiteData.label "Clear All Data"> > > +<!ENTITY clearSiteData.accesskey "l"> > > It looks like this conflicts with "limit cache to". All the letters are used. The "l" only gets one conflict so use it.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22f5846136a89d6d4cba648013b65d0c5d95fad0

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1cfad9b4a435 Clear all sites data from the Site Data section, r=Gijs