Skip to content
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

Perform crypto store operations directly after transaction #1233

Merged
merged 3 commits into from
Feb 27, 2020

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Feb 27, 2020

At least on Safari but perhaps other browsers as well, you must perform
IndexedDB operations in the same JS task as you start the transaction. As a
concrete example, you cannot open the transaction and await some promise before
actually using it.

This fixes the crypto store to meet this requirement.

Fixes element-hq/element-web#12207

This ensures we wait until after the device list writes to the crypto store
before marking thing as clean. This is particularly important for the error
path, as the write to the crypto store can fail.

Part of element-hq/element-web#12207
A few of the crypto store backend paths were missing try / catch wrappers to
abort the transaction if the inner callback throws.
At least on Safari but perhaps other browsers as well, you must perform
IndexedDB operations in the same JS task as you start the transaction. As a
concrete example, you cannot open the transaction and await some promise before
actually using it.

This fixes the crypto store to meet this requirement.

Fixes element-hq/element-web#12207
@jryans jryans requested a review from a team February 27, 2020 15:00
Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

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

Lgtm. So, any idea what was actually causing the transaction to close? If doTxn was able to create a transaction, _backendPromise must have been resolved already. So was it just awaiting _backendPromise that caused the transaction to close?

@jryans
Copy link
Collaborator Author

jryans commented Feb 27, 2020

If doTxn was able to create a transaction, _backendPromise must have been resolved already. So was it just awaiting _backendPromise that caused the transaction to close?

Well, _backendPromise is separate from the transaction, as it is always resolved after the store has started up.

Yes, just awaiting _backendPromise is the issue here: even if a promise is resolved, any then handlers are run asynchronously in a separate task, so that's enough for the transaction to close.

@jryans jryans merged commit fa19616 into develop Feb 27, 2020
@t3chguy t3chguy deleted the jryans/safari-e2e-idb branch May 10, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safari does not save e2e keys
2 participants