1866402 - Cover origin initialization in OpenStorageDirectory and OpenClientDirectory

Status: RESOLVED FIXED

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

Description

[Jan Varga [:janv]]

Quota clients and origin operations currently still have to call EnsurePersistentOriginIsInitializedInternal or EnsureTemporaryOriginIsInitializedInternal method on the QM IO thread. This should be covered by OpenStorageDirectory and OpenClientDirectory instead.

Comment 1

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Add a dedicated method for clearing storages of a specific client; r=#dom-storage

Client storages can be currently cleared by using the optional argument of
nsIQuotaManagerService::ClearStoragesForPrincipal. However, supporting clearing
storages for both a principal and a client is not ideal for several reasons.
Clearing of origins will have to unset some flags which is problematic when
a client type is specified and clearing of clients will eventually need to
unset some other flags as well.

Depends on D195400

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Add testing for clearing by client type; r=#dom-storage

Depends on D195400

Comment 3

[Jan Varga [:janv]]

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

There's now a dedicated nsIQuotaManagerService::ClearStoragesForClient method
for clearing storages by client type. All callers of
nsIQuotaManagerService::ClearStoragesForPrincipal which want to clear storages
for a specific client only can be now converted to call the new method.

Depends on D195403

Comment 4

[Jan Varga [:janv]]

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

Depends on D195504

Comment 5

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Rework QuotaManagerService::ResetStoragesForPrincipal to use an async message with an async response; r=#dom-storage

A sub actor is no longer created. Actual result is now returned as
an asynchronous response to an asynchronous message.

Depends on D195505

Comment 6

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Add a dedicated method for resetting storages of a specific client; r=#dom-storage

Client storages can be currently reset by using the optional argument of
nsIQuotaManagerService::ResetStoragesForPrincipal. However, supporting
resetting storages for both a principal and a client is not ideal for several
reasons. Resetting of origins will have to unset some flags which is
problematic when a client type is specified and resetting of clients will
eventually need to unset some other flags as well.

Depends on D195507

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Replace some nsIQuotaManagerService::ResetStoragesForPrincipal calls with nsIQuotaManagerService::ResetStoragesForClient; r=#dom-storage

There's now a dedicated nsIQuotaManagerService::ResetStoragesForClient method
for resetting storages by client type. All callers of
nsIQuotaManagerService::ResetStoragesForPrincipal which want to reset storages
for a specific client only can be now converted to call the new method.

Depends on D195508

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Change nsIQuotaManagerService::ResetStoragesForPrincipal to support resetting of entire origins only; r=#dom-storage

Depends on D195509

Comment 9

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Change all origin operations which uninit origins to return an array of origin metadata; r=#dom-storage

This is a preparation for unsetting some flags when origins are either cleared
or reset.

Depends on D195510

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Add nsIQuotaManagerService::PersistentOriginInitialized and nsIQuotaManagerService::TemporaryOriginInitialized methods; r=#dom-storage

This additional testing infrastructure is needed for verifying origin
initialization status.

Depends on D195522

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Move nsCStringHashKeyDM to a dedicated hash keys header file; r=#dom-storage

This removes some code duplication, but the primary motivation is to use it for
a member variable declaration in QuotaManager.h

Depends on D195526

Comment 12

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Implement origin initialization status tracking on the owning thread; r=#dom-storage

Origin initialization tracking on the owning thread is needed for covering
origin initialization in QuotaManager::OpenClientDirectory.

Depends on D195532

Comment 13

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Add UninitOrigins directory lock category and use it for directory locks which uninitialize origins; r=#dom-storage

The new category is needed for implementing an optimization in
QuotaManager::InitializePersistentOrigin,
QuotaManager::InitializeTemporaryOrigin and
QuotaManager::OpenClientDirectory to avoid creating a new origin initialization
operations when a given origin is already initialized.

Depends on D195535

Comment 14

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Resolve promises for origin initialization immediately if origins are already initialized; r=#dom-storage

It's now possible to check if an origin is already initialized on the owning
thread and it's also possible to evaluate existing directory locks if there are
any uninit origin operations pending. So we can add optimizations to origin
initialization methods to resolve promises immediately if origins are already
initialized and there are no pending uninit storage or uninit origin operations.

Depends on D195543

Comment 15

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Make it possible to initialize temporary origins without ensuring origin directories; r=#dom-storage

