From 9a7922210154ff658eb8924e589030f769028c53 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 11 Feb 2025 17:03:26 -0500 Subject: [PATCH] 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: 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. A regression test for #810 has been appropriated. The original issue is relevant because the scenario this integration test targeted still applies after this change. See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2083074 Signed-off-by: Zixuan James Li --- lib/model/message.dart | 19 +++++++----- lib/model/store.dart | 1 - lib/widgets/message_list.dart | 1 + test/model/message_test.dart | 12 -------- test/model/store_test.dart | 19 ------------ test/widgets/message_list_test.dart | 46 +++++++++++++++++++++++++++++ 6 files changed, 58 insertions(+), 40 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index f228d6da91..90e6fba9a8 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -60,14 +60,17 @@ 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(); - } - } + // No `dispose` method, because there's nothing for it to do. + // 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 reinstate this `dispose` method. + // Discussion: + // https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/.60MentionAutocompleteView.2Edispose.60/near/2083074 + // void dispose() { … } @override void reconcileMessages(List messages) { diff --git a/lib/model/store.dart b/lib/model/store.dart index 7603c7f452..322367c432 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -557,7 +557,6 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess assert(!_disposed); recentDmConversationsView.dispose(); unreads.dispose(); - _messages.dispose(); typingStatus.dispose(); typingNotifier.dispose(); updateMachine?.dispose(); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 25993efa30..134feda6be 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -483,6 +483,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat @override void onNewStore() { // TODO(#464) try to keep using old model until new one gets messages + model?.dispose(); _initModel(PerAccountStoreWidget.of(context)); } diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 43f17be61a..9c9a940d42 100644 --- a/test/model/message_test.dart +++ b/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(); diff --git a/test/model/store_test.dart b/test/model/store_test.dart index c8c6b4c266..df4c4b79e7 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -824,25 +824,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() { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index b3cc208463..211669cb1d 100644 --- a/test/widgets/message_list_test.dart +++ b/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? subscriptions, UnreadMessagesSnapshot? unreadMsgs, List 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,48 @@ 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 { + // Regression test for: https://github.com/zulip/zulip-flutter/issues/810 + 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', () {