Closed Bug 1344538 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: Networking, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird53 unaffected, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird53 --- unaffected
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file, 2 obsolete files)

Severity: critical → normal
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 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+
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)
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 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 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+
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+
https://hg.mozilla.org/comm-central/rev/26be243deea11c560a8e02e3548e4a3f65b039f2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
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+
Depends on: 1350687
Regressions: 1400432
You need to log in before you can comment on or make changes to this bug.