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

Element-R: lock out multiple tabs to avoid races #25157

Closed
richvdh opened this issue Apr 19, 2023 · 11 comments · Fixed by matrix-org/matrix-react-sdk#11425
Closed

Element-R: lock out multiple tabs to avoid races #25157

richvdh opened this issue Apr 19, 2023 · 11 comments · Fixed by matrix-org/matrix-react-sdk#11425
Assignees
Labels
A-Element-R Issues affecting the port of Element's crypto layer to Rust T-Enhancement

Comments

@richvdh
Copy link
Member

richvdh commented Apr 19, 2023

Using the Rust crypto SDK from two tabs at once will cause data corruption, due to the fact that the rust-sdk caches information in memory.

Examples of the sorts of problems that could arise:

@richvdh richvdh added T-Enhancement A-Element-R Issues affecting the port of Element's crypto layer to Rust labels Apr 19, 2023
@richvdh
Copy link
Member Author

richvdh commented Apr 19, 2023

A (somewhat hacky) option might be to lock out multiple tabs, by sending messages over a BroadcastChannel

@richvdh
Copy link
Member Author

richvdh commented Jul 14, 2023

for now at least, we will lock out one of the tabs.

Element Call have done something like this and we may be able to benefit from their implementation

@richvdh richvdh changed the title Element-R: support for multiple tabs Element-R: lock out multiple tabs to avoid races Jul 17, 2023
@richvdh richvdh self-assigned this Aug 8, 2023
richvdh added a commit to matrix-org/matrix-react-sdk that referenced this issue Aug 16, 2023
This isn't used yet, but will form part of the solution to
element-hq/element-web#25157.
richvdh added a commit to matrix-org/matrix-react-sdk that referenced this issue Aug 16, 2023
This isn't used yet, but will form part of the solution to
element-hq/element-web#25157.
@richvdh
Copy link
Member Author

richvdh commented Aug 17, 2023

I could probably do with some design input on this. Currently I have thrown together a page that looks like this:

image

@nadonomy
Copy link
Member

@richvdh can we add some kind of interaction to choose which tab to use Element in? For example, WhatsApp web has this:

Screenshot 2023-08-17 at 17 02 54

Separately, can we use Element UI components? Or does this page need to be some kind of barebones HTML/styling?

If the latter, I'll whip something together copying the mobile install guide.

@richvdh
Copy link
Member Author

richvdh commented Aug 17, 2023

the answer is (I think) "yes" to both, though we're deliberately trying to keep this feature lightweight as it's meant to be a quick alternative to "proper" multitab support.

I might need help from someone like @germain-gg in using UI components in EW properly, but it's just react components like the rest of EW so should be fine.

@richvdh
Copy link
Member Author

richvdh commented Aug 18, 2023

@nadonomy I made a loading screen that looks like this:

image

github-merge-queue bot pushed a commit to matrix-org/matrix-react-sdk that referenced this issue Aug 22, 2023
* Add mechanism to check only one instance of the app is running

This isn't used yet, but will form part of the solution to
element-hq/element-web#25157.

* disable instrumentation for SessionLock

* disable coverage reporting

* exclude SessionLock in sonar.properties

* Revert "disable coverage reporting"

This reverts commit 80c4336.

* only disable session storage

* use pagehide instead of visibilitychange

* Add `checkSessionLockFree`

* Give up waiting for a lock immediately when someone else claims

* Update src/utils/SessionLock.ts
@Flightkick
Copy link

@richvdh Thanks, this is awesome! Had my config drop twice this week because I accidentally opened element in another tab. Quite frustrating to re-auth and verify again. Lovely QOL feature!

@richvdh
Copy link
Member Author

richvdh commented Aug 29, 2023

  1. There's no need to be sarcastic. We know this is frustrating, but we didn't do it just to annoy people. The description at the top of the issue explains the reasons it is necessary.
  2. This shouldn't cause you to have to re-auth. Please open a new issue describing your symptoms more clearly.

@Flightkick
Copy link

@richvdh sorry for not replying earlier, I just noticed your response. My enthusiasm was genuine, although I can understand how it may have come across as sarcasm.

I added my frustration for context. I can't quite remember how I landed here at the time, but the concurrent access of browser storage may have had something to do with it. The exact details are vague to me at this point.

Anyhow, upon looking for a solution I stumbled upon this feature you made very recently and I was pleased to discover that opening another tab will soon be handled more gracefully. Sharing the frustration was meant to let you know that this solves a PITA for me, hence the compliment on the QOL feature.

@nukeop
Copy link

nukeop commented Sep 22, 2023

This sucks: not only do I open multiple tabs so that I don't have to switch between different contexts manually, this triggers even if it's open in a single tab, and I have to reload Element to fix it, with all the usual punishments for reloading. Please revert this.

@richvdh
Copy link
Member Author

richvdh commented Oct 3, 2023

I'm afraid we can't revert this, as it is currently required to support use of the new cryptography library, which is central to our strategy for making encryption more reliable.

We will track restoring multi-tab support at #26231.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Element-R Issues affecting the port of Element's crypto layer to Rust T-Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants