Runtime error if user logs out when reloading PerAccountStore

[PIG208]

loadPerAccount has a check for whether the account exists or not at the end:

  Future<PerAccountStore> loadPerAccount(int accountId) async {
    assert(_accounts.containsKey(accountId));
    final store = await doLoadPerAccount(accountId);
    if (!_accounts.containsKey(accountId)) {
      // [removeAccount] was called during [doLoadPerAccount].
      store.dispose();
      throw AccountNotFoundException();
    }
    return store;
  }

Hypothetically, this check can only be reached if the account is logged out while loading the PerAccountStore. However, because of a runtime error that would happen right after the inner request returns, we cannot reach the if-block here.

The error comes from a null-check at the start of PerAccountStore.fromInitialSnapshot:

    final account = globalStore.getAccount(accountId)!;

[ERROR:flutter/runtime/dart_vm_initializer.cc(40)] Unhandled Exception: Null check operator used on a null value
#0      new PerAccountStore.fromInitialSnapshot (package:zulip/model/store.dart:285:54)
#1      UpdateMachine.load (package:zulip/model/store.dart:913:35)
<asynchronous suspension>
#2      LiveGlobalStore.doLoadPerAccount (package:zulip/model/store.dart:836:27)
<asynchronous suspension>
#3      GlobalStore.loadPerAccount (package:zulip/model/store.dart:155:15)
<asynchronous suspension>
#4      GlobalStore._reloadPerAccount (package:zulip/model/store.dart:135:19)

PerAccountStore.fromInitialSnapshot is called near the end of LiveGlobalStore.doLoadPerAccount with no asynchronous gap, meaning that the null-check will always fail ahead of AccountNotFoundException being raised if the user logs out during the asynchronous gap from the /api/register-queue request or globalStore.updateAccount:

This is a bug that the if (!_accounts.containsKey(accountId)) check was meant to catch. The error should be thrown earlier, ideally every time before the account is accessed, similar to the if (mounted) checks and BuildContext.

[gnprice]

Hypothetically, this check can only be reached if the account is logged out while loading the PerAccountStore. However, because of a runtime error that would happen right after the inner request returns, we cannot reach the if-block here.

Hmm, I don't understand. If we can't ever reach that case, that should mean that that conditional has no effect, right?

But if I comment it out, there's a test that fails:

00:32 +1495 ~8 -1: /home/greg/z/flutterz/test/model/store_test.dart: GlobalStore.removeAccount when store loading [E]
  Expected: a Future<PerAccountStore> that:
    completes to an error of type AccountNotFoundException
  Actual: completed to <PerAccountStore#f4cfe>
  Which: did not throw
  package:checks/src/checks.dart 85:9    check.<fn>
  package:checks/src/checks.dart 782:14  _TestContext.nestAsync
  

To run this test again: /home/greg/n/flutter/flutter/bin/cache/dart-sdk/bin/dart test /home/greg/z/flutterz/test/model/store_test.dart -p vm --plain-name 'GlobalStore.removeAccount when store loading'

[PIG208]

Yeah. I meant that we cannot reach it live with how LiveGlobalStore is currently set up. Because it is related to its implementation of doLoadPerAccount.

[gnprice]

OK. Why is this a bug, then?

I guess the point is that there's a bug over at that line in PerAccountStore.fromInitialSnapshot, that it (or its caller) should be handling the case where the account no longer exists. Still, it seems like the logic in loadPerAccount is correct — if we deleted that conditional, that would create a bug, even if it might currently be a latent bug.

[PIG208]

Exactly. That part of the logic is correct. I think the bug is that we should always check for account existence prior to using a null-check on the result of getAccount if there is an asynchronous gap, to avoid runtime errors mentioned in this issue.

[gnprice]

Yeah, I agree with that. I think UpdateMachine.load should make the checks, because that's where the awaits are, and raise AccountNotFoundException.

The analogy with if (!context.mounted) checks is a good one, and points to making the checks shortly after each await.

[gnprice]

One thing that isn't yet clear to me in this thread is what the user-facing impact is — is there a way a user can trigger this bug in the app?

I think there's an answer to that in the question I asked at #1183 (comment) that caused you to investigate this code path:

suppose the app loses its event queue and goes to reload initial data, and while it's doing that the user navigates to the choose-account page and logs out the account.

I went on to suggest that that would lead loadPerAccount to throw an AccountNotFoundException (which would then trigger a bug where _handlePollError doesn't, in main, expect an exception there). I think the bug described above means that instead of that AccountNotFoundException, there'd be a null-check exception.

With either version of that bug, I believe there's no user-facing impact: just an unhandled exception in an unawaited Future. So it's something we would see when collecting logs of those, but would have no effect on the behavior the user sees directly.

The other situation is if the user logs out while the data for that account is first loading, instead of reloading. That has a different call stack. Is the outcome the same in that situation, or does it have a direct user-facing effect?

[PIG208]

It would be this code path:

      () async {
        try {
          // If this succeeds, globalStore will notify listeners, and
          // [didChangeDependencies] will run again, this time in the
          // `store != null` case above.
          await globalStore.perAccount(widget.accountId);
        } on AccountNotFoundException {
          // The account was logged out while its store was loading.
          // This widget will be showing [placeholder] perpetually,
          // but that's OK as long as other code will be removing it from the UI
          // (usually by using [routeToRemoveOnLogout]).
        }
      }();

There will also be an unhandled exception in an unawaited future, so it appears to be the same situation.