Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix the StorageManger detecting a false positive consistency check when manually migrating to rust from labs #12225

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Feb 5, 2024

Fixes element-hq/element-web#26970

Depends on matrix-org/matrix-js-sdk#4055

Note for reviewers:

In the react-sdk there is a class named StorageManager that is used to do some consistency checks on the data stored for element-web.
It is called by Lifecycle.ts when web is loaded and a existing session is beeing restored.

The StorageManager appears to do some tests on the persited storage prior to loading the session in order to give some feedbacks to users if something went wrong. As per the dialog shown (presumably) if your computer is low on space, or if you have been inactive for too long it is possible that some of the persisted storage (local storage, indexeddb) could be deleted.

In order to do that, StorageManager is doing some checks on what is persisted, by checking the presence of some internal things in persistant storage (some keys in local storage or the presence of indexeddb tables).

Now that we support 2 differents crypto stacks, the StorageManager has to do different checks depending on the stack. This was done by checking the feature_rust_crypto device level config.
In the normal case when you change stack, the new database is created and the feature_rust_crypto is set to true. So at the next reload the StorageManager can check for the rust database.

But there is an edge case, it's when triggering manual migration by using the experimental lab checkbox.
And in that case, only the feature_rust_crypto is set to true and the app restarted.

So in that case StorageManager was doing the consistenty checks for rust, that are failing because the rust database is created later in the Lifecycle.

So the fix is to modify the StorageManager to be able to detect that case, when rust crypto is not yet bootstraped, but there is a legacy database not yet migrated.

As a side note, the StorageManager is not well documented. There are some checks fields (healthy) that seems not used and not making very clear what they are supposed to do. Looks like it's false when there is no indexedb support?

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

@BillCarsonFr BillCarsonFr added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Feb 5, 2024
@BillCarsonFr BillCarsonFr requested review from a team as code owners February 5, 2024 13:21
indexedDB = window.indexedDB;
} catch (e) {}
// make this lazy in order to make testing easier
function getIndexedDb(): IDBFactory | undefined {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to do that better. If it's not done like that it makes it very difficult to test in unit test (StorageManager-test) because fake indexed db wont work

test/utils/StorageManager-test.ts Outdated Show resolved Hide resolved
test/utils/StorageManager-test.ts Outdated Show resolved Hide resolved
test/utils/StorageManager-test.ts Show resolved Hide resolved
@BillCarsonFr BillCarsonFr marked this pull request as draft February 5, 2024 14:02
@BillCarsonFr
Copy link
Member Author

Moved to draft as it depends on a js-sdk PR matrix-org/matrix-js-sdk#4055

@dbkr dbkr changed the title Fixes the StorageManger detecting a false positive consistency check when manually migrating to rust from labs Fix the StorageManger detecting a false positive consistency check when manually migrating to rust from labs Feb 5, 2024
@BillCarsonFr
Copy link
Member Author

This side effect was cause by this change #12206

Previously the setCryptoInitialised was never set to true, so the session restore process was never aborted even if the legacy database was missing

@BillCarsonFr BillCarsonFr added this pull request to the merge queue Feb 5, 2024
Merged via the queue into develop with commit cdfcd37 Feb 5, 2024
46 of 50 checks passed
@BillCarsonFr BillCarsonFr deleted the valere/element-r/fix_migrate_via_lab_flag branch February 5, 2024 18:17
@dbkr dbkr added the backport staging Label to automatically backport PR to staging branch label Feb 6, 2024
RiotRobot pushed a commit that referenced this pull request Feb 6, 2024
…en manually migrating to rust from labs (#12225)

* Fix StorageManager checks for rust migration manual opt-in

* fix restricted imports

* Moved utility to check db internals into js-sdk

* fix typos and test names

* more timeout for migration test

(cherry picked from commit cdfcd37)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport staging Label to automatically backport PR to staging branch T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
3 participants