store: Ensure sole ownership of MessageListView #1340

PIG208 · 2025-02-08T00:51:45Z

PerAccountStore shouldn't be an owner of the MessageListView elements.

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

With two owners, the MessageListView can be disposed twice:

before the frame is rendered, removeAccount disposes the PerAccountStoreWidget, which disposes the MessageListView; _MessageListState is not yet disposed;
during build, because store is set to null, PerAccountStoreWidget gets rebuilt. _MessageListState, a descendent of it, is no longer in the render tree;
during finalization, _MessageListState tries to dispose the MessageListView.

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

For reproduction, I created a testing branch with #1183 stacked on top of this, but has the fix reverted:

Checkout Testing branch for #1340 PIG208/zulip-flutter#1.
Go to any MessageListPage
Invalidate the API key (by regenerating it on web, for example)

You should see the following error:

======== Exception caught by widgets library =======================================================
The following assertion was thrown while finalizing the widget tree:
'package:zulip/model/message.dart': Failed assertion: line 54 pos 12: 'removed': is not true.

When the exception was thrown, this was the stack: 
#2      MessageStoreImpl.unregisterMessageList (package:zulip/model/message.dart:54:12)
#3      PerAccountStore.unregisterMessageList (package:zulip/model/store.dart:540:15)
#4      MessageListView.dispose (package:zulip/model/message_list.dart:427:11)
#5      _MessageListState.dispose (package:zulip/widgets/message_list.dart:491:12)
#6      StatefulElement.unmount (package:flutter/src/widgets/framework.dart:5940:11)
#7      _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:2085:13)
#8      _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:2083:7)
#9      ComponentElement.visitChildren (package:flutter/src/widgets/framework.dart:5781:14)
(elided 2 frames from class _AssertionError)
====================================================================================================

CZO discussion

chrisbobbe

Thanks! LGTM, with an indentation nit below. Another small comment: since there's no issue to refer to (with a "Fixes" line), could the commit message either link to the discussion or say something like "see discussion linked in a code comment"?

chrisbobbe · 2025-02-10T22:12:49Z

lib/model/actions.dart

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


nit: indentation

PIG208 · 2025-02-10T22:28:08Z

Updated the PR with a copy of the discussion link in the commit message, and fixed the indentation issue.

gnprice

Thanks for spotting and fixing this!

I believe the implementation logic here is all correct. But I'd like to see an assert, and one of these pieces still needs a test. See also other comments below.

gnprice · 2025-02-10T23:42:43Z

test/widgets/actions_test.dart

@@ -109,7 +111,7 @@ void main() {
      final newConnection = separateConnection()
        ..prepare(delay: unregisterDelay, json: {'msg': '', 'result': 'success'});

-      final future = logOutAccount(context, eg.selfAccount.id);
+      final future = logOutAccount(GlobalStoreWidget.of(context), eg.selfAccount.id);


nit: use the global store we already have:

Suggested change

final future = logOutAccount(GlobalStoreWidget.of(context), eg.selfAccount.id);

final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);

In particular that will facilitate migrating these tests to not use a widget tree. Since the code they're testing isn't about widgets (which is why we're moving it out of lib/widgets/), they shouldn't need to involve widgets either.

gnprice · 2025-02-10T23:44:08Z

test/widgets/actions_test.dart

@@ -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';


When moving the code, we should move its tests to match. (We're moving these methods because they don't really belong under lib/widgets/; so their tests don't belong under test/widgets/ either.)

gnprice · 2025-02-10T23:55:11Z

test/widgets/actions_test.dart

@@ -211,7 +213,7 @@ void main() {
      testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route);

      final context = tester.element(find.byType(MaterialApp));
-      final future = logOutAccount(context, account1.id);
+      final future = logOutAccount(GlobalStoreWidget.of(context), account1.id);
      await tester.pump(TestGlobalStore.removeAccountDuration);
      await future;
      check(removedRoutes).single.identicalTo(account1Route);


OTOH this test is about widgets. Which I guess underlines the fact that it's not really about this code at all, but an integration test of how other related code behaves when this happens.

Maybe the right home for this one is in widgets/store_test.dart. The implementation of the logic this is testing lives mainly on PerAccountStoreWidget, in the handling of routeToRemoveOnLogout.

gnprice · 2025-02-10T23:56:45Z

lib/model/message.dart

-      view.dispose();
-    }
-  }
+  // No `dispose` method, because there's nothing for it to do.


With two owners, the MessageListView can be disposed twice: 1. before the frame is rendered, `removeAccount` disposes the `PerAccountStoreWidget`, which disposes the `MessageListView`;

Do you mean PerAccountStore rather than PerAccountStoreWidget?

2. during build, because `store` is set to `null`, `PerAccountStoreWidget` gets rebuilt. `_MessageListState`,

