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

RESOLVED FIXED in Thunderbird 55.0

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 55.0

Thunderbird Tracking Flags

(thunderbird53 unaffected, thunderbird54 fixed, thunderbird55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Updated

2 years ago
Severity: critical → normal
(Assignee)

Comment 1

2 years 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)

Comment 3

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

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

Comment 10

2 years 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 12

2 years 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

Updated

2 years ago
Depends on: 1350687
You need to log in before you can comment on or make changes to this bug.