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 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: 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';
@@ -10,6 +12,9 @@ mixin MessageStore {
   /// All known messages, indexed by [Message.id].
   Map<int, Message> get messages;
 
+  @visibleForTesting
+  Set<MessageListView> get debugMessageListsViews;
+
   void registerMessageList(MessageListView view);
   void unregisterMessageList(MessageListView view);
 
@@ -37,6 +42,9 @@ class MessageStoreImpl with MessageStore {
 
   final Set<MessageListView> _messageListViews = {};
 
+  @override
+  Set<MessageListView> get debugMessageListsViews => _messageListViews;
+
   @override
   void registerMessageList(MessageListView view) {
     final added = _messageListViews.add(view);
@@ -56,7 +64,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 debugMessageListsViews => _messages.debugMessageListsViews;
+
   final MessageStoreImpl _messages;
 
   final Unreads unreads;

test/model/message_test.dart

@@ -77,6 +77,19 @@ 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.debugMessageListsViews).length.equals(2);
+
+    // When disposing, the `MessageListView`'s are expected to unregister
+    // themselves from the message store.
+    store.dispose();
+    check(store.debugMessageListsViews).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,33 @@ 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.debugMessageListsViews).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.debugMessageListsViews).isEmpty();
+    }));
+
     void checkRetry(void Function() prepareError) {
       awaitFakeAsync((async) async {
         await prepareStore(lastEventId: 1);