store: Ensure sole ownership of MessageListView

lib/model/actions.dart

@@ -0,0 +1,36 @@
+import 'dart:async';
+
+import '../notifications/receive.dart';
+import 'store.dart';
+
+// TODO: Make this a part of GlobalStore
+Future<void> logOutAccount(GlobalStore globalStore, int accountId) async {
+  final account = globalStore.getAccount(accountId);
+  if (account == null) return; // TODO(log)
+
+  // Unawaited, to not block removing the account on this request.
+  unawaited(unregisterToken(globalStore, accountId));
+
+  await globalStore.removeAccount(accountId);
+}
+
+Future<void> unregisterToken(GlobalStore globalStore, int accountId) async {
+  final account = globalStore.getAccount(accountId);
+  if (account == null) return; // TODO(log)
+
+  // TODO(#322) use actual acked push token; until #322, this is just null.
+  final token = account.ackedPushToken
+    // Try the current token as a fallback; maybe the server has registered
+    // it and we just haven't recorded that fact in the client.
+    ?? NotificationService.instance.token.value;
+  if (token == null) return;
+
+  final connection = globalStore.apiConnectionFromAccount(account);
+  try {
+    await NotificationService.unregisterToken(connection, token: token);
+  } catch (e) {
+    // TODO retry? handle failures?
+  } finally {
+    connection.close();
+  }
+}

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/actions.dart

@@ -15,44 +15,9 @@ import '../api/model/narrow.dart';
 import '../api/route/messages.dart';
 import '../generated/l10n/zulip_localizations.dart';
 import '../model/narrow.dart';
-import '../model/store.dart';
-import '../notifications/receive.dart';
 import 'dialog.dart';
 import 'store.dart';
 
-Future<void> logOutAccount(BuildContext context, int accountId) async {
-  final globalStore = GlobalStoreWidget.of(context);
-
-  final account = globalStore.getAccount(accountId);
-  if (account == null) return; // TODO(log)
-
-  // Unawaited, to not block removing the account on this request.
-  unawaited(unregisterToken(globalStore, accountId));
-
-  await globalStore.removeAccount(accountId);
-}
-
-Future<void> unregisterToken(GlobalStore globalStore, int accountId) async {
-  final account = globalStore.getAccount(accountId);
-  if (account == null) return; // TODO(log)
-
-  // TODO(#322) use actual acked push token; until #322, this is just null.
-  final token = account.ackedPushToken
-    // Try the current token as a fallback; maybe the server has registered
-    // it and we just haven't recorded that fact in the client.
-    ?? NotificationService.instance.token.value;
-  if (token == null) return;
-
-  final connection = globalStore.apiConnectionFromAccount(account);
-  try {
-    await NotificationService.unregisterToken(connection, token: token);
-  } catch (e) {
-    // TODO retry? handle failures?
-  } finally {
-    connection.close();
-  }
-}
-
 Future<void> markNarrowAsRead(BuildContext context, Narrow narrow) async {
   final store = PerAccountStoreWidget.of(context);
   final connection = store.connection;

lib/widgets/app.dart

@@ -6,11 +6,11 @@ import 'package:flutter/scheduler.dart';
 
 import '../generated/l10n/zulip_localizations.dart';
 import '../log.dart';
+import '../model/actions.dart';
 import '../model/localizations.dart';
 import '../model/store.dart';
 import '../notifications/display.dart';
 import 'about_zulip.dart';
-import 'actions.dart';
 import 'dialog.dart';
 import 'home.dart';
 import 'login.dart';
@@ -265,7 +265,7 @@ class ChooseAccountPage extends StatelessWidget {
                   actionButtonText: zulipLocalizations.logOutConfirmationDialogConfirmButton,
                   onActionButtonPress: () {
                     // TODO error handling if db write fails?
-                    logOutAccount(context, accountId);
+                    logOutAccount(GlobalStoreWidget.of(context), accountId);
                   });
               },
               child: Text(zulipLocalizations.chooseAccountPageLogOutButton)),

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/actions_test.dart

