One might expect 'Clear Recent History' (with 'Cache' checked) to clear asm.js cache entries and (with 'Offline Website Data' checked) to clear IDB storage, but neither do. Currently, the only way to clear this is buried deep down in Page Info -> Permissions -> Maintain Offline Storage.

How would this work with time ranges for these stores? For example, only deleting the last hour of IDB changes for an origin seems hard unless we are going to snapshot the quota dir on every change.

The QuotaManager maintains the last access time (which is access, not creation); perhaps it could also maintain creation time? Alternatively, we might be able to use the creation time (returned by PR_GetFileInfo) of the associated storage dir, although perhaps this would be fooled by file movements (version upgrades, etc)?

Whats the desired behavior here? Consider for the moment: 1) IDB database is created 1 month ago. 2) User accumulates data in IDB over the course of the month. 3) Current browsing session modifies the IDB database 1 minute ago. 4) User clicks "forget last 5 minutes". What should happen? 1) Delete the entire IDB database? 2) Backout the edits from the last 5 minutes only? I think (1) would be surprising and (2) might be hard to implement in an efficient way. Note, I'm interested in this because I'm building a new QuotaManager storage engine for the ServiceWorker Cache.

(In reply to Ben Kelly [:bkelly] from comment #5) > Whats the desired behavior here? Consider for the moment: > > 1) IDB database is created 1 month ago. > 2) User accumulates data in IDB over the course of the month. > 3) Current browsing session modifies the IDB database 1 minute ago. > 4) User clicks "forget last 5 minutes". > > What should happen? > > 1) Delete the entire IDB database? > 2) Backout the edits from the last 5 minutes only? > > I think (1) would be surprising and (2) might be hard to implement in an > efficient way. > > Note, I'm interested in this because I'm building a new QuotaManager storage > engine for the ServiceWorker Cache. I don't think doing (2) is a good idea, regardless of implementation problems and performance. QuotaManager doesn't have knowledge of data stored in IDB, so deleting all edits done in last 5 minutes can mess up the database. Even if we reverted whole transactions (some operations can consist of multiple transactions). So, one option is to just support the "Everything" time range for now.

Right, I meant QM would have to call back to the IDB client with a "Forget(timerange)" and then IDB would have to do extra work to implement it. Sounds like a ton of effort.

IDB should be part of "Offline Web Content and User Data". If you clear that, it should clear the IndexedDB too.

Bug 1047098,602743,506692,357704,536544,600367

(In reply to Ben Kelly [:bkelly] from comment #5) > Whats the desired behavior here? Consider for the moment: > > 1) IDB database is created 1 month ago. > 2) User accumulates data in IDB over the course of the month. > 3) Current browsing session modifies the IDB database 1 minute ago. > 4) User clicks "forget last 5 minutes". > > What should happen? > > 1) Delete the entire IDB database? > 2) Backout the edits from the last 5 minutes only? > > I think (1) would be surprising and (2) might be hard to implement in an > efficient way. > > Note, I'm interested in this because I'm building a new QuotaManager storage > engine for the ServiceWorker Cache. I think the solution is this: 1. Show a check box for clearing Offline Storage (including IDB) 2. If user chooses to remove all, then it delets that too 3. If use selects a time-limited option (e.g. last month), then FF does the following: a. If an IDB was created within that time, it's deleted b. If any IDB is older (not nec. the data in it though) than the timeframe, then an alert is popped up warning the user that one or more IDB's are older, shows them a list with checkboxes next to each and asks them to select the ones they want to delete

That sounds good to me.

Is there a special reason to provide the ability to select a timed range for deleting the cache? I wonder how often this is used and if it is effectively really useful for these people. Is there any telemetry data available for this?

(In reply to sworddragon2 from comment #18) > Is there a special reason to provide the ability to select a timed range for > deleting the cache? I wonder how often this is used and if it is effectively > really useful for these people. Is there any telemetry data available for > this? It's very important and should be added to the Android version. Remember it's not just deleting the cache, but offline data (cookies, site settings, etc can be in this)

I've stumbled over this issue too, after I noticed directories named after websites I had visited long ago in <profile>/storage/{default,temporary}, even though my preferences include a Clear All History at exit with everything selected. This was certainly disconcerting, and difficult to research. I'm not against local storage, but it feels like when one requests a total wipe out, they kinda expect a total wipe out. A remark about donrhummy's solution in comment 16: if step 3 is difficult and takes time to implement (it looks like it needs a lot of coding), how about only supporting timerange=Everything for now, as suggested by previous posters? Perhaps with something like: 3. If user selects a time-limited option (e.g. last month), gray out the box for IndexedDB data and don't delete any

(In reply to sworddragon2 from comment #18) > Is there a special reason to provide the ability to select a timed range for > deleting the cache? I wonder how often this is used and if it is effectively > really useful for these people. Is there any telemetry data available for > this? Deleting a timed range of the cache has never actually worked, see bug 646975. It's always completely cleared. IndexedDB should certainly not be treated that way! I think it would be reasonable to delete any databases created within the specified time, and just not touch any older than that. Otherwise, a UI like in comment 16, but 3.b. should only present a list of older databases that have actually been modified within the specified time range. An in-between method could be to simply show an alert that the following older databases have been modified within the time range, but won't be deleted, which is likely what the user wants.

What the user likely wants when clearing all data is this: >it feels like when one requests a total wipe out, they kinda expect a total wipe out. which isn't happening as we speak. If I choose to delete all data (especially *local storage*) with time range *everything* , I don't want to see anything left there, not just read an alert that something has been modified -unless I misunderstood your comment above, in which case I apologize.

(In reply to msth67 from comment #23) > What the user likely wants when clearing all data is this: > > >it feels like when one requests a total wipe out, they kinda expect a total wipe out. > > which isn't happening as we speak. > > If I choose to delete all data (especially *local storage*) with time range > *everything* , I don't want to see anything left there, not just read an > alert that something has been modified -unless I misunderstood your comment > above, in which case I apologize. The issue with the timerange is that IDB does not keep track of exactly what data has been added/edited/deleted during time frames. And even if it did, how would you undo the changes? It would need to keep full copies of all data and their past changes just like a source control system. That's not possible and would take too much space on the drive. So the databases can only be relied on to give creation time, and nothing more.

(In reply to msth67 from comment #23) > If I choose to delete all data (especially *local storage*) with time range > *everything* , I don't want to see anything left there, not just read an > alert that something has been modified -unless I misunderstood your comment > above, in which case I apologize. If the user chooses to clear "Everything", all databases should simply be erased immediately, no question. My comment above was about when a time range like "Last Hour" is chosen, and there's a database that has six months worth of data in it, which has been modified in the last hour. Since it's not practical to delete the last hour's worth of transactions, what should be done? (In reply to donrhummy from comment #24) > So the databases can only be relied on to give creation time, > and nothing more. According to comment 4, there's not even a reliable method to get the creation time. At this point, the suggestion in comment 6 to at least support the "Everything" option for now, seems like a good idea. The security/privacy risk of sites using IndexedDB for "supercookies" etc., is mitigated by the fact that the user has to authorize the creation of any persistent databases via an alert. Still, there should be a UI to delete them if desired.

(In reply to Elhem Enohpi from comment #25) > The security/privacy > risk of sites using IndexedDB for "supercookies" etc., is mitigated by the > fact that the user has to authorize the creation of any persistent databases > via an alert. Still, there should be a UI to delete them if desired. Except that doesn't work as users expect either… (bug 959985)

(In reply to Elhem Enohpi from comment #25) > The security/privacy > risk of sites using IndexedDB for "supercookies" etc., is mitigated by the > fact that the user has to authorize the creation of any persistent databases > via an alert. Is this still the case? Bug #1400582 implies that sites are able to create supercookies via IndexedDB without requiring the user to authorize it.

(In reply to sworddragon2 from comment #27) > (In reply to Elhem Enohpi from comment #25) > > The security/privacy > > risk of sites using IndexedDB for "supercookies" etc., is mitigated by the > > fact that the user has to authorize the creation of any persistent databases > > via an alert. > > Is this still the case? Bug #1400582 implies that sites are able to create > supercookies via IndexedDB without requiring the user to authorize it. We currently require a permission for "persistent" data which means the browser cannot purge it under storage pressure. Default/temporary storage does not require a permission. This default storage, though, often lives long enough to work for tracking purposes. I filed a separate bug to look at adding the ability to block default storage permissions. See bug 1400679. Regardless, though, the "clear recent history" feature is quite hard to make work with storage like IDB. It would be very costly to maintain rolling check points in order to revert storage changes across "recent" time periods. Probably the best we could do is note that the storage had been written in this period and give the option to wipe everything for the origin.

(In reply to sworddragon2 from comment #27) > (In reply to Elhem Enohpi from comment #25) > > The security/privacy > > risk of sites using IndexedDB for "supercookies" etc., is mitigated by the > > fact that the user has to authorize the creation of any persistent databases > > via an alert. > > Is this still the case? Bug #1400582 implies that sites are able to create > supercookies via IndexedDB without requiring the user to authorize it. Not that this is the only way, but it effectively results in a silent, hard-to-discard IDB-based supercookie and it still affects Firefox 55 and 57 which I tested today. STR: - Create fresh profile - Go to http://cr.23bit.net/idb.html - Reload a few times to accumulate visit timestamps in Indexed DB - Open Storage Inspector (Shift-F9) - Select website under Cookies and click "+" (below "Debugger") to add a few random cookies - Select website under Local Storage and click "+" to add some random data - Select "History / Clear Recent History" from Menu (Ctrl-Shift-Del on Windows) - Choose time range "Everything" and ensure that everything under "Details" is ticked, including "Offline Website Data" - Re-open Storage Inspector to update its view Expected result: - Data under Cookies, Indexed DB and Local Storage is gone. - Reloaded website forgets about previous visits. Actual result: - Databases under Indexed DB persist. - Reloaded website remembers previous visits.

Current press reporting suggests that this is affecting Firefox on Android, too. [1] https://www.heise.de/-3835084

(In reply to Fischer [:Fischer] from comment #33) > Created attachment 8910117 [details] > Bug 1047098 - Test to clear quota manger when clearing all history > > Review commit: https://reviewboard.mozilla.org/r/181606/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/181606/ Just a quick test patch. Clear Quota Manager when choosing to clear "Everything" timestamp in the history dialog. The test steps as below: 1. Visit https://static.raymondcamden.com/demos/2014/feb/7/index.html#/home and save some indexedDB data 2. Open the devtool storage panel, making sure did save some indexedDB data 3. Open the History > Clear Recent History dialog and choose clear "Everything" timestamp and then accept to clear 4. Open the devtool storage panel again, making sure no indexedDB data This dosen't cover localStorage because the work of integrating localStorage into Quota Manager is in progress and the api `SiteDataManager.removeAll` used in this patch is to clear quota usage. After the bug 742822 and 1286798 landed, localStorage usage should be included so currently only deal with indexedDB usage. And still more to discuss about how is the desire UX behavior to remove data.

(In reply to Fischer [:Fischer] from comment #35) > (In reply to Fischer [:Fischer] from comment #33) > > Created attachment 8910117 [details] > > Bug 1047098 - Test to clear quota manger when clearing all history > > > > Review commit: https://reviewboard.mozilla.org/r/181606/diff/#index_header > > See other reviews: https://reviewboard.mozilla.org/r/181606/ > Just a quick test patch. > Clear Quota Manager when choosing to clear "Everything" timestamp in the > history dialog. > > The test steps as below: > 1. Visit https://static.raymondcamden.com/demos/2014/feb/7/index.html#/home > and save some indexedDB data > 2. Open the devtool storage panel, making sure did save some indexedDB data > 3. Open the History > Clear Recent History dialog and choose clear > "Everything" timestamp and then accept to clear > 4. Open the devtool storage panel again, making sure no indexedDB data See attachment 8910119 [details]: Bug_1047098_Test.gif for the test in action > > This dosen't cover localStorage because the work of integrating localStorage > into Quota Manager is in progress and the api `SiteDataManager.removeAll` > used in this patch is to clear quota usage. > After the bug 742822 and 1286798 landed, localStorage usage should be > included so currently only deal with indexedDB usage. > > And still more to discuss about how is the desire UX behavior to remove data.

In yesterday's Nightly the `Clean Recent History` dialog's behavior is rather bizarre: - `Cookies` by itself deletes just the cookies. - `Cookies` + `Offline Website Data` clears cookies and Local Storage. - `Offline Website Data` by itself does nothing(*). (*) I can currently only test what Storage Inspector reveals (Cache Storage, Cookies, Indexed DB, Local Storage, Session Storage).

The rabbit hole seems to go a little deeper still. about:preferences#privacy / Site Data / Clear All Data seems to leave data behind as well. See bug 1401542.

Sounds like this likely affects 56 so I will mark it as a blocker.

Firefox 52.3.0 and 56.0 also retain Indexed DB data. They also have the odd requirement to pair offline website data deletion with cookie deletion to produce any effect.

Note that nsIQuotaManagerService::clearContentData is implemented in bug 1401850. But it's not reviewed yet. I'll ask a review when bug 1401850 is resolved.

Comment on attachment 8910638 [details] [diff] [review] part 2 - ServiceWorkers Review of attachment 8910638 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/sanitize.js @@ +291,5 @@ > Components.utils.import("resource:///modules/offlineAppCache.jsm"); > // This doesn't wait for the cleanup to be complete. > OfflineAppCacheHelper.clear(); > > + // Iterate through the service workers and remove them. We should really fix bug 1183245 instead. Can you add a TODO comment here to remove this once that bug is completed? And also comment on that bug about this.

sounds like we want this in 52.4.

Ok, looking at the patch one more time...

(In reply to Julien Cristau [:jcristau] from comment #48) > sounds like we want this in 52.4. For that we need to backport bug 1348660.

Comment on attachment 8910767 [details] [diff] [review] part 1 - QuotaManager Review of attachment 8910767 [details] [diff] [review]: ----------------------------------------------------------------- Ok, here we want to delete everything managed by quota manager. ::: browser/base/content/sanitize.js @@ +307,5 @@ > + resolve(); > + }); > + }); > + > + return Promise.all(promises); Here we could also clear LocalStorage ? But there were some many discussions about this recently so I'm not sure. In theory we don't have to wait for localStorage integration and just call; Services.obs.notifyObservers(null, "extension:purge-localStorage"); We do the same for app cache above and we even don't wait for the operation to finish.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbe00ad0203 "Clear Recent History" must always clean up QuotaManager data, r=janv https://hg.mozilla.org/integration/mozilla-inbound/rev/e89d2565799b "Clear Recent History" must clean up all the ServiceWorkers, r=bkelly

Backed out just e89d2565799b for xpcshell bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=132553527&repo=mozilla-inbound

Backout by kwierso@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b73ac232366 Backed out changeset e89d2565799b for xpcshell bustage a=backout

Once this sticks on m-c please request uplift to mozilla-release (and m-b). I need to build 56 RC2 today so we have time for testing, but will need to do another 56 build tomorrow (or as quickly as possible after this and the patches in bug 1348660 land on m-r) Thanks!

I think :baku pinged me about this, so I looked at it. I can reproduce this. The problem is that ServiceWorkerRegistrar::ProfileStopped is fast-bailing because the PBackground thread does not exist to send the shutdown request to, but this leaves the AsyncShutdown blocker in place. (ServiceWorkerRegistrar coordinates shutdown on the PBackground thread because that's the thread that mutations to its state happen on, as initiated by the PBackground ServiceWorkerManagerService which hears about changes from the main-thread ServiceWorkerManager instances.)

The reason this started happening was the change to toolkit/components/places/tests/head_common.js that changed do_get_profile(notifyProfileAfterChange=false) to do_get_profile(true). This change makes the ServiceWorkerRegistrar load its data from disk, which is necessary to avoid calls like ServiceWorkerManger::GetAllRegistrations() from hanging as they wait forever on the monitor to notify the completion of a load that never starts, but it makes the shutdown hang possible. Note that while ServiceWorkerManager::Init() unconditionally invokes BackgroundChild::GetOrCreateForCurrentThread(), ServiceWorkerManager is not initialized in the failing code path. However, ServiceWorkerRegistrar is because nsLayoutStatics::Initialize() invokes ServiceWorkerRegistrar::Initialize() in all cases. I have a locally working fix that modifies this fast-path edge case to directly trigger the shutdown that I'll push to try shortly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/14cf84cd05639c0bd7db54118102b1742e90cadf Bug 1047098 - "Clear Recent History" must clean up all the ServiceWorkers, r=bkelly

Andrew thanks you so much!

I can confirm that baku's patch resolves the data persistence problem for my tests. It also gets rid of the odd requirement of having to combine "Offline Website Data" with "Cookies" to produce any effect. "Offline Website Data" alone now clears both Local Storage and Indexed DB. All I can still observe is the weirdness about Dev Tools / Storage Inspector retaining state about cookies and Indexed DB database names (not tables). See bug 1401542 for that.

https://hg.mozilla.org/mozilla-central/rev/0fbe00ad0203 https://hg.mozilla.org/mozilla-central/rev/14cf84cd0563

Comment on attachment 8910638 [details] [diff] [review] part 2 - ServiceWorkers Approval Request Comment [Feature/Bug causing the regression]: "Clear Recent History" [User impact if declined]: indexedDB, ServiceWorkers, DOM Cache data are not deleted. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: not yet. [Needs manual test from QE? If yes, steps to reproduce]: See comment 31 [List of other uplifts needed for the feature/fix]:none [Is the change risky?]: low [Why is the change risky/not risky?]: We use QuotaManager API to remove 1 by 1 all the principals. This code is already used elsewhere for testing. [String changes made/needed]: none

Comment on attachment 8910638 [details] [diff] [review] part 2 - ServiceWorkers Fixes this issue, taking it. Should be in 57b3

Comment on attachment 8910638 [details] [diff] [review] part 2 - ServiceWorkers [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Fix Landed on Version: 57 Risk to taking this patch (and alternatives if risky): low. String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

(In reply to Andrea Marchesini [:baku] from comment #66) > Risk to taking this patch (and alternatives if risky): low. In light of the dependency on bug 1348660, does the "low" risk for the 52 uplift still stand? We don't have much time to iterate here, as we're already overdue for the 52.4 build. My preference would be to defer this to 52.5 in November if possible.

I didn't know we have to uplift bug 1348660. I let janv to say if this is a low or high risk. I still think that having this patch in 52 is important for privacy concern.

Seems that we will have to land also bug 1350564 which is a regression from bug 1348660

Comment on attachment 8910767 [details] [diff] [review] part 1 - QuotaManager From discussion with baku and overholt just now on Vidyo we want parts 1 and 2 for mozilla-release.

https://hg.mozilla.org/releases/mozilla-beta/rev/4fc491de353a https://hg.mozilla.org/releases/mozilla-beta/rev/a98d099fc079

https://hg.mozilla.org/releases/mozilla-release/rev/d923bde8d002 https://hg.mozilla.org/releases/mozilla-release/rev/1a93e2c21fac

(In reply to Andrea Marchesini [:baku] from comment #68) > I didn't know we have to uplift bug 1348660. I let janv to say if this is a > low or high risk. > I still think that having this patch in 52 is important for privacy concern. It's a bit riskier because we have to backport a C++ patch too, also a followup crash fix. However, this stuff landed long time ago and we didn't see any other regressions except the crash that was fixed.

We were able to reproduce the issue on Nightly build from 2017-09-20. Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 (20170920111019) We have verified the issue as fixed on today Nightly build on the following platforms: Windows 7 - 32-bits, Windows 10 - 64-bits, Ubuntu 16.04 - 64-bits and Mac OS X 10.12. Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 (20170922100051)

My fix for the ServiceWorkerRegistrar shutdown case was not uplift-safe due to a change in how ServiceWorkerRegistrar handled shutdown. On release it spins its own nested event loop rather than using the AsyncShutdown mechanism. The test failures were due to not initializing the mShutdownCompleteFlag to point at something. xpcshell tests experiencing the testing edge case running under optimized builds would end up writing to the null memory address. Debug builds would end up asserting that the pointer had no value. I've reordered the ProfileStopped method so the pointer is initialized earlier. I also added a death-grip in the observer call. I initially added it because of a hasty analysis of the problem, but the use of ClearOnShutdown() with its default shutdown phase should happen strictly after the profile-before-change notification should keep things safe. I left it there because of the nested event loop spinning and not wanting to have to think about that.

Comment on attachment 8911308 [details] [diff] [review] release branch fix for ServiceWorkerRegistrar shutdown Review of attachment 8911308 [details] [diff] [review]: ----------------------------------------------------------------- rs+

Updating the summary to reflect that clearing of ServiceWorkers was fixed in this bug. If I'm misunderstanding, please revert and undupe bug 1252998.

I'm backporting patches for bug 1348660, just in case we decide to take this for 52.

Build ID: 20170922200134 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 Verified as fixed on Firefox candidate 56.0(build 4) on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64. Based on comment 74 i'll put the tracking flag for firefox58 also.

Build ID: 20170925150345 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Verified as fixed on Firefox Developer Edition 57.0b3 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.

Comment on attachment 8910767 [details] [diff] [review] part 1 - QuotaManager Taking this to address privacy issues for ESR52.5

Note that this caused crashes in 56, bug 1404105, which is only getting fixed in nightly today. So bug 1348660 + this + 1404105 would all need to get uplifted together AIUI.

(In reply to Julien Cristau [:jcristau] from comment #88) > Note that this caused crashes in 56, bug 1404105, which is only getting > fixed in nightly today. So bug 1348660 + this + 1404105 would all need to > get uplifted together AIUI. Also bug 1350564. The order is: bug 1348660, bug 1350564, bug 1047098, bug 1404105

OK, it sounds like our options are to take both patches here, plus the 6 patches in bug 1348660, then the smaller patches from bug 1047098 and bug 1404105 -- Or, don't fix this issue in ESR. Andrew, what do you think?

From discussion on IRC. Andrew shares my feeling of trepidation over the amount of code change needed, but also agrees we need to fix this for ESR users. So, let's give it a try.

This needs a rebased patch for ESR52. Feel free to fold in bug 1404105 into it as well. Be sure to run xpcshell and mochitest-bc on it since my initial attempt at rebasing it resulting in very angry test_clearHistory_shutdown.js and browser_sanitizeDialog.js tests. And ESLint since "yield" is still in use on ESR52 :).

Extra n-i on myself for this for ESR

baku, overholt, ESR go to build is next Monday so this would need to land by Monday morning pacific time.

Looks like this isn't getting into this ESR release.

I can hold off for a bit (even until tomorrow) Is our other option backing out the rest of the patches across 4 different bugs?

(Clearing nedinfo as Liz and I spoke on IRC and I've emailed some people to see if we can make it before California's morning on the 7th)

I'm going to take a look (rebase + basic testing) right now ...

(In reply to Ryan VanderMeulen [:RyanVM] from comment #92) > This needs a rebased patch for ESR52. Feel free to fold in bug 1404105 into > it as well. I rebased part 1,2,3 and attached to this bug. I also attached a rebased patch in bug 1404105. > > Be sure to run xpcshell and mochitest-bc on it since my initial attempt at > rebasing it resulting in very angry test_clearHistory_shutdown.js and > browser_sanitizeDialog.js tests. And ESLint since "yield" is still in use on > ESR52 :). I did some basic testing, including browser_sanitizeDialog.js and test_clearHistory_shutdown.js Not sure what needs to be done for ESLint.

Please note that for full functionality we also need bug 1355576. Otherwise LocalStorage data won't be cleared (only QuotaManager storage and ServiceWorkers).

I am getting more and more uneasy about the risk to ESR here vs. the known problem with fingerprinting as we keep needing to add more patches. Jan, or baku, can you request uplift in bug 1355576, if you think we should do that? Would a partial fix here be ok? We can also have the option of mentioning the remaining known problem in the ESR release notes.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #104) > I am getting more and more uneasy about the risk to ESR here vs. the known > problem with fingerprinting as we keep needing to add more patches. > Jan, or baku, can you request uplift in bug 1355576, if you think we should > do that? > > Would a partial fix here be ok? We can also have the option of mentioning > the remaining known problem in the ESR release notes. Actually, we don't need the whole patch from bug 1355576, only a small piece of C++ code. I'll request approval for that.

https://hg.mozilla.org/releases/mozilla-esr52/rev/088577f0c46e https://hg.mozilla.org/releases/mozilla-esr52/rev/c7e3abf74023 https://hg.mozilla.org/releases/mozilla-esr52/rev/36bd15d14c5a

Note that Service Workers are turned off for esr52.

(In reply to Christiane Ruetten [:cr] from comment #31) > > - Select website under Cookies and click "+" (below "Debugger") to add a few > random cookies > - Select website under Local Storage and click "+" to add some random data The "+" button is missing from the Storage Inspector in the ESR builds. Are there any other methods to cover these 2 steps without the "+" button? Or is there a pref which can enable it?

(In reply to Iulia Cristescu, QA [:JuliaC] from comment #108) > The "+" button is missing from the Storage Inspector in the ESR builds. Are > there any other methods to cover these 2 steps without the "+" button? Or is > there a pref which can enable it? I don't expect that the functionality to add random data in Storage Inspector would be hidden away behind a pref. It likely means that it wasn't around, yet, when 52 was hot. This makes it rather hard to test and sounds like it would require writing a custom web app for your particular test case. I did so for testing IndexedDB at http://cr.23bit.net/idb.html , but I don't think it will run on 52. The only code base I can offer for writing your own little web app that adds all the data that you're interested in is at https://github.com/cr/StorageTester/blob/master/src/sidebar/sidebar.js . Unfortunately it doesn't cover adding cookies, because it is a web extensions, and web extensions do not have cookies. It should be easily extensible, though.

Comment from the peanut gallery: > This makes it rather hard to test and sounds like it would require writing a custom web app for your particular test case. Wouldn't it work to manually run a few JavaScript commands in the developer console to test cookies and local storage? E.g.: document.cookie = "test1047098cookie=testValue; expires=Tue, 19 Jan 2038 03:14:07 GMT"; localStorage.setItem("test1047098local", "testValue"); // Do the "Clear Recent History". document.cookie; // Should not contain test1047098cookie . localStorage.getItem("test1047098local"); // Should return null. In any case, if I understand comment #31 correctly, cookies and local storage were already working and the important thing to test as part of this bug is IndexedDB.

Focusing only on the IndexedDB related steps from comment 31, I managed to reproduce the initial issue on 52.4.1esr build1 (20171005074949) and also to confirm that 52.5.0esr build2 (20171107091003) is verified fixed using Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64. On the other side, using the above JavaScript commands to test cookies and local storage, I get the same results on the both esr builds: document.cookie; // Should not contain test1047098cookie -> the results contains test1047098cookie localStorage.getItem("test1047098local"); // Should return null -> the result is not null. Are these relevant? How should they be assessed?

(In reply to Matt McCutchen from comment #110) > Comment from the peanut gallery: > > > This makes it rather hard to test and sounds like it would require writing a custom web app for your particular test case. > > Wouldn't it work to manually run a few JavaScript commands in the developer > console to test cookies and local storage? Right, it's not hard at all for the sync APIs > In any case, if I understand comment #31 correctly, cookies and local > storage were already working and the important thing to test as part of this > bug is IndexedDB. When I tried my old idb.js script on current release FF, it would bail out with an error. Didn't expect it to run on ESR.

(In reply to Iulia Cristescu, QA [:JuliaC] from comment #111) > On the other side, using the above JavaScript commands to test cookies and > local storage, I get the same results on the both esr builds: > document.cookie; // Should not contain test1047098cookie -> the results > contains test1047098cookie > localStorage.getItem("test1047098local"); // Should return null -> the > result is not null. > Are these relevant? How should they be assessed? Clearing all the cookies and offline website data should get rid of those in ESR, too, so unless the test procedure was flawed this sounds like sanitize.js is not properly clearing cookies and localStorage. Baku, did your original patch build on patches that landed since ESR?

Support for clearing LocalStorage landed in ESR52 in bug 1355576, but I'm not sure if it's in 52.5.0esr.

Reluctant to state the obvious, but then it would require uplifting to get history clearing fixed completely in ESR.

The rebased patch from bug 1355576 was included in 52.5.0.

(In reply to Iulia Cristescu, QA [:JuliaC] from comment #111) > On the other side, using the above JavaScript commands to test cookies and > local storage, I get the same results on the both esr builds: > document.cookie; // Should not contain test1047098cookie -> the results > contains test1047098cookie > localStorage.getItem("test1047098local"); // Should return null -> the > result is not null. > Are these relevant? How should they be assessed? I ran this test on 52.5.0esr build2 x86_64 on Fedora 25, and the cookie and local storage item were correctly cleared. Iulia, are you sure you ran the "Clear Recent History" command (as in comment 31) between the first two JavaScript commands and the last two JavaScript commands? (Sorry if I've just slowed things down by trying to help...)

(In reply to Matt McCutchen from comment #117) > (In reply to Iulia Cristescu, QA [:JuliaC] from comment #111) > > On the other side, using the above JavaScript commands to test cookies and > > local storage, I get the same results on the both esr builds: > > document.cookie; // Should not contain test1047098cookie -> the results > > contains test1047098cookie > > localStorage.getItem("test1047098local"); // Should return null -> the > > result is not null. > > Are these relevant? How should they be assessed? > > I ran this test on 52.5.0esr build2 x86_64 on Fedora 25, and the cookie and > local storage item were correctly cleared. Iulia, are you sure you ran the > "Clear Recent History" command (as in comment 31) between the first two > JavaScript commands and the last two JavaScript commands? (Sorry if I've > just slowed things down by trying to help...) Tested again the fix, using 52.5.0esr build2 (20171107091003) and the steps provided in comment 31 and comment 110, across platforms. I initially confused the // Do the "Clear Recent History" with a comment for the second console entry, so my mistake. These is the console output: - "test1047098cookie=testValue; expires=Tue, 19 Jan 2038 03:14:07 GMT" //after the document.cookie = "test1047098cookie=testValue; expires=Tue, 19 Jan 2038 03:14:07 GMT"; - [Exception... "Component is not available" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1" data: no] //after the localStorage.setItem("test1047098local", "testValue"); -----(cleared all the history) - "test1047098cookie=testValue; expires=Tue, 19 Jan 2038 03:14:07 GMT" //after the document.cookie; - [Exception... "Component is not available" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1" data: no] //after the localStorage.getItem("test1047098local");

(In reply to Iulia Cristescu, QA [:JuliaC] from comment #118) > (In reply to Matt McCutchen from comment #117) > > (In reply to Iulia Cristescu, QA [:JuliaC] from comment #111) > > > On the other side, using the above JavaScript commands to test cookies and > > > local storage, I get the same results on the both esr builds: > > > document.cookie; // Should not contain test1047098cookie -> the results > > > contains test1047098cookie > > > localStorage.getItem("test1047098local"); // Should return null -> the > > > result is not null. > > > Are these relevant? How should they be assessed? > > > > I ran this test on 52.5.0esr build2 x86_64 on Fedora 25, and the cookie and > > local storage item were correctly cleared. Iulia, are you sure you ran the > > "Clear Recent History" command (as in comment 31) between the first two > > JavaScript commands and the last two JavaScript commands? (Sorry if I've > > just slowed things down by trying to help...) > > Tested again the fix, using 52.5.0esr build2 (20171107091003) and the steps > provided in comment 31 and comment 110, across platforms. > I initially confused the // Do the "Clear Recent History" with a comment for > the second console entry, so my mistake. > These is the console output: > - "test1047098cookie=testValue; expires=Tue, 19 Jan 2038 03:14:07 GMT" > //after the document.cookie = "test1047098cookie=testValue; expires=Tue, 19 > Jan 2038 03:14:07 GMT"; > - [Exception... "Component is not available" nsresult: "0x80040111 > (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: debugger eval code :: > <TOP_LEVEL> :: line 1" data: no] //after the > localStorage.setItem("test1047098local", "testValue"); > -----(cleared all the history) > - "test1047098cookie=testValue; expires=Tue, 19 Jan 2038 03:14:07 GMT" > //after the document.cookie; > - [Exception... "Component is not available" nsresult: "0x80040111 > (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: debugger eval code :: > <TOP_LEVEL> :: line 1" data: no] //after the > localStorage.getItem("test1047098local"); Did you run the test on an ordinary web site such as http://cr.23bit.net/idb.html ? I'm able to reproduce the behavior above if I run the test on certain special pages such as about:config and about:preferences. So the full procedure is: - Create fresh profile - Go to http://cr.23bit.net/idb.html - Reload a few times to accumulate visit timestamps in Indexed DB - Run in the developer console (associated with the tab at http://cr.23bit.net/idb.html): document.cookie = "test1047098cookie=testValue; expires=Tue, 19 Jan 2038 03:14:07 GMT"; - Run in the developer console: localStorage.setItem("test1047098local", "testValue"); - Select "History / Clear Recent History" from Menu (Ctrl-Shift-Del on Windows) - Choose time range "Everything" and ensure that everything under "Details" is ticked, including "Offline Website Data" - Run in the developer console: document.cookie; // Should not contain test1047098cookie - Run in the developer console: localStorage.getItem("test1047098local"); // Should return null - Reload the page. The previous timestamps should be gone and you should see only the single timestamp from the page just loaded. I should have written this out on the previous round-trip; sorry again. I'm trying to be helpful, but as an outsider to Mozilla I can't bear the responsibility for delaying the ESR; I'm relying on the other Mozilla folks here to step in and find a different way to proceed if they judge this verification is taking too long.

(In reply to Matt McCutchen from comment #119) > > Did you run the test on an ordinary web site such as > http://cr.23bit.net/idb.html ? I'm able to reproduce the behavior above if > I run the test on certain special pages such as about:config and > about:preferences. So the full procedure is: > > - Create fresh profile > - Go to http://cr.23bit.net/idb.html > - Reload a few times to accumulate visit timestamps in Indexed DB > - Run in the developer console (associated with the tab at > http://cr.23bit.net/idb.html): document.cookie = > "test1047098cookie=testValue; expires=Tue, 19 Jan 2038 03:14:07 GMT"; > - Run in the developer console: localStorage.setItem("test1047098local", > "testValue"); > - Select "History / Clear Recent History" from Menu (Ctrl-Shift-Del on > Windows) > - Choose time range "Everything" and ensure that everything under "Details" > is ticked, including "Offline Website Data" > - Run in the developer console: document.cookie; // Should not contain > test1047098cookie > - Run in the developer console: localStorage.getItem("test1047098local"); > // Should return null > - Reload the page. The previous timestamps should be gone and you should > see only the single timestamp from the page just loaded. > Yes, I used the above mentioned site http://cr.23bit.net/idb.html and exactly these steps. I apparently used the browser console instead of the developer console. Following exactly these steps across the platforms, the results are the expected ones. Thank you for clarifying this and for adding the last step in the above comment. Marking the 52.5.0esr build as verified fixed.