diff --git a/src/matrix/room/Room.js b/src/matrix/room/Room.js index f5c42097..61c10f24 100644 --- a/src/matrix/room/Room.js +++ b/src/matrix/room/Room.js @@ -120,7 +120,8 @@ export class Room extends BaseRoom { txn.roomMembers.removeAllForRoom(this.id); } const {entries: newEntries, updatedEntries, newLiveKey, memberChanges} = - await log.wrap("syncWriter", log => this._syncWriter.writeSync(roomResponse, isRejoin, txn, log), log.level.Detail); + await log.wrap("syncWriter", log => this._syncWriter.writeSync( + roomResponse, isRejoin, summaryChanges.hasFetchedMembers, txn, log), log.level.Detail); if (decryptChanges) { const decryption = await log.wrap("decryptChanges", log => decryptChanges.write(txn, log)); log.set("decryptionResults", decryption.results.size); diff --git a/src/matrix/room/members/RoomMember.js b/src/matrix/room/members/RoomMember.js index f810a9fe..78096060 100644 --- a/src/matrix/room/members/RoomMember.js +++ b/src/matrix/room/members/RoomMember.js @@ -141,6 +141,16 @@ export class MemberChange { return this.previousMembership === "join" && this.membership !== "join"; } + /** The result can be a false negative when all of these apply: + * - the complete set of room members hasn't been fetched yet. + * - the member event for this change was received in the + * state section and wasn't present in the timeline section. + * - the room response was limited, e.g. there was a gap. + * + * This is because during sync, in this case it is not possible + * to distinguish between a new member that joined the room + * during a gap and a lazy-loading member. + * */ get hasJoined() { return this.previousMembership !== "join" && this.membership === "join"; } diff --git a/src/matrix/room/timeline/persistence/MemberWriter.js b/src/matrix/room/timeline/persistence/MemberWriter.js index 3d35ff47..7579716e 100644 --- a/src/matrix/room/timeline/persistence/MemberWriter.js +++ b/src/matrix/room/timeline/persistence/MemberWriter.js @@ -24,17 +24,16 @@ export class MemberWriter { } writeTimelineMemberEvent(event, txn) { - return this._writeMemberEvent(event, false, txn); + const maybeLazyLoadMemberEvent = false; + return this._writeMemberEvent(event, maybeLazyLoadMemberEvent, txn); } - writeStateMemberEvent(event, isLimited, txn) { - // member events in the state section when the room response - // is not limited must always be lazy loaded members. - // If they are not, they will be repeated in the timeline anyway. - return this._writeMemberEvent(event, !isLimited, txn); + writeStateMemberEvent(event, hasFetchedMembers, txn) { + const maybeLazyLoadMemberEvent = !hasFetchedMembers; + return this._writeMemberEvent(event, maybeLazyLoadMemberEvent, txn); } - async _writeMemberEvent(event, isLazyLoadingMember, txn) { + async _writeMemberEvent(event, maybeLazyLoadMemberEvent, txn) { const userId = event.state_key; if (!userId) { return; @@ -56,17 +55,14 @@ export class MemberWriter { if (!existingMember || !existingMember.equals(member)) { txn.roomMembers.set(member.serialize()); this._cache.set(member); - // we also return a member change for lazy loading members if something changed, - // so when the dupe timeline event comes and it doesn't see a diff - // with the cache, we already returned the event here. - // - // it's just important that we don't consider the first LL event - // for a user we see as a membership change, or we'll share keys with - // them, etc... - if (isLazyLoadingMember && !existingMember) { - // we don't have a previous member, but we know this is not a - // membership change as it's a lazy loaded - // member so take the membership from the member + // if the member event appeared only in the state section, + // AND we haven't heard about it AND we haven't fetched all members yet (to avoid #470), + // this may be a lazy loading member (if it's not in a gap, we are certain + // it is a ll member, in a gap, we can't tell), so we pass in our own membership as + // as the previous one so we won't consider it a join to not have + // false positives to avoid #192. + // see also MemberChange.hasJoined + if (!existingMember && maybeLazyLoadMemberEvent) { return new MemberChange(member, member.membership); } return new MemberChange(member, existingMember?.membership); diff --git a/src/matrix/room/timeline/persistence/SyncWriter.js b/src/matrix/room/timeline/persistence/SyncWriter.js index 96551056..b1048e18 100644 --- a/src/matrix/room/timeline/persistence/SyncWriter.js +++ b/src/matrix/room/timeline/persistence/SyncWriter.js @@ -133,14 +133,14 @@ export class SyncWriter { return currentKey; } - async _writeStateEvents(roomResponse, memberChanges, isLimited, txn, log) { + async _writeStateEvents(roomResponse, memberChanges, hasFetchedMembers, txn, log) { // persist state const {state} = roomResponse; if (Array.isArray(state?.events)) { log.set("stateEvents", state.events.length); for (const event of state.events) { if (event.type === MEMBER_EVENT_TYPE) { - const memberChange = await this._memberWriter.writeStateMemberEvent(event, isLimited, txn); + const memberChange = await this._memberWriter.writeStateMemberEvent(event, hasFetchedMembers, txn); if (memberChange) { memberChanges.set(memberChange.userId, memberChange); } @@ -231,7 +231,7 @@ export class SyncWriter { * @param {Transaction} txn * @return {SyncWriterResult} */ - async writeSync(roomResponse, isRejoin, txn, log) { + async writeSync(roomResponse, isRejoin, hasFetchedMembers, txn, log) { let {timeline} = roomResponse; // we have rejoined the room after having synced it before, // check for overlap with the last synced event @@ -242,7 +242,7 @@ export class SyncWriter { const memberChanges = new Map(); // important this happens before _writeTimeline so // members are available in the transaction - await this._writeStateEvents(roomResponse, memberChanges, timeline?.limited, txn, log); + await this._writeStateEvents(roomResponse, memberChanges, hasFetchedMembers, txn, log); const {currentKey, entries, updatedEntries} = await this._writeTimeline(timeline, this._lastLiveKey, memberChanges, txn, log); log.set("memberChanges", memberChanges.size);