Avoid gaps in history when new message is received before MAM query is made

Fixes #2940

CHANGES.md

@@ -22,6 +22,7 @@
 - #2623: Merge MUC join and bookmark, leave and unset autojoin 
 - #2716: Fix issue with chat display when opening via URL
 - #2844: Contact stays in "pending contacts"
+- #2940: Avoid gaps in history when new message is received before MAM query is made
 - #2980: Allow setting an avatar for MUCs
 - #3033: Add the `muc_grouped_by_domain` option to display MUCs on the same domain in collapsible groups
 - #3038: Message to self from other client is ignored

src/headless/plugins/mam/utils.js

@@ -89,7 +89,7 @@ export function getMAMPrefsFromFeature(feature) {
 /**
  * @param {MUC} muc
  */
-export async function preMUCJoinMAMFetch(muc) {
+export function preMUCJoinMAMFetch(muc) {
     if (
         !api.settings.get("muc_show_logs_before_join") ||
         !muc.features.get("mam_enabled") ||
@@ -97,8 +97,8 @@ export async function preMUCJoinMAMFetch(muc) {
     ) {
         return;
     }
-    await fetchNewestMessages(muc);
-    muc.save({ "prejoin_mam_fetched": true });
+    fetchNewestMessages(muc);
+    muc.save({ prejoin_mam_fetched: true });
 }
 
 /**
@@ -258,24 +258,22 @@ export function createScrollupPlaceholder(model) {
  * the last archived message in our local cache.
  * @param {ChatBox|MUC} model
  */
-export async function fetchNewestMessages(model) {
+export function fetchNewestMessages(model) {
     if (model.disable_mam) return;
 
-    const is_muc = model.get("type") === CHATROOMS_TYPE;
-    const disco_jid = is_muc ? model.get("jid") : _converse.session.get("bare_jid");
-    const supported = await api.disco.supports(NS.MAM, disco_jid);
-    if (!supported) return;
-
+    // XXX: It's important to first get the most recent message, before making any
+    // async calls, like `api.disco.supports`, so that we can avoid a race
+    // condition with possible new incoming messages.
+    // We want the most recent cached message, otherwise we would query with
+    // the wrong `start` value. This function used to call
+    // `api.disco.supports`, but it's also called in `fetchArchivedMessages`,
+    // so it's not necessary here.
     const most_recent_msg = model.getMostRecentMessage();
     const should_page = api.settings.get("mam_request_all_pages") ? "backwards" : false;
 
     if (most_recent_msg) {
-        // FIXME: A race condition might happen where, where a new message comes in
-        // before we've fetched the archived messages.
-        // Can be fixed by preventing processing of new messages until MAM
-        // has been fetched and processed.
-        fetchArchivedMessages(model, { mam: { start: most_recent_msg.get("time") }, rsm: { before: "" } }, should_page);
+        return fetchArchivedMessages(model, { mam: { start: most_recent_msg.get("time") }, rsm: { before: "" } }, should_page);
     } else {
-        fetchArchivedMessages(model, { rsm: { before: "" } }, should_page);
+        return fetchArchivedMessages(model, { rsm: { before: "" } }, should_page);
     }
 }

src/headless/types/plugins/mam/utils.d.ts

@@ -25,7 +25,7 @@ export function getMAMPrefsFromFeature(feature: Model): void;
 /**
  * @param {MUC} muc
  */
-export function preMUCJoinMAMFetch(muc: MUC): Promise<void>;
+export function preMUCJoinMAMFetch(muc: MUC): void;
 /**
  * @param {ChatBox|MUC} model
  * @param {Object} result

src/plugins/mam-views/tests/mam.js

@@ -538,6 +538,48 @@ describe("Message Archive Management", function () {
                     </query>
                 </iq>`);
         }));
+
+        it("queries with the right 'start' value if a new message is received before the MAM query is made",
+            mock.initConverse(['discoInitialized'],
+                {
+                    auto_fill_history_gaps: false,
+                    archived_messages_page_size: 2,
+                    muc_nickname_from_jid: false,
+                    muc_clear_messages_on_leave: false,
+                }, async function (_converse) {
+
+            const { api } = _converse;
+            const sent_IQs = api.connection.get().IQ_stanzas;
+            const muc_jid = 'orchard@chat.shakespeare.lit';
+            const nick = 'romeo';
+            const own_jid = _converse.session.get('jid');
+            const muc = await mock.openAndEnterMUC(_converse, muc_jid, nick);
+            const first_msg_id = u.getUniqueId();
+
+            // We first receive a new message before the MAM query could have been made.
+            let message = stx`
+                <message xmlns="jabber:client" type="groupchat" id="${first_msg_id}" to="${own_jid}" from="${muc_jid}/juliet">
+                    <body>First p0st!</body>
+                    <active xmlns="http://jabber.org/protocol/chatstates"/>
+                    <stanza-id xmlns="urn:xmpp:sid:0" id="${first_msg_id}" by="${muc_jid}"/>
+                </message>`;
+            _converse.api.connection.get()._dataRecv(mock.createRequest(message));
+
+            await u.waitUntil(() => muc.messages.length);
+
+            let iq_get = await u.waitUntil(() => sent_IQs.filter(iq => sizzle(`query[xmlns="${Strophe.NS.MAM}"]`, iq).length).pop());
+            const query_id = iq_get.querySelector('query').getAttribute('queryid');
+
+            // Even though a new message was received, we don't want to
+            // use its `start` value, since we want to query for
+            // messages older than it.
+            expect(iq_get).toEqualStanza(stx`
+                <iq id="${iq_get.getAttribute('id')}" to="${muc_jid}" type="set" xmlns="jabber:client">
+                    <query queryid="${query_id}" xmlns="${Strophe.NS.MAM}">
+                        <set xmlns="http://jabber.org/protocol/rsm"><before></before><max>2</max></set>
+                    </query>
+                </iq>`);
+        }));
     });
 
     describe("An archived message", function () {