1333050 - Implement clearing of IndexedDB in browsingData API

Status: RESOLVED FIXED

(WebExtensions :: Compatibility, defect, P5)

Description

[Bob Silverberg [:bsilverberg]]

Chrome's browsingData API allows for clearing of IndexedDB and LocalStorage for all visited sites. This has not yet been implemented in Firefox, and a conversation with Jan Varga indicates that some work on the underlying Firefox APIs will be required in order to implement this. Some bugs will be opened for that work, which will block this bug, once it is better understood.

Comment 1

[Johannes Pfrang [:johnp]]

Will this tracking bug include removal of IndexedDB and LocalStorage content on a per-domain basis or should I open a separate bug for that?

Comment 2

[Bob Silverberg [:bsilverberg]]

(In reply to Johannes Pfrang [:johnp] from comment #1)
> Will this tracking bug include removal of IndexedDB and LocalStorage content
> on a per-domain basis or should I open a separate bug for that?

Bug 1340511 has already been opened for that.

Comment 3

[Thomas Wisniewski]

I almost have patches ready for this, but I've run into a stumbling block: it doesn't look like "since" support will be possible for localStorage, as the data for all hosts is all stored in one sqlite database, and there are no timestamp records in the database to work with.

Is this something we should block on, or should we simply throw some sort of "not implemented" error when users try to combine clearing localStorage with since for now, and file a follow-up bug?

Please note that I'd imagine that figuring out how to change the schema of webappstore.sqlite would require more time than I have to invest in this effort right now :(

PS: "since" support for indexedDBs is possible, since they're stored in per-host directories on the file system. As such it's not too much trouble to clear the host's IDBs if they're all newer-or-equal to the since date.

Comment 4

[Bob Silverberg [:bsilverberg]]

I believe that when Kris and I discussed this ages ago, he suggested that changes should be made to the data stores to support since, as it's possible, but just not currently available. We should probably wait for that, rather than implementing a "not quite there" solution.

Comment 5

[Thomas Wisniewski]

Ah, alright. Would it be alright to at least try landing the indexedDB support while we wait for the localStorage changes? I can't see that being a problem, and I'd prefer to not have that bitrot too much if possible.

Comment 6

[Bob Silverberg [:bsilverberg]]

Sure, that sounds fine, but we should probably split it into two bugs in that case, so we don't lose track of the localStorage case. Either this bug or a new one can be used to track the changes for indexedBD, and vice versa for localStorage. Can you please take care of that, Thomas?

Comment 7

[Thomas Wisniewski]

Sure, I've split out the localStorage case into bug 1355576, and will try to get a patch up for review tonight (with both yourself and :baku as reviewers, as suggested in bug 1340511).

Comment 8

[Thomas Wisniewski]

Attachment: Bug 1333050 - add support for browsingData.removeIndexedDB and remove(indexedDB);

Review commit: https://reviewboard.mozilla.org/r/129080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129080/

Comment 9

[Thomas Wisniewski]

Comment on attachment 8857146 [details]
Bug 1333050 - add support for browsingData.removeIndexedDB and remove(indexedDB);

https://reviewboard.mozilla.org/r/129080/#review131588

::: browser/components/extensions/ext-browsingData.js:129
(Diff revision 1)
> +  yield AllStorageHostsIterator(function* (hostPath) {
> +    let idbPath = OS.Path.join(hostPath.path, "idb");
> +    let idbStat = yield OS.File.stat(idbPath);
> +    if (idbStat.isDir) {
> +      let shouldClear = false;
> +      if (options.since) {

I'm not 100% sure which logic we want here, precisely. Would we want to only clear a host's IDBs if ALL of them have been accessed since the cutoff date, or if ANY of them have been accessed since the cutoff date?

If for some reason we'd like to be finer-grained and clear on a per-DB basis (rather than a per-host basis), then we will need further platform support, as right now there is only support to clear all storages for a given host.

Comment 10

[Jan Varga [:janv]]

I'm not sure collecting origin directories outside of quota manager is the right thing to do. Every access to <profile>/storage should use a directory lock.
Take a look at https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#18621

And btw, last access time is tracked in .metadata-v2 file.

Comment 11

[Thomas Wisniewski]

Thanks, :janv. I was actually hoping to avoid pointless locking entirely, since I'm only trying to find out the list of hosts with IDBs at that stage (I actually do my call to try clearing the host through the sanctioned APIs, which I believe does the necessary locking/etc).

That said, if this is too fragile and locking is required, then I'm obviously all for it. I just don't know how to get a list of hosts with IDBs in any other way, and I'm not sure how to access the info in .metadata-v2, for that matter. Do you have any advice on how I should go about exposing such info to JS in a better way? With luck I may be able to find the time to do it.

Comment 12

[Jan Varga [:janv]]

(In reply to Thomas Wisniewski from comment #11)
> Thanks, :janv. I was actually hoping to avoid pointless locking entirely,
> since I'm only trying to find out the list of hosts with IDBs at that stage

What if the origin directory you are trying to process in JS is being deleted at the same time ?
Yeah, it seems pointless until you see intermittent failures and strange crashes :)

> (I actually do my call to try clearing the host through the sanctioned APIs,
> which I believe does the necessary locking/etc).
> 
> That said, if this is too fragile and locking is required, then I'm
> obviously all for it. I just don't know how to get a list of hosts with IDBs
> in any other way, and I'm not sure how to access the info in .metadata-v2,
> for that matter. Do you have any advice on how I should go about exposing
> such info to JS in a better way? With luck I may be able to find the time to
> do it.

I think the list should be collected in C++ as part of a new method in nsIQuotaManagerService.

Comment 13

[Thomas Wisniewski]

>What if the origin directory you are trying to process in JS is being deleted at the same time ?

I'll defer to your wisdom :)

I had suspected that it would fail one way or the other in the event of such a race condition, and my code would end up trying to delete a DB that was already gone in the worst case.

But taking a closer look at DirectoryIterator, I doubt my code is fault-tolerant enough regardless, so I'm going to cancel review for now and try to figure out how to get this working in nsIQuotaManagerService as you suggest.

Comment 14

[Thomas Wisniewski]

Attachment: Bug 1333050 - Add a new method QuotaManagerService.clearStorages(in jsval aOptions), which is a dict with an array of storageTypes (only indexedDB supported for now), optional array of principals, and optional 'since' cutoff timestamp;

Review commit: https://reviewboard.mozilla.org/r/129690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129690/

Comment 15

[Thomas Wisniewski]

Hey :janv, I'm not sure if you're willing to actually review this latest patch, but if you could at least take a look to see if it's what you had in mind, I'd appreciate it.

Comment 16

[Jan Varga [:janv]]

Man, I appreciate your contribution.
However, I'm afraid that this is not right direction, sorry.

You would also need to expose directory locking stuff, etc. just because you want to do processing of origin directories in JS
Is there any particular reason for that ?

I thought that a new method in nsIQuotaManagerService would just take a "since" argument.

Comment 17

[Thomas Wisniewski]

Ah It's no big deal that I misinterpreted you, but clearly I should make 100% sure what you mean this time :)

Did you mean I should just add a clearSince method and a clearStoragesForPrincipalIfSince method? 

The second one is going to be needed soon. We need to be able to clear the iDBs for all origins that have been accessed since a given time, but also on a per-origin basis (we'll also have a "hostname" parameter in addition to a "since" parameter, in the browsingData.removeIndexedDBs API). I guess that means I'd either need to query for the origin's metadata access time, or just add an ifSince parameter to the existing method. The latter seems the easier way to go, but would you prefer something else?

Comment 18

[Jan Varga [:janv]]

Is the API and any possible future changes/improvements described somewhere ?

So options is like a dictionary, right ? It can contain one or all of these: hostname, since

And you propose these new methods:
ClearSince(since)
ClearStoragesForPrincipalIfSince(hostname, since)

I think it would be more flexible to just pass options to a new method:
ClearXXX(in jsval options)

I'm not sure about suitable naming for this right now, that can be decided later.

Also, is this only about IndexedDB ? I know that localStorage part of work can't be done at the moment, but what about DOM cache or any other storage APIs managed by quota manager ?

Comment 19

[Thomas Wisniewski]

>is this only about IndexedDB ?

This ticket, yes. We also want to deal with localStorage to achieve closer Chrome parity in the browsingData WebExtension API, but I can't see how to support "since" for localStorage without changing the schema of the sqlite stores to include per-host access times, and I'm not sure I have the time or knowledge to do that right without very careful mentoring.

>I know that localStorage part of work can't be done at the moment, but what about DOM cache or any other storage APIs managed by quota manager ?

I'm new to the quota manager code, so I don't yet know what other types of stores it handles. The priority right now for WebExtensions is just to add iDB support for clearing iDBs with an optional "since" date, then to add a "host(s)" filter. But later on we might expose the ability to clear more browsingData, so I'm certainly not against making a more general-purpose API. Maybe:

  qms.clearStorages({
    storages: ["indexedDB", "domCache"],
    hosts: ["http://www.host1.com"],
    since: timestamp
  }, callback)

Where if all parameters are omitted it just clears everything? If that's agreeable, then I don't mind implementing it as such. The issue is that I wouldn't be sure what storage types the quota manager handles aside from the two we've mentioned already. Is there an obvious list and set of APIs I can use to handle each type, or should I limit it to just iDBs for now and require at least one type to be passed in? (And would all the storage types even be able to support "since" as a parameter?)

Comment 20

[Thomas Wisniewski]

Gentle needinfo ping (but if you'd like more time to chew on this, then I don't mind).

Comment 21

[Jan Varga [:janv]]

(In reply to Thomas Wisniewski from comment #19)

Sorry for the delay...

> >is this only about IndexedDB ?
> 
> This ticket, yes. We also want to deal with localStorage to achieve closer
> Chrome parity in the browsingData WebExtension API, but I can't see how to
> support "since" for localStorage without changing the schema of the sqlite
> stores to include per-host access times, and I'm not sure I have the time or
> knowledge to do that right without very careful mentoring.

Yeah, supporting localStorage is not easy with the current localStorage architecture. Should be easier once we plug it into Quota Manager.

> 
> >I know that localStorage part of work can't be done at the moment, but what about DOM cache or any other storage APIs managed by quota manager ?
> 
> I'm new to the quota manager code, so I don't yet know what other types of
> stores it handles. The priority right now for WebExtensions is just to add
> iDB support for clearing iDBs with an optional "since" date, then to add a
> "host(s)" filter. But later on we might expose the ability to clear more
> browsingData, so I'm certainly not against making a more general-purpose
> API. Maybe:
> 
>   qms.clearStorages({
>     storages: ["indexedDB", "domCache"],
>     hosts: ["http://www.host1.com"],
>     since: timestamp
>   }, callback)
> 
> Where if all parameters are omitted it just clears everything? If that's
> agreeable, then I don't mind implementing it as such. The issue is that I
> wouldn't be sure what storage types the quota manager handles aside from the
> two we've mentioned already. Is there an obvious list and set of APIs I can
> use to handle each type, or should I limit it to just iDBs for now and
> require at least one type to be passed in? (And would all the storage types
> even be able to support "since" as a parameter?)

I would design it as an array of "storage types" as you suggested, but limit to IndexedDB for now.

Comment 22

[Jan Varga [:janv]]

Comment on attachment 8857736 [details]
Bug 1333050 - Add a new method QuotaManagerService.clearStorages(in jsval aOptions), which is a dict with an array of storageTypes (only indexedDB supported for now), optional array of principals, and optional 'since' cutoff timestamp;

https://reviewboard.mozilla.org/r/129690/#review138474

Comment 23

[Thomas Wisniewski]

No problem on the delay. But I'm relatively new to dealing with the idl stuff, so could you give me some pointers on what the best way to implement the "in jsval" parts would be? That is, I'm not sure if learning how to do this with isObject() and JS::ForOfIterator and so on would be the way to go, or if I should just avoid all of that and pass in several array arguments instead of a jsval, like this:

  clearStorages([optional] in ACString[] aStorageTypes,
                [optional] in nsIPrincipal[] aPrincipals,
                [optional] in int32_t aSince)

Comment 24

[Thomas Wisniewski]

Hmm. I'm indeed stuck on the jsval version of the implementation, and unsure what to do. When I create a principal like this in a mochitest:

  let ssm = SpecialPowers.Services.scriptSecurityManager;
  var principal = ssm.createCodebasePrincipalFromOrigin("http://google.ca");
  dump(principal);

The dump of the object shows that its indeed an nsIPrincipal:

  [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable) @ 0x7fcad3ca7320 (native @ 0x7fcad4aeb520)]

However, in my C++ code the Rooted JS object doesn't seem to be unwrappable using UnwrapArg<nsIPrincipal>, just UnwrapArg<nsISupports>.

Is there a special trick to grabbing an nsIPrincipal* for such objects? I seem to be able to unwrap a plain nsIURI just fine using UnwrapArg, but its dump doesn't show support for multiple XPCOM interfaces: [xpconnect wrapped nsIURI]. Is that perhaps related?

Comment 25

[Thomas Wisniewski]

In case a bit more context helps, this is the code I'm using to unwrap the principals in my inner loop (with some error-checking removed for clarity):

  JS::Rooted<JS::Value> temp(cx);
  iter.next(&temp, &done); // JS::ForOfIterator
  JS::RootedObject obj(cx, &temp.toObject());
  RefPtr<nsIPrincipal> principal;
  if (NS_FAILED(UnwrapArg<nsIPrincipal>(cx, obj, getter_AddRefs(principal)))) {
    ThrowErrorMessage(cx, MSG_DOES_NOT_IMPLEMENT_INTERFACE, "Argument of clearStorages", "Principal");
    return NS_ERROR_INVALID_ARG;
  }

Comment 26

[Jan Varga [:janv]]

Btw, do you really need to support an array of hosts/principals ?

Comment 27

[Thomas Wisniewski]

Hmm. It appears that the problem I was running into is specific to the mochitest I was using (or perhaps mochitests in general). I just tried running a test from an xpcshell-test instead, and it seems able to unwrap the nsIPrincipals just fine. Hopefully it was a false alarm.


>Btw, do you really need to support an array of hosts/principals?

No, I don't suppose I need an array of them, but figure that I might as well. It seems it may be more efficient to just batch up one request, rather than making multiple calls, and the extra code to iterate an array doesn't seem all that egregious to me. I'm not against changing it though if you feel that's the better approach.

Comment 28

[Thomas Wisniewski]

Comment on attachment 8857736 [details]
Bug 1333050 - Add a new method QuotaManagerService.clearStorages(in jsval aOptions), which is a dict with an array of storageTypes (only indexedDB supported for now), optional array of principals, and optional 'since' cutoff timestamp;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129690/diff/1-2/

Comment 29

[Thomas Wisniewski]

Whoops, my bad, I didn't realize reviewboard would make a useless interdiff for this new patch. Just ignore that.

Comment 30

[Thomas Wisniewski]

Just another friendly status ping.

Comment 31

[Tim Nguyen :ntim]

Comment on attachment 8857736 [details]
Bug 1333050 - Add a new method QuotaManagerService.clearStorages(in jsval aOptions), which is a dict with an array of storageTypes (only indexedDB supported for now), optional array of principals, and optional 'since' cutoff timestamp;

Pinging another indexeddb peer who might be able to help with review.

Comment 32

[Jan Varga [:janv]]

Sorry, but I repeated this multiple times, it seems to me that the locking is not handled in the last patch at all.
Using locking methods or even extending the locking API is quite hard, so I wouldn't try to make the new method too generic. I would just try to add the minimum you need for browsingData API.

For example, clearAllOrigins needs to exclusively lock the whole storage directory.
On the other hand, when you pass a list of origins, you need to exclusively lock only those origins.

You just can't go ahead and delete whichever files you want in <profile>/storage, they can be in use.

I can't review this, until this is addressed, sorry.

Comment 33

[Jan Varga [:janv]]

Comment on attachment 8857736 [details]
Bug 1333050 - Add a new method QuotaManagerService.clearStorages(in jsval aOptions), which is a dict with an array of storageTypes (only indexedDB supported for now), optional array of principals, and optional 'since' cutoff timestamp;

https://reviewboard.mozilla.org/r/129690/#review155876

Comment 34

[StefanB]

In addition to creating an API for clearing IndexedDB, Firefox should actually delete this data on exit, when the user has selected "Clear history when Firefox closes->Offline website data". Firefox 52 ESR does not clear this data, which is kind of fooling the user. Tracking possibilities galore! Tested it here https://static.raymondcamden.com/demos/2014/feb/7/index.html#/home

Comment 35

[Andrea Marchesini [:baku]]

Attachment: nsIQuotaManagerService::Clear()

This patch enabled nsIQuotaManagerService::clear() without having the testing prefs set to true.

Comment 36

[Andrea Marchesini [:baku]]

Attachment: BrowsingData.removeIndexedDB

"since" cannot be implemented in QuotaManager.

Note that this is not just about IndexedDB, but it's about any storage API managed by QuotaManager.

Personally, I think it's better to remove more than less for privacy concerns. Currently QM covers more than IDB: cache API, SW cache, wasm cache and so on.

Would be probably nice to introduce a removeStorage() method. But this is definitely a follow up.

Comment 37

[Jan Varga [:janv]]

Comment on attachment 8910139 [details] [diff] [review]
nsIQuotaManagerService::Clear()

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

baku, are you aware that clear() removes the storage directory completely ?
Including internal stuff, like storage/permanent/chrome, storage/permanent/moz-safe-about+home, storage/permanent/indexeddb+++fx-devtools, etc.

Comment 38

[Jan Varga [:janv]]

We currently support these schemes: http, https, about, moz-safe-about, indexeddb, file, app (obsolete), resource, moz-extension.
Some of them are used in storage/default and storage/temporary, including chrome
So we can't just delete everything except storage/permanent, it's not so easy to delete "everything".
This needs more thinking ...

Can we assume that "... clearing of IndexedDB and LocalStorage for all visited sites ..." in comment 0 is correct ?
It would mean that we only want to delete http and https (maybe also file) origins.

What about persisted origins ? Do we want to remove them, given that user must have explicitly agreed to use them ?

Andrew, Andrea, what do you think ?

Comment 39

[Jan Varga [:janv]]

I'm adding one more comment, I was thinking a bit about the "since" support. In theory with SQL databases, transactions and log files, one can restore a database to a point in time. It's a crazy idea, especially given extra overhead on disk, but maybe not so crazy if we only need to support changes made last 24 hours at most.

Comment 40

[Andrea Marchesini [:baku]]

> What about persisted origins ? Do we want to remove them, given that user
> must have explicitly agreed to use them ?

If we don't delete everything, we need to add something else able to do it. To me, at the moment, it's good enough to delete everything (http/https/file schemes + storage/permanent).

Comment 41

[Andrea Marchesini [:baku]]

Attachment: nsIQuotaManagerService::ClearContentData()

Comment 42

[Andrea Marchesini [:baku]]

Attachment: BrowsingData.removeIndexedDB

Comment 43

[Andrea Marchesini [:baku]]

Attachment: test

Comment 44

[Jan Varga [:janv]]

Comment on attachment 8910223 [details] [diff] [review]
nsIQuotaManagerService::ClearContentData()

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

::: dom/quota/ActorsParent.cpp
@@ +1300,5 @@
> +  : public QuotaRequestBase
> +{
> +public:
> +  ClearContentOp()
> +    : QuotaRequestBase(/* aExclusive */ true)

passing aExclusive here will end up as:
NormalOriginOperationBase(Nullable<PersistenceType>(),
                          OriginScope::FromNull(),
                          aExclusive)

So, it requests an exclusive lock for entire profile/storage.
Such a lock can only be acquired if all existing locks for profile/storage are released. OpenDirectoryInternal() will abort operations for all origins that block the requested lock to be acquired. I don't think we can do this.

I'm afraid the directory locking API doesn't support anything that would cover this new use case.

I can try to improve the locking API following days.

I have some ideas, but it's not trivial.

Comment 45

[Andrea Marchesini [:baku]]

I and Janv discussed this topic on IRC. We have a plan. Here what I'm going to submit asap:

In order to correctly delete QM directories, I have to:

1. create a ClearContentOp class, inheriting QuotaRequestBase, with a non-exclusive lock.

2. When ClearContentOp::DoDirectoryWork is executed on the IO thread, a list of origins is populated. This list contains all the origins we want to delete. This is true for any origin with http:, https: and file: schemes.

3. Using a runnable and a Monitor, DoDirectoryWork goes back to the owning thread (PBackground) and it obtains a lock for each origin in that list.

4. When all the locks are created and obtained, the operation continues on the IO thread where the directories are deleted for real.

Comment 46

[Andrea Marchesini [:baku]]

Attachment: nsIQuotaManagerService::ClearContentData()

Comment 47

[Kris Maglione [:kmag]]

Comment on attachment 8910225 [details] [diff] [review]
test

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

::: browser/components/extensions/test/browser/browser_ext_browsingData_indexedDB.js
@@ +23,5 @@
> +
> +  function contentScript() {
> +    window.addEventListener("message", msg => { // eslint-disable-line mozilla/balanced-listeners
> +      if (msg.data == "indexedDBCreated") {
> +        browser.runtime.sendMessage("indexedDBCreated");

No need to route this through the background page. You can use `browser.test.sendMessage` directly from the content script.

@@ +54,5 @@
> +  await extension.awaitMessage("indexedDBCreated");
> +  await extension.awaitMessage("indexedDBCreated");
> +
> +  let qms = SpecialPowers.Cc["@mozilla.org/dom/quota-manager-service;1"]
> +             .getService(Ci.nsIQuotaManagerService);

Is this safe to use from the content process?

Comment 48

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

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #47)
> @@ +54,5 @@
> > +  let qms = SpecialPowers.Cc["@mozilla.org/dom/quota-manager-service;1"]
> > +             .getService(Ci.nsIQuotaManagerService);
> 
> Is this safe to use from the content process?

Yes.  QuotaManagerService, whether in the parent or child main thread, communicates with the QuotaManager in the parent process PBackground thread via PBackground-managed PQuota protocol.

Comment 49

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

Marking this as a blocker for 56.  We still have a chance to uplift this to m-r for a 56 RC2 build.

Comment 50

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

(In reply to Jan Varga [:janv] from comment #39)
> I'm adding one more comment, I was thinking a bit about the "since" support.
> In theory with SQL databases, transactions and log files, one can restore a
> database to a point in time. It's a crazy idea, especially given extra
> overhead on disk, but maybe not so crazy if we only need to support changes
> made last 24 hours at most.

If we did this, we could use SQLite's session extension (https://sqlite.org/sessionintro.html) to simplify this.  Cleverness involving the write-ahead-log might also be possible.  Either way, the main complicating factors are IndexedDB's reference-counted Blobs and Cache API's body storage.

However, I don't think we should do this.  My view of "Clear Recent History" is an awkward solution to the "whoops, I meant to be using a private browsing tab/different container for that" or the "I don't know about private browsing tabs/containers or don't like them and this is how I keep things private" use cases.  

I think we want to:
- Implement double-keying.  In most of these bugs we're only talking about cleaning up local data because that's all we can do.  If tracking happened on the server, it's already too late.  However, if we implement double-keying, we both:
  - Mitigate the privacy ramifications of using non-private-browsing/the wrong container because it's very possible all the browsing will result in completely "new" principals and QM storage directories in use.
  - Make it easier to obliterate the results of the browsing because they are all new QM directories and we can remove them.
- Improve the "clear recent history" UI so it's no longer "guess the right amount of time that covers your session and hope it does the right thing" and instead becomes "here's a list of sites you just visited for the first time (and their third party dependencies), or that you hadn't visited for a long time... we think we should obliterate their storage for you" and "here's a list of other sites that you just visited that you tend to visit a lot, like your open gmail tab and your dropbox tab that are still open right now, maybe we don't nuke those".  And of course, the user can change our settings.

Comment 51

[Andy McKay]

I don't think this is important enough to uplift to 56 at this stage. Would be nice for 57 though.

Comment 52

[Andrea Marchesini [:baku]]

Definitely we need the QM patch in 56 because it's needed for other top-priority bugs.

Comment 53

[Kris Maglione [:kmag]]

(In reply to Jan Varga [:janv] from comment #39)
> I'm adding one more comment, I was thinking a bit about the "since" support.
> In theory with SQL databases, transactions and log files, one can restore a
> database to a point in time. It's a crazy idea, especially given extra
> overhead on disk, but maybe not so crazy if we only need to support changes
> made last 24 hours at most.

If at some point we decide to support "since", I think it may be enough to just delete any record that was modified after that time, and not worry about restoring the previous state.

Comment 54

[Jan Varga [:janv]]

Comment on attachment 8910369 [details] [diff] [review]
nsIQuotaManagerService::ClearContentData()

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

::: dom/quota/ActorsParent.cpp
@@ +7588,5 @@
> +    });
> +
> +  MonitorAutoLock lock(mMonitor);
> +  MOZ_ALWAYS_SUCCEEDS(mOwningThread->Dispatch(r.forget(), NS_DISPATCH_NORMAL));
> +  lock.Wait();

I suspect using a monitor here can lead to a dead lock...

So you are on quota manager IO thread and dispatch a runnable to PBackground thread that requests exclusive directory locks for selected origins. The IO thread is now blocked, other runnables dispatched to the IO thread can't be executed. Now imagine that there were directory locks already queued that wait for processing, so they get acquired before the locks you requested and locks that you requested will be blocked. Some of those already queued directory locks may use the IO thread for processing, so once they get acquired they dispatch runnables to the IO thread which you block here...
Is that clear ?

Comment 55

[Jan Varga [:janv]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #53)
> (In reply to Jan Varga [:janv] from comment #39)
> > I'm adding one more comment, I was thinking a bit about the "since" support.
> > In theory with SQL databases, transactions and log files, one can restore a
> > database to a point in time. It's a crazy idea, especially given extra
> > overhead on disk, but maybe not so crazy if we only need to support changes
> > made last 24 hours at most.
> 
> If at some point we decide to support "since", I think it may be enough to
> just delete any record that was modified after that time, and not worry
> about restoring the previous state.

I think that may lead to inconsistent databases. For example we would delete modified records, but we wouldn't add back records that were deleted during a session.
I would rather get rid of "since" support if possible as :asuth suggested.

Comment 56

[Kris Maglione [:kmag]]

(In reply to Jan Varga [:janv] from comment #55)
> I think that may lead to inconsistent databases. For example we would delete
> modified records, but we wouldn't add back records that were deleted during
> a session.

That's pretty similar to what happens with, for example, cookies. We delete cookies that were modified after the "since" timestamp, but don't restore the values they had before they were modified, or restore any cookies that were cleared in that time frame.

> I would rather get rid of "since" support if possible as :asuth suggested.

I'm probably fine with that, but there might be objections from extension developers. It would be nice to hear their arguments before making a decision.

Comment 57

[Jan Varga [:janv]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #56)
> (In reply to Jan Varga [:janv] from comment #55)
> > I think that may lead to inconsistent databases. For example we would delete
> > modified records, but we wouldn't add back records that were deleted during
> > a session.
> 
> That's pretty similar to what happens with, for example, cookies. We delete
> cookies that were modified after the "since" timestamp, but don't restore
> the values they had before they were modified, or restore any cookies that
> were cleared in that time frame.

That only confirms that supporting "since" is a nightmare :)
Cookies is much less complex database though, so it can "work" that way.

Comment 58

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #50)
> If we did this, we could use SQLite's session extension
> (https://sqlite.org/sessionintro.html) to simplify this.  Cleverness
> involving the write-ahead-log might also be possible.  Either way, the main
> complicating factors are IndexedDB's reference-counted Blobs and Cache API's
> body storage.

Yeah, stuff stored outside SQL databases makes it even harder to implement.

Comment 59

[Andrea Marchesini [:baku]]

Moving patch 1 to bug 1401850.

Comment 60

[Andrea Marchesini [:baku]]

Attachment: BrowsingData.removeIndexedDB

Comment 61

[Andrea Marchesini [:baku]]

Attachment: BrowsingData.removeIndexedDB

Comment 62

[dessant]

> I would rather get rid of "since" support if possible as :asuth suggested.

> I'm probably fine with that, but there might be objections from extension developers. It would be nice to hear their arguments before making a decision.

I think it is important to work towards making it possible to remove browsing data in a granular way. Being able to filter by time (and hostname) would let us implement features similar to Clear Recent History at about:preferences#privacy. IndexedDB is not listed there, but clearing it completely when the user requests only the last hour would mean potentially lost data.

I've mentioned hostnames because that would also enable us to have an extension like the Forget button, but for specific tabs or sites.

Chrome and Opera does not yet support clearing data by hostnames, but filtering by time works as expected for everything except the cache.

Comment 63

[Jan Varga [:janv]]

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

Comment 64

[Jan Varga [:janv]]

Comment on attachment 8910766 [details] [diff] [review]
BrowsingData.removeIndexedDB

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

::: browser/components/extensions/ext-browsingData.js
@@ +82,5 @@
>  const clearHistory = options => {
>    return sanitizer.items.history.clear(makeRange(options));
>  };
>  
> +const clearIndexedDB = async function(options) {

Technically, this clears IDB, DOM cache and AsmJSCache. Do we really want to delete all of them here ?

Comment 65

[Jan Varga [:janv]]

Does anybody know what other browsers do for removeIndexedDB() ?

Does it only clear IndexedDB data ? or it also clear DOM cache ?

The current patch would clear IndexedDB, DOM cache, AsmJSCache and LocalStorage in future.

Comment 66

[Kris Maglione [:kmag]]

(In reply to Jan Varga [:janv] from comment #65)
> Does anybody know what other browsers do for removeIndexedDB() ?
> 
> Does it only clear IndexedDB data ? or it also clear DOM cache ?
> 
> The current patch would clear IndexedDB, DOM cache, AsmJSCache and
> LocalStorage in future.

We should only be clearing indexedDB. DOM cache and localStorage have their own browsingData items. asm.js might need one in the future, but we probably don't want to clear it when indexedDB is cleared.

Comment 67

[Jan Varga [:janv]]

So we need another patch for adding optional argument clientType to clearStoragesForPrincipal().
It can be a followup bug, I'll let baku decide, but should be fixed ASAP.

Comment 68

[Jan Varga [:janv]]

Comment on attachment 8910766 [details] [diff] [review]
BrowsingData.removeIndexedDB

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

r+ with a promise from baku that the followup would be fixed soon and get to 57

Comment 69

[Andrea Marchesini [:baku]]

> We should only be clearing indexedDB. DOM cache and localStorage have their
> own browsingData items. asm.js might need one in the future, but we probably
> don't want to clear it when indexedDB is cleared.

localstorage is already covered by removeLocalStorage(). Here we delete indexedDB + cache API + SW cache and asm.js.
Note that browsingData doesn't have methods to delete cache, sw or asm.js at the moment.

If we don't implement removeIndexedDB(), any 'privacy-releated' addon will be broken because they don't have a way to clean up browsing data correctly. We had a similar issue with Clear-Recent-History.

I propose this:
1. we land these patches.
2. as follow up we implement removeIndexedDB() and at the same time we land removeCache, removeASMJS.

I'll work on these follow up asap.

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff6b103fa9b
Introduce BrowsingData.removeIndexedDB, r=kmag, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0551ee71c8
Test for BrowsingData.removeIndexedDB, r=kmag

Comment 71

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

https://hg.mozilla.org/mozilla-central/rev/3ff6b103fa9b
https://hg.mozilla.org/mozilla-central/rev/4f0551ee71c8

Comment 72

[Sylvestre Ledru [:Sylvestre]]

Baku, in comment #52 you said that we need that for 56? is that still the case?
What about 57?
Thanks

Comment 73

[Andrea Marchesini [:baku]]

It's important to have it for 57. No 56.

Comment 74

[Sylvestre Ledru [:Sylvestre]]

OK, gracie!

Could you fill the uplift request to 57 then?

Comment 75

[Andrea Marchesini [:baku]]

Comment on attachment 8910225 [details] [diff] [review]
test

Approval Request Comment
[Feature/Bug causing the regression]: BrowsingData
[User impact if declined]: in 57, the only way for addons to cleanup data is via BrowsingData. We must support the removing of indexedDB of privacy concerns.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: n/a 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: Similar code is in 57 for Clear Recent History. The code deletes all the QM data.
[String changes made/needed]: none

Comment 76

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910225 [details] [diff] [review]
test

Fix some private concerns. Taking it.
Should be in 57b3

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/930d53440126
https://hg.mozilla.org/releases/mozilla-beta/rev/a747acffe3e8

Comment 78

[Andrei Vaida [:avaida]]

(In reply to Andrea Marchesini [:baku] from comment #75)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: n/a
> [Needs manual test from QE? If yes, steps to reproduce]: n/a 

Setting qe-verify- based on Andrea's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 79

[K Mozej]

Is there a bug tracking clearing IndexedDB data by host?

Comment 80

[Marion Daly [:mdaly]]

removing need-info