Olm is prone to race conditions across multiple tabs

[richvdh]

If you have multiple tabs open for the same account, they will fight over the localstorage and corrupt each other.

Simple example: Two tabs run the "generate ephemeral keys" timer at the same time. They generate different keys, but only one set is successfully persisted in the Olm account. I suspect this has been happening to trilobite17, who likes to have multiple tabs open.

Another example:

• User sends a message in Tab 1. We do not have a session with Bob, so we claim a one-time key for him and half the rest of the room
• Tab 1 starts generating outbound sessions with all of the devices in the room
• Meanwhile, Tab 2 receives a prekey message from Bob. It creates the inbound session and removes the ephemeral key from the account (and saves the account and new session).
• Tab 1 generates a new outbound session with Bob and saves it, dropping the inbound session

[richvdh]

I think this is important.

[richvdh]

I've been doing some thinking about this.

Implementing race-free concurrency between tabs via localstorage is hard, because Chrome (at least) just doesn't provide any promises in this area. We have no idea how long it takes a write in one process to be reflected in another, which makes any read-modify-write process inherently racy.

Some options might be:

1. Something quite slow via localstorage, like writing to a slot to claim a lock, then sleeping for a while to check we got it.
2. Use a shared Web Worker (ie, a single worker thread shared between all tabs on a domain), to do the crypto work. This would solve any concurrency problems for us, although it might introduce headaches in terms of how we manage upgrades of the worker thread. However, the main problem is that they aren't supported by Safari, nor do they look likely to be.
3. Use a Service Worker (some sort of fancy-pants cache manager: again, shared between the tabs). All the problems of Shared Web Workers with none of the advantages (although Webkit are still thinking about implementing this one).
4. Use IndexedDB for storage, instead of localstorage. This has the benefit of having defined transactional semantics; it may also provide a larger storage capacity. However, it isn't supported in Firefox incognito mode, currently.

I guess we want to use IndexedDB with fallback to Localstorage.

[richvdh]

This causes corrupted olm sessions, so really does need to be p1

[richvdh]

The situation is even worse with electron, which totally ignores updates which happen in another process: see electron/electron#2493. I have no idea how to fix this, currently.

[richvdh]

I can definitely reproduce this. Opening riot in two tabs (and sending messages from them) can break communication with peers in both directions.

[lampholder]

The secret third option is to bully people into only using one tab (but I don't like this).

I like the idea of using IndexedDB; falling back to localstorage sounds like it will still have all the same problems when we do fall back - could we consider just not supporting firefox incognito until we have the bandwidth to carve out a reliable localstorage fallback option?

[dbkr]

The electron app is now single-instance, so this should mitigate this on electron.

[richvdh]

This will also be a problem for syncing device lists.

[dbkr]

Conclusion for Firefox private tabs is to fall back to in-memory storage for the crypto store rather than trying to keep the localstorage-backed session store as a fallback. The one downside of this is that e2e data won't be persisted if you hit refresh.

[dbkr]

Actually thinking about this more, the failure mode on upgrading would be terrible since you'd hit the update button in the new version bar and it would throw away all your e2e data. I've made it fall back to localstorage.

[dbkr]

This should be fixed now all the various parts have been migrated to indexeddb!