Skip to content

MessageListView could be disposed twice #1358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gnprice opened this issue Feb 15, 2025 · 5 comments · Fixed by #1340
Closed

MessageListView could be disposed twice #1358

gnprice opened this issue Feb 15, 2025 · 5 comments · Fixed by #1340
Labels
a-model Implementing our data model (PerAccountStore, etc.)

Comments

@gnprice
Copy link
Member

gnprice commented Feb 15, 2025

This is the issue thread for the bug fixed by:

See also chat thread:
https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2083074

I believe this was a crash bug — the stack trace in #1340 was while the Flutter framework is updating the element tree, and I think an exception there can leave things in an inconsistent state where the app no longer responds normally. Not sure of that, though.

OTOH it's probably hard to trigger: as far as I know we never heard from someone running into symptoms from it, and learned about it instead because @PIG208 was studying the code as part of work on #1183.

@gnprice gnprice added the a-model Implementing our data model (PerAccountStore, etc.) label Feb 15, 2025
@gnprice gnprice added this to the M6: Post-launch milestone Feb 15, 2025
@gnprice gnprice closed this as completed Feb 15, 2025
@gnprice
Copy link
Member Author

gnprice commented Feb 15, 2025

@PIG208 do you know what the symptom was for this bug? (Did you happen to try triggering it in the app at some point, and see the effect?)

If you don't already know, no need to spend time investigating; the bug's already fixed at this point. (And it's not among the few kinds of bug that need followup after that.)

@PIG208
Copy link
Member

PIG208 commented Feb 15, 2025

This only happens when you are on a message list page and log out. At the time I triggered on a draft branch #1183 before we have the fix in #1340, by invalidating the API key. The error dialog would show up, and the app become unusable afterwards with runtime errors about references to inactive/out-of-tree elements, but I'm not sure about the details. This was on a development build, though.

@gnprice
Copy link
Member Author

gnprice commented Feb 15, 2025

Thanks. OK, and I believe the condition "you are on a message list page and log out" is impossible in main in the actual app, right? I think the only way to get logged out is by using the UI for that on the choose-account page, and that can't be above a message-list page on the stack.

So in that case this was a latent bug: it wasn't actually possible to trigger in the app given the whole rest of the codebase.

@PIG208
Copy link
Member

PIG208 commented Feb 15, 2025

You can probably be on a message list page, follow notification for another account to see the loading screen, and navigate to the choose-account page from there. But that's of course not a common situation.

@gnprice
Copy link
Member Author

gnprice commented Feb 15, 2025

Yeah, and I don't think the notification would get you a way to navigate to the choose-account page anyway — that's a special feature of the HomePage loading page.

(With the MessageListPage that gets pushed by following a notification, if the loading takes a long time and you want to bail out you can just hit back. That's not an option on HomePage, which is why we added that wrinkle.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-model Implementing our data model (PerAccountStore, etc.)
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants