Closed Bug 1684854 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox86 --- fixed

People

(Reporter: sg, Assigned: sg)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).

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

Assignee: nobody → sgiesecke
Status: NEW → ASSIGNED

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

(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.

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!

Pushed by sgiesecke@mozilla.com: 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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: