message: Handle disposal of message store correctly

This fixes a bug caused by MessageListView instances unregistering themselves
from the collection containing these instances, while we are iterating the exact
same set when disposing the message store.

Part of the work of disposing a MessageListView is to remove it from
a collection of MessageListViews, and we've been doing this in a loop
over the exact same collection (the set MessageStoreImpl._messageListViews).
This has been causing concurrent modification errors.

The errors originated from this requirement of dart Iterable:

> Changing a collection while it is being iterated is generally not allowed

This fix avoids those errors by having the loop iterate over a clone of
that collection instead.

See also: https://api.flutter.dev/flutter/dart-core/Iterable-class.html

Fixes: zulip#810

Signed-off-by: Zixuan James Li <[email protected]>

Author: PIG208

lib/model/message.dart

@@ -1,5 +1,7 @@
 import 'dart:convert';
 
+import 'package:flutter/foundation.dart';
+
 import '../api/model/events.dart';
 import '../api/model/model.dart';
 import '../log.dart';
@@ -37,6 +39,9 @@ class MessageStoreImpl with MessageStore {
 
   final Set<MessageListView> _messageListViews = {};
 
+  @visibleForTesting
+  Set<MessageListView> get debugMessageLists => _messageListViews;
+
   @override
   void registerMessageList(MessageListView view) {
     final added = _messageListViews.add(view);
@@ -56,7 +61,10 @@ class MessageStoreImpl with MessageStore {
   }
 
   void dispose() {
-    for (final view in _messageListViews) {
+    // When a MessageListView is disposed, it removes itself from the set
+    // `MessageStoreImpl._messageListViews`, Instead of iterating on that Set,
+    // iterate on a copy, to avoid concurrent modifications.
+    for (final view in _messageListViews.toList()) {
       view.dispose();
     }
   }

lib/model/store.dart

@@ -381,6 +381,9 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
 
   final MessageStoreImpl _messages;
 
+  @visibleForTesting
+  MessageStoreImpl get debugMessageStore => _messages;
+
   final Unreads unreads;
 
   final RecentDmConversationsView recentDmConversationsView;

test/model/message_test.dart

@@ -919,4 +919,23 @@ void main() {
       });
     });
   });
+
+  test('disposing multiple registered MessageListView instances', () async {
+    // Regression test for: https://github.com/zulip/zulip-flutter/issues/810
+    await prepare(narrow: const MentionsNarrow());
+    MessageListView.init(store: store, narrow: const StarredMessagesNarrow());
+
+    Condition<Object?> conditionMessageListView(Narrow narrow) =>
+      (it) => it.isA<MessageListView>().narrow.equals(narrow);
+
+    check(store.debugMessageStore.debugMessageLists).deepEquals([
+      conditionMessageListView(const MentionsNarrow()),
+      conditionMessageListView(const StarredMessagesNarrow()),
+    ]);
+
+    // When disposing, the `MessageListView`'s are expected to unregister
+    // themselves from the message store.
+    store.dispose();
+    check(store.debugMessageStore.debugMessageLists).isEmpty();
+  });
 }

test/model/store_test.dart

@@ -8,6 +8,8 @@ import 'package:zulip/api/model/events.dart';
 import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/route/events.dart';
 import 'package:zulip/api/route/messages.dart';
+import 'package:zulip/model/message_list.dart';
+import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/store.dart';
 import 'package:zulip/notifications/receive.dart';
 
@@ -427,6 +429,36 @@ void main() {
       check(store.userSettings!.twentyFourHourTime).isTrue();
     }));
 
+    test('expired queue disposes registered MessageListView instances', () => awaitFakeAsync((async) async {
+      // Regression test for: https://github.com/zulip/zulip-flutter/issues/810
+      await prepareStore();
+      updateMachine.debugPauseLoop();
+      updateMachine.poll();
+      check(globalStore.perAccountSync(store.accountId)).identicalTo(store);
+
+      // Make sure there are message list views in the message store.
+      MessageListView.init(store: store, narrow: const MentionsNarrow());
+      MessageListView.init(store: store, narrow: const StarredMessagesNarrow());
+      check(store.debugMessageStore.debugMessageLists).length.equals(2);
+
+      // Let the server expire the event queue.
+      connection.prepare(httpStatus: 400, json: {
+        'result': 'error', 'code': 'BAD_EVENT_QUEUE_ID',
+        'queue_id': updateMachine.queueId,
+        'msg': 'Bad event queue ID: ${updateMachine.queueId}',
+      });
+      updateMachine.debugAdvanceLoop();
+      async.flushMicrotasks();
+      await Future<void>.delayed(Duration.zero);
+      check(store).isLoading.isTrue();
+      check(store.debugMessageStore.debugMessageLists).isEmpty();
+
+      // The global store has a new store.
+      check(globalStore.perAccountSync(store.accountId)).not((it) => it.identicalTo(store));
+      updateFromGlobalStore();
+      check(store).isLoading.isFalse();
+    }));
+
     void checkRetry(void Function() prepareError) {
       awaitFakeAsync((async) async {
         await prepareStore(lastEventId: 1);