Closed Bug 1529122 Opened 8 months ago Closed 7 months ago

browsingData.remove with indexedDB: true sometimes not working correctly

Categories

(WebExtensions :: General, defect, P3)

65 Branch
defect

Tracking

(firefox68 verified)

VERIFIED FIXED
mozilla68
Tracking Status
firefox68 --- verified

People

(Reporter: core, Assigned: tt)

References

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

  1. Visit pages that use indexedDB
  2. See <profile folder>/storage/default/<domainname> appear
  3. Call browser.browsingData.remove({}, { indexedDB: true });

Actual results:

<profile folder>/storage/default/<domainname> is not removed. Instead, it seems that two files always remain: .metadata and .metadata-v2
This in itself is not the main concern. The main issue here is, that sometimes even the idb folder including the sqlite files are not getting deleted.

I have yet to find out a pattern. Some of my users say, that the likelihood increases if you have a lot of indexeddbs stored.

My best guess is, that there is an exception that happens when deleting one of the indexeddbs, which prevents others from getting deleted.

Expected results:

The above folder should be removed completely, including the metadata files.

Sure. Looking into this.

Flags: needinfo?(shes050117)

I'm not sure whether do I find out all the issues, but I do find something that needs to be modified a bit.

What I found is that:

  • If the origin is obsolete and its metadata (.metadata-v2) doesn't exist, we would fail here [1]. However, it seems that it's okay to proceed.
  • If we failed to initialize the given origin, then it just fails. However, it might make sense to remove the origin if it fails to be initialized because it's a clear operation. [2]
  • If there is an unexpected client, then it fails. I guess we should either ignore or delete that. (I prefer to ignore that since it matches current approach)

I would like to start by fixing the things I found.

[1] https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/dom/quota/ActorsParent.cpp#7368
[2] https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/dom/quota/ActorsParent.cpp#7415
[3] https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/dom/quota/ActorsParent.cpp#7380

(In reply to Lusito from comment #0)

Hey Lusito,

Could you provide more information so that it is easier to check whether does the issues I found include the issue you had got? Such as the error message from the web console or stderr (that requires a debug build), and if the promise is resolved or rejected. Thanks!

Flags: needinfo?(core)

(In reply to Tom Tung [:tt, :ttung] from comment #3)

  • If the origin is obsolete and its metadata (.metadata-v2) doesn't exist, we would fail here [1]. However, it seems that it's okay to proceed.
  • If we failed to initialize the given origin, then it just fails. However, it might make sense to remove the origin if it fails to be initialized because it's a clear operation. [2]

The "fails" I meant for these is that the origin is not deleted (although the promise would be resolved).

  • If there is an unexpected client, then it fails. I guess we should either ignore or delete that. (I prefer to ignore that since it matches current approach)

Oops, this is not the case.

(In reply to Tom Tung [:tt, :ttung] from comment #4)

(In reply to Lusito from comment #0)

Hey Lusito,

Could you provide more information so that it is easier to check whether does the issues I found include the issue you had got? Such as the error message from the web console or stderr (that requires a debug build), and if the promise is resolved or rejected. Thanks!

I could reproduce the issue by the conditions I mentioned above, but I'm not really sure if they are the cases you got, so it would be nice if you can provide your profile (if you don't mind) or more information. So that I could verify if the issue is fixed or not when the patch is provided.

(In reply to Tom Tung [:tt, :ttung] from comment #5)

(In reply to Tom Tung [:tt, :ttung] from comment #3)

  • If the origin is obsolete and its metadata (.metadata-v2) doesn't exist, we would fail here [1]. However, it seems that it's okay to proceed.

Restate: It turns out that it's not possible to happen if we get the using origins with GetUsage. (Obsolete origin won't be collected).

However, I found that if the deleting client is the only remaining client, we would leave an empty origin directory (but still with the metadata file). I guess it's the thing we want to improve here because such empty directories would increase the time for initialization.

Priority: -- → P3

This patch do:

  • Removing the directroy if the requesting client is the only client.
  • Avoid unnecessary initialization for a client if it hasn't been initialized.
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d3a8789d7d84
P1 - Remove the origin directory if the requesting client is the only client in the directroy; r=asuth

Setting the assignment to reflect reality.

Assignee: nobody → shes050117
Flags: needinfo?(ddurst) → needinfo?(shes050117)

I had a fix and looks okay on try. Right now, I am working on a test to ensure that. Thanks!

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

Flags: needinfo?(shes050117)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

I don't really have more information myself, but I've instructed the users who had this issue before to supply some information.
I will try to do some tests tomorrow and see if anything pops up. Any specific place to look for error messages? Or would it be shown within the add-on debug interface?

Flags: needinfo?(core)

(In reply to Lusito from comment #13)

I don't really have more information myself, but I've instructed the users who had this issue before to supply some information.
I will try to do some tests tomorrow and see if anything pops up. Any specific place to look for error messages? Or would it be shown within the add-on debug interface?

For the information we might want to have:

  • Checking the result of this website "https://firefox-storage-test.glitch.me/". It's a website to check whether could your profile be initialized.

  • Any information related to "QuotaManager", "IndexedDB", or "IDB". I guess you could find some messages on web console or browser console. If you can provide a test, STR, or profile would be really appreciated.

Please note that the current patch will ensure the origin directory (<profile folder>/storage/default/<domainname>) would be removed when there is no client folder in it. I plan to land it in days. Thus, if they can provide a test about the other case you mentioned (still having indexeddb database after clear), that would help us a lot.

Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5cdeddb92f16
P1 - Remove the origin directory if the requesting client is the only client in the directroy; r=asuth
https://hg.mozilla.org/integration/autoland/rev/9e380dd146f3
P2 - Make sure checking initialized if clear all is requested; r=asuth

I tried that glitch.me website and it does create a storage folder, which does not get removed upon cleaning.
After a call to remove(), the following changed:

  • A file .metadata-v2 was created!
  • The folder "idb", including the sqlite file has been removed.
  • everything else is left unchanged. I.e. the whole cache folder is still there.

I did not get any error messages.

I did another test with my full storage folder (not an empty profile) and for some reason, it did not remove any files. My guess is, that one of the storage folders breaks the cleanup and prevents the rest of them from being cleared.

I also noticed, that files have been added. mostly in moz-extension+++* folders, but some in permanent/chrome too.

Never mind my last comment.. My storage folder had a gazillion folders for webpages, but none of them had an idb folder. Only cache and metadata folders. I can't reproduce the issue of the idb folder not getting deleted.

(In reply to Lusito from comment #17)

I tried that glitch.me website and it does create a storage folder, which does not get removed upon cleaning.

Sorry, I didn't tell you clearly. So, the correcting patch for clearing origin directory if there is no any client directory hasn't been updated to our Nightly yet. Besides, the "glitch.me" website is just a test for testing whether your profile/storage can be initialized successfully. It won't test whether does clear function work or not.

I will reply to this bug while the patches are updated to the newest Nightly.

After a call to remove(), the following changed:

  • A file .metadata-v2 was created!
  • The folder "idb", including the sqlite file has been removed.
  • everything else is left unchanged. I.e. the whole cache folder is still there.

I did not get any error messages.

It sounds like the metadata file failed to be created while the origin directory was created. (The metadata file is supposed to be created successfully while creating origin directory) So that the ".metadata-v2" was created [1].

So, did you see any message with red color on the glitch.me website? It seems that it failed to create an origin directory with the metadata file.

I assumed that you meant you called the remove() for the glitch.me website. So the remove() should only remove the idb directory based on comment #1. If there is another client directory (e.g. cache), then that client directory is expected to exist after remove().

[1] https://searchfox.org/mozilla-central/rev/56705678f5fc363be5e0237e1686f619b0d23009/dom/quota/ActorsParent.cpp#7645

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

(In reply to Tom Tung [:tt, :ttung] from comment #20)

I will reply to this bug while the patches are updated to the newest Nightly.

(In reply to Raul Gurzau (:RaulGurzau) from comment #21)

https://hg.mozilla.org/mozilla-central/rev/5cdeddb92f16
https://hg.mozilla.org/mozilla-central/rev/9e380dd146f3

It's updated to the lasted Nightly.

There was no error on the glitch.me website console. I called remove() from my extension.
I will instruct my users to test on nightly and see if the issue has been fixed.

If remove(...indexedDB:true) should only remove the idb directory, then what is supposed to remove the cache directory? the "cache" option of remove() doesn't remove that directory either. And how can I delete the metadatafiles from web-extension API?

Don't worry about metadata files, it's an internal detail that they exist. They don't count towards quota.

(In reply to Lusito from comment #23)

There was no error on the glitch.me website console. I called remove() from my extension.
I will instruct my users to test on nightly and see if the issue has been fixed.

Thanks!

To be more clear, I only correct the behavior below:
[Before calling remove(), only idb directory in the origin/website folder]
<profile folder>/storage/default/<domain name A>/idb/
[After calling remove()]
<profile folder>/storage/default/

For the case like having more than one client folders:
<profile folder>/storage/default/<domain name A>/idb/
<profile folder>/storage/default/<domain name A>/cache/
Calling remove() would only remove the idb folder: (no idb folder)
<profile folder>/storage/default/<domain name A>/cache/

And, note that please ignore the .metadata .metadata-v2 files. They are for internal use and they don't count towards quota as Jan said.

(In reply to Lusito from comment #24)

If remove(...indexedDB:true) should only remove the idb directory, then what is supposed to remove the cache directory? the "cache" option of remove() doesn't remove that directory either. And how can I delete the metadatafiles from web-extension API?

The "cache" folder is for DOM Cache rather than the HTTP cache. If the website registers a service worker, then it is also stored stuff in the cache folder. So, I guess it would be somehow dangerous if this API can remove the DOM Cache without removing the service worker in a function. (If so, it's easy to broke a website by doing that accidentally)

Maybe I'm wrong, but I cannot find a way to remove DOM cache in current browseringData API [1].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browsingData

I guess maybe Luca can elaborate more about the question for "how do we remove the DOM Cache from web-extension API"?

Flags: needinfo?(lgreco)

I see. Thanks for clarifying :)

Based on comment #28 and the discussion on IRC, there is no way to remove DOM Cache now.

Flags: needinfo?(lgreco)
Regressions: 1548748

Can you please provide some steps to verify the fix for this issue? It will be really helpful for the qa team to be able to reproduce this issue on an affected build and verify the fix in the nightly builds. At this moment I'm not sure I understand correctly how we should test this fix.

Flags: needinfo?(shes050117)

Two scenarios
A:

  1. Create&go to a web/testing page which only using IndexedDB. (Say, "https://foo.com")
  2. Check the folder on the storage directory of the profile is created. (You should find out that <profile folder>/storage/default/https+++foo.com/idb and there shouldn't have another folder in this folder [/https+++foo.com/])
  3. Call browser.browsingData.remove({}, { indexedDB: true });
  4. The folder <profile folder>/storage/default/https+++foo.com should be removed.

B:

  1. Create&go to a web/testing page which using IndexedDB and DOMCache (Say, "https://foo.com")
  2. Check the folder on the storage directory of the profile is created. (You should find out that <profile folder>/storage/default/https+++foo.com/idb and <profile folder>/storage/default/https+++foo.com/cache)
  3. Call browser.browsingData.remove({}, { indexedDB: true });
  4. Only the folder <profile folder>/storage/default/https+++foo.com/idb should be removed. (<profile folder>/storage/default/https+++foo.com/cache should still exist)

Note:

  • If you want to test it by a unit test and make a website by yourself (e.g. using localhost), you could refer to this test [1].
  • If you want to test it by visiting a website and creating storage by the web console; you can go to any website, open the web console, and then type indexedDB.open("test") to create a idb storage; type caches.open("test") to create a DOM Cache storage. However, please ensure the website is not using these storages if you are going to test in this way.
  • Affect build (the previous m-c before the patches in this bug updated to the m-c https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=4146cbd2103cb97b239033f215eb750f1ca880f1)

Please let me know if anything is not clear. Thanks!

[1] https://searchfox.org/mozilla-central/source/dom/quota/test/unit/test_clearStorageForPrincipal.js

Flags: needinfo?(shes050117)

(In reply to Tom Tung [:tt, :ttung] from comment #32)

Sorry, browser.browsingData can be called on an extension https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browsingData

Two scenarios
A:

  1. Create&go to a web/testing page which only using IndexedDB. (Say, "https://foo.com")
  1. Create & run an extension which only using IndexedDB.

The way for creating an extension should be: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Your_first_WebExtension

  1. Check the folder on the storage directory of the profile is created. (You should find out that <profile folder>/storage/default/https+++foo.com/idb and there shouldn't have another folder in this folder [/https+++foo.com/])

For find the directory for the extension, you should be able to find the corresponding directory for the extension like:

<profile folder>/storage/defaul/moz-extension+++<id> so the idb folder will be located like

<profile folder>/storage/defaul/moz-extension+++<id>/idb

  1. Call browser.browsingData.remove({}, { indexedDB: true });
    (on the script of the extension)
  1. The folder <profile folder>/storage/default/https+++foo.com should be removed.
    Folder <profile folder>/storage/defaul/moz-extension+++<id> should be removed

B:

  1. Create&go to a web/testing page which using IndexedDB and DOMCache (Say, "https://foo.com")
  1. Create & run an extension which only using IndexedDB and DOMCache.
  1. Check the folder on the storage directory of the profile is created. (You should find out that <profile folder>/storage/default/https+++foo.com/idb and <profile folder>/storage/default/https+++foo.com/cache)
    <profile folder>/storage/defaul/moz-extension+++<id>/idb and <profile folder>/storage/defaul/moz-extension+++<id>/cache
  1. Call browser.browsingData.remove({}, { indexedDB: true });
    (on the script of the extension)
  1. Only the folder <profile folder>/storage/default/https+++foo.com/idb should be removed. (<profile folder>/storage/default/https+++foo.com/cache should still exist)

Folder <profile folder>/storage/defaul/moz-extension+++<id>/idb should be removed

Attached file removeIndexedDb.zip

Verified in win7x64 FF68.0a1 (20190514214217)

I have followed the above steps but It seems I can still reproduce the issue (using the extension attached).

I'm using (1) in the background script to initialize a indexedDb and on icon click I'm using (2) to clear it.

(1) - window.indexedDB.open("MyTestDatabase", 3);
(2) - browser.browserAction.onClicked.addListener(() => {
browser.browsingData.remove({}, { indexedDB: true }).then(onRemoved, onError);
});

When running (2) I'm not getting a console error but the idb folder is not removed.

Not sure if I'm doing something wrong. Tom can you have a look over the attached extension?

(In reply to Madalin Cotetiu from comment #34)

Not sure if I'm doing something wrong. Tom can you have a look over the attached extension?

I installed the extension and while I click the bottom, I got:

[Parent 66561, QuotaManager IO] WARNING: 'NS_FAILED(rv)', file /Users/tomtung/Work/mozilla-central/dom/indexedDB/ActorsParent.cpp, line 16497
[Parent 66561, QuotaManager IO] WARNING: 'NS_FAILED(rv)', file /Users/tomtung/Work/mozilla-central/dom/indexedDB/ActorsParent.cpp, line 16148
[Parent 66561, QuotaManager IO] WARNING: 'NS_FAILED(rv)', file /Users/tomtung/Work/mozilla-central/dom/quota/ActorsParent.cpp, line 8308

It's clear that that is another issue because in this case even the idb folder cannot be removed, but I need to look into it more. (The issue in this bug is that the idb folder was removed but the origin directory wasn't)

(In reply to Tom Tung [:tt, :ttung] from comment #35)

It's clear that that is another issue because in this case even the idb folder cannot be removed, but I need to look into it more. (The issue in this bug is that the idb folder was removed but the origin directory wasn't)

That seems some OS issues.

I checked the usage of the browser.browsingData.remove() again, and I found I had misunderstood that. So, it's a function for an extension to storage from other general websites excluding itself.

The general websites here is that the websites use https, http, file URI.

So, the step for checking should be:

  1. Create&go to web/testing pages which only using IndexedDB. (Say, "https://foo.com", "https://boo.com")
  2. Having idb storage on both websites and cache storage on only one of them
  3. Ensure the storage is generated by checking profile (e.g. "https+++foo.com/idb", "https+++foo.com/cache", "https+++boo.com/idb"")
  4. Test browser.browsingData.remove() by running that in an extension
  5. Check the profile. In this case, your "https+++foo.com/cache" should still exist and "https+++boo.com" shouldn't exist.

Madalin, would you mind checking by steps above again? Sorry for the back and forth and thanks!

(In reply to Tom Tung [:tt, :ttung] from comment #36)

  1. Check the profile. In this case, your "https+++foo.com/cache" should still exist and "https+++boo.com" shouldn't exist.
    to be more clear, both "https+++foo.com/idb" and "https+++boo.com/idb" should be removed.

I have used the following steps to verify this issue:

  • Create 2 different test pages. First one having only indexedDb, second one indexedDb + cache. To set indexedDb I have used (1) and for cache (2).
  • Create a webextension that clears indexDb using (3)
  • Open the page with only indexDb set. Open the profile folder and observe that the storage/default folder contains the "https+++boo.com/idb" folder
  • Use the webextension to clear the indexDb. Check the "https+++boo.com/" folder was completely removed
  • Open the page with indexDB + cache set. Open the profile folder and observe that the storage.default folder contains the "https+++boo.com/idb" and "https+++boo.com/cache" folders
  • Use the webextension to clear the indexDb. Check that only the "https+++boo.com/cache" folder was removed

(1) window.indexedDB.open("test_indexedDB", 1);
(2) window.caches.open("test_cache");
(3) browser.browsingData.remove({}, { indexedDB: true })

Marking the bug as verified fixed.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.