@@ -0,0 +1,188 @@
+import 'package:checks/checks.dart';
+import 'package:flutter/foundation.dart';
+import 'package:flutter_test/flutter_test.dart';
+import 'package:http/http.dart' as http;
+import 'package:zulip/api/exception.dart';
+import 'package:zulip/model/actions.dart';
+import 'package:zulip/model/store.dart';
+import 'package:zulip/notifications/receive.dart';
+
+import '../api/fake_api.dart';
+import '../example_data.dart' as eg;
+import '../fake_async.dart';
+import '../model/binding.dart';
+import '../model/store_checks.dart';
+import '../model/test_store.dart';
+import '../stdlib_checks.dart';
+import 'store_test.dart';
+
+void main() {
+  TestZulipBinding.ensureInitialized();
+
+  late PerAccountStore store;
+  late FakeApiConnection connection;
+
+  Future<void> prepare({String? ackedPushToken = '123'}) async {
+    addTearDown(testBinding.reset);
+    final selfAccount = eg.selfAccount.copyWith(ackedPushToken: Value(ackedPushToken));
+    await testBinding.globalStore.add(selfAccount, eg.initialSnapshot());
+    store = await testBinding.globalStore.perAccount(selfAccount.id);
+    connection = store.connection as FakeApiConnection;
+  }
+
+  /// Creates and caches a new [FakeApiConnection] in [TestGlobalStore].
+  ///
+  /// In live code, [unregisterToken] makes a new [ApiConnection] for the
+  /// unregister-token request instead of reusing the store's connection.
+  /// To enable callers to prepare responses for that request, this function
+  /// creates a new [FakeApiConnection] and caches it in [TestGlobalStore]
+  /// for [unregisterToken] to pick up.
+  ///
+  /// Call this instead of just turning on
+  /// [TestGlobalStore.useCachedApiConnections] so that [unregisterToken]
+  /// doesn't try to call `close` twice on the same connection instance,
+  /// which isn't allowed. (Once by the unregister-token code
+  /// and once as part of removing the account.)
+  FakeApiConnection separateConnection() {
+    testBinding.globalStore
+      ..clearCachedApiConnections()
+      ..useCachedApiConnections = true;
+    return testBinding.globalStore
+      .apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection;
+  }
+
+  String unregisterApiPathForPlatform(TargetPlatform platform) {
+    return switch (platform) {
+      TargetPlatform.android => '/api/v1/users/me/android_gcm_reg_id',
+      TargetPlatform.iOS     => '/api/v1/users/me/apns_device_token',
+      _                      => throw Error(),
+    };
+  }
+
+  void checkSingleUnregisterRequest(
+    FakeApiConnection connection, {
+    String? expectedToken,
+  }) {
+    final subject = check(connection.takeRequests()).single.isA<http.Request>()
+      ..method.equals('DELETE')
+      ..url.path.equals(unregisterApiPathForPlatform(defaultTargetPlatform));
+    if (expectedToken != null) {
+      subject.bodyFields.deepEquals({'token': expectedToken});
+    }
+  }
+
+  group('logOutAccount', () {
+    test('smoke', () => awaitFakeAsync((async) async {
+      await prepare();
+      check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id);
+      const unregisterDelay = Duration(seconds: 5);
+      assert(unregisterDelay > TestGlobalStore.removeAccountDuration);
+      final newConnection = separateConnection()
+        ..prepare(delay: unregisterDelay, json: {'msg': '', 'result': 'success'});
+
+      final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);
+      // Unregister-token request and account removal dispatched together
+      checkSingleUnregisterRequest(newConnection);
+      check(testBinding.globalStore.takeDoRemoveAccountCalls())
+        .single.equals(eg.selfAccount.id);
+
+      async.elapse(TestGlobalStore.removeAccountDuration);
+      await future;
+      // Account removal not blocked on unregister-token response
+      check(testBinding.globalStore).accountIds.isEmpty();
+      check(connection.isOpen).isFalse();
+      check(newConnection.isOpen).isTrue(); // still busy with unregister-token
+
+      async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration);
+      check(newConnection.isOpen).isFalse();
+    }));
+
+    test('unregister request has an error', () => awaitFakeAsync((async) async {
+      await prepare();
+      check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id);
+      const unregisterDelay = Duration(seconds: 5);
+      assert(unregisterDelay > TestGlobalStore.removeAccountDuration);
+      final exception = ZulipApiException(
+        httpStatus: 401,
+        code: 'UNAUTHORIZED',
+        data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"},
+        routeName: 'removeEtcEtcToken',
+        message: 'Invalid API key',
+      );
+      final newConnection = separateConnection()
+        ..prepare(delay: unregisterDelay, exception: exception);
+
+      final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);
+      // Unregister-token request and account removal dispatched together
+      checkSingleUnregisterRequest(newConnection);
+      check(testBinding.globalStore.takeDoRemoveAccountCalls())
+        .single.equals(eg.selfAccount.id);
+
+      async.elapse(TestGlobalStore.removeAccountDuration);
+      await future;
+      // Account removal not blocked on unregister-token response
+      check(testBinding.globalStore).accountIds.isEmpty();
+      check(connection.isOpen).isFalse();
+      check(newConnection.isOpen).isTrue(); // for the unregister-token request
+
+      async.elapse(unregisterDelay - TestGlobalStore.removeAccountDuration);
+      check(newConnection.isOpen).isFalse();
+    }));
+  });
+
+  group('unregisterToken', () {
+    testAndroidIos('smoke, happy path', () => awaitFakeAsync((async) async {
+      await prepare(ackedPushToken: '123');
+
+      final newConnection = separateConnection()
+        ..prepare(json: {'msg': '', 'result': 'success'});
+      final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id);
+      async.elapse(Duration.zero);
+      await future;
+      checkSingleUnregisterRequest(newConnection, expectedToken: '123');
+      check(newConnection.isOpen).isFalse();
+    }));
+
+    test('fallback to current token if acked is missing', () => awaitFakeAsync((async) async {
+      await prepare(ackedPushToken: null);
+      NotificationService.instance.token = ValueNotifier('asdf');
+
+      final newConnection = separateConnection()
+        ..prepare(json: {'msg': '', 'result': 'success'});
+      final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id);
+      async.elapse(Duration.zero);
+      await future;
+      checkSingleUnregisterRequest(newConnection, expectedToken: 'asdf');
+      check(newConnection.isOpen).isFalse();
+    }));
+
+    test('no error if acked token and current token both missing', () => awaitFakeAsync((async) async {
+      await prepare(ackedPushToken: null);
+      NotificationService.instance.token = ValueNotifier(null);
+
+      final newConnection = separateConnection();
+      final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id);
+      async.flushTimers();
+      await future;
+      check(newConnection.takeRequests()).isEmpty();
+    }));
+
+    test('connection closed if request errors', () => awaitFakeAsync((async) async {
+      await prepare(ackedPushToken: '123');
+
+      final newConnection = separateConnection()
+        ..prepare(exception: ZulipApiException(
+            httpStatus: 401,
+            code: 'UNAUTHORIZED',
+            data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"},
+            routeName: 'removeEtcEtcToken',
+            message: 'Invalid API key',
+          ));
+      final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id);
+      async.elapse(Duration.zero);
+      await future;
+      checkSingleUnregisterRequest(newConnection, expectedToken: '123');
+      check(newConnection.isOpen).isFalse();
+    }));
+  });
+}

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() {