1474608 - Storage folders of containers removed with contextualIdentities.remove aren't deleted

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P3)

Description

[08xjcec48]

Attachment: undeleted storage folders.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704003137

Steps to reproduce:

- Install https://addons.mozilla.org/addon/temporary-containers/
- Set it to delete containers "After the last tab in it closes"
- Browse websites that make use of local storage, like https://www.facebook.com and https://www.folha.uol.com.br/
- Close those tabs


Actual results:

Firefox keeps storage folders for those containers in my profile folder, even though they've been removed with contextualIdentities.remove (https://github.com/stoically/temporary-containers/issues/127#issuecomment-385382460). Even after restarting Firefox, the folders are still kept.




Expected results:

Storage folders of removed containers should be completely wiped.

Comment 1

[Rares Doghi, Desktop QA]

Hi, I managed to reproduce this issue using the latest version of Firefox Nightly 63.0a1 (2018-07-12)and the created folders are not deleted from the Profile folder after the user closes the last tab from temporary containers.

Comment 2

[Andrew Swan [:aswan]]

The webextensions API just calls ContextualIdentityService.remove().  That does appear to clear associated storage:
https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/toolkit/components/contextualidentity/ContextualIdentityService.jsm#298-299

Over to the containers maintainers to look at (or WONTFIX)

Comment 3

[Jonathan Kingston [:jkt] he/him]

This storage probably should be cleaned up. ni Baku because I know you are working on storage things at the moment.

Comment 4

[max.nordlund]

Same for me, and to make it more exciting about:newtab creates an SQLite database. I'm not sure if this is still the case, but a couple of months ago I had thousands of these newtab databases lying around, each 5Mb, which ate a decently large chunk of my hard drive.

I've been using a small shell script to manually delete these files, but that's fragile, especially since extension data got moved to the same folder.

