-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat: add workarounds for database corruption scenarios #39010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
4867fbd to
13ffee0
Compare
Builds ready [ea89449]
UI Startup Metrics (1279 ± 106 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [ea89449]
UI Startup Metrics (1279 ± 106 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
MajorLift
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the edge case of repeated persistence errors while the user ignores the toast, would it make sense to just keep on showing the toast?
Thanks for the review. It's updated. |
Builds ready [84b2ee7]
UI Startup Metrics (1296 ± 101 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [7a23cab]
UI Startup Metrics (1298 ± 107 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
MajorLift
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
nit: I feel like the UX in the vault recovery screen's a little confusing. Users might close and re-open the app repeatedly without discovering the "restore accounts" button timeout. Thoughts on reducing the wait a little bit? Doesn't need to happen in this pr but will be happy to reapprove if it does. diff --git a/ui/helpers/utils/state-corruption-html.ts b/ui/helpers/utils/state-corruption-html.ts
index ec8b4f7c90..c7b501925f 100644
--- a/ui/helpers/utils/state-corruption-html.ts
+++ b/ui/helpers/utils/state-corruption-html.ts
@@ -129,6 +129,6 @@ export async function displayStateCorruptionError(
button.disabled = false;
// wait a while before enabling the button to try to prevent accidental
// or rush clicks.
- }, 5000);
+ }, 3000);
}
} |
No one can read, understand, and consider the ramifications of pressing that button within 5 seconds. The wait was intentionally long to make users a bit uneasy about pressing this button. Pressing the button is scary. |
Builds ready [03a318d]
UI Startup Metrics (1278 ± 105 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|




Description
This PR implements workarounds for Firefox database corruption issues that cause
browser.storage.localoperations to fail with "An unexpected error occurred".Problem
Firefox stores extension data in SQLite databases with external files for large blobs. Some corruption scenarios, like missing or renamed external files (file IDs mismatch), can break these databases.
When this happens,
browser.storage.local.get()orbrowser.storage.local.set()fail entirely, leaving users locked out of their wallet:Solution
This PR adds two recovery mechanisms:
Vault Recovery on
get()failure: Whenbrowser.storage.local.get()fails and a backup vault exists in IndexedDB, users are redirected to the vault recovery flow to restore their wallet.Informative Toast on
set()failure: Whenbrowser.storage.local.set()fails (even after vault recovery), a persistent toast is displayed informing users that the storage is failing and MetaMask needs to be reinstalled. The toast includes a link to save the Secret Recovery Phrase before reinstalling.Slack thread
Out of scope for this PR
Changelog
CHANGELOG entry: Fixes Firefox database corruption causing "An unexpected error occurred" by redirecting to vault recovery when possible, or displaying guidance to reinstall when recovery is not possible.
Related issues
Fixes: #10091
Fixes: #35681
Manual testing steps
Prerequisites
yarn dist:mv2)test-vault-corruption-firefox.shscript from this PRTest 1: Vault Recovery flow started on storage
get()failure./test-vault-corruption-firefox.shand select option 5 (Rename files off-by-one)Test 2: Toast displayed on storage
set()failureScreenshots/Recordings
Before
Users see "An unexpected error occurred" with no recovery path.

After
Loom recording
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Introduces recovery UX and error signaling for storage failures, plus improved error context.
PersistenceManager.get()to handlestorage.local.geterrors, trigger vault recovery viaPersistenceError(now includescause) when an IndexedDB backup exists; onsetfailures, notify UI via a newsetOnSetFailedcallback (background wires this toAppStateController.setShowStorageErrorToast(true)).appState.showStorageErrorToast(non-persisted) and expose in Sentry snapshots and background types.StorageErrorToastrendered globally inToastMaster(gated byselectShowStorageErrorToast), with action to navigate to SRP reveal; add i18n strings for title/description/action.causeMessagefromPersistenceError.causewhen notifying UI.Written by Cursor Bugbot for commit 03a318d. This will update automatically on new commits. Configure here.