1733107 - Avoid repository traversal during simple origin clearing

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task, P1)

Description

[Jan Varga [:janv]]

nsIQuotaManagerService::clearStoragesForPrincipal has an option to set "clearAll". Clear all here means that an origin prefix will be used, so a full repository traversal is needed in that case.

However, when we clear exact origin (clearAll=false), we don't have to traverse entire repository.

I checked current callers in JS:
https://searchfox.org/mozilla-central/search?q=clearstoragesforprincipal%28&path=&case=false&regexp=false

SiteDataManager passes clearAll=true
https://searchfox.org/mozilla-central/rev/79f93e7a8b9aa1903f1349f2dd46fb71596f2ae9/browser/modules/SiteDataManager.jsm#448
but that shouldn't be used during shutdown

ClearDataService passes clearAll=false
https://searchfox.org/mozilla-central/rev/79f93e7a8b9aa1903f1349f2dd46fb71596f2ae9/toolkit/components/cleardata/ClearDataService.jsm#515

I believe the latter is used during shutdown in some cases, so if we can avoid the repository traversal, it should help with reducing shutdown hangs (but not only that, some clear origin operations will be faster in general).

I think we should split clearStoragesForPrincipal into two separate methods (and split also the operation in the parent process which handles it), one for simple/exact origin clearing and another one for the clearing by prefix.

Comment 1

[Haiyang Xu]

Attachment: Bug 1733107 - Split nsIQuotaManagerService::clearStoragesForPrincipal into tow methods: one clears the exact origin, the other clears all under that origin; r=#dom-storage-reviewers

In this patch, clearStoragesForPrincipal is split into two methods: one with same name, which only clear the storage under that exact origin, the other named clearStoragesForPrincipalWithPrefix clears all the storage prefixed with that origin.

Comment 2

[Haiyang Xu]

Attachment: Bug 1733107 - Avoid directory traversal during simple origin clearing. r=#dom-storage-reviewers

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jstutte, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 4

[Jens Stutte [:jstutte]]

Hi Jan, I saw you took the two pending patches here.

Comment 5

[Jan Varga [:janv]]

I now have time for this. It seems this can help to reduce shutdown hangs as well.

Comment 6

[Pascal Chevrel:pascalc]

Tracking for 114 as we need a fix for this cycle due to spiking crashes in bug 1588498

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox114 (beta). However, the bug still has low severity.

:jstutte, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 8

[Jan Varga [:janv]]

(In reply to Pascal Chevrel:pascalc from comment #6)

Tracking for 114 as we need a fix for this cycle due to spiking crashes in bug 1588498

I'm focusing on this bug too, but I just found out the root cause of the spike in crashes.
It's described in bug 1833810.

Comment 9

[Jan Varga [:janv]]

Attachment: Bug 1733107 - Move directory locking to derived classes of ClearRequestBase; r=#dom-storage

ClearRequestBase currently provides generic directory locking based on generic
member variables. It's not totally clear what derived classes do in terms of
directory locking and work on the QuotaManager IO thread. It would be better if
ClearRequestBase only provided a generic DeleteFiles function instead.

Changed done in this patch:

• moved implementation of directory locking methods to derived classes of
  ClearRequestBase
• moved implementation of DoDirectoryWork to derived classes as well
• adjusted member variables

Depends on D186208

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1733107 - Rework remaining QuotaManagerService clearing methods to use asynchronous messages instead of creating sub actors; r=#dom-storage

QuotaManagerService::ClearStoragesForOriginAttributesPattern and
QuotaManagerService::ClearStoragesForPrincipal still create sub actors which
makes it hard to add new clearing operations which would use async IPC messages
and which would inherit from the ClearRequestBase class as well.

Changes done in this patch:

• added QuotaManager::ClearStoragesForOrigin
• added QuotaManager::ClearStoragesForOriginAttributesPattern
• changed ClearRequestBase to inherit from ResolvableNormalOriginOp
• QuotaManagerService::ClearStoragesForOriginAttributesPattern reworked to use
  an asynchronous message instead of a sub actor
• QuotaManagerService::ClearStoragesForPrincipal reworked to use an
  asynchronous message instead of a sub actor
• added a new mactor QM_CUF_AND_IPC_FAIL similar to QM_IPC_FAIL

creating a sub actor

Depends on D186627

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1733107 - Add nsIQuotaManagerService::ClearStoragesForOriginPrefix; r=#dom-storage

nsIQuotaManagerService::ClearStoragesForPrincipal currently supports both
clearing storages for a specific origin only and clearing storages for a group
of origins sharing the same prefix. It would be better to have separate methods
for this.

Changes done in this patch:

• added nsIQuotaManagerService::ClearStoragesForOriginPrefix
• added a new helper for testing clearOriginsByPrefix
• changed verifyStorage to accept an optional shared key name
• added thorough testing for the new method

Depends on D186628

Comment 12

[Jan Varga [:janv]]

Attachment: Bug 1733107 - Replace some nsIQuotaManagerService::ClearStoragesForPrincipal calls with nsIQuotaManagerService::ClearStoragesForOriginPrefix; r=#dom-storage

There's now a dedicated nsIQuotaManagerService::ClearStoragesForOriginPrefix
method for clearing storages for origin prefix. All callers of
nsIQuotaManagerService::ClearStoragesForPrincipal which want to clear all
storages for given origin prefix can be now converted to call the new method.

Changed done in this patch:

• replaced some nsIQuotaManagerService::ClearStoragesForPrincipal calls with
  nsIQuotaManagerService::ClearStoragesForOriginPrefix

Depends on D186777

Comment 13

[Jan Varga [:janv]]

Attachment: Bug 1733107 - Change nsIQuotaManagerService::ClearStoragesForPrincipal to support clearing of exact origins only; r=#dom-storage

There are now no callers of nsIQuotaManagerService::ClearStoragesForPrincipal
which would request clearing of all storages for given prefix. The support for
that can be removed.

Changed done in this patch:

• removed the aClearAll argument from
  nsIQuotaManagerService::ClearStoragesForPrincipal
• removed the aClearAll argument from the async IPC message
• removed the aClearAll argument from QuotaManager::ClearStoragesForOrigin
• changed ClearOriginOp to support clearing of exact origins only

Depends on D186778

Comment 14

[Jan Varga [:janv]]

Attachment: Bug 1733107 - Add QuotaManager::RemoveOriginDirectory; r=#dom-storage

The removing of an origin directory became a bit more complex because when the
app shutdown already started, origin directories shouldn't be fully removed
from disk. They need to be only moved to a special directory. All this
complexity should be covered by a dedicated QuotaManager method.

Changes done in this patch:

• added a new method QuotaManager::RemoveOriginDirectory
• added a prefilled string for the special to-be-removed directory
• adjusted ClearRequestBase::DeleteFiles to use the new
  QuotaManager::RemoveOriginDirectory method

Depends on D186779

Comment 15

[Jan Varga [:janv]]

Attachment: Bug 1733107 - Avoid repository traversals during clearing of exact origins; r=#dom-storage

ClearRequestBase::DeleteFiles currently provides a way to clear origins for any
combination of persistence type, origin scope and client type. All origin
directiories need to be traversed to find relevant matches. This can be slow if
there are many origin directories. Fortunately, the traversal can be completely
avoided when exact origin is being cleared.

Changed done in this patch:

• added a dedicated ClearRequestBase::DeleteFiles method for clearing of exact
  origins
• changed ClearOriginOp::DoDirectoryWork to use the new variant of
  ClearRequestBase::DeleteFiles

Depends on D186780

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e01641c5e0fb
Move directory locking to derived classes of ClearRequestBase; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/d1257bed6024
Rework remaining QuotaManagerService clearing methods to use asynchronous messages instead of creating sub actors; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/490c2555afd1
Add nsIQuotaManagerService::ClearStoragesForOriginPrefix; r=dom-storage-reviewers,asuth,jstutte
https://hg.mozilla.org/integration/autoland/rev/f57bf101a4d7
Replace some nsIQuotaManagerService::ClearStoragesForPrincipal calls with nsIQuotaManagerService::ClearStoragesForOriginPrefix; r=dom-storage-reviewers,pbz,asuth
https://hg.mozilla.org/integration/autoland/rev/01a294b6bd17
Change nsIQuotaManagerService::ClearStoragesForPrincipal to support clearing of exact origins only; r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/e9af992a25bf
Add QuotaManager::RemoveOriginDirectory; r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/988cc1efe8f1
Avoid repository traversals during clearing of exact origins; r=dom-storage-reviewers,asuth

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e01641c5e0fb
https://hg.mozilla.org/mozilla-central/rev/d1257bed6024
https://hg.mozilla.org/mozilla-central/rev/490c2555afd1
https://hg.mozilla.org/mozilla-central/rev/f57bf101a4d7
https://hg.mozilla.org/mozilla-central/rev/01a294b6bd17
https://hg.mozilla.org/mozilla-central/rev/e9af992a25bf
https://hg.mozilla.org/mozilla-central/rev/988cc1efe8f1

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/42180 for changes under testing/web-platform/tests

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot