Skip to content

store: Ensure sole ownership of MessageListView#1340

Merged
gnprice merged 4 commits intozulip:mainfrom
PIG208:pr-msglist
Feb 13, 2025
Merged

store: Ensure sole ownership of MessageListView#1340
gnprice merged 4 commits intozulip:mainfrom
PIG208:pr-msglist

Conversation

@PIG208
Copy link
Member

@PIG208 PIG208 commented Feb 8, 2025 •
edited by gnprice
Loading

Fixes #1358.

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:

  1. before the frame is rendered, removeAccount disposes the PerAccountStoreWidget, which disposes the MessageListView; _MessageListState is not yet disposed;

  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 #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:

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

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnprice gnprice gnprice left review comments

@chrisbobbe chrisbobbe chrisbobbe left review comments

Assignees

@gnprice gnprice

Labels

integration review Added by maintainers when PR may be ready for integration

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MessageListView could be disposed twice

3 participants

@PIG208 @gnprice @chrisbobbe