From d8acf63e1d3d962fbc5a2e8266c34d0fed15324a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 2 Jun 2021 18:38:16 +0200 Subject: [PATCH 1/7] change mock event api a bit to be easier to read --- src/matrix/room/sending/SendQueue.js | 8 ++++---- src/mocks/event.js | 14 +++++++++----- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/matrix/room/sending/SendQueue.js b/src/matrix/room/sending/SendQueue.js index cdab31a8..0e1b116d 100644 --- a/src/matrix/room/sending/SendQueue.js +++ b/src/matrix/room/sending/SendQueue.js @@ -315,7 +315,7 @@ export class SendQueue { import {HomeServer as MockHomeServer} from "../../../mocks/HomeServer.js"; import {createMockStorage} from "../../../mocks/Storage.js"; import {NullLogger} from "../../../logging/NullLogger.js"; -import {event, withTextBody, withTxnId} from "../../../mocks/event.js"; +import {createEvent, withTextBody, withTxnId} from "../../../mocks/event.js"; import {poll} from "../../../mocks/poll.js"; export function tests() { @@ -326,12 +326,12 @@ export function tests() { const hs = new MockHomeServer(); // 1. enqueue and start send event 1 const queue = new SendQueue({roomId: "!abc", storage, hsApi: hs.api}); - const event1 = withTextBody(event("m.room.message", "$123"), "message 1"); + const event1 = withTextBody("message 1", createEvent("m.room.message", "$123")); await logger.run("event1", log => queue.enqueueEvent(event1.type, event1.content, null, log)); assert.equal(queue.pendingEvents.length, 1); const sendRequest1 = hs.requests.send[0]; // 2. receive remote echo, before /send has returned - const remoteEcho = withTxnId(event1, sendRequest1.arguments[2]); + const remoteEcho = withTxnId(sendRequest1.arguments[2], event1); const txn = await storage.readWriteTxn([storage.storeNames.pendingEvents]); const removal = await logger.run("remote echo", log => queue.removeRemoteEchos([remoteEcho], txn, log)); await txn.complete(); @@ -339,7 +339,7 @@ export function tests() { queue.emitRemovals(removal); assert.equal(queue.pendingEvents.length, 0); // 3. now enqueue event 2 - const event2 = withTextBody(event("m.room.message", "$456"), "message 2"); + const event2 = withTextBody("message 2", createEvent("m.room.message", "$456")); await logger.run("event2", log => queue.enqueueEvent(event2.type, event2.content, null, log)); // even though the first pending event has been removed by the remote echo, // the second should get the next index, as the send loop is still blocking on the first one diff --git a/src/mocks/event.js b/src/mocks/event.js index b7d20c8a..01cff281 100644 --- a/src/mocks/event.js +++ b/src/mocks/event.js @@ -14,18 +14,22 @@ See the License for the specific language governing permissions and limitations under the License. */ -export function event(type, id = null) { +export function createEvent(type, id = null) { return {type, event_id: id}; } -export function withContent(event, content) { +export function withContent(content, event) { return Object.assign({}, event, {content}); } -export function withTextBody(event, body) { - return withContent(event, {body, msgtype: "m.text"}); +export function withSender(sender, event) { + return Object.assign({}, event, {sender}); } -export function withTxnId(event, txnId) { +export function withTextBody(body, event) { + return withContent({body, msgtype: "m.text"}, event); +} + +export function withTxnId(txnId, event) { return Object.assign({}, event, {unsigned: {transaction_id: txnId}}); } From 4a8a6168cd2357868c87e3a728b795aa38c6dd45 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 2 Jun 2021 18:41:03 +0200 Subject: [PATCH 2/7] add failing test for race between sync & subscribing after openTimeline --- src/matrix/room/timeline/Timeline.js | 49 ++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/matrix/room/timeline/Timeline.js b/src/matrix/room/timeline/Timeline.js index d6895312..dfb75aca 100644 --- a/src/matrix/room/timeline/Timeline.js +++ b/src/matrix/room/timeline/Timeline.js @@ -232,3 +232,52 @@ export class Timeline { return this._ownMember; } } + +import {FragmentIdComparer} from "./FragmentIdComparer.js"; +import {Clock as MockClock} from "../../../mocks/Clock.js"; +import {createMockStorage} from "../../../mocks/Storage.js"; +import {createEvent, withTextBody, withSender} from "../../../mocks/event.js"; +import {NullLogItem} from "../../../logging/NullLogger.js"; +import {EventEntry} from "./entries/EventEntry.js"; +import {User} from "../../User.js"; +import {PendingEvent} from "../sending/PendingEvent.js"; + +export function tests() { + const fragmentIdComparer = new FragmentIdComparer([]); + const roomId = "$abc"; + return { + "adding or replacing entries before subscribing to entries does not loose local relations": async assert => { + const pendingEvents = new ObservableArray(); + const timeline = new Timeline({ + roomId, + storage: await createMockStorage(), + closeCallback: () => {}, + fragmentIdComparer, + pendingEvents, + clock: new MockClock(), + }); + // 1. load timeline + await timeline.load(new User("@alice:hs.tld"), "join", new NullLogItem()); + // 2. test replaceEntries and addOrReplaceEntries don't fail + const event1 = withTextBody("hi!", withSender("@bob:hs.tld", createEvent("m.room.message", "!abc"))); + const entry1 = new EventEntry({event: event1, fragmentId: 1, eventIndex: 1}, fragmentIdComparer); + timeline.replaceEntries([entry1]); + const event2 = withTextBody("hi bob!", withSender("@alice:hs.tld", createEvent("m.room.message", "!def"))); + const entry2 = new EventEntry({event: event2, fragmentId: 1, eventIndex: 2}, fragmentIdComparer); + timeline.addOrReplaceEntries([entry2]); + // 3. add local relation (redaction) + pendingEvents.append(new PendingEvent({data: { + roomId, + queueIndex: 1, + eventType: "m.room.redaction", + txnId: "t123", + content: {}, + relatedEventId: event2.event_id + }})); + // 4. subscribe (it's now safe to iterate timeline.entries) + timeline.entries.subscribe({}); + // 5. check the local relation got correctly aggregated + assert.equal(Array.from(timeline.entries)[0].isRedacting, true); + } + } +} \ No newline at end of file From 84ecaa2ee1e42a60b0acef4f978ffcfe442eefc9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 2 Jun 2021 18:41:52 +0200 Subject: [PATCH 3/7] don't trip over missing create events when loading power levels as the test from previous commit fails because of that, and powerlevels works fine without --- src/matrix/room/timeline/Timeline.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/matrix/room/timeline/Timeline.js b/src/matrix/room/timeline/Timeline.js index dfb75aca..af7ab405 100644 --- a/src/matrix/room/timeline/Timeline.js +++ b/src/matrix/room/timeline/Timeline.js @@ -84,10 +84,14 @@ export class Timeline { }); } const createState = await txn.roomState.get(this._roomId, "m.room.create", ""); - return new PowerLevels({ - createEvent: createState.event, - ownUserId: this._ownMember.userId - }); + if (createState) { + return new PowerLevels({ + createEvent: createState.event, + ownUserId: this._ownMember.userId + }); + } else { + return new PowerLevels({ownUserId: this._ownMember.userId}); + } } _setupEntries(timelineEntries) { From 365bd5cad025fb2b634b13dff83aa09b456be7d4 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 2 Jun 2021 18:42:46 +0200 Subject: [PATCH 4/7] fix the race --- src/matrix/room/timeline/Timeline.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/matrix/room/timeline/Timeline.js b/src/matrix/room/timeline/Timeline.js index af7ab405..73086db4 100644 --- a/src/matrix/room/timeline/Timeline.js +++ b/src/matrix/room/timeline/Timeline.js @@ -145,6 +145,17 @@ export class Timeline { } _addLocalRelationsToNewRemoteEntries(entries) { + // because it is not safe to iterate a derived observable collection + // before it has any subscriptions, we bail out if this isn't + // the case yet. This can happen when sync adds or replaces entries + // before load has finished and the view has subscribed to the timeline. + // + // Once the subscription is setup, MappedList will set up the local + // relations as needed with _applyAndEmitLocalRelationChange, + // so we're not missing anything by bailing out. + if (!this._localEntries.hasSubscriptions) { + return; + } // find any local relations to this new remote event for (const pee of this._localEntries) { // this will work because we set relatedEventId when removing remote echos From d2f5b412ac282676715451ea3d43375d16775bae Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 2 Jun 2021 18:43:16 +0200 Subject: [PATCH 5/7] don't try to hook up local relations for events that are not relations and do unnecessary work --- src/matrix/room/timeline/Timeline.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/matrix/room/timeline/Timeline.js b/src/matrix/room/timeline/Timeline.js index 73086db4..49c171c1 100644 --- a/src/matrix/room/timeline/Timeline.js +++ b/src/matrix/room/timeline/Timeline.js @@ -120,12 +120,17 @@ export class Timeline { return params ? params : false; }; // first, look in local entries based on txn id - const foundInLocalEntries = this._localEntries.findAndUpdate( - e => e.id === pe.relatedTxnId, - updateOrFalse, - ); + if (pe.relatedTxnId) { + const found = this._localEntries.findAndUpdate( + e => e.id === pe.relatedTxnId, + updateOrFalse, + ); + if (found) { + return; + } + } // now look in remote entries based on event id - if (!foundInLocalEntries && pe.relatedEventId) { + if (pe.relatedEventId) { this._remoteEntries.findAndUpdate( e => e.id === pe.relatedEventId, updateOrFalse From 0c4c018ceb11beb1bfd2215b8c82660d87221ded Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 2 Jun 2021 18:43:47 +0200 Subject: [PATCH 6/7] add note that powerlevels won't update when the state event is changed --- src/matrix/room/timeline/Timeline.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/matrix/room/timeline/Timeline.js b/src/matrix/room/timeline/Timeline.js index 49c171c1..eab71255 100644 --- a/src/matrix/room/timeline/Timeline.js +++ b/src/matrix/room/timeline/Timeline.js @@ -76,6 +76,7 @@ export class Timeline { } async _loadPowerLevels(txn) { + // TODO: update power levels as state is updated const powerLevelsState = await txn.roomState.get(this._roomId, "m.room.power_levels", ""); if (powerLevelsState) { return new PowerLevels({ From d965d57be7b3b342ffabb2ea61fd100f028741ed Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 2 Jun 2021 18:44:03 +0200 Subject: [PATCH 7/7] don't leak timeline when an error is thrown while opening it or you are just stuck with "not dealing with race" errors until refresh --- src/matrix/room/BaseRoom.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/matrix/room/BaseRoom.js b/src/matrix/room/BaseRoom.js index f3d2a707..531f6a1a 100644 --- a/src/matrix/room/BaseRoom.js +++ b/src/matrix/room/BaseRoom.js @@ -404,10 +404,16 @@ export class BaseRoom extends EventEmitter { clock: this._platform.clock, logger: this._platform.logger, }); - if (this._roomEncryption) { - this._timeline.enableEncryption(this._decryptEntries.bind(this, DecryptionSource.Timeline)); + try { + if (this._roomEncryption) { + this._timeline.enableEncryption(this._decryptEntries.bind(this, DecryptionSource.Timeline)); + } + await this._timeline.load(this._user, this.membership, log); + } catch (err) { + // this also clears this._timeline in the closeCallback + this._timeline.dispose(); + throw err; } - await this._timeline.load(this._user, this.membership, log); return this._timeline; }); }