Skip to content

Commit

Permalink
msglist: Ensure sole ownership of MessageListView
Browse files Browse the repository at this point in the history
PerAccountStore shouldn't be an owner of the MessageListView objects.

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

With two owners, MessageListView can be disposed twice:

1. before the frame is rendered, `GlobalStore.removeAccount`
   disposes the `PerAccountStore`, which disposes the
  `MessageListView` (via `MessageStoreImpl`), notifying listeners of
  `GlobalStore`.

  At this point `_MessageListState` is not yet disposed.

2. As a dependent of `GlobalStore`, a rebuilt is triggered for
   `PerAccountStoreWidget`. This time, the `MessageList` Element is no
   longer in the element tree

2. during build, because `store` is set to `null`,
  `PerAccountStoreWidget` gets rebuilt. `_MessageListState`,
  a descendent of it, is no longer in the render tree;

3. during finalization, `_MessageListState` tries to dispose the
   `MessageListView`.

This removes regression tests added for zulip#810, because
`MessageStoreImpl.dispose` no longer exists.  `MessageListView` does not
get disposed unless there is a `_MessageListState` owner.

See discussion:
  https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/.60MentionAutocompleteView.2Edispose.60/near/2083074

Signed-off-by: Zixuan James Li <zixuan@zulip.com>
  • Loading branch information
PIG208 committed Feb 11, 2025
1 parent 6836c84 commit 20e6b9e
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 19 deletions.
14 changes: 8 additions & 6 deletions lib/model/message.dart
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,14 @@ 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.
// The [MessageListView]s are owned by (i.e., they get [dispose]d by)
// the [_MessageListState], including in the case where the [PerAccountStore]
// is replaced. Discussion:
// https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/.60MentionAutocompleteView.2Edispose.60/near/2083074
assert(_messageListViews.isEmpty,
'Unexpected [MessageListView]s;\n'
'all should have been disposed by [_MessageListState]');
}

@override
Expand Down
1 change: 0 additions & 1 deletion lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand Down
12 changes: 0 additions & 12 deletions test/model/message_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
21 changes: 21 additions & 0 deletions test/widgets/message_list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ 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';
Expand Down Expand Up @@ -130,6 +131,26 @@ void main() {
final state = MessageListPage.ancestorOf(tester.element(find.text("a message")));
check(state.composeBoxController).isNull();
});

testWidgets('dispose MessageListView when logged out', (tester) async {
addTearDown(testBinding.reset);
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
(store.connection as FakeApiConnection).prepare(json: eg.newestGetMessagesResult(
foundOldest: true, messages: [eg.streamMessage()]).toJson());
await tester.pumpWidget(TestZulipApp(
accountId: eg.selfAccount.id,
skipAssertAccountExists: true,
child: MessageListPage(initNarrow: const CombinedFeedNarrow())));
await tester.pump();
await tester.pump();
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', () {
Expand Down

0 comments on commit 20e6b9e

Please sign in to comment.