Startup Crash in [@ mozilla::dom::quota::QuotaManager::GetInfoFromValidatedPrincipalInfo]
Categories
(Core :: Storage: Quota Manager, defect, P1)
Tracking
()
| 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)
|
48 bytes,
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-beta+
dmeehan
:
approval-mozilla-release+
|
Details | Review |
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
|
Assignee | |
Updated•1 year ago
|
|
||
Comment 1•1 year ago
|
||
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.
|
|
Assignee | |
Comment 2•1 year ago
|
||
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.
|
||
Updated•1 year ago
|
|
|
||
Comment 3•1 year ago
|
||
This change in https://phabricator.services.mozilla.com/D176877 affects also non-PBM origins.
It might be enough to add a IsPrincipalInfoValid check here ?
|
|
||
Comment 4•1 year ago
|
||
(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.
|
|
||
Updated•1 year ago
|
|
|
Assignee | |
Comment 5•1 year ago
|
||
|
||
Comment 6•1 year ago
|
||
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
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 8•1 year ago
|
||
Comment on attachment 9338104 [details]
Bug 1837201 - Ignore unsupported origin directories during various operations; r=#dom-storage
Approved for 115.0b3.
|
|
||
Comment 9•1 year ago
|
||
| bugherder uplift | ||
|
||
Comment 10•1 year ago
|
||
Backed out changeset 0f4f33638a08 (bug 1837201) for causing build bustage at ActorsParent.cpp
Backout: https://hg.mozilla.org/integration/autoland/rev/cede3b34d1b5cd132f57be3355e348564a37303e
Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=0f4f33638a084b413745d9d148c40c59d04ca676
Failure log: https://treeherder.mozilla.org/logviewer?job_id=418611380&repo=autoland&lineNumber=13174
|
|
||
Comment 11•1 year ago
|
||
Backed out changeset 9fd0bb67278 (bug 1837201) from beta for causing build bustage
|
||
Comment 12•1 year ago
|
||
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 13•1 year ago
|
||
| bugherder uplift | ||
|
|
||
Comment 14•1 year ago
|
||
Comment on attachment 9338104 [details]
Bug 1837201 - Ignore unsupported origin directories during various operations; r=#dom-storage
Approved for 114.0.1
|
|
||
Comment 15•1 year ago
|
||
| bugherder uplift | ||
|
|
||
Comment 16•1 year ago
|
||
| bugherder | ||
Description
•