LSNG already uses some QuotaManager APIs to achieve that origin directories are
not created if they don't exist during datastore preparation, but the feature
is not easy to use and it's also not generalized enough for use in other quota
clients. Besides that, the way how it's currently done in LSNG complicates
removal of QuotaManager::EnsureTemporaryOriginIsInitializedInternal calls from
LSNG. This patch is about generalizing of the feature, making it available to
all quota clients.

Depends on D195548

Comment 16

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Add QuotaManager::PrincipalMetadataToPrincipalInfo method; r=#dom-storage

Depends on D195551

Comment 17

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Add support for initializing corresponding origin to QuotaManager::OpenClientDirectory; r=#dom-storage

QuotaManager::OpenClientDirectory can now get initialization status of
corresponding origin and initialize the origin if it's needed.

Depends on D195570

Comment 18

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Replace most of QuotaManager::EnsurePersistentOriginIsInitializedInternal and QuotaManager::EnsureTemporaryOriginIsInitializedInternal calls; r=#dom-storage

QuotaManager::OpenClientDirectory already makes sure that corresponding origin
is initialized so all places where a client directory lock is obtained in such
way can start using QuotaManager::GetOrCreateOriginDirectory instead of
QuotaManager::Ensure(Persistent|Temporary)OriginIsInitializedInternal.

Depends on D195582

Comment 19

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Implement initialization of entire persistent storage; r=#dom-storage

IndexedDB still calls QuotaManager::EnsurePersistentOriginIsInitializedInternal
from its idle database maintenance. However, we want to get rid of that so the
method can be made private. The idle database maintenance should only rely on
QuotaManager::OpenStorageDirectory method to do all necessary initialization.
Before we can do that, we need a method dedicated for initialization of entire
persistent storage.

Depends on D195589

Comment 20

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Add support for initializing persistent storage to QuotaManager::OpenStorageDirectory; r=#dom-storage

QuotaManager::OpenStorageDirectory can now detect the requested persistence
scope and initialize persistent storage if it's needed.

Depends on D195653

Comment 21

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Remove QuotaManager::EnsurePersistentOriginIsInitializedInternal calls from IndexedDB maintenance; r=#dom-storage

QuotaManager::OpenStorageDirectory already makes sure that persistent storage
is initialized so this call is now redundant.

Depends on D195661

Comment 22

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Change QuotaManager::Ensure(Persistent|Temporary)OriginIsInitializedInternal to be private methods; r=#dom-storage

QuotaManager::Ensure(Persistent|Temporary)OriginIsInitializedInternal are now
called only from Initialize(Persistent|Temporary)OriginOp. It's now easy to
change the methods to be private methods.

Depends on D195665

Comment 23

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Clean up OriginInfo::mIsExtension initialization; r=#dom-storage

The getter IsExtensionOrigin doesn't require the lock to be held, so the member
must be const.

Depends on D195548

Comment 24

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Use the cached value if it's available in PersistOp::DoDirectoryWork; r=#dom-storage

If temporary storage is already initialized, we can use the cached value and
avoid reading it from the metadata file (which was used for loading the cached
value).

Depends on D196657

Comment 25

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Update the cached value in PersistOp::DoDirectoryWork only when the value changed in the metadata file; r=#dom-storage

Currently, we call QuotaManager::PersistOrigin even when the origin was already
persisted. QuotaManager::PersistOrigin won't try to update the value again,
but it will have to do a hash lookup. We can easilly avoid this by calling
QuotaManager::PersistOrigin only when the value changed in the metadata file.

Depends on D198664

Comment 26

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Update the cached value in PersistOp::DoDirectoryWork only after successful creation of the metadata file; r=#dom-storage

Currently, the cached value is updated before creation of the metadata file
which can lead to a mismatch between the metadata file and the cached value if
creation of the metadata file fails. It will be safer to update the cached
value only after successful creation of the metadata file.

Depends on D198668

Comment 27

[Jan Varga [:janv]]

Attachment: Bug 1866402 - QM: Add a helper for mapping MozPromise values; r=#dom-storage

Comment 28

[Jan Varga [:janv]]

Attachment: Bug 1866402 - QM: Generalize the helper for mapping MozPromise values to support both exclusive and non-exclusive promises; r=#dom-storage

Comment 29

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Rename nsCStringHashKeyDM to nsCStringHashKeyWithDisabledMemmove; r=#dom-storage

Comment 30

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Reduce code duplication in QuotaManager::OpenStorageDirectory and QuotaManager::OpenClientDirectory; r=#dom-storage

Comment 31

[Jan Varga [:janv]]

