1572395 - fetchhttp.js deals with connection errors incorrectly

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[[e-mail]]

Relevant comment from fetchhttp.js:

// If we can't resolve the hostname in DNS etc., .statusText throws

While this was correct at the time (10 years ago), bug 608939 changed the behaviour, so this code hasn't been working for over 8 years.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Actually, status might have still thrown as late as 7 years ago; that was changed in bug 301705.

Comment 2

[Ben Bucksch (:BenB)]

Comment on attachment 9083964 [details] [diff] [review]
Proposed patch

Review of attachment 9083964 [details] [diff] [review]:
-----------------------------------------------------------------

Great idea, but please keep the try/catch, just in case (!). In the catch, do `console.error(ex)`, then continue with the `if (!errorMsg)` check as if there was no exception:

```
      try {
        errorCode = this._request.status;
        errorStr = this._request.statusText;
      } catch (ex) {
        console.error(ex);
        // continue with empty errorStr
      }
      if (!errorStr) {
        // If we can't resolve the hostname in DNS etc., .statusText is empty
```

::: mail/components/accountcreation/content/fetchhttp.js
@@ -246,5 @@
> -      try {
> -        errorCode = this._request.status;
> -        errorStr = this._request.statusText;
> -      } catch (e) {
> -        // If we can't resolve the hostname in DNS etc., .statusText throws

Please keep the comment, it's highly useful and important. Just adapt it.

Comment 3

[[e-mail]]

Attachment: Addressed review comments

Comment 4

[Ben Bucksch (:BenB)]

Comment on attachment 9084026 [details] [diff] [review]
Addressed review comments

Like I said above, keep the old comment, it's useful and important to know which case we are handling, because it's not clear from the code and these errors are happening in reality.

Please do it like this:
```
       } catch (e) {
        // In case .statusText throws (it's marked as [Throws] in the webidl),
        // continue with empty errorStr.
        console.error(e);
      }
      if (!errorStr) {
         // If we can't resolve the hostname in DNS etc., .statusText is empty.
         errorCode = -2;
```

r=BenB with this change

Comment 5

[[e-mail]]

Attachment: Addressed review comments

Ah, so I needed to keep the comment in both places, effectively. Sorry for not realising that.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1e07285df111
Update to current statusText semantics. r=BenB

Comment 7

[Ben Bucksch (:BenB)]

Comment on attachment 9087683 [details] [diff] [review]
Addressed review comments

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: If we have a wrong hostname (likely, in the setup dialog), user doesn't get a proper error message.
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9087683 [details] [diff] [review]
Addressed review comments

Sigh. I wish you'd finally acknowledge how releases work. Trunk first, then beta, then ESR. Sadly you didn't ask for beta approval, so the beta 69 ship has sailed, so technically this can only go into TB 68.2 in late October.

Comment 9

[Ben Bucksch (:BenB)]

Comment on attachment 9087683 [details] [diff] [review]
Addressed review comments

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 10

[Jorg K (CEST = GMT+2)]

Too late. It is in TB 70 and will go to TB 70 beta next week. I'll cancel all pending requests or approvals then.

Comment 11

[Jorg K (CEST = GMT+2)]

TB 68.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/437beb41ea65f7906aea0d4a10f9cc5b8f297ba3

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9087683 [details] [diff] [review]
Addressed review comments

Sorry, too late, will go into TB 70 beta.