Closed Bug 1837201 Opened 1 year ago Closed 1 year ago

Startup Crash in [@ mozilla::dom::quota::QuotaManager::GetInfoFromValidatedPrincipalInfo]

Categories

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

defect

Tracking

()

RESOLVED FIXED
116 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox114 blocking fixed
firefox115 --- fixed
firefox116 --- fixed

People

(Reporter: aryx, Assigned: janv)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

10 crashes from 8 installations of Firefox 114.0 released yesterday, all on startup

Crash report: https://crash-stats.mozilla.org/report/index/7b455f60-b108-4e38-b8f1-801220230607

MOZ_CRASH Reason: MOZ_CRASH(Should never get here!)

Top 10 frames of crashing thread:

0  xul.dll  mozilla::dom::quota::QuotaManager::GetInfoFromValidatedPrincipalInfo  dom/quota/ActorsParent.cpp:6613
1  xul.dll  mozilla::dom::quota::QuotaManager::LoadFullOriginMetadata  dom/quota/ActorsParent.cpp:4500
2  xul.dll  mozilla::dom::quota::QuotaManager::LoadFullOriginMetadataWithRestore  dom/quota/ActorsParent.cpp:4541
3  xul.dll  mozilla::dom::quota::QuotaManager::InitializeRepository<`lambda at /builds/worker/checkouts/gecko/dom/quota/ActorsParent.cpp:3879:7'&>::<lambda_2>::operator const  dom/quota/ActorsParent.cpp:4634
3  xul.dll  mozilla::dom::quota::QuotaManager::InitializeRepository<`lambda at /builds/worker/checkouts/gecko/dom/quota/ActorsParent.cpp:3879:7'&>::<lambda_2>::operator const  dom/quota/ActorsParent.cpp:4634
3  xul.dll  mozilla::CollectEach  dom/quota/QuotaCommon.h:1141
3  xul.dll  mozilla::dom::quota::detail::CollectEachFile  dom/quota/QuotaCommon.h:1530
3  xul.dll  mozilla::dom::quota::CollectEachFile  dom/quota/QuotaCommon.h:1546
3  xul.dll  mozilla::dom::quota::QuotaManager::InitializeRepository<`lambda at /builds/worker/checkouts/gecko/dom/quota/ActorsParent.cpp:3879:7'&>::<lambda_2>::operator const  dom/quota/ActorsParent.cpp:4634
3  xul.dll  mozilla::dom::quota::QuotaManager::InitializeRepository  dom/quota/ActorsParent.cpp:4634
Flags: needinfo?(bugmail)
Assignee: nobody → jvarga
Severity: -- → S2
Status: NEW → ASSIGNED
Priority: -- → P1

All instances so far come from LoadFullOriginMetadata. IIUC these are cases where we read the metadata from what we find on disk and pass it directly to GetInfoFromValidatedPrincipalInfo. I think that LoadFullOriginMetadata might want to check IsPrincipalInfoValid before using it and in case return an error as we would do on any other file/disk corruption problem (assuming that we never will write something wrong). And whatever is writing this data should maybe have a sanity check to exclude we are actually writing unexpected things, which seems not too unlikely given we are seeing this suddenly after 114 is released and on various desktop platforms and OS versions.

Yeah, it's very likely that we fail here https://searchfox.org/mozilla-central/rev/887d4b5da89a11920ed0fd96b7b7f066927a67db/caps/BasePrincipal.cpp#1258
We should return an error instead of crashing.

Regressed by: 1781201

This change in https://phabricator.services.mozilla.com/D176877 affects also non-PBM origins.

It might be enough to add a IsPrincipalInfoValid check here ?

Flags: needinfo?(bugmail)

(from a chat with Jan)

We assume the reason for this to be old metadata files on disk containing origin types that are now unsupported.

  • Previous to bug 1781201 we would just have loaded that metadata file and continued with storage initialization. Everything would work normally, except if users would have tried to really navigate to such an origin - they would have been blocked, probably.
  • The change in bug 1781201 makes us implicitly check now, if the read origin is currently supported. If not, we fail with this crash, which is bad. We need to find a way to ignore them again, instead.

Comment on attachment 9338104 [details]
Bug 1837201 - Ignore unsupported origin directories during various operations; r=#dom-storage

Beta/Release Uplift Approval Request

  • User impact if declined: Some unknown subset of users may experience startup crashes.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Jan has reproduced one of the situations locally which can cause the code in question to cause a browser startup crash and confirmed that the fix avoids a crash (and this was also before we removed the logic that could cause a crash).

We do not have enough data to know that this is the exact data-flow that caused the crash but the fix 100% removes the ability to cause a crash in this case and to have it cause an error instead. The fix also comprehensively adds new error handling code along the call paths that could previously lead to the crash location, and ensures that we ignore directories with problems rather than transmuting them into a broken QuotaManager. (That said, a broken QuotaManager would still be significantly preferred over a QuotaManager that is crashing the browser!) Our QM_TRY error handling and telemetry will also help us quantify instances where this new error handling is coming into play.

  • String changes made/needed:
  • Is Android affected?: Yes
Attachment #9338104 - Flags: approval-mozilla-release?
Attachment #9338104 - Flags: approval-mozilla-beta?
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/0f4f33638a08
Ignore unsupported origin directories during various operations; r=dom-storage-reviewers,jstutte,asuth

Comment on attachment 9338104 [details]
Bug 1837201 - Ignore unsupported origin directories during various operations; r=#dom-storage

Approved for 115.0b3.

Attachment #9338104 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Backed out changeset 9fd0bb67278 (bug 1837201) from beta for causing build bustage

Pushed by jstutte@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d88dedbf2b2d
Ignore unsupported origin directories during various operations; r=dom-storage-reviewers,jstutte,asuth

Comment on attachment 9338104 [details]
Bug 1837201 - Ignore unsupported origin directories during various operations; r=#dom-storage

Approved for 114.0.1

Attachment #9338104 - Flags: approval-mozilla-release? → approval-mozilla-release+
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch

Jens relanded the patch with a fix.

Flags: needinfo?(jvarga)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: