Under some conditions, the accounts are not removed.

There is an early return in the function if the user is not signed in, or if signin is in progress.

There is at least one case where this leads to wrong code:


void OneClickSigninSyncStarter::CancelSigninAndDelete() {
  if (signin::IsDicePrepareMigrationEnabled()) {
    SigninManagerFactory::GetForProfile(profile_)->SignOutAndKeepAllAccounts(
        signin_metrics::ABORT_SIGNIN,
        signin_metrics::SignoutDelete::IGNORE_METRIC);
  } else {
    SigninManagerFactory::GetForProfile(profile_)->SignOutAndRemoveAllAccounts(
        signin_metrics::ABORT_SIGNIN,
        signin_metrics::SignoutDelete::IGNORE_METRIC);
  }

In that example, the if is actually unnecessary, because accounts are not going to be removed in either case, which is clearly not what the developper intended. Luckily I think this has no actual consequence on the product, but this looks dangerous.

SigninManager::DoSignout also has this strange comment, in the case signout is called while the user is not signed in:

// Clean up our transient data and exit if we aren't signed in.
// This avoids a perf regression from clearing out the TokenDB if
// SignOut() is invoked on startup to clean up any incomplete previous
// signin attempts.

This comment implies that tokens are not deleted in this case, to improve performance (but at the cost of privacy/security). This case is probably rare enough (at least now, maybe it was different at the time this comment was written), that this tradeoff should be re-examined.