This isn't quite right — if we didn't have this line (which I believe is what you're referring to as "store is set to null"):

_setStore(null);

then that element would still get rebuilt. That's because that line is within that state's didChangeDependencies, and the base method's doc says:

/// Subclasses rarely override this method because the framework always /// calls [build] after a dependency changes. Some subclasses do override

So if that method was getting called, then build was going to get called next anyway.

It's a good small exercise to work out exactly what does cause build to get called in this scenario.

gnprice · 2025-02-11T00:03:02Z

lib/model/message.dart

-      view.dispose();
-    }
-  }
+  // No `dispose` method, because there's nothing for it to do.


`PerAccountStoreWidget` gets rebuilt. `_MessageListState`, a descendent of it, is no longer in the render tree;

nit: element tree, not render tree — the render tree is a different thing:
https://main-api.flutter.dev/flutter/rendering/RenderObject-class.html

and a _MessageListState doesn't directly correspond to anything in the render tree (i.e. to any render object).

gnprice · 2025-02-11T00:08:04Z

lib/model/message.dart

-      view.dispose();
-    }
-  }
+  // No `dispose` method, because there's nothing for it to do.


nit:

PerAccountStore shouldn't be an owner of the MessageListView elements.

A MessageListView isn't an "element"; that term has a specific meaning in Flutter:
https://main-api.flutter.dev/flutter/widgets/Element-class.html
and is also used (following math terminology) for an item in a collection data structure, like a List, but it's not a general synonym for "object" or "value".

Instead of "elements", "objects" would be accurate.

gnprice · 2025-02-11T00:16:30Z

lib/model/message.dart

+  // 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
+  // void dispose() { … }


This makes me a bit nervous that it would be easy for us to spring a leak where MessageListView objects stick around. They can be memory-hungry, I think, so that's good to avoid.

That risk is highlighted by the fact that you had to add a model?.dispose(); line elsewhere in this commit — we had such a line when the state gets disposed, but had omitted to add one for when it gets a new store.

How about we instead have a dispose method here but it just asserts that its set of MessageListView objects to manage is empty? I believe that's implied by the logic we're relying on in order to justify not needing to dispose them here:

[…] But it also is tied to a given PerAccountStore, and so shouldn't outlive that store. So it should get disposed when the store goes away, too.

I think that might already be getting accomplished by the dispose calls in _ComposeAutocompleteState. If the store is getting disposed of, then it shouldn't be the case that a _ComposeAutocompleteState sticks around in the tree under that store — any such _ComposeAutocompleteState should either be told of a new store with onNewStore, or be leaving the tree entirely and so have its own dispose called. Both of those call dispose on the view-model.

Followed up on CZO and we decided to leave a TODO on this for now.

Yeah. As a nit, let's keep the method, though — it'll just have an empty body containing that TODO. That provides a bit more framing to make clear exactly what the TODO calls for, and it also means we preserve the record of what's meant to call this method.

gnprice · 2025-02-11T00:23:02Z

test/model/store_test.dart

@@ -824,25 +822,6 @@ void main() {
      checkReload(prepareHandleEventError);
    });

