msglist: Ensure sole ownership of MessageListView

`PerAccountStore` shouldn't be an owner of the `MessageListView`
objects.

Its relationship to `MessageListView` is similar to that of
`AutocompleteViewManager` to `MentionAutocompleteView` (#645).

With two owners, `MessageListView` can be disposed twice when
`removeAccount` is called (when the user logs out, for example):

1. Before the frame is rendered, after removing the `PerAccountStore`
   from `GlobalStore`, `removeAccount` disposes the `PerAccountStore` ,
   which disposes the `MessageListView` (via `MessageStoreImpl`).

   `removeAccount` also notifies the listeners of `GlobalStore`.

   At this point `_MessageListState` is not yet disposed.

2. Being dependent on `GlobalStore`, `PerAccountStoreWidget` is rebuilt.
   This time, the StatefulElement corresponding to the `MessageList`
   widget, is no longer in the element tree because `PerAccountStoreWidget`
   cannot find the account and builds a placeholder instead.

3. During finalization, `_MessageListState` tries to dispose the
   `MessageListView`, and fails to do so.

We couldn't've kept `MessageStoreImpl.dispose` with an assertion
`_messageListView.isEmpty`, because `PerAccountStore` is disposed before
`_MessageListState.dispose` (and similarily
`_MessageListState.onNewStore`) is called.  Fixing that will be a future
follow-up to this, as noted in the TODO comment.

See discussion:
  https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893

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

Author: PIG208

lib/model/message.dart

@@ -61,12 +61,20 @@ class MessageStoreImpl with MessageStore {
   }
 
   void dispose() {
-    // 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();
-    }
+    // Not disposing the [MessageListView]s here, because they are owned by
+    // (i.e., they get [dispose]d by) the [_MessageListState], including in the
+    // case where the [PerAccountStore] is replaced.
+    //
+    // TODO: Add assertions that the [MessageListView]s are indeed disposed, by
+    //   first ensuring that [PerAccountStore] is only disposed after those with
+    //   references to it are disposed, then reinstating this `dispose` method.
+    //
+    //   We can't add the assertions as-is because the sequence of events
+    //   guarantees that `PerAccountStore` is disposed (when that happens,
+    //   [GlobalStore] notifies its listeners, causing widgets dependent on the
+    //   [InheritedNotifier] to rebuild in the next frame) before the owner's
+    //   `dispose` or `onNewStore` is called.  Discussion:
+    //     https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893
   }
 
   @override

lib/widgets/message_list.dart

@@ -483,6 +483,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
 
   @override
   void onNewStore() { // TODO(#464) try to keep using old model until new one gets messages
+    model?.dispose();
     _initModel(PerAccountStoreWidget.of(context));
   }
 

test/model/message_test.dart

@@ -77,18 +77,6 @@ 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

@@ -12,8 +12,6 @@ import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/route/events.dart';
 import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/api/route/realm.dart';
-import 'package:zulip/model/message_list.dart';
-import 'package:zulip/model/narrow.dart';
 import 'package:zulip/log.dart';
 import 'package:zulip/model/store.dart';
 import 'package:zulip/notifications/receive.dart';
@@ -824,25 +822,6 @@ void main() {
       checkReload(prepareHandleEventError);
     });
 
-    test('expired queue disposes registered MessageListView instances', () => awaitFakeAsync((async) async {
-      // Regression test for: https://github.com/zulip/zulip-flutter/issues/810
-      await preparePoll();
-
-      // 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.
-      prepareExpiredEventQueue();
-      updateMachine.debugAdvanceLoop();
-      async.elapse(Duration.zero);
-
-      // The old store's [MessageListView]s have been disposed.
-      // (And no exception was thrown; that was #810.)
-      check(store.debugMessageListViews).isEmpty();
-    }));
-
     group('report error', () {
       String? lastReportedError;
       String? takeLastReportedError() {

test/widgets/message_list_test.dart

@@ -7,11 +7,13 @@ import 'package:flutter/rendering.dart';
 import 'package:flutter_checks/flutter_checks.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:http/http.dart' as http;
+import 'package:zulip/api/exception.dart';
 import 'package:zulip/api/model/events.dart';
 import 'package:zulip/api/model/initial_snapshot.dart';
 import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/model/narrow.dart';
 import 'package:zulip/api/route/messages.dart';
+import 'package:zulip/model/actions.dart';
 import 'package:zulip/model/localizations.dart';
 import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/store.dart';
@@ -56,6 +58,7 @@ void main() {
     List<Subscription>? subscriptions,
     UnreadMessagesSnapshot? unreadMsgs,
     List<NavigatorObserver> navObservers = const [],
+    bool skipAssertAccountExists = false,
   }) async {
     TypingNotifier.debugEnable = false;
     addTearDown(TypingNotifier.debugReset);
@@ -77,6 +80,7 @@ void main() {
       eg.newestGetMessagesResult(foundOldest: foundOldest, messages: messages).toJson());
 
     await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
+      skipAssertAccountExists: skipAssertAccountExists,
       navigatorObservers: navObservers,
       child: MessageListPage(initNarrow: narrow)));
 
@@ -130,6 +134,47 @@ void main() {
       final state = MessageListPage.ancestorOf(tester.element(find.text("a message")));
       check(state.composeBoxController).isNull();
     });
+
+    testWidgets('dispose MessageListView when event queue expired', (tester) async {
+      final message = eg.streamMessage();
+      await setupMessageListPage(tester, messages: [message]);
+      final oldViewModel = store.debugMessageListViews.single;
+      final updateMachine = store.updateMachine!;
+      updateMachine.debugPauseLoop();
+      updateMachine.poll();
+
+      updateMachine.debugPrepareLoopError(ZulipApiException(
+        routeName: 'events', httpStatus: 400, code: 'BAD_EVENT_QUEUE_ID',
+        data: {'queue_id': updateMachine.queueId}, message: 'Bad event queue ID.'));
+      updateMachine.debugAdvanceLoop();
+      await tester.pump();
+      // Event queue has been replaced; but the [MessageList] hasn't been
+      // rebuilt yet.
+      final newStore = testBinding.globalStore.perAccountSync(eg.selfAccount.id)!;
+      check(connection.isOpen).isFalse(); // indicates that the old store has been disposed
+      check(store.debugMessageListViews).single.equals(oldViewModel);
+      check(newStore.debugMessageListViews).isEmpty();
+
+      (newStore.connection as FakeApiConnection).prepare(json: eg.newestGetMessagesResult(
+        foundOldest: true, messages: [message]).toJson());
+      await tester.pump();
+      await tester.pump(Duration.zero);
+      // As [MessageList] rebuilds, the old view model gets disposed and
+      // replaced with a fresh one.
+      check(store.debugMessageListViews).isEmpty();
+      check(newStore.debugMessageListViews).single.not((it) => it.equals(oldViewModel));
+    });
+
+    testWidgets('dispose MessageListView when logged out', (tester) async {
+      await setupMessageListPage(tester,
+        messages: [eg.streamMessage()], skipAssertAccountExists: true);
+      check(store.debugMessageListViews).single;
+
+      final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);
+      await tester.pump(TestGlobalStore.removeAccountDuration);
+      await future;
+      check(store.debugMessageListViews).isEmpty();
+    });
   });
 
   group('app bar', () {