LL setting state gets corrupted if you have two tabs open?

[ara4n]

No description provided.

[michaelkaye]

One was in /develop one was in /app, the develop one had the issue, the app one was fine, refreshing develop informed me it would be 3x smaller memory again, then worked.

[lampholder]

@michaelkaye was able to reproduce this quite easily by:

• using /develop to look at a largish room
• open up a new tab on /app
• switch back to /develop and see how now everything lazyloady is a little bit wonky (the memberlist panel seems to have lost a bunch of people
• switch back to /app to see more weirdness no occurring there, too

I wasn't able to reproduce the above myself, though loading /app did seem to retrigger the 'Riot now uses 3 to 5 times less memory' notice when I returned to /develop.

[lampholder]

@bwindels let's chat about this when you get back

[bwindels]

So, for tabs where one is on /app and the other is on /develop:

/app (LL:off) retriggering the "Upgrade Riot" dialog in /develop (LL:on) is sort of working as designed:

1. If you would use a non-LL indexeddb/sync state with LL:on, it would probably work but you wouldn't get any of the RAM benefits, so we/I decided to resync in this case.
2. If you would use a LL indexeddb/sync state with LL:off, you would have missing members everywhere.

What happens in the above scenario is:

• user opens /develop, resyncs if it hasn't already because LL is now enabled, indexeddb now describes sync state with LL:on
• user goes to /app, which shares indexeddb with /develop, riot sees indexeddb with LL:on, and resyncs because of reason 2 stated above and afterwards indexeddb describes sync state with LL:off. This resyncing does not show a dialog (it's only shown when enabling LL).
• now the user goes back to /develop, which finds an indexeddb with LL:off, and resyncs because reason 1.

As the databases are incompatible, some ways to avoid these two deployments from sharing it would be:

• make them use a different origin (e.g. deploy /develop to develop.riot.im)
• put url path in indexeddb name

[bwindels]

About the missing members on /develop, this should not happen (scenario 1 above) as the LL-riot should happily LL if a room hasn't been marked as already LL'ed in the oob_membership_events object store, even with a non-LL database. Debugging the issue, reading that object store fails because indexeddb can't open a transaction to read that object store:

rageshake.js:66 DOMException: Failed to execute 'transaction' on 'IDBDatabase': The database connection is closing.
    at https://riot.im/develop/bundles/faf0cae3efaf9039cd16/bundle.js:25140:32
    at LocalIndexedDBStoreBackend.getOutOfBandMembers (https://riot.im/develop/bundles/faf0cae3efaf9039cd16/bundle.js:25139:16)

Persisting the sync state fails in the same way:

Persisting sync data up to  s697247543_570291280_34453_179995053_81909600_482269_11283822_9291958_20815
rageshake.js:66 sync fail: DOMException: Failed to execute 'transaction' on 'IDBDatabase': The database connection is closing.
    at https://riot.im/develop/bundles/faf0cae3efaf9039cd16/bundle.js:25417:33
From previous event:
    at LocalIndexedDBStoreBackend._persistUserPresenceEvents (https://riot.im/develop/bundles/faf0cae3efaf9039cd16/bundle.js:25416:38)
    at LocalIndexedDBStoreBackend.syncToDatabase (https://riot.im/develop/bundles/faf0cae3efaf9039cd16/bundle.js:25359:45)

I imagine this is because the database got deleted before resyncing by riot/app

[bwindels]

The only thing I'm still wondering is what

switch back to /app to see more weirdness no occurring there, too

is about. If you haven't refreshed /develop, /app should still have it's database and be happy.
@lampholder any more specifics you can give about that last switch back to /app? /develop was not refreshed, right?

[bwindels]

Summary

a lazy loading riot on the same origin as a non-lazy-loading riot end up fighting for the database, as they delete it and resync on page load. Deleting the database from under the other riot session's feet causes all sorts of other weirdness (all transactions fail). This is sort of as designed for the reasons stated above, and a fix would be that we namespace the database with the riot path (which could make either riot run out of idb storage double as fast) OR put develop (and staging? experimental?) on a different origin.

If LL were enabled on /app, this problem would be a lot less prominent, which might affect how we want to prioritize this issue. When we decide to do an RC with LL enabled, it would be good to test this scenario between /staging and /develop, to see that using the same database then doesn't give any major issues.

[bwindels]

Also, some issues seen last week might have been due to the fact that up to Riot 0.16.4 (/app until this Monday) was requesting version 2 of the database and /develop would have upgraded to version 3 (to store the client options, to detect a LL/non-LL database). This will make the store initialization fail, but will still start the client, probably with a lot of weirdness ensuing as all store operations would fail. As of Riot 0.16.5, the database is upgraded to version 3, even when not LL'ing, so this likely not playing a part in the issue reported here.

[ara4n]

can we not use the same locking that we have already for stopping multiple tabs from corrupting indexeddb for e2e? (or store the settings in local_storage, as indexeddb is probably overkill for simple settings?)

[bwindels]

Not really, as discussed on matrix, the core of the issue is that while syncing in two tabs, both will overwrite the sync data. This is fine as long as that data is the same, which is only true if the sync filter is the same.

[bwindels]

@michaelkaye said he wasn't able to reproduce the weirdness on /app anymore (described in the last step of the repro case) but said he was running from a fresh boot as opposed to resuming from sleep when reproing this with Tom.

[bwindels]

Got the version downgrade dialog working, but switching to a memory store with a LL-disabled client and LL-enabled store is proving difficult, in that it's infinispinnering. Thinking to give up on this last part, and just keep the current behaviour, as it's only a problem until /app has LL enabled.

As an easier to implement solution, we could also show a dialog in the above case with only the option of resyncing, but warning you'll have to close the other tab? As opposed to just resync without any warning as we do now.

[bwindels]

Went with the dialog, released as part of riot 0.16.6