Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Fix error handling in OnProxyAvailable() for SMTP, POP, NNTP and IMAP.

RESOLVED FIXED in Thunderbird 55.0

Status

MailNews Core
Networking
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 55.0

Thunderbird Tracking Flags

(thunderbird53 unaffected, thunderbird54 fixed, thunderbird55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 months ago
+++ This bug was initially created as a clone of Bug #791645 +++

See bug 791645 comment #71:

First, none of the OnProxyAvailable() callbacks implemented in SMTP, POP, NNTP and IMAP never checks the passed into it:
https://dxr.mozilla.org/comm-central/rev/a263d7290af0172fbcb1f5ccf4f89e2732e9b714/mailnews/compose/src/nsSmtpProtocol.cpp#419
https://dxr.mozilla.org/comm-central/rev/a263d7290af0172fbcb1f5ccf4f89e2732e9b714/mailnews/imap/src/nsImapProtocol.cpp#849
https://dxr.mozilla.org/comm-central/rev/a263d7290af0172fbcb1f5ccf4f89e2732e9b714/mailnews/local/src/nsPop3Protocol.cpp#521
https://dxr.mozilla.org/comm-central/rev/a263d7290af0172fbcb1f5ccf4f89e2732e9b714/mailnews/news/src/nsNNTPProtocol.cpp#1119

That's clearly an omission.

The next problem is that |NS_ENSURE_SUCCESS(rv, rv);| is not the appropriate handling here:
https://dxr.mozilla.org/comm-central/rev/a263d7290af0172fbcb1f5ccf4f89e2732e9b714/mailnews/imap/src/nsImapProtocol.cpp#853
(Assignee)

Updated

5 months ago
Severity: critical → normal
(Assignee)

Comment 1

5 months ago
Created attachment 8843752 [details] [diff] [review]
1344538-fix-error-handling.patch (v1).

I don't really understand how this proxy stuff on all the protocols ended up in my lap. My mission is to keep TB buildable, so whenever M-C deprecates something they announced years ago, I'll have to fix the technical debt with very little mailnews knowledge?

Kent, I think you're in the boat with me since Neil and Joshua have jumped ship. You have better knowledge of those protocols.

I've done what appears to be reasonable here but it is fiendishly difficult to get all the error handling right. 

We note the following:
1) IMAP, POP and SMTP have a fallback when MsgExamineForProxyAsync() fails,
   NNTP does not. You've already questioned that in bug 791645 comment #56
   (1st paragraph).
2) The nsSmtpProtocol class doesn't appear to have a Cancel() method, so
   there is no call |m_proxyRequest->Cancel()| anywhere.

Let me know what you think and give it an r- to your heart's content ;-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8843752 - Flags: review?(rkent)
(Assignee)

Comment 2

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1a061380a54ba5420f00996cf948cdbea7dafaea
shows that this patch doesn't cause any damage.

Comment 3

5 months ago
Comment on attachment 8843752 [details] [diff] [review]
1344538-fix-error-handling.patch (v1).

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

Sorry to reverse myself on the OnProxyAvailable status handling.

I think the rest of the error handling is OK and an improvement, but I wish one of us understood this better. But let's try it, after you remove the OnProxyAvailable aStatus failure handling.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +418,5 @@
>  NS_IMETHODIMP
>  nsSmtpProtocol::OnProxyAvailable(nsICancelable *aRequest, nsIChannel *aChannel,
>                                   nsIProxyInfo *aProxyInfo, nsresult aStatus)
>  {
> +  if (NS_FAILED(aStatus))

Spending more time on this, I see the following comment for OnProxyAvailable in http:


    // If status is a failure code, then it means that we failed to resolve
    // proxy info.  That is a non-fatal error assuming it wasn't because the
    // request was canceled.  We just failover to DIRECT when proxy resolution
    // fails (failure can mean that the PAC URL could not be loaded).

So given that, let me reverse my earlier comment and say that we should failover here, which means leave this method unchanged.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +856,5 @@
> +      if (m_mockChannel)
> +        m_mockChannel->Cancel(aStatus);
> +    }
> +    return NS_ERROR_FAILURE;
> +  }

See comment above, let's leave the error handling out for the aStatus check, just defaulting to no proxy.

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +525,5 @@
> +  if (NS_FAILED(aStatus)) {
> +    m_proxyRequest = nullptr;
> +    Cancel(aStatus);
> +    return NS_ERROR_FAILURE;
> +  }

Again following my earlier comment, do not abort with aStatus failure.

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +1123,5 @@
> +
> +  if (NS_FAILED(aStatus)) {
> +    m_proxyRequest = nullptr;
> +    Cancel(aStatus);
> +    return NS_ERROR_FAILURE;

Again following my earlier comment, do not abort with sStatus failure.
Attachment #8843752 - Flags: review?(rkent) → review+
(Assignee)

Comment 4

5 months ago
Created attachment 8844401 [details] [diff] [review]
1344538-fix-error-handling.patch (v2).

Hi Kent, thanks for the quick review. You invested some time digging through nsHttpChannel::OnProxyAvailable(), which was a good thing to do.

I removed overzealous aborting. As mentioned, NNTP didn't have a fallback, so I put that in.

I also noticed that for the async refactoring, some purely debug assertions had been left out of the async path, so I made sure they get run in both cases.

Maybe an interdiff will save you time here.
Attachment #8843752 - Attachment is obsolete: true
Attachment #8844401 - Flags: review?(rkent)
(Assignee)

Comment 5

5 months ago
Yep, interdiff is useful in this case:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8843752&action=interdiff&newid=8844401&headers=1

I noticed one nit: The comment 
  // If we're called with NS_BINDING_ABORTED <-- comma
has a comma missing here. I'll fix that in the next round, or when landing this, I just want to avoid the noise of another patch.

Comment 6

4 months ago
Comment on attachment 8844401 [details] [diff] [review]
1344538-fix-error-handling.patch (v2).

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

OK, looks good with one nit.

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +1135,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  nsresult rv = LoadUrlInternal(aProxyInfo);
> +  if (NS_FAILED(rv)) {
> +    Cancel(rv);

It seems to me that you do not want to run the assertions after a cancel, so do:

return Cancel(rv);

Comment 7

4 months ago
Comment on attachment 8844401 [details] [diff] [review]
1344538-fix-error-handling.patch (v2).

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

OK, looks good with one nit.

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +1135,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  nsresult rv = LoadUrlInternal(aProxyInfo);
> +  if (NS_FAILED(rv)) {
> +    Cancel(rv);

It seems to me that you do not want to run the assertions after a cancel, so do:

return Cancel(rv);
Attachment #8844401 - Flags: review?(rkent) → review+
(Assignee)

Comment 8

4 months ago
Created attachment 8846398 [details] [diff] [review]
1344538-fix-error-handling.patch (v3).

OK, I fixed the issue Kent pointed out. Also, I'm resetting m_proxyRequest to avoid cancelling the proxy twice. Last not least I noticed that the proxy needs to be cancelled on the main tread, otherwise we get:
"Hit MOZ_CRASH(nsProtocolProxyService not thread-safe) at c:/mozilla-source/comm-central/mozilla/netwerk/base/nsProtocolProxyService.cpp:398"
(See bug 791645 comment #74).

So I fixed that, too, and we should finally be done with the proxy stuff.
Attachment #8844401 - Attachment is obsolete: true
Attachment #8846398 - Flags: review+
(Assignee)

Comment 9

4 months ago
https://hg.mozilla.org/comm-central/rev/26be243deea11c560a8e02e3548e4a3f65b039f2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
(Assignee)

Comment 10

4 months ago
Comment on attachment 8846398 [details] [diff] [review]
1344538-fix-error-handling.patch (v3).

We need to fix TB 54, too.
Attachment #8846398 - Flags: approval-comm-aurora+
(Assignee)

Comment 11

4 months ago
Oops, didn't compile on Linux and Mac, so here the bustage fix:
https://hg.mozilla.org/comm-central/rev/26be243deea11c560a8e02e3548e4a3f65b039f2 (comment #9)
https://hg.mozilla.org/comm-central/rev/88de56694bc0a21020e3d8b24d13fc7d17cfec98
(Assignee)

Comment 12

4 months ago
Aurora (TB 54):
https://hg.mozilla.org/releases/comm-aurora/rev/4b95007deedb156e83d64527827e1310b0cfc3df
https://hg.mozilla.org/releases/comm-aurora/rev/5f1f732ea4e0874bac9fab4315f3de03e14ffdd8
status-thunderbird53: --- → unaffected
status-thunderbird54: --- → fixed
status-thunderbird55: --- → fixed
Depends on: 1350687
You need to log in before you can comment on or make changes to this bug.