message: Handle disposal of message store correctly

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 same collection (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: #810

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

Author: PIG208

lib/model/message.dart

@@ -10,6 +10,8 @@ mixin MessageStore {
   /// All known messages, indexed by [Message.id].
   Map<int, Message> get messages;
 
+  Set<MessageListView> get debugMessageListViews;
+
   void registerMessageList(MessageListView view);
   void unregisterMessageList(MessageListView view);
 
@@ -37,6 +39,9 @@ class MessageStoreImpl with MessageStore {
 
   final Set<MessageListView> _messageListViews = {};
 
+  @override
+  Set<MessageListView> get debugMessageListViews => _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

@@ -379,6 +379,9 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
     // TODO(#650) notify [recentDmConversationsView] of the just-fetched messages
   }
 
+  @override
+  Set<MessageListView> get debugMessageListViews => _messages.debugMessageListViews;
+
   final MessageStoreImpl _messages;
 
   final Unreads unreads;

test/model/message_test.dart

@@ -77,6 +77,18 @@ void main() {
     checkNotified(count: messageList.fetched ? messages.length : 0);
   }
 
+  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());
+    check(store.debugMessageListViews).length.equals(2);
+
+    // When disposing, the [MessageListView]s are expected to unregister
+    // themselves from the message store.
+    store.dispose();
+    check(store.debugMessageListViews).isEmpty();
+  });
+
   group('reconcileMessages', () {
     test('from empty', () async {
       await prepare();

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,32 @@ 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();
+
+      // Make sure there are [MessageListView]s in the message store.
+      MessageListView.init(store: store, narrow: const MentionsNarrow());
+      MessageListView.init(store: store, narrow: const StarredMessagesNarrow());
+      check(store.debugMessageListViews).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);
+
+      // The old store's [MessageListView]s have been disposed.
+      // (And no exception was thrown; that was #810.)
+      check(store.debugMessageListViews).isEmpty();
+    }));
+
     void checkRetry(void Function() prepareError) {
       awaitFakeAsync((async) async {
         await prepareStore(lastEventId: 1);