From 0a1cbf87b8071d12c022d78359f35d7389d3daa4 Mon Sep 17 00:00:00 2001 From: JC Brand Date: Wed, 3 Feb 2021 10:20:35 +0100 Subject: [PATCH] MUC: Ensure that MAM query is from latest cached messages In some cases a race condition can occur where a new message is received before the MAM query starts. Previously, the newly received message would be considered the latest message to query from, thereby causing a gap in the history. --- spec/mam.js | 54 +++++++++++++++++++++++++++++- spec/muc.js | 6 +++- src/headless/plugins/chat/model.js | 13 ++++--- src/headless/plugins/mam/utils.js | 2 +- 4 files changed, 67 insertions(+), 8 deletions(-) diff --git a/spec/mam.js b/spec/mam.js index ab43fea6c..02567c240 100644 --- a/spec/mam.js +++ b/spec/mam.js @@ -272,10 +272,62 @@ describe("Message Archive Management", function () { .map(e => e.textContent).join(' ') === "2nd Message 3rd Message 4th Message 5th Message 6th Message", 1000); done(); })); + + it("queries for messages since the most recent cached message in a newly entered MUC", + mock.initConverse(['discoInitialized'], + { + 'archived_messages_page_size': 2, + 'muc_nickname_from_jid': false, + 'muc_clear_messages_on_leave': false, + }, async function (done, _converse) { + + const { api } = _converse; + const sent_IQs = _converse.connection.IQ_stanzas; + const muc_jid = 'orchard@chat.shakespeare.lit'; + const nick = 'romeo'; + const room_creation_promise = api.rooms.open(muc_jid); + await mock.getRoomFeatures(_converse, muc_jid); + await mock.waitForReservedNick(_converse, muc_jid, nick); + await mock.receiveOwnMUCPresence(_converse, muc_jid, nick); + await room_creation_promise; + const view = _converse.chatboxviews.get(muc_jid); + await u.waitUntil(() => (view.model.session.get('connection_status') === converse.ROOMSTATUS.ENTERED)); + + // Create "cached" message to test that only messages newer than the + // last cached message with body text will be fetched + view.model.messages.create({ + 'type': 'groupchat', + 'to': muc_jid, + 'from': `${_converse.bare_jid}/orchard`, + 'body': 'Hello world', + 'message': 'Hello world', + 'time': '2021-02-02T12:00:00Z' + }); + // Hack: Manually set attributes that would otherwise happen in fetchMessages + view.model.messages.fetched_flag = true; + view.model.afterMessagesFetched(view.model.messages); + view.model.messages.fetched.resolve(); + + const affs = _converse.muc_fetch_members; + const all_affiliations = Array.isArray(affs) ? affs : (affs ? ['member', 'admin', 'owner'] : []); + await mock.returnMemberLists(_converse, muc_jid, [], all_affiliations); + + const iq_get = await u.waitUntil(() => sent_IQs.filter(iq => iq.querySelector(`iq query[xmlns="${Strophe.NS.MAM}"]`)).pop()); + expect(Strophe.serialize(iq_get)).toBe( + ``+ + ``+ + ``+ + `urn:xmpp:mam:2`+ + `2021-02-02T12:00:00.000Z`+ + ``+ + `2`+ + ``+ + ``); + return done(); + })); }); describe("An archived message", function () { - describe("when received", function () { it("is discarded if it doesn't come from the right sender", diff --git a/spec/muc.js b/spec/muc.js index fd75bad1b..a660e8bc0 100644 --- a/spec/muc.js +++ b/spec/muc.js @@ -385,7 +385,11 @@ describe("Groupchats", function () { 'time': '2021-02-02T12:00:00Z' }); expect(view.model.session.get('connection_status')).toBe(converse.ROOMSTATUS.NICKNAME_REQUIRED); - expect(view.el.querySelectorAll('converse-chat-message').length).toBe(1); + await u.waitUntil(() => view.el.querySelectorAll('converse-chat-message').length === 1); + + const sel = 'converse-message-history converse-chat-message .chat-msg__text'; + await u.waitUntil(() => view.el.querySelector(sel)?.textContent.trim()); + expect(view.el.querySelector(sel).textContent.trim()).toBe('Hello world') view.el.querySelector('[name="nick"]').value = nick; view.el.querySelector('.muc-nickname-form input[type="submit"]').click(); diff --git a/src/headless/plugins/chat/model.js b/src/headless/plugins/chat/model.js index 7f7f14ebd..7dddc4c65 100644 --- a/src/headless/plugins/chat/model.js +++ b/src/headless/plugins/chat/model.js @@ -103,7 +103,8 @@ const ChatBox = ModelWithContact.extend({ this.notifications = new Model(); }, - afterMessagesFetched () { + afterMessagesFetched (messages) { + this.most_recent_cached_message = messages ? this.getMostRecentMessage(messages) : null; /** * Triggered whenever a `_converse.ChatBox` instance has fetched its messages from * `sessionStorage` but **NOT** from the server. @@ -119,11 +120,12 @@ const ChatBox = ModelWithContact.extend({ log.info(`Not re-fetching messages for ${this.get('jid')}`); return; } + this.most_recent_cached_message = null; this.messages.fetched_flag = true; const resolve = this.messages.fetched.resolve; this.messages.fetch({ 'add': true, - 'success': () => { this.afterMessagesFetched(); resolve() }, + 'success': msgs => { this.afterMessagesFetched(msgs); resolve() }, 'error': () => { this.afterMessagesFetched(); resolve() } }); return this.messages.fetched; @@ -301,9 +303,10 @@ const ChatBox = ModelWithContact.extend({ } }, - getMostRecentMessage () { - for (let i=this.messages.length-1; i>=0; i--) { - const message = this.messages.at(i); + getMostRecentMessage (messages) { + messages = messages || this.messages; + for (let i=messages.length-1; i>=0; i--) { + const message = messages.at(i); if (message.get('type') === this.get('message_type')) { return message; } diff --git a/src/headless/plugins/mam/utils.js b/src/headless/plugins/mam/utils.js index 5bd97ec59..c4e73c70e 100644 --- a/src/headless/plugins/mam/utils.js +++ b/src/headless/plugins/mam/utils.js @@ -161,8 +161,8 @@ export function fetchNewestMessages (model) { if (model.disable_mam) { return; } + const most_recent_msg = model.most_recent_cached_message; - const most_recent_msg = model.getMostRecentMessage(); // if clear_messages_on_reconnection is true, than any recent messages // must have been received *after* connection and we instead must query // for earlier messages