1684854 - Consistently handle non-existence of usage file in QuotaClient::InitOrigin

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, defect)

Description

[Simon Giesecke [:sg] [he/him]]

Currently, there is an inconsistency in handling the non-existing of the usage file, which seems to cause a large number (ca. 50%) of temporary storage initialization failures.

It is first checked if the usage file exists at https://searchfox.org/mozilla-central/rev/2fcab997046ba9e068c5391dc7d8848e121d84f8/dom/localstorage/ActorsParent.cpp#8763 (possibly removing it afterwards, depending on the existing of the usage journal file).

However, even if it did not exist (or was removed), it is attempted to load it at https://searchfox.org/mozilla-central/rev/2fcab997046ba9e068c5391dc7d8848e121d84f8/dom/localstorage/ActorsParent.cpp#8792, if a database file exists. This necessary fails. In that case, CreateStorageConnection is called at https://searchfox.org/mozilla-central/rev/2fcab997046ba9e068c5391dc7d8848e121d84f8/dom/localstorage/ActorsParent.cpp#8798. If that detects a corrupted database file, it will attempt to remove the usage file at https://searchfox.org/mozilla-central/rev/2fcab997046ba9e068c5391dc7d8848e121d84f8/dom/localstorage/ActorsParent.cpp#508, which necessarily fails, causing the error to be propagated.

This doesn't seem to be the right handling of this case. Instead, removing the usage file in CreateStorageConnection should only be attempted if it existed (or a failure to remove it because it did not exist should be ignored).

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1684854 - Consistently handle non-existence of usage file in QuotaClient::InitOrigin. r=#dom-workers-and-storage

Specifically, do not attempt and fail removal of the usage file in
CreateStorageConnection if the usage file did not exist.

Comment 2

[Jan Varga [:janv]]

I can't believe it's 50% of temporary storage initialization failures. Maybe you mean 50% of LSNG origin init.
Anyway, did you find this by checking the new telemetry ?
Also, I think we should have a test for this, there's already https://searchfox.org/mozilla-central/source/dom/localstorage/test/unit/test_originInit.js
A corrupted database can be found here: https://searchfox.org/mozilla-central/source/dom/localstorage/test/unit/corruptedDatabase_profile.zip

Comment 3

[Simon Giesecke [:sg] [he/him]]

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

I can't believe it's 50% of temporary storage initialization failures. Maybe you mean 50% of LSNG origin init.

Sorry, I wanted to add two important constraints here, but forgot to when I submitted the bug: this is for build id 20201229095620 (i.e. a Nightly build) in the last week of 2020. I also don't think this will be true for the general Beta/Release population.

Anyway, did you find this by checking the new telemetry ?

Yes, based on https://sql.telemetry.mozilla.org/queries/77015/source (not sure if the query is stable).

If I interpret that correctly, there were a total of 3,927 failures, and 1,919 were due to this situation.

Also, I think we should have a test for this, there's already https://searchfox.org/mozilla-central/source/dom/localstorage/test/unit/test_originInit.js
A corrupted database can be found here: https://searchfox.org/mozilla-central/source/dom/localstorage/test/unit/corruptedDatabase_profile.zip

Definitely. Thanks for the pointer. I'll give that a look.

Comment 4

[Jan Varga [:janv]]

Ok, I checked the query and the code and it all makes sense to me now. Thanks for the detailed description of the problem in comment 0.
I'm really glad that the QM_TRY macros and the new event based telemetry finally bear fruit!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84699b60367c
Consistently handle non-existence of usage file in QuotaClient::InitOrigin. r=dom-workers-and-storage-reviewers,janv

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/84699b60367c