1971303 - AccountHub: configVerifier is null and breaks error handling

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect, P2)

Description

[Ben Bucksch (:BenB)]

If there's an error during account creation, the error handling code fails with cleanup() not existing due to configVerifier being null, and fails to show the correct error message (e.g. "account already exists") and instead shows a non-sensical null reference error. This is a logic bug, because

1. the configVerifier object is created only after certain checks (e.g. in validateAndFinish() in line 1086 in email.mjs)
2. the call to validateAndFinish() is wrapped in a try block that calls configVerifier.cleanup() in the finally block.

Consequently, if the checks at the beginning of validateAndFinish() fail and throw, the finally block will run, configVerifier.cleanup() will be called, but configVerifier hasn't been created yet.

Additionally, the actual error did not appear in the Thunderbird error console, as expected, making the diagnosis of this bug and any other bug really hard. Many bugs in account creation appear only at end users and are hard or even strictly impossible to reproduce, so the error logging is important. The console.error() was simply missing.

The patch fixes this

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Bug 1971303 - AccountHub: configVerifier is null and breaks error handling. r=vineet

If there's an error during account creation, the error handling code fails with cleanup() not existing due to configVerifier being null, and fails to show the correct error message (e.g. "account already exists") and instead shows a non-sensical null reference error. This is a logic bug, because

1. the configVerifier object is created only after certain checks (e.g. in validateAndFinish() in line 1086 in email.mjs)
2. the call to validateAndFinish() is wrapped in a try block that calls configVerifier.cleanup() in the finally block.

Consequently, if the checks at the beginning of validateAndFinish() fail and throw, the finally block will run, configVerifier.cleanup() will be called, but configVerifier hasn't been created yet.

Additionally, the actual error did not appear in the Thunderbird error console, as expected, making the diagnosis of this bug and any other bug really hard. Many bugs in account creation appear only at end users and are hard or even strictly impossible to reproduce, so the error logging is important. The console.error() was simply missing.

This patch fixes it.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: Bug 1971303 - Fix some docs and inconsistencies for showNotification(). r=vineet

The actual error was not provided for account-hub-addon-error.
Not finding an config is not an error.
Fixed docs to state what arguments are optional.
In one instance error?.message was checked but then error.message accessed. Could blow up if so.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6e7518fee00b
AccountHub: configVerifier is null and breaks error handling. r=vineet

Comment 4

[Ben Bucksch (:BenB)]

Requesting backport to TB 140 before releasing ESR 140.0.

This fixes completely broken error handling in the Account Hub.

Alternatively, the account hub should be disabled. (There are a number of other serious bugs in the Account Hub, which should not ship.)

Comment 5

[Wayne Mery (:wsmwk)]

Thanks for raising the flag. If this is considered to be a release blocker it is unfortunate to not have flagged earlier. But if an error is shown to the user in all cases, just not the correct error, is it really a release blocker?

Additional questions/concerns: a) are the strings localized? b) this hasn't gone through beta, c) is "untested in the field" error code covered by automated tests, and the tests have been working despite this incorrect code? d) are there manual testing steps that have been exercised by QA?

If it's not a true release blocker, and the items above are concerns, we could uplift this in a point release about a week after 140 ships (which we would probably do anyway), at which point it will have been through beta 141, but only for a few days. (I doubt beta will ship on Tuesday - might ship as late as Wednesday or Thursday, given everything on Daniel's plate and until it gets through QA)

To give you some scope, after a week I would expect several hundred thousand users on release 140 and esr 140. During that week, if this only affects potentially second and subsequent new accounts, would we expect user impact to be minimal?

Timing wise, we need to decide by Monday roughly noon Eastern time.

Comment 6

[Wayne Mery (:wsmwk)]

Chatted with Vineet. We won't be blocking releases on this issue at this time. Paraphrasing ...

"This issue is on errors with creating an account, specifically if an account already exists or this issue related to bug 1973399. Only for exchange. And if necessary can be bypassed by the user by disabling account hub."

Comment 7

[Ben Bucksch (:BenB)]

But if an error is shown to the user in all cases, just not the correct error

The error shown is a code bug, showing a null reference, instead of the real error. No user will understand the error cause. Not even I understood why the error appeared, until I fixed this bug.

Additionally, even other errors in the entire dialog (unrelated to the above) were not printed on the error console, which makes diagnosing and fixing bugs that appear in the wild much harder. Given that a lot of account creation is dependent on the actual account, my experience is that such error logs from actual end users are crucial in finding and fixing bugs in the account creation.

This fix is very low risk.

Only for exchange

From what I can see in the code, the bug appears for all account types, not only Exchange accounts.

a) are the strings localized?

There are no localized strings. There are only hardcoded strings for the error console.

we could uplift this in a point release about a week after 140 ships (which we would probably do anyway)

That's up to you to decide.

Comment 8

[Wayne Mery (:wsmwk)]

