1047098 - 'Clear Recent History' with 'Cache' or 'Offline Website Data' doesn't clear QuotaManager storage and ServiceWorkers

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Luke Wagner [:luke]]

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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Luke Wagner [:luke]]

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Jan Varga [:janv]]

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

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 13

[donrhummy]

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

Comment 14

[u557809]

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

Comment 16

[donrhummy]

(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

Comment 17

[Jan Varga [:janv]]

That sounds good to me.

Comment 18

[sworddragon2]

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?

Comment 19

[donrhummy]

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

Comment 20

[Nathanaël Naeri]

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

Comment 22

[Elhem Enohpi]

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

Comment 23

[msth67]

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.

Comment 24

[donrhummy]

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

Comment 25

[Elhem Enohpi]

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

Comment 26

[Scott Macpherson]

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

Comment 27

[sworddragon2]

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

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 31

[Christiane Ruetten [INACTIVE]]

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

Comment 32

[Christiane Ruetten [INACTIVE]]

Current press reporting suggests that this is affecting Firefox on Android, too.

[1] https://www.heise.de/-3835084

Comment 33

[Fischer [:Fischer]]

Attachment: 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/

Comment 34

[Fischer [:Fischer]]

Attachment: Bug_1047098_Test.gif

Comment 35

[Fischer [:Fischer]]

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

Comment 36

[Fischer [:Fischer]]

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

Comment 37

[Christiane Ruetten [INACTIVE]]

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

Comment 40

[Christiane Ruetten [INACTIVE]]

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.

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 42

[Christiane Ruetten [INACTIVE]]

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.

Comment 43

[Andrea Marchesini [:baku]]

Attachment: part 1 - QuotaManager

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 44

[Andrea Marchesini [:baku]]

Attachment: part 2 - ServiceWorkers

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 46

[Andrea Marchesini [:baku]]

Attachment: part 1 - QuotaManager

Comment 47

[Andrea Marchesini [:baku]]

Attachment: part 1 - QuotaManager

Comment 48

[Julien Cristau [:jcristau]]

sounds like we want this in 52.4.

Comment 49

[Jan Varga [:janv]]

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

Comment 50

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

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.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
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

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 54

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b73ac232366
Backed out changeset e89d2565799b for xpcshell bustage a=backout

Comment 55

[Liz Henry (:lizzard) (relman/hg->git project)]

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!

Comment 56

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 57

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 58

[Andrew Sutherland [:asuth] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a230cc005b2a6a71f16ef84a55851ee2f4f66e

Comment 59

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 60

[Andrea Marchesini [:baku]]

Andrew thanks you so much!

Comment 62

[Christiane Ruetten [INACTIVE]]

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.

Comment 63

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 64

[Andrea Marchesini [:baku]]

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 65

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910638 [details] [diff] [review]
part 2 - ServiceWorkers

Fixes this issue, taking it.
Should be in 57b3

Comment 66

[Andrea Marchesini [:baku]]

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.

Comment 67

[Julien Cristau [:jcristau]]

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

Comment 68

[Andrea Marchesini [:baku]]

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.

Comment 69

[Sylvestre Ledru [:Sylvestre]]

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

Comment 70

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

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

Comment 72

[Ryan VanderMeulen [:RyanVM]]

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

Comment 73

[Jan Varga [:janv]]

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

Comment 74

[Stefan [:StefanG_QA]]

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)

Comment 75

[Andrew Sutherland [:asuth] (he/him)]

Attachment: release branch fix for ServiceWorkerRegistrar shutdown

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 76

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8911308 [details] [diff] [review]
release branch fix for ServiceWorkerRegistrar shutdown

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

rs+

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/f2dc7e32e4ea

Comment 78

[Matt McCutchen]

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

Comment 80

[Jan Varga [:janv]]

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

Comment 81

[Hani Yacoub, Desktop QA]

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.

Comment 82

[Hani Yacoub, Desktop QA]

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 87

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8910767 [details] [diff] [review]
part 1 - QuotaManager

Taking this to address privacy issues for ESR52.5

Comment 88

[Julien Cristau [:jcristau]]

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.

Comment 89

[Jan Varga [:janv]]

(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

Comment 90

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 91

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 92

[Ryan VanderMeulen [:RyanVM]]

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

Comment 93

[Liz Henry (:lizzard) (relman/hg->git project)]

Extra n-i on myself for this for ESR

Comment 94

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 95

[Andrew Overholt [:overholt]]

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

Comment 96

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 97

[Andrew Overholt [:overholt]]

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

Comment 98

[Jan Varga [:janv]]

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

Comment 99

[Jan Varga [:janv]]

Attachment: Part 1 for ESR52

Comment 100

[Jan Varga [:janv]]

Attachment: Part 2 for ESR52

Comment 101

[Jan Varga [:janv]]

Attachment: Part 3 for ESR52

Comment 102

[Jan Varga [:janv]]

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

Comment 103

[Jan Varga [:janv]]

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

Comment 104

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 105

[Jan Varga [:janv]]

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

Comment 106

[Ryan VanderMeulen [:RyanVM]]

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

Comment 107

[Andrew Overholt [:overholt]]

Note that Service Workers are turned off for esr52.

Comment 108

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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

Comment 109

[Christiane Ruetten [INACTIVE]]

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

[Matt McCutchen]

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.

Comment 111

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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?

Comment 112

[Christiane Ruetten [INACTIVE]]

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

Comment 113

[Christiane Ruetten [INACTIVE]]

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

Comment 114

[Jan Varga [:janv]]

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

Comment 115

[Christiane Ruetten [INACTIVE]]

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

Comment 116

[Ryan VanderMeulen [:RyanVM]]

The rebased patch from bug 1355576 was included in 52.5.0.

Comment 117

[Matt McCutchen]

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

Comment 118

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(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");

Comment 119

[Matt McCutchen]

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

Comment 120

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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