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.

The error originated from this requirement of dart Iterable:

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

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);

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);