Attachment: Bug 1866402 - Further reduce code duplication in QuotaManager::OpenStorageDirectory and QuotaManager::OpenClientDirectory; r=#dom-storage

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe4a050ac4da
Add testing for clearing by client type; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/396d9243d417
Add a dedicated method for clearing storages of a specific client; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/281a926003fd
Replace some nsIQuotaManagerService::ClearStoragesForPrincipal calls with nsIQuotaManagerService::ClearStoragesForClient; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/a604c144191f
Change nsIQuotaManagerService::ClearStoragesForPrincipal to support clearing of entire origins only; r=dom-storage-reviewers,jari

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fe4a050ac4da
https://hg.mozilla.org/mozilla-central/rev/396d9243d417
https://hg.mozilla.org/mozilla-central/rev/281a926003fd
https://hg.mozilla.org/mozilla-central/rev/a604c144191f

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67a39d84be12
Rework QuotaManagerService::ResetStoragesForPrincipal to use an async message with an async response; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/ddb496eb39ab
Add a dedicated method for resetting storages of a specific client; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/6a90f36c8bf5
Replace some nsIQuotaManagerService::ResetStoragesForPrincipal calls with nsIQuotaManagerService::ResetStoragesForClient; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/b12a2f55233f
Change nsIQuotaManagerService::ResetStoragesForPrincipal to support resetting of entire origins only; r=dom-storage-reviewers,jari

Comment 35

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/67a39d84be12
https://hg.mozilla.org/mozilla-central/rev/ddb496eb39ab
https://hg.mozilla.org/mozilla-central/rev/6a90f36c8bf5
https://hg.mozilla.org/mozilla-central/rev/b12a2f55233f

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba51eb2a6d1f
QM: Add a helper for mapping MozPromise values; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/0afeb47f38b0
QM: Generalize the helper for mapping MozPromise values to support both exclusive and non-exclusive promises; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/f799f35b1c84
Change all origin operations which uninit origins to return an array of origin metadata; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/aaa63956f3f0
Add nsIQuotaManagerService::PersistentOriginInitialized and nsIQuotaManagerService::TemporaryOriginInitialized methods; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/f2254b3d51d9
Move nsCStringHashKeyDM to a dedicated hash keys header file; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/e18f8613cd79
Rename nsCStringHashKeyDM to nsCStringHashKeyWithDisabledMemmove; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/8380179aa33f
Implement origin initialization status tracking on the owning thread; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/5c07a7c55bf3
Add UninitOrigins directory lock category and use it for directory locks which uninitialize origins; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/83daa2e3f527
Resolve promises for origin initialization immediately if origins are already initialized; r=dom-storage-reviewers,jari

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/ba51eb2a6d1f
https://hg.mozilla.org/mozilla-central/rev/0afeb47f38b0
https://hg.mozilla.org/mozilla-central/rev/f799f35b1c84
https://hg.mozilla.org/mozilla-central/rev/aaa63956f3f0
https://hg.mozilla.org/mozilla-central/rev/f2254b3d51d9
https://hg.mozilla.org/mozilla-central/rev/e18f8613cd79
https://hg.mozilla.org/mozilla-central/rev/8380179aa33f
https://hg.mozilla.org/mozilla-central/rev/5c07a7c55bf3
https://hg.mozilla.org/mozilla-central/rev/83daa2e3f527

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc03e4975743
Clean up OriginInfo::mIsExtension initialization; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/a649d075c9b3
Use the cached value if it's available in PersistOp::DoDirectoryWork; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/48781c71d287
Update the cached value in PersistOp::DoDirectoryWork only when the value changed in the metadata file; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/2ab08c49f2b1
Update the cached value in PersistOp::DoDirectoryWork only after successful creation of the metadata file; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/d63dac1ee4d4
Make it possible to initialize temporary origins without ensuring origin directories; r=dom-storage-reviewers,jari

Comment 39

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/cc03e4975743
https://hg.mozilla.org/mozilla-central/rev/a649d075c9b3
https://hg.mozilla.org/mozilla-central/rev/48781c71d287
https://hg.mozilla.org/mozilla-central/rev/2ab08c49f2b1
https://hg.mozilla.org/mozilla-central/rev/d63dac1ee4d4

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f25dfc157ef7
Add QuotaManager::PrincipalMetadataToPrincipalInfo method; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/6180f2e753bd
Add support for initializing corresponding origin to QuotaManager::OpenClientDirectory; r=devtools-reviewers,nchevobbe

Comment 41

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

