search

MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.91k stars 4.86k forks source link

TypeError: Cannot read properties of undefined (reading 'version') #20860

Open sentry-io[bot] opened 1 year ago

sentry-io[bot] commented 1 year ago

Sentry Issue: METAMASK-X678

TypeError: Cannot read properties of undefined (reading 'version')
  at pendingMigrations (app/scripts/lib/migrator/index.js:83:53)
  at Array.filter (<anonymous>)
  at Migrator.migrateData (app/scripts/lib/migrator/index.js:35:47)
  at loadStateFromPersistence (app/scripts/background.js:400:33)
  at initialize (app/scripts/background.js:259:11)

Comment: >99% errors occur on Windows

Gudahtt commented 2 months ago

We don't know how the persisted state can be persisted without metadata, it doesn't seem possible (on any affected version). We've audited a wide range of versions trying to understand how it could occur and have come up blank.

Another odd thing about this error is the volume. It's very high volume, but we aren't getting very many user reports about it (none confirmed). From looking at the error reports, it seems that the error is repeating every 30 seconds or so for some affected wallet installations. But when testing this locally (by artificially corrupting state in the same way we see here), the error only happens once. This suggests to me that the bulk of the volume comes from some sort of automated workflow, not real user activity.

From what I read about how the chrome.storage.local API works, it seems that it does treat each top-level key as a separate piece of data. e.g. if you set the object { data: [...], meta: [...] }, it seems that it might be two separate write operations internally. Lots of the storage API methods work directly on top-level keys, e.g. remove takes a top-level key as the parameter.

It does seem possible that state might get corrupted in this way in production for a real user, though if so, it's probably not likely. I can't find similar reports on the Chromium bug tracker or google group.

Probably this is either a real, rare bug affecting a small percentage of users, but the numbers are inflated by some type of automated workflow. Or it could be completely automated e.g. with bad fixture data (but probably not, I did find some events that did not seem to repeat).

Gudahtt commented 2 months ago

I'd recommend that we address this by migrating to IndexedDb as our storage backend. Supposedly it's more reliable and intended for larger data sets than storage.local (according to some comments in the bug tracker made by Chrome devs). The already-in-progress vault recovery flow would serve as another mitigation.

I don't think we should attempt state recovery for affected users. It would be too difficult to figure out which migrations were run for each person, and you can tell from the debug snapshots that the answer is different from case to case.

gauthierpetetin commented 1 month ago

Task created to migrate to IndexedDb as our storage backend: https://github.com/MetaMask/metamask-extension/issues/25903