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

VERIFIED FIXED in Firefox -esr52

Status

()

defect
P1
normal
VERIFIED FIXED
5 years ago
a month ago

People

(Reporter: luke, Assigned: baku)

Tracking

(Blocks 2 bugs, {privacy})

unspecified
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox-esr5257+ verified, firefox55 wontfix, firefox56blocking verified, firefox57+ verified, firefox58 verified)

Details

(Whiteboard: [tor][fingerprinting])

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 527667
Blocks: 1102808
Status: RESOLVED → REOPENED
Flags: firefox-backlog+
Resolution: DUPLICATE → ---
Summary: 'Clear Recent History' with 'Cache' or 'Offline Website Data' doesn't clear QuotaManager storage → 'Clear Recent History' with 'Cache' or 'Offline Website Data' doesn't clear QuotaManager storage (indexedDB, asm.js cache)
Duplicate of this bug: 602743
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.
Priority: -- → P1
(Reporter)

Comment 4

5 years ago
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.

Comment 6

5 years ago
(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.

Updated

4 years ago
Duplicate of this bug: 1135370

Updated

4 years ago
Duplicate of this bug: 1146522

Updated

4 years ago
Duplicate of this bug: 986616

Updated

4 years ago
Duplicate of this bug: 1206515
See Also: → 781984
Duplicate of this bug: 1214445
See Also: → 1108265

Comment 13

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

Updated

3 years ago
Flags: needinfo?(luke)

Comment 14

3 years ago
Bug 1047098,602743,506692,357704,536544,600367
Flags: needinfo?(luke)

Updated

3 years ago
Duplicate of this bug: 1231559

Comment 16

3 years ago
(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.

Comment 18

3 years ago
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

3 years ago
(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

3 years ago
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
Duplicate of this bug: 1201842

Comment 22

3 years ago
(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

3 years ago
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

3 years ago
(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

3 years ago
(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

2 years ago
(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

2 years ago
(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.
Status: REOPENED → NEW
Keywords: privacy
Whiteboard: [tor][fingerprinting]
Duplicate of this bug: 1400582
See Also: → 1398303
Duplicate of this bug: 1398303
(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
Has Regression Range: --- → irrelevant
Has STR: --- → yes
(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).
Duplicate of this bug: 1367607

Updated

2 years ago
Duplicate of this bug: 1401352
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.
Assignee: nobody → amarchesini
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.
(Assignee)

Comment 43

2 years ago
Posted patch part 1 - QuotaManager (obsolete) — Splinter Review
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.
(Assignee)

Updated

2 years ago
Depends on: 1401850
(Assignee)

Comment 44

2 years ago
Attachment #8910638 - Flags: review?(bkelly)
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.
Attachment #8910638 - Flags: review?(bkelly) → review+
(Assignee)

Comment 46

2 years ago
Posted patch part 1 - QuotaManager (obsolete) — Splinter Review
Attachment #8910763 - Flags: review?(jvarga)
(Assignee)

Updated

2 years ago
Attachment #8910635 - Attachment is obsolete: true
(Assignee)

Comment 47

2 years ago
Attachment #8910763 - Attachment is obsolete: true
Attachment #8910763 - Flags: review?(jvarga)
Attachment #8910767 - Flags: review?(jvarga)
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.
Attachment #8910767 - Flags: review?(jvarga) → review+

Comment 52

2 years ago
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
Flags: needinfo?(amarchesini)

Comment 54

2 years ago
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.
(Assignee)

Comment 60

2 years ago
Andrew thanks you so much!
Flags: needinfo?(amarchesini)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 527667
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 64

2 years ago
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
Attachment #8910638 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Attachment #8910767 - Flags: approval-mozilla-beta?
Comment on attachment 8910638 [details] [diff] [review]
part 2 - ServiceWorkers

Fixes this issue, taking it.
Should be in 57b3
Attachment #8910638 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8910767 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 66

2 years ago
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.
Attachment #8910638 - Flags: approval-mozilla-esr52?
(Assignee)

Updated

2 years ago
Attachment #8910767 - Flags: approval-mozilla-esr52?
(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.
Flags: needinfo?(jvarga)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 68

2 years ago
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.
Flags: needinfo?(amarchesini)
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.
Attachment #8910767 - Flags: approval-mozilla-release+
Attachment #8910638 - Flags: approval-mozilla-release+
(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.
Flags: needinfo?(jvarga)
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)
Status: RESOLVED → VERIFIED
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.
Attachment #8911308 - Flags: review?(bkelly)
Comment on attachment 8911308 [details] [diff] [review]
release branch fix for ServiceWorkerRegistrar shutdown

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

rs+
Attachment #8911308 - Flags: review?(bkelly) → review+

Comment 78

2 years ago
Updating the summary to reflect that clearing of ServiceWorkers was fixed in this bug.  If I'm misunderstanding, please revert and undupe bug 1252998.
Summary: 'Clear Recent History' with 'Cache' or 'Offline Website Data' doesn't clear QuotaManager storage (indexedDB, asm.js cache) → 'Clear Recent History' with 'Cache' or 'Offline Website Data' doesn't clear QuotaManager storage and ServiceWorkers

Updated

2 years ago
Duplicate of this bug: 1252998
I'm backporting patches for bug 1348660, just in case we decide to take this for 52.

Comment 81

2 years ago
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

2 years ago
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.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1253031
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1253009
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1253005
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1253027

Updated

2 years ago
Depends on: 1404105
Comment on attachment 8910767 [details] [diff] [review]
part 1 - QuotaManager

Taking this to address privacy issues for ESR52.5
Attachment #8910767 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8910638 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
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?
Flags: needinfo?(overholt)
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 :).
Flags: needinfo?(overholt) → needinfo?(amarchesini)
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.
Flags: needinfo?(overholt)
Flags: needinfo?(lhenry)
Looks like this isn't getting into this ESR release.
Flags: needinfo?(overholt)
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?
Flags: needinfo?(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)
Flags: needinfo?(overholt)
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.
Flags: needinfo?(jvarga)
(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.
Flags: needinfo?(jvarga)
Note that Service Workers are turned off for esr52.
Flags: needinfo?(amarchesini)
Flags: needinfo?(lhenry)

Updated

2 years ago
Blocks: 1415342
(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?
Flags: needinfo?(cr)
Flags: needinfo?(amarchesini)
(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.
Flags: needinfo?(cr)

Comment 110

2 years ago
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?
Flags: needinfo?(cr)
(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?
Flags: needinfo?(cr)
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.

Comment 117

2 years ago
(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");
Flags: needinfo?(matt)

Comment 119

2 years ago
(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.
Flags: needinfo?(matt)
(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.
Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.