Backed out the last 7 changesets for causing crashes (Bug 1762908):
https://hg.mozilla.org/mozilla-central/rev/326f317b084883b87bd495f2d1b715bd5bc1be8e

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9d5a469bdbd
Clean up OriginInfo::mIsExtension initialization; r=dom-storage-reviewers,jari

Comment 43

[Jan Varga [:janv]]

(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #41)

Backed out the last 7 changesets for causing crashes (Bug 1762908):
https://hg.mozilla.org/mozilla-central/rev/326f317b084883b87bd495f2d1b715bd5bc1be8e

Looking ...
So far it seems my try preset is not broad enough.
I don't see any reference to concrete failed tests (nor in bug 1762908), so for now I'm only guessing that one of those failed tests is dom/serviceworkers/test/browser_storage_recovery.js
So I probably need to extend my try preset to avoid backouts like this in future.

Comment 44

[Jan Varga [:janv]]

(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #41)

Backed out the last 7 changesets for causing crashes (Bug 1762908):
https://hg.mozilla.org/mozilla-central/rev/326f317b084883b87bd495f2d1b715bd5bc1be8e

What was the main reason for the backout ?
Crashes on our CI or increased crashes in our crash stats ?

Comment 45

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

(In reply to Jan Varga [:janv] from comment #44)

What was the main reason for the backout ?
Crashes on our CI or increased crashes in our crash stats ?

Bug 1762908 is for crashes submitted by users and for [@ mozilla::Maybe<T>::value | mozilla::dom::quota::OriginInfo::LockedDecreaseUsage] it lists 7 crashes from 7 installs for the last Nightly (built during the European night, these builds have lower usage volume than the ones released during the European afternoon).

Comment 46

[Jan Varga [:janv]]

Ok, thanks, that's what I needed to know (I already checked crash stats but I needed to be sure). So there's actually no particular existing test, I probably need to check the code and find out which scenario can lead to the crash and eventually create a test for it.

Comment 47

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/d9d5a469bdbd

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9feaa9086222
Use the cached value if it's available in PersistOp::DoDirectoryWork; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/8b98c798b362
Update the cached value in PersistOp::DoDirectoryWork only when the value changed in the metadata file; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/2fd502bdc2d9
Update the cached value in PersistOp::DoDirectoryWork only after successful creation of the metadata file; r=dom-storage-reviewers,jari

Comment 49

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/9feaa9086222
https://hg.mozilla.org/mozilla-central/rev/8b98c798b362
https://hg.mozilla.org/mozilla-central/rev/2fd502bdc2d9

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b84880c5de44
Make it possible to initialize temporary origins without ensuring origin directories; r=dom-storage-reviewers,jari

Comment 51

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/b84880c5de44

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b3ae7465c92
Add QuotaManager::PrincipalMetadataToPrincipalInfo method; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/5ae0cf36c38b
Add support for initializing corresponding origin to QuotaManager::OpenClientDirectory; r=devtools-reviewers,nchevobbe

Comment 53

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7b3ae7465c92
https://hg.mozilla.org/mozilla-central/rev/5ae0cf36c38b

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3dde61076462
Replace most of QuotaManager::EnsurePersistentOriginIsInitializedInternal and QuotaManager::EnsureTemporaryOriginIsInitializedInternal calls; r=dom-storage-reviewers,jari

Comment 55

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/3dde61076462

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46a5babaafd2
Implement initialization of entire persistent storage; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/84592335a026
Reduce code duplication in QuotaManager::OpenStorageDirectory and QuotaManager::OpenClientDirectory; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/013f6b408358
Further reduce code duplication in QuotaManager::OpenStorageDirectory and QuotaManager::OpenClientDirectory; r=dom-storage-reviewers,jari

Comment 57

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/46a5babaafd2
https://hg.mozilla.org/mozilla-central/rev/84592335a026
https://hg.mozilla.org/mozilla-central/rev/013f6b408358

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dd238ab3298
Add support for initializing persistent storage to QuotaManager::OpenStorageDirectory; r=dom-storage-reviewers,jari

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5102d235d544
Remove QuotaManager::EnsurePersistentOriginIsInitializedInternal calls from IndexedDB maintenance; r=dom-storage-reviewers,aiunusov,jari
https://hg.mozilla.org/integration/autoland/rev/cfd1cf5aaeaf
Change QuotaManager::Ensure(Persistent|Temporary)OriginIsInitializedInternal to be private methods; r=dom-storage-reviewers,jari

Comment 60

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/9dd238ab3298

Comment 61

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/5102d235d544
https://hg.mozilla.org/mozilla-central/rev/cfd1cf5aaeaf