STR: 1. Add the "Forget" button to your toolbar in Menu -> Customize 2. Visit https://www.pokedex.org/ 3. Forget -> 24 hours What should happen: - The service worker is unregistered - All cached data is cleared What actually happens: - The service worker is still visible in about:serviceworkers - All data cached by the service worker is still present To verify that cached data persisted, visit https://www.pokedex.org/manifest.json and run the following in the console: caches.match('/img/icon-48.png').then(x => console.log(`Resource ${(x && x.ok) ? "WAS" : "WAS NOT"} found in cache:`, x)) Oddly enough, restarting the browser a second time does clear the service worker registration, but the caches still persist.

Looks like this was fixed as part of bug 1047098.

Oops, I just tested Firefox Nightly (build ID 20170923100045) and this bug is not fixed. "Clear Recent History" with the default set of boxes checked (Browsing & Download History, Form & Search History, Cookies, Cache, Active Logins) removes the service worker regardless of the selected time range, but the Forget button doesn't. The Forget button must be doing something differently. I was reviewing the list of remaining dependencies of bug 1102808 and based on the summary of that bug, I incorrectly assumed that "Clear Recent History" and "Forget" were equivalent. Sorry for the noise.

The forget button clears like this: https://dxr.mozilla.org/mozilla-central/rev/7e962631ba4298bcefa571008661983d77c3e652/browser/components/customizableui/CustomizableWidgets.jsm#964-966 let itemsToClear = [ "cookies", "history", "openWindows", "formdata", "sessions", "cache", "downloads" ]; let newWindowPrivateState = PrivateBrowsingUtils.isWindowPrivate(doc.defaultView) ? "private" : "non-private"; this._sanitizer.items.openWindows.privateStateForNewWindow = newWindowPrivateState; let promise = this._sanitizer.sanitize(itemsToClear); It looks like to delete service workers we would need to include "offlineApps". We don't include offlineApps because AIUI the only way to delete stuff is to delete all of it (ie timespan-based deletion doesn't work), per bug 1069300 comment 11 and further. Has that changed since the forget button was implemented 3 years ago, :baku? It wouldn't seem reasonable to remove all service worker (cache) data from a website that you visited, say, 20 times, only because 1 of those times happened to be within the timeframe you asked the forget button to clear.

> We don't include offlineApps because AIUI the only way to delete stuff is to > delete all of it (ie timespan-based deletion doesn't work), per bug 1069300 > comment 11 and further. Has that changed since the forget button was > implemented 3 years ago, :baku? appCache must be deleted all. But ServiceWorkers can be deleted per origin/principal. Same for QuotaManager. See: https://dxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm#159-172 https://dxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#299-304 Note that ForgetAboutSite doesn't clean up serviceworkers. it should.

> It wouldn't seem reasonable to remove all service worker (cache) data from a > website that you visited, say, 20 times, only because 1 of those times > happened to be within the timeframe you asked the forget button to clear. Ah! Time Range is still not supported. But from my point of view, it's better to delete more than less. ForgetButton should delete all the SW if we don't support time-range. Same for QuotaManager.

