matrix-org / matrix-js-sdk

Matrix Client-Server SDK for JavaScript
Apache License 2.0
1.56k stars 583 forks source link

Indexed DB can have inconsistency in membership event content #4198

Open AndrewFerr opened 4 months ago

AndrewFerr commented 4 months ago

It's possible for the sync store to have incorrect content of a m.room.member event in roomsData.join.$roomID.state[*], but correct content for that same event in roomsData.join.$roomID.timeline[*].

To reproduce:

With that said, any client that uses timeline to load state won't have any problems (and I assume Element Web does that, as it has accurate member lists in rooms). But if a client were read from state, it would get incorrect data.

AndrewFerr commented 4 months ago

For example:

image

Element devtools & Indexed DB are showing the same event (as the room ID & event ID between the two are the same), but they each have a different value for "membership".

The correct content is in the timeline version of the event, though:

image

I assume that, unlike /sync's Joined Room response objects, the js-sdk's Indexed DB state should be a snapshot of the current room state, and not whatever the state was up until the start of timeline. But even if that weren't the case, it's still a problem that two occurrences of the same event in the Indexed DB have different "content"s.

dbkr commented 4 months ago

Just looking at this for triage, but how are you getting two tabs signed in as different users? I don't see how this would be possible since they share the same localstorage etc: they would be logged in as the same user.

MidhunSureshR commented 4 months ago

To add to the above comment, the app should also show you an error:

Image

AndrewFerr commented 4 months ago

how are you getting two tabs signed in as different users?

With Firefox container tabs.

But since the repro steps involve only 2 users, an alternative to using container tabs is to have one of the 2 users in a private browsing / incognito window.

toger5 commented 3 months ago

This is potentially related. https://github.com/matrix-org/matrix-js-sdk/pull/4153/files

There was a switch from using a currentState cache to reading the state from the timeline.getState() method.

I tried refactoring this with the help of @t3chguy but there was more to do so the PR is currently put on ice.

toger5 commented 3 months ago

There is a new finding outlined here: https://matrix.to/#/!jxlRxnrZCsjpjDubDX:matrix.org/$2X7ysozWciiyDHVev8iJ5yaFcZbZr57H2u8v5-7ysXw?via=matrix.org&via=envs.net&via=mozilla.org

State seems to be updated with older state. The sync loop passes state to setStateEvents twice (occasionally) so we end up with multiple state changes.