function clean-firefox-containers -d "Removes storage from old temporary containers"
  set -l storage_dir ~/Library/Application\ Support/Firefox/Profiles/*.default/storage/default

  find $storage_dir -name "http*^userContextId=????*" | xargs rm -r
  find $storage_dir -name "about+newtab^userContextId=*" | xargs rm -r
end

Comment 5

[08xjcec48]

Max, I no longer feel alone in this world. I've lost my extensions settings because of a similar PowerShell script I've been using since I opened this issue. Here's the fixed version:

Get-ChildItem -path "C:\Users\123\AppData\Roaming\Mozilla\Firefox\Profiles\123\storage\default" | Where{$_.Name -Match "userContextId|instagram|gazeta|github.io|globo|googleusercontent|yandex"} | Where{$_.Name -NotMatch "moz-"} | Remove-Item -Recurse

I'm adding some domains manually too because another Firefox limitation prevents Cookie AutoDelete from deleting some kind of storage created by tabs not in containers (IndexedDB, I think).

Comment 6

[Andrea Marchesini [:baku]]

Maybe janv knows more about what is happening here.
When a container is removed, we call: Services.clearData.deleteDataFromOriginAttributesPattern({ userContextId });
https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/toolkit/components/contextualidentity/ContextualIdentityService.jsm#298

Why do we still have container-related folders in the user profile directory?

Comment 7

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

I think changes by Tom and Jan here to origin clearing in 68 should cause these directories to be removed, but redirecting to Tom to confirm and/or further analysis.

Comment 8

[Tom Tung [:tt, :ttung]]

(In reply to Andrea Marchesini [:baku] from comment #6)

When a container is removed, we call: Services.clearData.deleteDataFromOriginAttributesPattern({ userContextId });
https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/toolkit/components/contextualidentity/ContextualIdentityService.jsm#298

This should be observed by QMS [1], IIUC.

While trying to reproduce the issue, I found QMS didn't receive the message. I wonder if it hasn't been initialized/registered or something else so that it didn't propagate the message to the QM. I'll look into it more.

[1] https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/dom/quota/QuotaManagerService.cpp#791

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #7)

I think changes by Tom and Jan here to origin clearing in 68 should cause these directories to be removed, but redirecting to Tom to confirm and/or further analysis.

Yes, that's https://bugzilla.mozilla.org/show_bug.cgi?id=1529122. If the folder is empty, it would be removed in 68.

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1474608 - QuotaManagerService should call AddObserver() for clear-origin-attributes-data during Init();

Comment 10

[Jan Varga [:janv]]

See also bug 1537882. The category entry doesn't work there too.

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1474608 - P1 - Clear origin attributes directly for deleteDataFromOriginAttributesPattern if it's possible;

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1474608 - P2 - Clear origin attributes data directly on cache2;

Depends on D33758

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1474608 - P3 - Add a argument (aCallback) to deleteDataFromOriginAttributesPattern and add a browser test to verify the behavior on quota storage;

Depends on D34324

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1474608 - P1a - Clear origin attributes directly for PushService;

Depends on D33758

Comment 15

[Tom Tung [:tt, :ttung]]

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

I will ask one more review when the result of the try is good.

Comment 16

[Tom Tung [:tt, :ttung]]

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

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

I will ask one more review when the result of the try is good.

I need to make the test security/manager/ssl/tests/unit/test_sss_sanitizeOnShutdown.js pass. A fix for that should be in another patch.

Comment 17

[Tom Tung [:tt, :ttung]]

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

I need to make the test security/manager/ssl/tests/unit/test_sss_sanitizeOnShutdown.js pass. A fix for that should be in another patch.

Hmm, so this seems to be a complicated issue:

• sanitizeOnShutdown seems to be triggered by "profile-change-teardown"
• If the SWM hasn't been initialized yet, it would be lazily initialized in Services.clearData.deleteDataFromOriginAttributesPattern because of the change in D33758
• SWM::Init will add a shutdown blocker and the will be removed when it gets the topic "profile-change-teardown". (But I suspect, in this case, it won't because the topic has already fired)
• Because the added blocker is not removed, it blocks the process of shutting down.
• I need to check if this scenario occurs in other functions for clearData

If the sequence is not expected, SWM would just block the shutdown after the change I made in this bug.

Maybe a solution is not to init the SWM if it hasn't been initialized.

Comment 18

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1474608 - P4 - Initialize swm before the profile-change-teardown in test_sss_sanitizeOnShutdown.js to avoid test timed out;

The test starts timed out after applying the changes in P1-P3. The main reason is that P1-P3 ensure SWM and the QuotaManger clear their storage in any condition in clear data service. However, since the SWM adds a shutdown blocker during the initialization and it's initialized during the profile-change-teardown because of the changes and the test scenario.

To fix that, ideally, SWM should differentiate if it's initialized before or during the profile-change-teardown and that requires a non-small change. Since we haven't got this signature in the real world and similar cases (SWM gets initialized during profile-change-teardown) have been taken care of, this patch only adds a workaround to avoid the issue.

Depends on D34325

Comment 19

[Tom Tung [:tt, :ttung]]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20dbddeeb3a6
P1 - Clear origin attributes directly for deleteDataFromOriginAttributesPattern if it's possible; r=asuth,baku
https://hg.mozilla.org/integration/autoland/rev/51ddf8bf0847
P2 - Clear origin attributes data directly on cache2; r=asuth,michal,baku
https://hg.mozilla.org/integration/autoland/rev/ea125fda1920
P3 - Add a argument (aCallback) to deleteDataFromOriginAttributesPattern and add a browser test to verify the behavior on quota storage; r=asuth,baku
https://hg.mozilla.org/integration/autoland/rev/bd7b2d89fa96
P4 - Initialize swm before the profile-change-teardown in test_sss_sanitizeOnShutdown.js to avoid test timed out; r=dom-workers-and-storage-reviewers,perry

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/20dbddeeb3a6
https://hg.mozilla.org/mozilla-central/rev/51ddf8bf0847
https://hg.mozilla.org/mozilla-central/rev/ea125fda1920
https://hg.mozilla.org/mozilla-central/rev/bd7b2d89fa96