(In reply to Andrea Marchesini [:baku] from comment #5) > > It wouldn't seem reasonable to remove all service worker (cache) data from a > > website that you visited, say, 20 times, only because 1 of those times > > happened to be within the timeframe you asked the forget button to clear. > > Ah! Time Range is still not supported. Right. > But from my point of view, it's > better to delete more than less. Is it? If you want to delete more, we provide several ways of doing that. But if we delete too much, there is no way for the user to get it back (obviously!). > ForgetButton should delete all the SW if we don't support time-range. This I think makes sense. I'm assuming that no designed-to-be-permanent data is likely to be stored as part of a SW itself or its own cache. > Same for QuotaManager. This I'm less sure of. If any site stores data in indexeddb, you're saying we should wipe all of it when the user uses the forget button? The typical usecase I'd see here is having pinned tabs with e.g. gmail-type apps, and a new tab whose data you want to remove. If you use the forget button to forget the last 5 minutes or hour, removing all the other local data any other apps might have stored at that point would be surprising to the user. My understanding is that it's not possible right now to separate the two things in the current sanitizer implementation. Of course, we could change that...

Comment on attachment 8911778 [details] [diff] [review] forget.patch Per my earlier comment, I'm not convinced this is right and/or that the landscape here has changed enough to warrant changing the decision made in bug 1069300.

> My understanding is that it's not possible right now to separate the two > things in the current sanitizer implementation. Of course, we could change > that... We should have this in sync with Clear Recent History, where we delete all for components unable to deal with time-range. If I use Forget, or Clear Recent History, it's because I have privacy concern. For privacy reasons, deleting more is better. I would like to NI tanvi about this point. Note that as web-developer, Forget will also result as a broken feature. In general, we need to support time range or change the UX/UI. But adding the support of time-range for indexedDB, SW and QuotaManager is extremely complex.

(In reply to Andrea Marchesini [:baku] from comment #9) > > My understanding is that it's not possible right now to separate the two > > things in the current sanitizer implementation. Of course, we could change > > that... > > We should have this in sync with Clear Recent History, where we delete all > for components unable to deal with time-range. But "Offline Website Data" is unchecked in CRH by default, to mitigate the exact same issue we're discussing here (ie deleting more than expected if clearing small timeranges). The user has to opt-in. The forget button has more minimalist UI and so, unlike CRH, doesn't provide a way to do that right now. But opting in by default isn't a good idea, IMHO. > If I use Forget, or Clear Recent History, it's because I have privacy > concern. For privacy reasons, deleting more is better. I would like to NI > tanvi about this point. Makes sense to me, but ultimately this is also a UX question. "Forget about the last 5 minutes of what I did" is very different from "forget all the stored offline data". (Tanvi doesn't seem to be accepting NI requests right now.) > Note that as web-developer, Forget will also result > as a broken feature. I don't really understand what you're getting at here. Can you elaborate? > In general, we need to support time range or change the UX/UI. But adding > the support of time-range for indexedDB, SW and QuotaManager is extremely > complex. Would an acceptable middle-ground be to separate SW and its cache from the other items, so we could delete those with the forget button if the SW was created in the timerange specified (I assume you have a creation timestamp and could thus filter for this)? We could keep 1 checkbox in CRH.

I agree that, in this case, we are better off deleting more than less. The forget button should forget every type of storage for the last X minutes. I understand that time range is not supported, so we have to delete more than we'd like to. What are the negative consequences to that? How often is the Forget button used? What issues would a user run into if this data was cleared, when really they just wanted to forget the website they visited 5 minutes ago? Is it just performance the next time they visit a handful of select sites, or more than that?

(In reply to Tanvi Vyas[:tanvi] from comment #11) > What are the negative consequences to [deleting data]? What issues would a user run into if this data was > cleared, when really they just wanted to forget the website they visited 5 > minutes ago? Is it just performance the next time they visit a handful of > select sites, or more than that? It's impossible to know. It depends on whether sites store critical data in indexeddb and friends (using either old or new permanent storage APIs), or merely cache things. (In reply to Tanvi Vyas[:tanvi] from comment #11) > How often is the Forget button used? Blake, do you know?

(In reply to :Gijs from comment #12) > (In reply to Tanvi Vyas[:tanvi] from comment #11) > > What are the negative consequences to [deleting data]? What issues would a user run into if this data was > > cleared, when really they just wanted to forget the website they visited 5 > > minutes ago? Is it just performance the next time they visit a handful of > > select sites, or more than that? > > It's impossible to know. It depends on whether sites store critical data in > indexeddb and friends (using either old or new permanent storage APIs), or > merely cache things. > The website would also store this critical data. So if it was missing, the website could provide it, it would just take longer (performance hit). In addition, if a user is browsing offline sometime after they click the Forget button but before they visit siteA.com, the data that previously existed for offline browsing of siteA.com would be gone. This second issue doesn't seem like a big deal. Perhaps the website stores some form data in offline storage that gets prefilled for the user when they make purchases (for example). This data might be lost and the user may have to refill out their shipping/etc info. Are there any other issues you can think of? Or do you think that mostly covers it?

I hear from :mreid that we'd "have to do a custom analysis via atmo". Since that's quite a bit out of my area of expertise, I suggest we ask someone from some metrics-related team to run that for us. :)

(In reply to Tanvi Vyas[:tanvi] from comment #13) > (In reply to :Gijs from comment #12) > > (In reply to Tanvi Vyas[:tanvi] from comment #11) > > > What are the negative consequences to [deleting data]? What issues would a user run into if this data was > > > cleared, when really they just wanted to forget the website they visited 5 > > > minutes ago? Is it just performance the next time they visit a handful of > > > select sites, or more than that? > > > > It's impossible to know. It depends on whether sites store critical data in > > indexeddb and friends (using either old or new permanent storage APIs), or > > merely cache things. > > > > The website would also store this critical data. So if it was missing, the > website could provide it, it would just take longer (performance hit). I mean, that's an assumption, right? I think it's a common use of these local storage types (to effectively cache data the website stores, too), but it's by no means the only conceivable use. There's no way for the browser to know how critical (or not) the data in the local datastores is, or if there are backups. We have a whole web standard about effectively "guaranteeing" to the website that we don't just delete it based on disk space pressure (from bug 1254428), but even besides that standard, I'm unsure how many people putting data in these DBs actually assume that the data could just be removed whenever we felt like it. To be clear, I understand the desire for this to remove data naturally associated with the visits that the "forget" usage is about. But this would clear *everything*. That increases the risk that some of the data was critical, and no reasonable user would expect that saying "forget the last 5 minutes / 24 hours of browsing data" would in fact remove all locally stored data from every app they ever used, without so much as a peep.

I really think we need to err on the side of caution here. Data on the usage of the forget button will certainly help. (In reply to :Gijs from comment #10) > (In reply to Andrea Marchesini [:baku] from comment #9) > > > In general, we need to support time range or change the UX/UI. But adding > > the support of time-range for indexedDB, SW and QuotaManager is extremely > > complex. > > Would an acceptable middle-ground be to separate SW and its cache from the > other items, so we could delete those with the forget button if the SW was > created in the timerange specified (I assume you have a creation timestamp > and could thus filter for this)? We could keep 1 checkbox in CRH. Lets see what baku says about this. What if a Service Worker existed before the timerange? Do we know whether it was initiated? Does that matter, since initiating doesn't change the Service Worker which was created/stored before the time period? It may change the state of the service worker cache though. I'm not sure what types of data exist in that cache or what its for - could it cache data from other origins? Then we might need to wipe it completely too if we don't have timestamps on that data.

Note that here we are talking about ServiceWorker but what we don't delete is: localStorage, appCache, QuotaManager (indexedDB, DOMCache, asmJS, ServiceWorker cache) and ServiceWorker Registrations. Here a way to fix this bug: Let's implement a new thread-safe component that is able to store storageType, origin(+OriginAttributes) and timestamp. All the data storages (localStorage, appCache, indexedDB...) can send "pings" to this component each time they do a writing operation. The component stores the information in a map, in memory. Clear Recent History and the Forget button can use this component to know the list of active origins in a time range. Only data of those origins will be deleted. Pro: . it fixes this issue. Cons: . we have to implement a new component. . we delete all the data of the active origins and not the data created in the time range. This is still good to me. Implementing something more will be extremely hard to do. Follow ups: . We can store the map of what changed on disk. In this way, this feature will work also when FF is closed and reopened. Feedback?

(In reply to Andrea Marchesini [:baku] from comment #17) > Let's implement a new thread-safe component that is able to store > storageType, origin(+OriginAttributes) and timestamp. > All the data storages (localStorage, appCache, indexedDB...) can send > "pings" to this component each time they do a writing operation. The > component stores the information in a map, in memory. This makes sense to me, though I don't know enough about storage to determine how difficult it would be. (In reply to Tanvi Vyas[:tanvi] from comment #16) > I really think we need to err on the side of caution here. I don't think anyone disagrees with this - but caution goes both ways: not leave stuff that should be deleted, not delete stuff that the user wouldn't expect us to delete. :-)

Along the lines of what I said in https://bugzilla.mozilla.org/show_bug.cgi?id=1333050#c50 for "clear recent history", I think our current UX for "clear recent history" and "forget" is attempting to be too magic. As a developer who touches a lot of the storage subsystems and knows about containers and PrincipalOriginAttributes/etc., I find myself unable to intuitively reason about what the UI will actually do; I find it unlikely most users will either. I think the reactions we've seen are also consistent with this interpretation. The UIs should be augmented with content-page UI that shows exactly the things we think the user wants to clear and does not want to clear and let the user confirm/decide. We can also potentially further refine things. For example the forget button could offer "forget about this tab" and show what that means, or "forget about recent activity" and act browser-wide. Especially the latter could show "here's a list of sites you just visited for the first time (and their third party dependencies), or that you hadn't visited for a long time... we think we should obliterate their storage for you" and "here's a list of other sites that you just visited that you tend to visit a lot, like your open gmail tab and your dropbox tab that are still open right now, maybe we don't nuke those". In order to make all this more effective, I think we do absolutely want to consider enabling double-keying like Safari does, and our tor support has been implementing in the form of the firstPartyDomain OriginAttribute. This would be beneficial because As to baku's comment 17, such functionality would indeed be needed for such a UI. Also, it would be handy for letting WebExtensions provide additional privacy functionality, such as the requested-elsewhere UI to indicate "hey, this site just started storing data locally on your machine, do you want to revoke its access?" baku's proposed functionality would allow us to expose such hooks. In terms of how to implement it, I think it's worth considering whether the data should actually be stored in Places, so I'm needinfo-ing :mak. My strawman would be that we'd annotate visits with a bitmask of storage mutations that occurred during the visit, and we'd flow the bitmask back to Places. I think the risk/complexity here is that Places is already so overloaded with performance-critical behavior and data and that since we need to also store any 3rd-party iframes' info, Places would explode. Places previously did some fancy stuff with in-memory databases as a hack; it's possible something like that could be done, just as an in-memory map was proposed. If Places isn't the "place", then QuotaManager might be a reasonable spot for the data owned by QuotaManager, if only because QuotaManager itself might benefit from having an improved understanding of origin usage for eviction. And because I expect at some point soon we'll enhance PROFILE/storage.sqlite (or something like it) to persistently track origin usage so we can avoid scanning the quota-controlled contents of PROFILE/storage at ~startup. (Also, to support things like background-sync and notifications and background-fetch that want to know derived facts about origins without doing all the book-keeping themselves or relying on the startup scans.) Once we've done that work, it's not a lot more work to store something like "the first and last 3 session timestamps of when this origin was mutated per client". (We don't need an exact history journal, just the ability to inform the clearing UI and eviction logic.) Using QuotaManager avoids having another source of truth about persistent storage. And it will soon manage LocalStorage, which leaves us with cookies and history as separate data sources. However, they already have their own timestamps, and the analysis logic that would inform the erasure UI would at least want to consider history separately. :janv, thoughts?

(In reply to Andrew Sutherland [:asuth] from comment #19) > In order to make all this more effective, I think we do absolutely want to > consider enabling double-keying like Safari does, and our tor support has > been implementing in the form of the firstPartyDomain OriginAttribute. This > would be beneficial because ...it makes it easier to delete only the effects of the user's recent browsing. It also majorly improves privacy. And it's worth mentioning that :bkelly and I and others have been discussing about how we really should be doing this.

(In reply to Andrew Sutherland [:asuth] from comment #19) > In terms of how to implement it, I think it's worth considering whether the > data should actually be stored in Places, so I'm needinfo-ing :mak. There are a few things to keep into account here: 1. Places is desktop only, Android Firefox wouldn't be supported (not sure what we clear there) 2. In PB mode we may not store the page in Places at all, though I suppose it's the same for all of these consumers, thus we may not care(?). 3. In general I think the metadata should stay close to the data it's annotating. What happens if the user moves places.sqlite to a different profile? This happens often, and even more often due to the Firefox Reset feature. All of these are surely solvable problems, but is it worth it VS a separate store? what do we gain from reusing the Places store? As a side note, I must note that (excluding PB mode) Places/(the android history system) knows if a page/domain has been visited in a given timeframe, so we could probably add an API like history.getHostsVisitedInTimeframe and use the result to clear only those hosts. If the requirements for each component we want to clear are defined, we can surely know if that page/host was visited in a given timeframe.

This is the first patch. Here we have a new service: StorageActivityService. This does: 1. memory-pressure: clean internal data. 2. exposed nsIStorageActivityService: this will be improved in following patches. 3. it cleans internal data when older than 1 hour. 4. runs on child and parent process but data is stored and available only on the parent side.

nsIStorageActivityService exposes data via getActiveOrigins(range). This is used in sanitize.js (tests in a separate patch)

I wanted to add a test but I cannot do it in general. So I moved all the 'sanitize'-related tests in a separate directory.

Comment on attachment 8913144 [details] [diff] [review] part 3 - getActiveOrigins Review of attachment 8913144 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/sanitize.js @@ +291,5 @@ > > offlineApps: { > async clear(range) { > // AppCache > + let {OfflineAppCacheHelper} = Components.utils.import("resource:///modules/offlineAppCache.jsm", {}); Why this change? If anything, maybe we should define this as a lazy getter at the top of the file? @@ +298,5 @@ > OfflineAppCacheHelper.clear(); > > + if (range) { > + let seconds = (range[1] - range[0]) / 1000000; > + let origins = sas.getActiveOrigins(seconds) This won't do the right thing if the range isn't based on 'now'. Why can't we just pass both parts of the range? That seems simpler... @@ +305,5 @@ > + let promises = []; > + > + for (let i = 0; i < origins.length; ++i) { > + let origin = origins.queryElementAt(i, Components.interfaces.nsISupportsCString) > + .data; This is a little ugly. Can we not return the data in a way that xpconnect automatically turns into a JS array of strings? @@ +307,5 @@ > + for (let i = 0; i < origins.length; ++i) { > + let origin = origins.queryElementAt(i, Components.interfaces.nsISupportsCString) > + .data; > + > + if (!origin.startsWith("http") && !origin.startsWith("file")) { Please check including the ":" here, to avoid mismatching things. Really, I would prefer it if we just created the principal and did an actual scheme check on its URI, but I assume that `createCodebasePrincipalFromOrigin` throws for some of these values? A comment about this would be helpful. @@ +314,5 @@ > + > + let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(origin); > + > + // LocalStorage > + Services.obs.notifyObservers(null, "browser:purge-domain-data", principal.URI.host); What's responsible for pushing this to the content process, given this code runs in the parent, and some of the observers look like they'd only exist in the content process? It looks like dom storage does it itself, privately, but I don't see session store doing anything at all in terms of propagating it. ::: dom/storage/StorageActivityService.cpp @@ +228,5 @@ > } > > +NS_IMETHODIMP > +StorageActivityService::GetActiveOrigins(int64_t aDuration, > + nsIArray** aRetval) I'm not a good reviewer for this code, so leaving this for :asuth.

Comment on attachment 8913148 [details] [diff] [review] part 6 - moving tests Review of attachment 8913148 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for moving these. ::: browser/base/content/test/sanitize/browser.ini @@ +5,5 @@ > + > +[browser_purgehistory_clears_sh.js] > +[browser_sanitize-passwordDisabledHosts.js] > +[browser_sanitize-sitepermissions.js] > +[browser_sanitizeDialog.js] Nit: can we sort this alphabetically while we're here? ::: browser/base/content/test/sanitize/browser_purgehistory_clears_sh.js @@ +1,4 @@ > /* Any copyright is dedicated to the Public Domain. > * http://creativecommons.org/publicdomain/zero/1.0/ */ > > +const url = "http://example.org/browser/browser/base/content/test/sanitize/dummy_page.html"; Nit: getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.org") + "dummy_page.html"; doesn't hardcode the full path, so it's a little neater. ::: browser/base/content/test/sanitize/head.js @@ +1,1 @@ > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); Shouldn't need this. @@ +58,5 @@ > + * @resolves An object with subject and data properties from the observed > + * notification. > + * @rejects Never. > + */ > +function promiseTopicObserved(aTopic) { While we're here, you can leave this out of the new head.js and replace the 1 callsite in browser_sanitizeDialog.js (which I think is the only one in this set of tests, right?) with: TestUtils.topicObserved("sanitizer-sanitization-complete"); which has a slightly different resolution value for the promise, but that doesn't matter because none of the callsites uses it.

Comment on attachment 8913149 [details] [diff] [review] part 7 - tests Review of attachment 8913149 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/sanitize/browser_sanitize-offlineData.js @@ +1,3 @@ > +// Bug 380852 - Delete permission manager entries in Clear Recent History > + > +var tempScope = {}; Nit: let. @@ +17,5 @@ > + "@mozilla.org/dom/quota-manager-service;1", > + "nsIQuotaManagerService"); > + > +const sanitizeURL = "http://example.org/browser/browser/base/content/test/sanitize/sanitize.html"; > +const dummyURL = "http://example.org/browser/browser/base/content/test/sanitize/dummy_page.html"; Please make these relative URLs like in the other test. @@ +23,5 @@ > +async function createData() { > + // Let's open the tab again. > + let tab = BrowserTestUtils.addTab(gBrowser, sanitizeURL); > + let browser = gBrowser.getBrowserForTab(tab); > + await BrowserTestUtils.browserLoaded(browser); you could make this a normal function that just does: return BrowserTestUtils.withNewTab(url, async function(browser) { await ContentTask.spawn(browser, null, () => { }); }); then you don't need to manually open/wait/close a tab. @@ +33,5 @@ > + if ("foobar" in content.window.localStorage) { > + content.window.clearInterval(id); > + resolve(true); > + } > + }, 1000); How about: return ContentTaskUtils.waitForEvent(content.window, "pagesetlocalstorage", true, true); and dispatching a custom event from the sanitize.html when we've set the localstorage var? That avoids the ugly interval-based waiting. @@ +44,5 @@ > +async function getData() { > + // Let's open the tab again. > + let tab = BrowserTestUtils.addTab(gBrowser, dummyURL); > + let browser = gBrowser.getBrowserForTab(tab); > + await BrowserTestUtils.browserLoaded(browser); Again, can use withNewTab() here to simplify. @@ +46,5 @@ > + let tab = BrowserTestUtils.addTab(gBrowser, dummyURL); > + let browser = gBrowser.getBrowserForTab(tab); > + await BrowserTestUtils.browserLoaded(browser); > + > + // Let's create some data. This comment seems wrong? @@ +87,5 @@ > +add_task(async function testWithRange() { > + await SpecialPowers.pushPrefEnv({'set': [ > + ['dom.serviceWorkers.enabled', true], > + ['dom.serviceWorkers.exemptFromPerDomainMax', true], > + ['dom.serviceWorkers.testing.enabled', true] Nit: double quotes throughout please. @@ +136,5 @@ > + let dataPost = await getData(); > + ok (!dataPost.localStorage, "We don't have localStorage data"); > + ok (!dataPost.indexedDB, "We don't have indexedDB data"); > + > + // This cannot be easily checked because SW are unregistered async: How async? Can we use BrowserTestUtils.waitForCondition, or does it take too long or something?

Comment on attachment 8913149 [details] [diff] [review] part 7 - tests Review of attachment 8913149 [details] [diff] [review]: ----------------------------------------------------------------- As I read it, this test only verifies that we erase things, not that there are some things we don't erase. While we clearly need to improve data erasure, I think it's also essential to have confidence that we're not wiping out more data than the user wants wiped. Perhaps it could be enhanced a little to: - Create data that we don't want to be erased in "example.com". - Call nsIStorageActivityService::SetTestingMark() which is a thing we introduce for testing that populates a Maybe<TimeStamp> that's saved off so that we don't need to use setTimeout() here to create padding. GetActiveOrigins() consumes the Maybe<TimeStamp> if it's populated. This could be put behind a preference test guard like how QuotaManager refuses to do some things if the testing pref isn't set. - Alternately, we do Performance.now() sampling here and before running the sanitize so we can calculate the appropriate relative time. We also do a multi-second setTimeout() here after the first Performance.now() so that we can provide the sanitize() call some padding so that example.org doesn't fall out of the time range if things are running slow before getActiveOrigins() completes its time delta calculations. - Existing: Create the data we do want erased; already done, for example.org. - Half-existing: Verify both sets of data exist. - Existing: Run sanitize - Existing: Verify example.org data gone. - Verify example.com data still there. I'm going to mark this r- for now because I do think this is vitally important. ::: browser/base/content/test/sanitize/browser_sanitize-offlineData.js @@ +133,5 @@ > + // Clear it > + await s.sanitize(); > + > + let dataPost = await getData(); > + ok (!dataPost.localStorage, "We don't have localStorage data"); Heads up, no action required here: This may intermittently fail because we moved localStorage to PBackground. It probably won't fail a lot because the QuotaManager clearing is async and that gets us a PBackground round-trip, so then the race becomes an IPC channel race. (Unless something else has a promise that goes to PBackground, then down to the content process main threads, then back to the parent process before resolving. Then everything is fine.) I'm planning to address this in some other bug by letting LocalStorage return a promise when we ask it to clear stuff. @@ +136,5 @@ > + let dataPost = await getData(); > + ok (!dataPost.localStorage, "We don't have localStorage data"); > + ok (!dataPost.indexedDB, "We don't have indexedDB data"); > + > + // This cannot be easily checked because SW are unregistered async: nsIServiceWorkerManager has a listener mechanism that notifies unregistration and there's existing helper logic to this end that can be stolen from https://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/chrome_helpers.js#30 (with the helper invoked prior to invoking sanitize, of course, so the event doesn't get missed).

Comment on attachment 8913140 [details] [diff] [review] part 2 - LocalStorage Review of attachment 8913140 [details] [diff] [review]: ----------------------------------------------------------------- All 3 of these want to go inside the `if (!aRv.ErrorCodeIs(NS_SUCCESS_DOM_NO_OPERATION))` blocks with the `BroadcastChangeNotification(aKey, old, VoidString());` bit. Otherwise we're losing out on the no-op optimization which could matter in terms of IPC traffic.

Comment on attachment 8913139 [details] [diff] [review] part 1 - StorageActivityService Review of attachment 8913139 [details] [diff] [review]: ----------------------------------------------------------------- This looks very nice, but I'd like to re-review after the changes are made, especially because there may be some test-related changes that happen. ::: dom/interfaces/storage/nsIStorageActivityService.idl @@ +5,5 @@ > + > +#include "domstubs.idl" > + > +[scriptable, builtinclass, uuid(fd1310ba-d1be-4327-988e-92b39fcff6f4)] > +interface nsIStorageActivityService : nsISupports This really wants a multi-sentence block comment. ::: dom/storage/StorageActivityService.cpp @@ +9,5 @@ > +#include "mozilla/ipc/BackgroundUtils.h" > +#include "mozilla/StaticPtr.h" > +#include "nsXPCOM.h" > + > +#define TIME_MAX 3600 /* 1 hour */ This needs a comment explaining why it's one hour. Also, I'd suggest explicitly appending units so this is TIME_MAX_SECS. @@ +51,5 @@ > +StorageActivityService::GetOrCreate() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (!gStorageActivityService) { It seems possible for the service to be resurrected here if `StorageActivityService::SendActivity(nsIPrincipal* aPrincipal)` runs after the shutdown observer fires. @@ +82,5 @@ > +} > + > +StorageActivityService::~StorageActivityService() > +{ > + MOZ_ASSERT(NS_IsMainThread()); I'd add a MOZ_ASSERT(!mTimer) here. Despite the resurrection issue, I think this will hold, but this is obviously nice to help be sure we don't need to worry about timer-related UAFs. @@ +116,5 @@ > + > + PBackgroundChild* actor = BackgroundChild::GetForCurrentThread(); > + if (!actor) { > + mPendingActivities.AppendElement(aPrincipal); > + BackgroundChild::GetOrCreateForCurrentThread(this); PBackground creation is now synchronous, thanks to :janv in bug 1283609 which landed in Firefox 56. Just use BackgroundChild::GetOrCreateForCurrentThread() and lose all the mPendingActivities stuff. @@ +168,5 @@ > + gStorageActivityService = nullptr; > + return NS_OK; > + } > + > + if (!strcmp(aTopic, "memory-pressure")) { What's your rationale for having memory-pressure clear everything? I don't think this is the place to save memory. If you do think this is important enough, it seems like some type of "degraded" state where we instead just decide to clear everything would be more appropriate than failing in this manner. And if we kept this, I think this would want a really big block comment somewhere explaining the rationale behind this. Also, a test. ::: ipc/glue/PBackground.ipdl @@ +131,5 @@ > async PHttpBackgroundChannel(uint64_t channelId); > > async PWebAuthnTransaction(); > > + async StorageActivity(PrincipalInfo principalInfo); This really wants at least a short comment, possibly just referencing nsIStorageActivityService.

Comment on attachment 8913144 [details] [diff] [review] part 3 - getActiveOrigins Review of attachment 8913144 [details] [diff] [review]: ----------------------------------------------------------------- I need to do a deeper review on this when I re-review it. ::: browser/base/content/sanitize.js @@ +307,5 @@ > + for (let i = 0; i < origins.length; ++i) { > + let origin = origins.queryElementAt(i, Components.interfaces.nsISupportsCString) > + .data; > + > + if (!origin.startsWith("http") && !origin.startsWith("file")) { (the http prefix check gets https too, I think is the rationale) @@ +314,5 @@ > + > + let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(origin); > + > + // LocalStorage > + Services.obs.notifyObservers(null, "browser:purge-domain-data", principal.URI.host); browser:purge-domain-data is also monitored by the ServiceWorkerManager, and so will already trigger the lines below. See https://public.etherpad-mozilla.org/p/data-clearing-apis and ctrl-f for "browser:purge-domain-data" where I audited stuffs. That section should also answer :Gijs question about what session store and SessionStorageManager do. Alternately, if you only care about LocalStorage and Firefox 58-only is fine, bug 1388428 just extended "extension:purge-localStorage" to take a hostname arg and you can avoid the side-effects. ::: dom/interfaces/storage/nsIStorageActivityService.idl @@ +10,5 @@ > [scriptable, builtinclass, uuid(fd1310ba-d1be-4327-988e-92b39fcff6f4)] > interface nsIStorageActivityService : nsISupports > { > + // This returns an array of origins as strings, active in the last > + // |timeDuration| seconds. This needs to document that TIME_MAX establishes an upper-bound on the timeDuration that can be passed. (You don't need to mention TIME_MAX itself, just the upper limit.) ::: dom/storage/StorageActivityService.cpp @@ +227,5 @@ > return NS_OK; > } > > +NS_IMETHODIMP > +StorageActivityService::GetActiveOrigins(int64_t aDuration, Given the TIME_MAX limitation, I think we should MOZ_DIAGNOSTIC_ASSERT here if the duration exceeds TIME_MAX but not return an error. The point is to surface as dramatically as possible that the value is unacceptable from a privacy perspective and we can't ship that, but to still perform a best effort in release builds. Alternately, we could introduce the elsewhere-mentioned degraded mode and trigger that path that just causes us to wipe everything. @@ +242,5 @@ > + > + for (auto iter = mActivities.Iter(); !iter.Done(); iter.Next()) { > + if ((now - iter.UserData()).ToSeconds() < aDuration) { > + > + nsSupportsCString* str = new nsSupportsCString(); Isn't this infallible as invoked, negating the error check on the next line? And why isn't this an nsCOMPtr? It seems like the str leaks in the error returns below.

Comment on attachment 8913147 [details] [diff] [review] part 5 - QuotaManager Review of attachment 8913147 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +3000,5 @@ > QuotaManager* quotaManager = QuotaManager::Get(); > MOZ_ASSERT(quotaManager); > > quotaManager->mQuotaMutex.AssertCurrentThreadOwns(); > + mWritingDone = true; very quick drive-by, I haven't even checked my notes (I need to go to sleep :)... Shouldn't this send activity on the first transition from mWritingDone == false to true? Also, possibly informed by a timestamp-check? Really, I think there should be a big comment around at least one of the SendActivity invocations explaining how activity will be reliably sent periodically, even in the face of long-lived QM clients. (Because QM is complex and still pretty far from obvious or easily understood after skimming some comments... hence my comment about my notes...)

Comment on attachment 8913145 [details] [diff] [review] part 4 - ServiceWorkers Review of attachment 8913145 [details] [diff] [review]: ----------------------------------------------------------------- This seems straightforward in terms of being just mechanically about the registrations. I'll think about the general SW case issues a little more for part 3. Or more specifically, I'll just make sure I've located the relevant SW bugs and list them here in the bug so we're on the same page about what additional ServiceWorker work may still be required.

Comment on attachment 8913140 [details] [diff] [review] part 2 - LocalStorage Review of attachment 8913140 [details] [diff] [review]: ----------------------------------------------------------------- Actually, scratch that. This is what BroadcastLocalStorageChange's signature looks like: async BroadcastLocalStorageChange(nsString documentURI, nsString key, nsString oldValue, nsString newValue, PrincipalInfo principalInfo, bool isPrivate); Note the PrincipalInfo, let's just do this in BackgroundParentImpl::RecvBroadcastLocalStorageChange and save the extra IPC overhead. (And it appears that thanks to PBackground-normalization, this works in non-e10s mode too.)

Comment on attachment 8914252 [details] [diff] [review] part 1 - StorageActivityService Review of attachment 8914252 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for making the changes! This patch is there modulo the issue below. ::: dom/storage/StorageActivityService.cpp @@ +11,5 @@ > +#include "nsXPCOM.h" > + > +// This const is used to know when origin activities should be purged because > +// too old. This value should be in sync with what the UI needs. > +#define TIME_MAX_SECS 3600 /* 1 hour */ Thank you for adding the comment. But I think you need to bump this up to 24 hours or provide a better explanation, given that the sanitize.js UI has three radio button time settings, none of which are 1 hour: - 5 minutes - 2 hours - 24 hours Also, if we use this mechanism to also support the "Clear Recent History..." mechanism, its UI has values of: - 1 hours - 2 hours - 4 hours - "Today"

The localStorage broadcasting is still undone.

Comment on attachment 8918247 [details] [diff] [review] part 3 - getActiveOrigins Review of attachment 8918247 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review until the tests patch is updated. I will review then.

waitForUnregister seems not to work with RemoteAndPropagate

This method is for testing only and it's used by patch 8.

Comment on attachment 8930435 [details] [diff] [review] part 3 - getActiveOrigins Review of attachment 8930435 [details] [diff] [review]: ----------------------------------------------------------------- In testing the most recent push to try, I discovered a problem where if you do "forget 5 minutes" followed by a second "forget 5 minutes", things will get deleted that should not be deleted. It turns out the problem chain goes like this: - sanitize.js' "history" logic notifies observers of "browser:purge-session-history" - ServiceWorkerManager listens for this and triggers RemoveAll and PropagateRemoveAll, ignoring the time constraint. - ServiceWorkerManagerService::PropagateRemoveAll invokes ServiceWorkerRegistrar::RemoveAll which does StorageActivityService::SendActivity for the origin. So in summary, existing logic already uninstalls all service workers if "history" is selected, and this is reported as activity for the origin, which results in the second forget wiping out the rest of the storage for the origin. Accordingly, I think you want to: - Remove ServiceWorkerManager's handling of "browser:purge-session-history". - Make sure the Fennec-parity bug 1415342 knows about this bug because its Sanitizer.jsm does depend on that, although it's currently already planned to do other porting work. - Update the part 8 tests to trigger the sanitize() operation twice so that the test would potentially have caught this. I'm setting this to r- because there's enough changes required that I want to look again, and I'd also like to re-do manual testing, etc. ::: browser/base/content/sanitize.js @@ +310,5 @@ > + continue; > + } > + > + // LocalStorage > + Services.obs.notifyObservers(null, "browser:purge-domain-data", principal.URI.host); This currently is handled by ServiceWorkerManager too, making the next direct call to removeAndPropagate redundant. This observer path is also used by SiteDataManager.jsm and ForgetAboutSite.jsm. SiteDataManager.jsm already has an explicit ServiceWorker path thanks to bug 1410416 addressing race issues. ForgetAboutSite.jsm, however, only invokes QuotaManagerService.clearStoragesForPrincipal and so does depend on its _removeCookie(site) path to generate the observer notification. Really, it seems like we want to leverage the lessons learned about ServiceWorkers doing stuff in shutdown that led to bug 1410416 everywhere... so this suggests we want to: - remove handling of browser:purge-domain-data by ServiceWorkerManager in favor of direct calls against the SWM API. - Either by copying-and-pasting, selecting one module as the source of truth, or creating a new Service Worker-owned jsm, have this logic and ForgetAboutSite.jsm convert to using the same idiom as SiteDataManager.jsm such that they wait for the serviceworkers to be removed before triggering the quotamanager clearing. This may be too much scope creep, in which case, let's file a follow-up and add a comment among these lines that indicates there's known redundancy and an async race issue a la bug 1410416. If you end up going with copying-and-pasting, we'll probably still want some kind of cleanup follow-up bug too. ::: dom/interfaces/storage/nsIStorageActivityService.idl @@ +20,5 @@ > [scriptable, builtinclass, uuid(fd1310ba-d1be-4327-988e-92b39fcff6f4)] > interface nsIStorageActivityService : nsISupports > { > + // This returns an array of nsIPrincipals, active between |from| and |to| > + // timestamps. Note activities older than 1 day are forgotten. I'd suggest adding some additional comment details like: Activity details are not persisted, so this only covers activity since Firefox was started. All codebase principals are logged, which includes non-system principals like "moz-extension://ID", "moz-safe-about:home", "about:newtab", so principals may need to be filtered before being used. @@ +21,5 @@ > interface nsIStorageActivityService : nsISupports > { > + // This returns an array of nsIPrincipals, active between |from| and |to| > + // timestamps. Note activities older than 1 day are forgotten. > + nsIArray getActiveOrigins(in DOMTimeStamp from, in DOMTimeStamp to); s/DOMTimeStamp/PRTime/ since that's what you're using. (DOMTimeStamp is defined to be in millis.) ::: dom/storage/StorageActivityService.h @@ +59,5 @@ > void > MaybeStopTimer(); > > // Activities grouped by origin (+OriginAttributes). > + nsDataHashtable<nsCStringHashKey, uint64_t> mActivities; s/uint64_t/PRTime/ so the type self-describes units.

Comment on attachment 8930447 [details] [diff] [review] part 8 - tests Review of attachment 8930447 [details] [diff] [review]: ----------------------------------------------------------------- This test is pretty great now, but since I want to take another look at part 3 and there's some related changes desired here, I'm minusing. Next review will be super quick though! ::: browser/base/content/test/sanitize/browser_sanitize-offlineData.js @@ +9,5 @@ > + > +XPCOMUtils.defineLazyServiceGetter(this, "sas", > + "@mozilla.org/storage/activity-service;1", > + "nsIStorageActivityService"); > +XPCOMUtils.defineLazyServiceGetter(this, "swm", You're not using this redundant "swm" getter that you re-define as "serviceWorkerManager" below. @@ +30,5 @@ > + if ("foobar" in content.window.localStorage) { > + content.window.clearInterval(id); > + resolve(true); > + } > + }, 1000); :gijs had a proposal for avoiding the setinterval that I don't think was discussed. But I think that proposal potentially still faces a race condition. However, you can totally wrap the top-level script in sanitize.html into a function that returns a promise that this ContentTask.spawn() can directly trigger via content.wrappedJSObject.yourHelperFunc() and return the returned promise. @@ +88,5 @@ > + let s = new Sanitizer(); > + s.ignoreTimespan = false; > + s.prefDomain = "privacy.cpd."; > + let itemPrefs = gPrefService.getBranch(s.prefDomain); > + itemPrefs.setBoolPref("history", false); As noted in part 3, fallout from the history logic meant breakage if forget was used twice in succession (with the second forget overlapping the time window of the first forget). It seems inadvisable to cause these preferences to diverge from the defaults Firefox ships with in firefox.js without a good reason. If there is a good reason, it should definitely be a comment! ;) (Also, these should have been set using pushPrefEnv too. Mutating them like this risks breaking other tests.) @@ +103,5 @@ > + await createData("example.org"); > + await createData("example.com"); > + > + let endDate = Date.now() * 1000; > + let principals = sas.getActiveOrigins(endDate - 3600000000 /* 1 hour */, endDate); You got the constant right, but this is bad hygiene (and we have had millis/usecs bugs in the codebase). Please introduce and use constants for both lines here. @@ +133,5 @@ > + let done = false; > + for (let i = 0; i < principals.length; ++i) { > + let principal = principals.queryElementAt(i, Components.interfaces.nsIPrincipal); > + if (principal.URI.host == "example.com") { > + sas.moveOriginInTime(principal, endDate - (3600000000 * 5 /* 5 hours ago */)); And this somewhat sillier use here too ;) @@ +149,5 @@ > + ok(!dataPost.localStorage, "We don't have localStorage data"); > + ok(!dataPost.indexedDB, "We don't have indexedDB data"); > + // This is not easy to check because we don't have a callback when > + // removeAndPropagate is executed. > + // ok(!dataPost.serviceWorker, "We don't have serviceWorker data"); > waitForUnregister seems not to work with RemoteAndPropagate Did you call the function before you invoked sanitize()? It just listens for a registration with the given scope to be removed; if you don't invoke it beforehand, you could indeed hang because the registration has already been removed. Alternately, as noted in part 3, if we adapt sanitize to wait for the propagation of the unregister, then we don't need to wait for anything as part of the test logic, since awaiting the sanitize() call will already take care of things. ::: browser/base/content/test/sanitize/sanitize.html @@ +1,1 @@ > +<script> Please wrap this in html tags with a charset defined so that we don't get the spam about undefined charset.

Comment on attachment 8930448 [details] [diff] [review] part 7 - moveOriginInTime Review of attachment 8930448 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/storage/nsIStorageActivityService.idl @@ +26,5 @@ > nsIArray getActiveOrigins(in DOMTimeStamp from, in DOMTimeStamp to); > + > + // NOTE: This method is meant to be used for testing only. > + // The activity of |origin| is moved to the specified timestamp |when|. > + void moveOriginInTime(in nsIPrincipal origin, in DOMTimeStamp when); s/DOMTimeStamp/PRTime/

I just realized the 3600000000 probably came from sanitize.js where it does weird stuff like: startDate = endDate - 3600000000; // 1*60*60*1000000 which is a sketchy optimization to make given the existence of constant-folding. Consider those nits relaxed, but I won't complain if you improve on that code! :)

Is this what you mean?

Comment on attachment 8934131 [details] [diff] [review] part 3a - no purge session storage for SWM Review of attachment 8934131 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Andrea Marchesini [:baku] from comment #51) > Created attachment 8934131 [details] [diff] [review] > part 3a - no purge session storage for SWM > > Is this what you mean? Yes, exactly, for part 3's review-level comment. There's still also the "browser:purge-domain-data" case from my comments on the first hunk too as a larger change to be dealt with.

Comment on attachment 8930435 [details] [diff] [review] part 3 - getActiveOrigins Review of attachment 8930435 [details] [diff] [review]: ----------------------------------------------------------------- Another issue that should be addressed is the fallout from the switch to using PRTime wall-clock time from TimeStamp time. I believe the immediate choice to change was made out of necessity to support the back-dating of changes for testing purposes. The change also makes some sense if we're planning to persist this data to survive restarts of the browser. However, there is the obvious cost that time synchronization can cause wall-clock time to change. This concern is mitigated somewhat by the fact that sanitize.js uses Date.now() as its wall-clock time, and things like history and most other persisted stores are inherently also going to use wall-clock time, meaning the service is consistent with the others. So I'd suggest: - add a comment on nsIStorageActivityService about the use of wall-clock time. - add a comment in sanitize.js in getClearRange where Date.now() is used about how there could be problems with changes to wall-clock time due to network time synchronization and how it might be appropriate to have some XPCOM service that does a TimeStamp() at xpcom startup and periodically (or by monitoring events if supported by the system) that can provide us with an adjustment factor if needed to add onto the time range. So we wouldn't actually be addressing the underlying risk, which should currently be small-ish, but would be surfacing it better.

I also removed RemoveAndPropagate()

Comment on attachment 8934499 [details] [diff] [review] part 3b - ServiceWorkerCleanUp.jsm Review of attachment 8934499 [details] [diff] [review]: ----------------------------------------------------------------- browser/components/extensions/ext-browsingData.js uses removeAndPropagate, so you'll need to update its clearServiceWorkers method at https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browsingData.js#156 to use your new ServiceWorkerCleanUp.jsm too. ::: browser/components/preferences/SiteDataManager.jsm @@ +264,5 @@ > Services.cache2.clear(); > Services.cookies.removeAll(); > OfflineAppCacheHelper.clear(); > > + await unregisterServiceWorker.removeAll(); I believe you want this to be ServiceWorkerCleanUp.removeAll(). ::: toolkit/forgetaboutsite/ForgetAboutSite.jsm @@ +149,5 @@ > } > > + // ServiceWorkers > + promises.push(ServiceWorkerCleanUp.removeFromHost("http://" + aDomain)); > + promises.push(ServiceWorkerCleanUp.removeFromHost("https://" + aDomain)); The calls to nsIQuotaManagerService in the next paragraph need to be made to wait for the service workers to be removed to avoid the resurrection of the origin due the scriptloader (or other SW machinery) racing the QM clearing. (Like the logic in SiteDataManager.jsm does.)

Andrew, can you review the QuotaManager part?

(In reply to Andrea Marchesini [:baku] from comment #57) > Andrew, can you review the QuotaManager part? I think so, but :janv may just have been waiting for the rest of the patches to solidify, so switching NI to him to review or delegate now that they have. Also, I have an existing drive-by review comment on the QM patch that I'd like to see addressed before I'd do a final review.

Comment on attachment 8930435 [details] [diff] [review] part 3 - getActiveOrigins (obsoleted by r+'ed attachment 8934500 [details] [diff] [review])

(see comment 58... there was an existing needinfo I needed to create a rising edge for)

(In reply to Andrew Sutherland [:asuth] from comment #58) > I think so, but :janv may just have been waiting for the rest of the patches > to solidify, so switching NI to him to review or delegate now that they > have. Also, I have an existing drive-by review comment on the QM patch that > I'd like to see addressed before I'd do a final review. I somehow lost track of this. Can someone quickly summarize purpose of this new service, is it a temporary solution or something we want to keep and evolve, especially regarding with comment 19. What trade-offs you had to make and how we will compare to other browser once this lands.

Also, how this affects performance.

(In reply to Jan Varga [:janv] from comment #61) > I somehow lost track of this. Can someone quickly summarize purpose of this > new service, is it a temporary solution or something we want to keep and > evolve, especially regarding with comment 19. > What trade-offs you had to make and how we will compare to other browser > once this lands. I view the StorageActivityService as an incremental effort to improve the situation for our multiple ways for users to retroactively clean up recent browsing history. Ideally everyone would know about private browsing and use it, but even if that was the policy, people can and will to forget to use private browsing and so we need a way to wipe the last N hours/what not. As per my (partial) investigation/summary at https://public.etherpad-mozilla.org/p/data-clearing-apis there are a bunch of UI affordances for this. Without this service, as referenced in comment 3, our options are either to clear all persistent offline-storage that's not inherently time-tracked, or to not clear it at all. As referenced a bit in comment 19 and related discussion, QuotaManager seems like a good long-term place to track this data, especially since it can easily have a persistent understanding of what was recently modified that survives reboots. However, LocalStorage is not yet QM-managed, and ServiceWorkers have additional statefulness in terms of the non-QM managed ServiceWorkerRegistrar storage. (In particular, the dom/cache implementation's QuotaClient does not yet cause existing SW's to be unregistered and terminated if storage for an origin gets cleared.) Cookies of course, are already time-stamped and so not tracked by this subsystem. :baku had proposed the introduction of the service as a stop-gap to quickly let us wipe offline storage that we could then iterate upon. Because of the multiple data clearing mechanisms and the ad-hoc layered legacy of our observer notification schemes, this has taken a bit more work and time than originally hoped for. (Also, tests :) But I think it does leave us in a much better state going forward, and it's something we can refactor to be more directly implemented by QuotaManager. In particular, the new service and its ability to enumerate recently modified origins potentially allows for a better UX for erasure of recent data, with user control over what gets wiped.

(In reply to Jan Varga [:janv] from comment #62) > Also, how this affects performance. The service is in-memory only, so it's really a question of the number of runnables dispatched when in the parent process, and the number of IPC messages dispatched (and resulting) runnables when in the child process. In practice, I think everything happens in the parent process because we were able to piggy-back the LocalStorage notifications on top if the existing broadcast IPC messages and the ServiceWorkerRegistrar and all QuotaManager clients exist in the parent. Which really leaves the QuotaManager QuotaObject patch as the big performance question. I believe :baku has attempted to optimize performance by only setting a boolean flag when LockedMaybeUpdateSize is invoked, since that should happen when writes happen. However, I worry that only flushing the write at QuotaObject destruction time may fail to capture pending/in-flight write activities in the origin, which is why I proposed also sending activity when the write-flag is first set. For long-lived QuotaObjects, we might potentially want to report even more frequently than first-mutation and destruction time. Reporting every time could be too frequently and have a performance, which is why I proposed that some timestamp-based mechanism might be appropriate. Your expertise (and review! :) would be appreciated there. I think the long-term path of the storage activity service and related issues would be a great thing for interested parties to discuss at the all-hands. (With Bevis also being essential, and maybe we can vidyo-loop-in bkelly for dom/cache and SW issues, plus :ttung in person for SW discussion too.)

Ok, that helps a lot. Thanks!

> The service is in-memory only, so it's really a question of the number of > runnables dispatched when in the parent process, and the number of IPC > messages dispatched (and resulting) runnables when in the child process. Probably it's not going to be in-memory only. I would like to make it persistent in order to support quick close-restart of the browser.

Comment on attachment 8935039 [details] [diff] [review] part 8 - tests Review of attachment 8935039 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to provide a part 8.1 later today for you to merge in with the tests. I think there was some confusion about my comments on the setInterval issue in Comment 48 that are probably easiest for me to address directly, plus I'd like to try not disabling the other sanitize preferences since, as noted, that choice hid dangerous side-effects that I think we absolutely do want tests to catch. This will be an r+ with those changes, from my perspective. That said, once bug 1361330 lands the test-only simple quota client and gives us an easy way to test QM-related edge-cases, we probably will want to add more to the test leveraging the simple quota client.

Here are the test fixes I was thinking of, plus some related stability changes like making sure we reset the storage activity service at the start of our test so we don't get confused by other tests that ran prior to us. Unfortunately, it seems like the setInterval(1000) was papering over some races. I'm seeing a tremendous number of recursive deletion errors output in the logs, and at least one time I saw the cache/ subtree survive the attempt at deletion. In some other cases it (or IDB) appears to survive the first attempt to delete it, but the second time we try and sanitize things does end up working (because the is() checks don't throw, they just indicate failure, so we keep going). I haven't done enough investigation to understand whether the problem is one of serviceworker coordination on the unregistration as it relates to the things we're waiting on, or if this could be more of an issue with QM operations not having terminated yet, etc.

Comment on attachment 8913147 [details] [diff] [review] part 5 - QuotaManager Review of attachment 8913147 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine as a temporary solution until we have LocalStorage plugged into QM. ::: dom/quota/ActorsParent.cpp @@ +3000,5 @@ > QuotaManager* quotaManager = QuotaManager::Get(); > MOZ_ASSERT(quotaManager); > > quotaManager->mQuotaMutex.AssertCurrentThreadOwns(); > + mWritingDone = true; Well, I'm not sure how we should optimize here, if you are ok with calling SendActivity in the destructor then I think it won't hurt performance much, so I'm ok with that for now. But for correctness from activity service point of view we might need to call it as asuth suggested or maybe even at each LockedMaybeUpdateSize. There's one more thing, calling SendActivity in QuotaObject should cover must of the cases, however there's a case when an IndexedDB database is deleted, in that case a quota object is not used, we call QuotaManager::DecreaseUsageForOrigin() instead, not sure if you want to cover that too. ::: dom/storage/StorageActivityService.cpp @@ +70,5 @@ > + service->SendActivityInternal(origin); > + }); > + > + if (NS_IsMainThread()) { > + r->Run(); Unused << r->Run();

asuth, I would land these patches without yours. Mine are currently green on try. I also merged your sas.testOnlyReset(). Are you OK with this? After these patches landed, I want to make sas able to write the list of active principals as discussed during the all hands.

Of course I also changed the quota patch as suggested.

Sure. I'll file a follow-up and assign to myself to clean-up my test changes and add implementation logic that makes the modified tests to be green. I do think we want/need that extra coverage, so I'll try and make sure to get that in before your persistence follow-up happens.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4955cf6447f StorageActivityService - part 1 - Introduce StorageActivityService to monitor origin activities, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/609093736110 StorageActivityService - part 2 - Use of StorageActivityService in LocalStorage, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/5be23a98b47c StorageActivityService - part 3 - ServiceWorkerManager must not cleanup data when purge-session-history notification is dispatched, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/da38d133549e StorageActivityService - part 4 - Introduce ServiceWorkerCleanUp.jsm to clean up ServiceWorker data, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/92e8cc81b417 StorageActivityService - part 5 - nsIStorageActivityService::getActiveOrigins, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/a45e28e573a4 StorageActivityService - part 6 - StorageActivityService in ServiceWorkerRegistrar, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd2523680d1 StorageActivityService - part 7 - StorageActivityService in QuotaManager, r=janv https://hg.mozilla.org/integration/mozilla-inbound/rev/5875848a48ab StorageActivityService - part 8 - Sanitize tests in a separate folder, r=gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/aa511b206e21 StorageActivityService - part 9 - nsIStorageActivityService::moveOriginInTime() for testing, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/a3e5299b882a StorageActivityService - part 10 - Test for nsIStorageActivityService, r=gijs, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ec63c01c50 StorageActivityService - part 11 - nsIStorageActivityService::testOnlyReset() for testing, r=asuth

Backed out 11 changesets (bug 1252998) for faling browser-chrome on browser/base/content/test/sanitize/browser_sanitize-offlineData.js https://treeherder.mozilla.org/logviewer.html#?job_id=154685706&repo=mozilla-inbound&lineNumber=2717 https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0c739344f216aa733dc1c540cb69bc9193db71 https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a9ec63c01c5073dc71a8b708b0690b14b633558f&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Since this is also affecting the exact patches I need to provide for bug 1415342, what's the likely ETA for relanding this?

Comment on attachment 8934500 [details] [diff] [review] part 3 - getActiveOrigins Review of attachment 8934500 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/sanitize.js @@ +323,5 @@ > // LocalStorage > Services.obs.notifyObservers(null, "extension:purge-localStorage"); > > // ServiceWorkers > await ServiceWorkerCleanUp.removeAll(); Maybe there's another changeset which removes this but I don't think you should be doing this here (maybe in an else block to if (range)?). I'd wager that's failing your test. I would really like this to land in 61. I can take a stab at unbitrotting it (a lot of the bitrot is probably from my changes). :)

I rebased all the patches but I don't have time to fix the last test. Wonder if you want/can continue from here.

That was probably meant for me :D I'll try to push this over the finish line next week!

This should do it. The problem was just that the Sanitizer implementation slightly changed underneath (to not accept global attributes anymore). Let's see if try is happy and then we can land...

The changes to ext-BrowsingData.js need rebasing for bug 1372406.

(In reply to Jan Henning [:JanH] from comment #91) > The changes to ext-BrowsingData.js need rebasing for bug 1372406. It was moved using hg so it should (and does afaict) apply cleanly.

Baku, see commit message, I was able to resolve some of the issues with that tests but had to opt for ignoring two of them. At least for the LocalStorage issues I think I have a good idea how they are caused and it's not related to your test. At this point I'm just hoping for bug 1286798 to land soon so that we can add reliable tests for clearing localStorage (I don't think there are any in the tree). I'm happy to file follow-up bugs as you see fit. Thanks!

With this patch tests should be passing now. Please note that I couldn't rebase on the latest central since it had some merge conflicts with worker code in some of your patches that I wasn't perfectly sure how to resolve. It would be helpful if you could do the rebase as soon as you're happy with my patches :) Thanks!

Comment on attachment 8967560 [details] [diff] [review] part 9 - Fix sanitize-offlineData test failures, move SW utility functions to SiteDataTestUtils.jsm. r=baku Review of attachment 8967560 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/sanitize/browser_sanitize-offlineData.js @@ +71,5 @@ > +add_task(async function testWithRange() { > + // We have intermittent occurrences of NS_ERROR_ABORT being > + // thrown at closing database instances when using Santizer.sanitize(). > + // This does not seem to impact cleanup, since our tests run fine anyway. > + PromiseTestUtils.whitelistRejectionsGlobally(/NS_ERROR_ABORT/); If we want to land this, I would like to have a bug filed and written here.

Comment on attachment 8967563 [details] [diff] [review] part 10 - Use hosts instead of principals to delete ServiceWorkers in the SiteDataManager. r=baku Review of attachment 8967563 [details] [diff] [review]: ----------------------------------------------------------------- OK, I'll rebase all these patches. Do you want me to land this code or you want to do it?

All the patches renamed and rebased.

(In reply to Andrea Marchesini [:baku] from comment #98) > Comment on attachment 8967560 [details] [diff] [review] > part 9 - Fix sanitize-offlineData test failures, move SW utility functions > to SiteDataTestUtils.jsm. r=baku > > Review of attachment 8967560 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/test/sanitize/browser_sanitize-offlineData.js > @@ +71,5 @@ > > +add_task(async function testWithRange() { > > + // We have intermittent occurrences of NS_ERROR_ABORT being > > + // thrown at closing database instances when using Santizer.sanitize(). > > + // This does not seem to impact cleanup, since our tests run fine anyway. > > + PromiseTestUtils.whitelistRejectionsGlobally(/NS_ERROR_ABORT/); > > If we want to land this, I would like to have a bug filed and written here. I filed bug 1453901. Since you already have the patches in your tree, can you add the bug and land this? Thank you!

Hey baku, since the QuantumRender failures are from bug 1454531, are you landing this or would you like me to do it? :) Thanks!

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/558fef850d42 StorageActivityService - part 1 - Introduce StorageActivityService to monitor origin activities, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/3220024f0ed3 StorageActivityService - part 2 - Use of StorageActivityService in LocalStorage, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/903099bf9c40 StorageActivityService - part 3 - ServiceWorkerManager must not cleanup data when purge-session-history notification is dispatched, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/f60bc2fa2566 StorageActivityService - part 4 - Introduce ServiceWorkerCleanUp.jsm to clean up ServiceWorker data, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/4367167b71d8 StorageActivityService - part 5 - nsIStorageActivityService::getActiveOrigins, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/185d6fcf4eee StorageActivityService - part 6 - StorageActivityService in ServiceWorkerRegistrar, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/830a2e991b0c StorageActivityService - part 7 - StorageActivityService in QuotaManager, r=janv https://hg.mozilla.org/integration/mozilla-inbound/rev/428f49f692ce StorageActivityService - part 8 - nsIStorageActivityService::moveOriginInTime() for testing, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/38653c75863b StorageActivityService - part 9 - Test for nsIStorageActivityService, r=gijs, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/db8bf70e7847 Fix sanitize-offlineData test failures, move SW utility functions to SiteDataTestUtils.jsm. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/0a880af67162 Use hosts instead of principals to delete ServiceWorkers in the SiteDataManager. r=baku

https://hg.mozilla.org/mozilla-central/rev/558fef850d42 https://hg.mozilla.org/mozilla-central/rev/3220024f0ed3 https://hg.mozilla.org/mozilla-central/rev/903099bf9c40 https://hg.mozilla.org/mozilla-central/rev/f60bc2fa2566 https://hg.mozilla.org/mozilla-central/rev/4367167b71d8 https://hg.mozilla.org/mozilla-central/rev/185d6fcf4eee https://hg.mozilla.org/mozilla-central/rev/830a2e991b0c https://hg.mozilla.org/mozilla-central/rev/428f49f692ce https://hg.mozilla.org/mozilla-central/rev/38653c75863b https://hg.mozilla.org/mozilla-central/rev/db8bf70e7847 https://hg.mozilla.org/mozilla-central/rev/0a880af67162

This is pretty useful. I've added a note about it here: https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API/Using_Service_Workers#Developer_tools https://developer.mozilla.org/en-US/Firefox/Releases/61#Service_workers Does that sound OK?

Thanks Chris! I think it's good to have these 2 notes.