This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 828
Fix the StorageManger detecting a false positive consistency check when manually migrating to rust from labs #12225
Merged
BillCarsonFr
merged 5 commits into
develop
from
valere/element-r/fix_migrate_via_lab_flag
Feb 5, 2024
Merged
Fix the StorageManger detecting a false positive consistency check when manually migrating to rust from labs #12225
BillCarsonFr
merged 5 commits into
develop
from
valere/element-r/fix_migrate_via_lab_flag
Feb 5, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BillCarsonFr
added
the
T-Defect
Bugs, crashes, hangs, vulnerabilities, or other reported problems
label
Feb 5, 2024
BillCarsonFr
commented
Feb 5, 2024
indexedDB = window.indexedDB; | ||
} catch (e) {} | ||
// make this lazy in order to make testing easier | ||
function getIndexedDb(): IDBFactory | undefined { |
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.
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
andybalaam
approved these changes
Feb 5, 2024
3 tasks
Moved to draft as it depends on a js-sdk PR matrix-org/matrix-js-sdk#4055 |
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
dbkr
approved these changes
Feb 5, 2024
This side effect was cause by this change #12206 Previously the |
dbkr
added
the
backport staging
Label to automatically backport PR to staging branch
label
Feb 6, 2024
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 thefeature_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 theStorageManager
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
Here's what your changelog entry will look like:
🐛 Bug Fixes
Missing Session Data
Dialog element-hq/element-web#26970.