Ben, thanks for the clarifications.

(In reply to Wayne Mery (:wsmwk) from comment #6)

Chatted with Vineet. We won't be blocking releases on this issue at this time. Paraphrasing ...

"This issue is on errors with creating an account, specifically if an account already exists or this issue related to bug 1973399. Only for exchange. And if necessary can be bypassed by the user by disabling account hub."

My paraphrase may have been slightly off. Here is vineet's full posting from yesterday...

Unfortunately in a recent update in the account hub, this bug slipped through, but I don't think this is a release blocker. This issue is on errors with creating an account, specifically if an account already exists or this issue related to this bug reported by the same user https://bugzilla.mozilla.org/show_bug.cgi?id=1973399 (Which is how this user encountered the bug in the first place, exchange specific)

We're only enabling account hub for additional accounts, and we have an experimental pref in the settings pane that allows users to use the old account set up in case of issues.

As for comment "There are a number of other serious bugs in the Account Hub, which should not ship." we've had account hub for in 140 beta and we've only had 3 bug reports, and we've had QA run through a lot of the scenarios, I guess the exchange add-on scenarios were not tested properly. Other than the two bugs reported by this user, there is just this one which i'm investigating https://bugzilla.mozilla.org/show_bug.cgi?id=1967923

We have automated tests, and some tests for errors, but there wasn't a test for adding an account that already exists, which would have caught this bug. We didn't add tests for add-on specific exchange accounts, as there was some discussion that we'll be removing the OWL add-on in favor of our EWS implementation when it's ready. There is definitely a lot of room for more error testing, which is on the list after address book work is done (which is actually fully tested).

Comment 9

[Ben Bucksch (:BenB)]

FYI, vineet, I'm not a "user", but a core TB developer and happen to be the original author of most of this dialog. ;-)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a360edd30358
Fix some docs and inconsistencies for showNotification(). r=vineet,BenB

Comment 11

[Wayne Mery (:wsmwk)]

Uplift next week?

Comment 13

[Corey Bryant]

Ben/Vineet, what do you think about uplifting this? If regression potential is low, it would be nice to fix this in 140.0 and 140esr. If so, can you add an uplift request?

Comment 14

[Ben Bucksch (:BenB)]

Comment on attachment 9493810 [details]
Bug 1971303 - AccountHub: configVerifier is null and breaks error handling. r=vineet

Comment 15

[Ben Bucksch (:BenB)]

Comment on attachment 9493810 [details]
Bug 1971303 - AccountHub: configVerifier is null and breaks error handling. r=vineet

Uplift Approval Request

• Please state case for uplift consideration and ensure bug severity is set: Fixes broken error handling during account setup.
• User impact if declined: Non-sensical message that looks like the app is broken. No indication at what the actual problem is. User likely won't know. I only knew the cause when I traced the code back. People do run into this, see e.g. bug 1975814.
• Is this code covered by automated tests?: No
• Has the fix been verified in Daily?: Yes
• Has the fix been verified in Beta?: Yes
• Needs manual test from QA?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Makes code more error-proof
• Does the fix cause any migrations to be skipped?: No
• String changes made/needed: none (only error logs in English)

Comment 16

[Corey Bryant]

Comment on attachment 9493810 [details]
Bug 1971303 - AccountHub: configVerifier is null and breaks error handling. r=vineet

[Triage Comment]
This will be included in Monday's merge of central->beta

Comment 17

[Corey Bryant]

Comment on attachment 9493810 [details]
Bug 1971303 - AccountHub: configVerifier is null and breaks error handling. r=vineet

[Triage Comment]
Rejected for esr140. Apologies.. I know I had suggested the uplift. We've since made a decision to be strict with uplifts to stable releases, and in this case in particular, users can disable the experimental account hub.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9493810 [details]
Bug 1971303 - AccountHub: configVerifier is null and breaks error handling. r=vineet

Uplift Approval Request

• Please state case for uplift consideration and ensure bug severity is set: For this case please re-consider and approve for esr140.
  "Users can disable experimental account setup" isn't a valid workaround since few users would reasonably figure that out.
  ESR will live for a year. This bug could impact huge amount of users.
• User impact if declined: Users would think Thunderbird is broken.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Daily?: Yes
• Has the fix been verified in Beta?: Yes
• Needs manual test from QA?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's very limited scope.
• Does the fix cause any migrations to be skipped?: Yes
• String changes made/needed:

Comment 19

[Corey Bryant]

Comment on attachment 9493810 [details]
Bug 1971303 - AccountHub: configVerifier is null and breaks error handling. r=vineet

[Triage Comment]
Approved for esr140

Comment 20

[Daniel Darnell [:dandarnell]]

Thunderbird 140.2.0esr:
https://hg.mozilla.org/releases/comm-esr140/rev/f3e7ed2edf82f8652c4749b1c45df51dd7e8a49f