-    test('expired queue disposes registered MessageListView instances', () => awaitFakeAsync((async) async {


This scenario is still highly relevant — see my discussion above of the potential for a leak.

So we should still test it. The change to make is just that the test now needs appropriate widgets as part of the setup, because the MessageList widget is now part of what makes this behave correctly.

gnprice · 2025-02-11T00:24:07Z

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


If this line is removed, it looks like all tests still pass. There should be a test that would notice if we dropped this.

(I think this is exactly what the test discussed in my previous comment would do.)

PIG208 · 2025-02-12T00:26:14Z

Updated the PR to address the comments. Thanks for the review! The insights on the Flutter's widgets/elements are really helpful.

gnprice

Thanks for tracking down these subtle interactions!

This all looks good now, modulo a few small comments.

gnprice · 2025-02-12T00:35:01Z

lib/model/message.dart

+  // 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
+  // void dispose() { … }


Yeah. As a nit, let's keep the method, though — it'll just have an empty body containing that TODO. That provides a bit more framing to make clear exactly what the TODO calls for, and it also means we preserve the record of what's meant to call this method.

gnprice · 2025-02-12T00:37:53Z

lib/model/message.dart

-      view.dispose();
-    }
-  }
+  // No `dispose` method, because there's nothing for it to do.


nit:

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.

It'd be good to call out explicitly at the start of this scenario (before the numbered steps) that you're talking about a case where removeAccount is called, like if the user logs out. I read this step a couple of times and initially thought it was mistaken, because the main scenario that comes to my mind for a store getting disposed is when it's replaced by a new store.

gnprice · 2025-02-12T00:39:53Z

lib/model/message.dart

+  // 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.
+  //   Discussion:
+  //     https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/.60MentionAutocompleteView.2Edispose.60/near/2083074


nit: let's have this point here:
https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893

since that's where the discussion relevant to this particular TODO begins.

gnprice · 2025-02-12T00:46:12Z

test/widgets/message_list_test.dart

@@ -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


This isn't a regression test for #810 anymore — to do that, there'd have to be more than one MessageListView.

That's fine, though. We don't really need a regression test for that, now that the code that the bug was in (the loop through view-models in MessageStoreImpl.dispose) is gone. Instead this is just testing the thing the test's name says it is.

Suggested change

// Regression test for: https://github.com/zulip/zulip-flutter/issues/810

PIG208 · 2025-02-12T01:05:31Z

Thanks! Updated the PR.

gnprice · 2025-02-12T19:30:27Z

lib/model/message.dart

+    //   [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/.60MentionAutocompleteView.2Edispose.60/near/2083074


bump #1340 (comment) 🙂

Hmm, I read that as "have this point, here" and incorporated this new paragraph. I think you are meant to have the link to "point here", right?

I see. Yeah, I meant "have this link point to here", i.e. to the following URL.

PIG208 · 2025-02-12T19:39:41Z

Updated the references to the CZO discussion with the link from #1340 (comment).

This allows us to call it from model code when GlobalStore is available. Signed-off-by: Zixuan James Li <[email protected]>

This is more about testing the implementation of PerAccountStoreWidget to handle routeToRemoveOnLogout, instead of logOutAccount. Signed-off-by: Zixuan James Li <[email protected]>

plus moving and refactoring the tests to match Signed-off-by: Zixuan James Li <[email protected]>

`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 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]>

gnprice · 2025-02-13T04:54:14Z

Thanks! Looks good; merging.

PIG208 force-pushed the pr-msglist branch 2 times, most recently from ed21485 to af0cc46 Compare February 8, 2025 00:55

PIG208 requested a review from chrisbobbe February 8, 2025 00:58

PIG208 assigned chrisbobbe Feb 8, 2025

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Feb 8, 2025

PIG208 mentioned this pull request Feb 8, 2025

store: Handle invalid API key on register-queue #1183

Open

chrisbobbe reviewed Feb 10, 2025

View reviewed changes

chrisbobbe requested a review from gnprice February 10, 2025 22:18

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Feb 10, 2025

chrisbobbe assigned gnprice Feb 10, 2025

chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Feb 10, 2025

PIG208 force-pushed the pr-msglist branch from af0cc46 to c10aaff Compare February 10, 2025 22:26

chrisbobbe removed their assignment Feb 10, 2025

gnprice reviewed Feb 11, 2025

View reviewed changes

PIG208 force-pushed the pr-msglist branch 4 times, most recently from 0f67ea8 to 2f7a67b Compare February 12, 2025 00:24

gnprice reviewed Feb 12, 2025

View reviewed changes

PIG208 force-pushed the pr-msglist branch from 2f7a67b to 0b442f0 Compare February 12, 2025 01:05

gnprice reviewed Feb 12, 2025

View reviewed changes

PIG208 force-pushed the pr-msglist branch 2 times, most recently from b2aa4a6 to d88a995 Compare February 12, 2025 19:38

PIG208 added 3 commits February 12, 2025 20:53

login [nfc]: Pass GlobalStore to logOutAccount

f6655b8

This allows us to call it from model code when GlobalStore is available. Signed-off-by: Zixuan James Li <[email protected]>

actions test [nfc]: Move a store-related test to a better home

8b601be

This is more about testing the implementation of PerAccountStoreWidget to handle routeToRemoveOnLogout, instead of logOutAccount. Signed-off-by: Zixuan James Li <[email protected]>

actions [nfc]: Move logOutAccount and unregisterToken under lib/model

d186ed0

plus moving and refactoring the tests to match Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the pr-msglist branch from d88a995 to a055486 Compare February 13, 2025 04:54

gnprice merged commit a055486 into zulip:main Feb 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store: Ensure sole ownership of MessageListView #1340

store: Ensure sole ownership of MessageListView #1340

PIG208 commented Feb 8, 2025 •

edited

Loading

chrisbobbe left a comment

chrisbobbe Feb 10, 2025

PIG208 commented Feb 10, 2025

gnprice left a comment

gnprice Feb 10, 2025

gnprice Feb 10, 2025

gnprice Feb 10, 2025

gnprice Feb 10, 2025

gnprice Feb 10, 2025

gnprice Feb 11, 2025

gnprice Feb 11, 2025

gnprice Feb 11, 2025

gnprice Feb 11, 2025

PIG208 Feb 12, 2025

gnprice Feb 12, 2025

gnprice Feb 11, 2025

gnprice Feb 11, 2025

PIG208 commented Feb 12, 2025

gnprice left a comment

gnprice Feb 12, 2025

gnprice Feb 12, 2025

gnprice Feb 12, 2025

gnprice Feb 12, 2025

PIG208 commented Feb 12, 2025

gnprice Feb 12, 2025

PIG208 Feb 12, 2025

gnprice Feb 13, 2025

PIG208 commented Feb 12, 2025

gnprice commented Feb 13, 2025

	final future = logOutAccount(GlobalStoreWidget.of(context), eg.selfAccount.id);
	final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);

store: Ensure sole ownership of MessageListView #1340

store: Ensure sole ownership of MessageListView #1340

Conversation

PIG208 commented Feb 8, 2025 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Feb 10, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Feb 12, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Feb 12, 2025

gnprice commented Feb 13, 2025

PIG208 commented Feb 8, 2025 •

edited

Loading