Prevent duplicate message on fetch/event race

[gnprice]

When the user navigates to a narrow so we fetch the messages there, and then a recent message in the narrow belatedly arrives as a MessageEvent, we currently end up showing the message duplicated in the message list. Instead, we should suppress the duplicate.

This seems to happen reasonably often in practice: @PIG208 saw it last week, and then I saw it today. We've also had code to handle it in zulip-mobile since forever.

Details in chat thread:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/duplicate.20message.20from.20fetch.2Fevent.20race/near/1931798

[PIG208]

Just ran into this again. Looks like this bug can fail a message list invariant when you unresolve the topic containing duplicate message:

BUG: Error handling an event: 'package:zulip/model/message_list.dart': Failed assertion: line 245 pos 12: 'contents.length == messages.length': is not true.
  event: Instance of 'UpdateMessageEvent'

  /// Removes the given messages, if present.
  ///
  /// Returns true if at least one message was present, false otherwise.
  /// If none of [messageIds] are found, this is a no-op.
  bool _removeMessagesById(Iterable<int> messageIds) {
    final messagesToRemoveById = <int>{};
    final contentToRemove = Set<ZulipMessageContent>.identity();
    for (final messageId in messageIds) {
      final index = _findMessageWithId(messageId);
      if (index == -1) continue;
      messagesToRemoveById.add(messageId);
      contentToRemove.add(contents[index]);
    }
    if (messagesToRemoveById.isEmpty) return false;

    assert(contents.length == messages.length);
    messages.removeWhere((message) => messagesToRemoveById.contains(message.id));
    contents.removeWhere((content) => contentToRemove.contains(content));
    assert(contents.length == messages.length);
                           ^^
    _reprocessAll();

    return true;
  }

The duplicate message can be created by chance when you resolve a topic. So it's reasonable to think that someone resolving a topic by mistake can create such a message, then unresolve the topic, triggering the bug.

[gnprice]

Interesting, thanks.

Reading that code, I think specifically the reason that assertion fires is that the code deletes both copies of the message from messages (because they have the same message ID) but only the first copy from contents. The quoted code is correct in itself — it's assuming the message list should have at most one message object with a given message ID, and that's an invariant that indeed should hold.

In a release build where that assertion doesn't fire, I think that will probably leave the message list in a visibly confused state.