Rewrite calls to synchronous nsIProtocolProxyService::DeprecatedBlockingResolve with Async code as DeprecatedBlockingResolve disappeared with bug 436344

RESOLVED FIXED in Thunderbird 54.0

Status

MailNews Core
Networking
--
critical
RESOLVED FIXED
5 years ago
22 hours ago

People

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

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

Trunk
Thunderbird 54.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 12 obsolete attachments)

16.89 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
56.60 KB, patch
jcranmer
: feedback+
Details | Diff | Splinter Review
43.89 KB, patch
Jorg K (GMT+2)
: review+
Details | Diff | Splinter Review
1.48 KB, patch
Details | Diff | Splinter Review
24.71 KB, patch
Details | Diff | Splinter Review
5.35 KB, patch
Jorg K (GMT+2)
: review+
Details | Diff | Splinter Review
2.29 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Bug 791457 was a stop gap measure. DeprecatedBlockingResolve will be going away as well once the java plugin code is rewritten to use AsyncResolve. We need to make our code use the async path before that happens.

+++ This bug was initially created as a clone of Bug #791457 +++

https://tbpl.mozilla.org/php/getParsedLog.php?id=15239279&tree=Thunderbird-Trunk#error0
../../../../mailnews/base/util/nsMsgUtils.cpp:2057:19: error: 'class nsIProtocolProxyService' has no member named 'Resolve'

https://tbpl.mozilla.org/php/getParsedLog.php?id=15240530&tree=Thunderbird-Trunk#error0

e:/builds/moz2_slave/tb-c-cen-w64/build/mailnews/base/util/nsMsgUtils.cpp(2057) : error C2039: 'Resolve' : is not a member of 'nsIProtocolProxyService'

From Bug 769764 46:

> (In reply to Philip Chee from comment #45)
>> comm-central appears broken:
>> e:/builds/slave/comm-cen-trunk-w32-dbg/build/mailnews/base/util/nsMsgUtils.
>> cpp(2057) : error C2039: 'Resolve' : is not a member of
>> 'nsIProtocolProxyService'
>> 
>> and
>> 
>> ../../../../mailnews/base/util/nsMsgUtils.cpp:2057:19: error: ‘class
>> nsIProtocolProxyService’ has no member named ‘Resolve’
> 
> yes, you'll need to use nsIProtocolProxyService::AsyncResolve() .. The synchronous
> version cannot safely be implemented to be non-blocking so it was removed as a source
> of jank.
Whiteboard: [m-c sync]
Created attachment 813561 [details] [diff] [review]
WIP patch

I couldn't make the call to OnProxyAvailable() to get the nsIProxyInfo.
So, how should I proceed?
Attachment #813561 - Flags: feedback?(neil)

Comment 2

4 years ago
Comment on attachment 813561 [details] [diff] [review]
WIP patch

Unfortunately I don't really have any good ideas. I can give you a high-level description of the problem though. Most consumers of this code are subclasses of nsMsgProtocol, and when they want to load a URL they check for a proxy. They eventually call through to nsMsgProtocol::LoadUrl, which is itself expected to be asynchronous, and could therefore deal with not having the proxy info immediately, so when it gets to opening the socket, it wouldn't have a transport yet, and would need to asynchronously resolve the proxy, then it would be able to create the transport and thus open the socket. But I have no idea whether this scheme would actually work, and I think IMAP works differently anyway, so...
Attachment #813561 - Flags: feedback?(neil) → feedback-
Is this related to bug 778201 that was wontfixed?

Updated

2 years ago
Blocks: 1054354
(Reporter)

Updated

2 years ago
Blocks: 436344
(Reporter)

Comment 4

2 years ago
c:/t1/hg/comm-central/mailnews/base/util/nsMsgUtils.cpp(2086) : error C2039: 'DeprecatedBlockingResolve' : is not a member of 'nsIProtocolProxyService2'
        c:\t1\hg\objdir-sm\dist\include\nsIProtocolProxyService2.h(25) : see declaration of 'nsIProtocolProxyService2'
nsMailDatabase.cpp
c:/t1/hg/comm-central/mozilla/config/rules.mk:930: recipe for target 'nsMsgUtils.obj' failed
mozmake[4]: *** [nsMsgUtils.obj] Error 2
mozmake[4]: Leaving directory 'c:/t1/hg/objdir-sm/mailnews/base/util'
c:/t1/hg/comm-central/mozilla/config/recurse.mk:74: recipe for target 'mailnews/base/util/target' failed
mozmake[3]: *** [mailnews/base/util/target] Error 2

Updated

2 years ago
Severity: major → blocker
Summary: Rewrite calls to synchronous nsIProtocolProxyService::DeprecatedBlockingResolve with Async code before DeprecatedBlockingResolve disappears as well. → Rewrite calls to synchronous nsIProtocolProxyService::DeprecatedBlockingResolve with Async code as DeprecatedBlockingResolve disappeared with bug 436344
Whiteboard: [m-c sync] → dogfood
(Reporter)

Updated

2 years ago
No longer blocks: 436344
Severity: blocker → major
Summary: Rewrite calls to synchronous nsIProtocolProxyService::DeprecatedBlockingResolve with Async code as DeprecatedBlockingResolve disappeared with bug 436344 → Rewrite calls to synchronous nsIProtocolProxyService::DeprecatedBlockingResolve with Async code before DeprecatedBlockingResolve disappears as well.
Whiteboard: dogfood → [m-c sync]
(Reporter)

Updated

2 years ago
Depends on: 1124841
(Reporter)

Comment 5

2 years ago
> Rewrite calls to synchronous nsIProtocolProxyService::DeprecatedBlockingResolve with Async code as DeprecatedBlockingResolve disappeared with bug 436344
It isn't gone, just moved from nsIProtocolProxyService2 to nsIProtocolProxyService
This is somewhat hard. Querying the proxy load info happens in the ::Initialize step of our four protocol implementations. This is then followed by setting up the transport, which needs the proxy info already.

POP, NNTP, and SMTP may be as simple as moving the calls to OpenNetworkSocketWithInfo and nsMsgProtocol::LoadUrl into an async dispatch. IMAP looks hard: <https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#838>.
Severity: major → blocker
Summary: Rewrite calls to synchronous nsIProtocolProxyService::DeprecatedBlockingResolve with Async code before DeprecatedBlockingResolve disappears as well. → Rewrite calls to synchronous nsIProtocolProxyService::DeprecatedBlockingResolve with Async code as DeprecatedBlockingResolve disappeared with bug 436344
The other option is dropping proxy support for these protocols.
(Reporter)

Comment 8

2 years ago
FYI I've filed a new bug:
Bug 1124841 - nsMsgUtils.cpp(2086) : error C2039: 'DeprecatedBlockingResolve' : is not a member of 'nsIProtocolProxyService2'
Created attachment 8553469 [details] [diff] [review]
async_proxy_resolve.diff

This is a patch for people who want to get back normal work.

* smtp and pop3 protocols now use nsIProtocolProxyService.asyncResolve, news and imap do not yet.
* MsgExamineForProxy returns NS_ERROR_NOT_IMPLEMENTED so news and imap protocol with proxy are broken.
* test_sendMailAddressIDN.js fails because the test depends on synchronous call.
Created attachment 8553673 [details] [diff] [review]
synchronous hack

This is probably not a good idea since we link into network code, but since I needed my builds to succeed I thought I'd do the same as they did in bug 436344. Local builds succeed and it might be an intermediate solution, but ideally we should make it asynchronous.

Another option would be to convince them to add it back to the interface as [noscript]. This way their concern that its being used by extensions is taken care of and we can still use it.

Comment 11

2 years ago
(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Created attachment 8553673 [details] [diff] [review]
> synchronous hack

We should probably land this as a bandaid, not to have many days (or who knows how long) of total bustage.
Comment on attachment 8553673 [details] [diff] [review]
synchronous hack

In that case requesting review. I'm fine with anyone doing the review if someone wants to take it off of Neil's queue.
Attachment #8553673 - Flags: review?(neil)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=55af95fc6316
Comment on attachment 8553673 [details] [diff] [review]
synchronous hack

r=me by code inspection but not on the choice of workaround; please find someone else to sign off on that.

>+  nsCOMPtr<nsIProtocolProxyService> proxyService =
>       do_GetService("@mozilla.org/network/protocol-proxy-service;1");
>+  nsRefPtr<nsProtocolProxyService> rawProxyService = do_QueryObject(proxyService);
I'd prefer nsRefPtr<nsProtocolProxyService> rawProxyService = do_GetService("@mozilla.org/network/protocol-proxy-service;1"); if that works.

>+          nsCOMPtr <nsIIOService> ioService =
>+            mozilla::services::GetIOService();
Bah, 81 characters :-( But the other one doesn't need to be wrapped.

>+  nsCOMPtr<nsIProtocolProxyService> proxyService =
>+      do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsRefPtr<nsProtocolProxyService> rawProxyService = do_QueryObject(proxyService, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
Ditto.

>+  nsAutoCString spec(scheme);
>+  spec.Append("://");
>+  spec.Append(host);
>+  spec.Append(':');
>+  spec.AppendInt(port);
>+  // XXXXX - Under no circumstances whatsoever should any code which
>+  // wants a uri do this. I do this here because I do not, in fact,
>+  // actually want a uri (the dummy uris created here may not be
>+  // syntactically valid for the specific protocol), and all we need
>+  // is something which has a valid scheme, hostname, and a string
>+  // to pass to PAC if needed - bbaetz
>+  nsCOMPtr<nsIURI> uri = do_CreateInstance(NS_STANDARDURL_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = uri->SetSpec(spec);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIChannel> tempChannel;
>+  nsCOMPtr <nsIIOService> ioService =
>+    mozilla::services::GetIOService();
>+  rv = ioService->NewChannelFromURI(uri, getter_AddRefs(tempChannel));
>+  NS_ENSURE_SUCCESS(rv, rv);
[I guess this still works... I know in some places we use NS_NewInputStreamChannel to create a dummy channel; I wonder whether that would be more reliable.]
Attachment #8553673 - Flags: review?(neil) → review+
Created attachment 8554212 [details] [diff] [review]
Updated fix of synchronous hack [checked-in: comment 20]

This hack makes the protocol proxy service work, albeit via a very ugly manner (the patch added an interface accessor, but neglected to add it to a QI). That the original method failed to work caused crashes in compose, since that had an assertion that said method worked.

Since we have an smtp: protocol handler now, and the scheme doesn't matter all that much internally in the proxy code past getting the protocol handler, I simplified the SMTP implementation to use that instead of rewriting-to-mailto.

This fixes the code to use the existing nsIChannel implementation, i.e., nsMsgProtocol. I'm not sure if the original protocol proxy make-channel-from-URI would have worked or not, since I switched to passing the nsMsgProtocol instance instead of URI details before I made the accessor work.

As I was doing the switch, I did realize one thing: we probably use hostname instead of realHostname to fill the original URIs, which would mean we break proxying if realHostname were different (i.e., you changed the location of the server after account creation). I'm not sure it would have worked before then, since we may fail to build the URI simply because we expect the servers to be specified using hostname instead of realHostname.

...

I hear the debt collector at our door, and I suspect we may be unable to afford the interest on our technical debt.
Attachment #8553673 - Attachment is obsolete: true
Attachment #8554212 - Flags: review?(neil)
Comment on attachment 8554212 [details] [diff] [review]
Updated fix of synchronous hack [checked-in: comment 20]

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

::: mailnews/base/util/moz.build
@@ +64,5 @@
>      'templateUtils.js',
>      'traceHelper.js',
>  ]
>  
> +LOCAL_INCLUDES = [

Might want to use += here, my fault :)
(In reply to Joshua Cranmer from comment #15)
> This hack makes the protocol proxy service work, albeit via a very ugly
> manner (the patch added an interface accessor, but neglected to add it to a
> QI). That the original method failed to work caused crashes in compose,
> since that had an assertion that said method worked.
Bah, I checked for the interface accessor, but not the QI implementation; that probably breaks the original patch...
(In reply to Joshua Cranmer from comment #15)
> Since we have an smtp: protocol handler now, and the scheme doesn't matter
> all that much internally in the proxy code past getting the protocol
> handler, I simplified the SMTP implementation to use that instead of
> rewriting-to-mailto.
Slightly confusing is that the smtp protocol handler doesn't know how to open a channel...

> As I was doing the switch, I did realize one thing: we probably use hostname
> instead of realHostname to fill the original URIs, which would mean we break
> proxying if realHostname were different (i.e., you changed the location of
> the server after account creation). I'm not sure it would have worked before
> then, since we may fail to build the URI simply because we expect the
> servers to be specified using hostname instead of realHostname.
I noticed while trying to test the previous patch that it's sensitive to the host name. I wouldn't be surprised if it was sensitive to the wrong host name.
Comment on attachment 8554212 [details] [diff] [review]
Updated fix of synchronous hack [checked-in: comment 20]

>+  // XXX: this "interface" ID is exposed, but it's not hooked up to the QI.
>+  //nsRefPtr<nsProtocolProxyService> rawProxyService = do_QueryObject(proxyService, &rv);
>+  //NS_ENSURE_SUCCESS(rv, rv);
[Use a C-style comment here to make it easier to uncomment when the underlying bug is fixed?]

>+  nsRefPtr<nsProtocolProxyService> rawProxyService = static_cast<nsProtocolProxyService*>(proxyService.get());
[Could just use a raw pointer here]
Attachment #8554212 - Flags: review?(neil) → review+
Comment on attachment 8554212 [details] [diff] [review]
Updated fix of synchronous hack [checked-in: comment 20]

I checked in the patch, with the aforementioned changes:
https://hg.mozilla.org/comm-central/rev/cd505091d791

I'm not closing this bug since we technically didn't fulfill the summary of bug (stop using DepecratedBlockingResolve), which is still worthwhile to do, even if difficult.
Attachment #8554212 - Attachment description: Updated fix of synchronous hack → Updated fix of synchronous hack [checked-in: comment 20]
Severity: blocker → normal
Duplicate of this bug: 1124841

Updated

2 years ago
Depends on: 1125672
Blocks: 1137274

Updated

2 years ago
Depends on: 1159448
given the crashes in bug 1159448, we may want to make this a priority.
Severity: normal → major

Comment 23

2 years ago
Giving imap a shot in this bug.
Assignee: nobody → jesper

Comment 24

2 years ago
Collected the existing patches and modified them a tad, so now I got imap, pop3 and smtp working.
nntp still needs touching up, but it works after the 1st request.
The problem I have with it is that in nsNntpIncomingServer::LoadNewsUrl needs to become async as well, which depends on GetNntpConnection having a callback added to it. 
What I then currently have issue with is the idl interface change I need to make from:
 nsIChannel getNntpChannel(in nsIURI uri, in nsIMsgWindow window);
To something like:
 void getNntpChannel(in nsIURI uri, in nsIMsgWindow window, nsISomeCallbackInterfaceName callback);
Flags: needinfo?(mkmelin+mozilla)

Comment 25

2 years ago
Jesper:

If you want to modify an IDL and also land that in Thunderbird 38, what you need to do is to create a second interface that only has the new variant method, and we can land that. Modifying existing interfaces is not allowed after major version has shipped, but we could allow adding a new interface with a new method.

Comment 26

2 years ago
Created attachment 8632349 [details] [diff] [review]
async patch for smtp, pop3, nntp, imap

This is a unified patch for proving asynchronous proxyinfo for imap, pop3, smtp and nntp.
Attachment #813561 - Attachment is obsolete: true
Attachment #8553469 - Attachment is obsolete: true
Attachment #8632349 - Flags: review?
Comment on attachment 8632349 [details] [diff] [review]
async patch for smtp, pop3, nntp, imap

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

I have not looked at the patch in detail yet, but the changes to news code as it stands are completely unacceptable.

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +342,5 @@
> +  rv = MsgExamineForProxyAsync(this, this, getter_AddRefs(m_proxyRequest));
> +
> +  if (NS_FAILED(rv))
> +  {
> +    rv = InitializeInternal(nullptr);

What you're trying to do, effectively, is to delay the call in Initialize until you eliminate the protocol, and this is causing messiness since LoadNewsUrl/AsyncOpen can't be called synchronously afterwards. What you should be doing is only delaying the socket construction and instead gate forward progress in LoadUrl (more specifically, the call to the nsMsgProtocol::LoadUrl inside LoadUrl on the asynchronous proxy load.

Actually, for bonus points, you could hide the entire async proxy stuff in an nsMsgProtocol and simply have the inherent asynchronicity be handled at socket load time instead of having to run an async callback to call the (async) socket creation.

::: mailnews/news/src/nsNNTPProtocol.h
@@ +128,5 @@
> +
> +/**
> +* This class defines a callback interface used by InitializeInternal.
> +*/
> +class NS_NO_VTABLE nsNntpConnectionCallback : public nsISupports

This class appears to be completely unused.

@@ +174,5 @@
>  
>    nsresult LoadUrl(nsIURI * aURL, nsISupports * aConsumer);
> +  nsresult LoadUrlInternal(nsIURI *aURL, nsISupports *aConsumer);
> +  nsresult InitializeInternal(nsIProxyInfo* proxyInfo);
> +  void SetCallback(nsNntpConnectionCallbackFun aCallback, nsISupports *aConsumer);

The design your patch is proposing would be inherently confusing and difficult to work with, and it is simply unacceptable. The fact that the internal socket creation is asynchronous should be completely hidden from the rest of the world by nsNNTPProtocol, whereas your code is forcing the consumers of nsNNTPProtocol to bear the burden.

@@ +220,5 @@
>    nsCOMPtr <nsINNTPArticleList> m_articleList;
>  
>    nsCOMPtr <nsIMsgNewsFolder> m_newsFolder;
>    nsCOMPtr <nsIMsgWindow> m_msgWindow;
> +  nsCOMPtr <nsIMsgWindow> m_aMsgWindow;

This is completely superfluous with m_msgWindow.

::: mailnews/news/src/nsNntpIncomingServer.cpp
@@ +615,3 @@
>  
>    // No protocol? We need our mock channel.
> +  /*nsNntpMockChannel *channel = new nsNntpMockChannel(aURI, aMsgWindow,

Never propose a patch for review that contains commented-out code.

Unfortunately, the code that you're commenting out is absolutely critical for the proper functioning of NNTP--it is how the connection queue gets set up. For this reason alone, this patch is unacceptable.
Attachment #8632349 - Flags: review? → review-

Comment 28

2 years ago
Created attachment 8638539 [details] [diff] [review]
async proxy resolve patch for smtp, pop3, nntp, imap

Addressed the issues the best I could.
Attachment #8632349 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8638539 - Flags: feedback?(Pidgeot18)

Updated

2 years ago
Flags: needinfo?(mkmelin+mozilla)

Updated

2 years ago
See Also: → bug 1175051
Sorry for getting to the feedback so late, I've been busy.

I did spend (a lot more time than I expected) some time setting up a simple proxy server test for NNTP to make this easier to review. (YAY! that generality should also allow us to get SSL tests in the future). Unfortunately, I've noticed that this patch no longer applies, probably due to accumulated bitrot. Since it's just a feedback request, I'll still try to work through it as best as I can.
Depends on: 1208087
Comment on attachment 8638539 [details] [diff] [review]
async proxy resolve patch for smtp, pop3, nntp, imap

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

There's some random whitespace issues (one line had an extra CR, and there were some hard tabs).

I didn't look at the IMAP code in detail (I won't trust that without testing it, and this patch no longer applies).

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +306,5 @@
>  
>      PR_LOG(SMTPLogModule, PR_LOG_ALWAYS, ("SMTP Connecting to: %s", hostName.get()));
>  
> +    // It seems quite weird to initialize proxy info here just once. It should
> +    // be per request

Drop this comment. Note that we probably don't even reuse the SMTP protocol instance, and, if we do, we certainly have no reason to reuse it with a different server.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +872,5 @@
> +    rv = mailnewsUrl->GetServer(getter_AddRefs(server));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    m_server = do_GetWeakReference(server);
> +  }
> +  if (proxyCallback)

Why are you hiding the actual socket creation like this? Effectively, you're turning a proxy configuration failure into a complete inability to create an IMAP socket, which is not what the prior code was doing.

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +520,5 @@
> +nsresult nsPop3Protocol::InitializeInternal(nsIProxyInfo* proxyInfo)
> +{
> +  nsresult rv;
> +
> +  m_proxyRequest = nullptr;

Ditto here about cancelling the proxy request (and probably SMTP as well).

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +1094,5 @@
> +                           systemPrincipal,
> +                           nsILoadInfo::SEC_NORMAL,
> +                           nsIContentPolicy::TYPE_OTHER);
> +        rv = pps->AsyncResolve(channel, 0, this, getter_AddRefs(m_proxyRequest));
> +        NS_ENSURE_SUCCESS(rv, rv);

The channel in question is in fact the nsNntpProtocol instance. There should be no need to create a new channel with the given URL, just pass this instead of that channel.

You'll also want to cancel the proxy request if nsNNTPProtocol::Cancel is called.

Also, you probably just want to use MsgExamineForProxyAsync here as well.

@@ +1132,5 @@
> +// nsIProtocolProxyCallback
> +NS_IMETHODIMP
> +nsNNTPProtocol::OnProxyAvailable(nsICancelable *aRequest, nsIChannel *aChannel,
> +                                 nsIProxyInfo *aProxyInfo, nsresult aStatus)
> +{

Given what I've said above about the channel, you'll also want to MOZ_ASSERT(aChannel == this, "Should never request a proxy for anyone but ourselves");

@@ +1167,5 @@
> +
> +  nsCOMPtr<nsIURI> uri;
> +  rv = aChannel->GetURI(getter_AddRefs(uri));
> +
> +  rv = nsMsgProtocol::LoadUrl(uri, m_consumer);

Use m_url here instead of grabbing the uri from this. Otherwise, it's just a really expensive way of getting the same thing.
Attachment #8638539 - Flags: feedback?(Pidgeot18) → feedback+
per irc, it's now back to jesper
Flags: needinfo?(jesper)
Whiteboard: [m-c sync] → [m-c sync][not strictly dependent on tests from bug 1208087]

Updated

2 years ago
Flags: needinfo?(jesper)
Component: Backend → Networking
Unfortunately the blocklist in bug 1183890 is not terribly effective. 
So this still causes #3 crash for release, #4 for beta.  #4 crash for Seamonkey
Whiteboard: [m-c sync][not strictly dependent on tests from bug 1208087] → [m-c sync][not strictly dependent on tests from bug 1208087][patchlove]
(Assignee)

Updated

4 months ago
Duplicate of this bug: 1336789
(Assignee)

Comment 34

4 months ago
Refreshing the patch now.
(Assignee)

Comment 35

4 months ago
Created attachment 8833802 [details] [diff] [review]
791645-DeprecatedBlockingResolve.patch (JK v1).

Refreshed patch and made it compile, not so easy after 1.5 years. TB still runs and seems to receive IMAP mail. So something is working. I haven't addressed the issues from comment #30. I'll do that next and then ship off a try run.
(Assignee)

Comment 36

4 months ago
Created attachment 8833813 [details] [diff] [review]
791645-DeprecatedBlockingResolve.patch (JK v2).

OK, addressed issues from comment #30. Removed tabs and trailing spaces.

Let's see what works:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=be17eb1afc05ee64d2387c53fb51b8a32c6502de

Jesper, hi, today M-C finally removed DeprecatedBlockingResolve() so we're busted and really need this code *now*. I've rebased the patch, made it compile and addressed the review comments.

Since it's your work and most of it has f+, I can just r+ the patch, but I will get a second opinion from Kent, who's the resident IMAP expert (and Joshua didn't look at the IMAP parts). Are you OK with that? Of course it will be committed to the code base in your name.

Kent, you can you please get ready for review here. I did a very quick job with the rebasing, basically I took the added lines from the patch, so it's possible I clobbered some changes that have happened in the last 1.5 years. So it would be good to carefully look at the larger chunks.
Attachment #8833802 - Attachment is obsolete: true
Flags: needinfo?(jesper)
Attachment #8833813 - Flags: review?(rkent)
(Assignee)

Comment 37

4 months ago
Created attachment 8833824 [details] [diff] [review]
791645-DeprecatedBlockingResolve.patch (JK v3).

Picky compilers on Linux and Mac. New try for Linux, Mac opt doesn't compile anyway.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6d597c2bd170598431a3a50a54a495eee183062
Attachment #8833813 - Attachment is obsolete: true
Attachment #8833813 - Flags: review?(rkent)
Attachment #8833824 - Flags: review?(rkent)

Comment 38

4 months ago
I looked at this tonight, and I really cannot add any value to the review for this without investing several days of effort coming up to speed on proxies. We've had both jcranmer and now jorgk look this over. I can't really spare the couple of days this would take me anytime soon. I realize this is putting you out on a limb a bit here jorkg, but I think that the best thing is for you to clean up the patch and land it with your review, as you are doing, then get some testing feedback after it is landed.

Updated

4 months ago
Attachment #8833824 - Flags: review?(rkent)
(Assignee)

Comment 39

4 months ago
Thanks for looking.

(In reply to Kent James (:rkent) from comment #38)
> ... I think that the
> best thing is for you to clean up the patch and land it with your review, as
> you are doing, then get some testing feedback after it is landed.
Nothing to land here yet. This is causing massive test failures (I haven't looked why), even worse than without the patch :-(
(Assignee)

Updated

4 months ago
Severity: major → blocker
(Assignee)

Comment 40

4 months ago
On a positive note, the patch fixes
TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_smtpProxy.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapProxy.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_pop3Proxy.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/news/test/unit/test_nntpProxy.js | xpcshell return code: 0

... but causes ... in fact, the list is too long and too sad:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6d597c2bd170598431a3a50a54a495eee183062&selectedJob=74654065
(Assignee)

Comment 41

4 months ago
Created attachment 8834280 [details] [diff] [review]
791645-DeprecatedBlockingResolve.patch (JK v4), POP, SMTP, NNTP only

I decided to split up the problem. With this patch the POP, SMTP, NNTP proxy test failures are fixed, however, there is one additional test failure:

TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_sendMailAddressIDN.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_sendMailAddressIDN.js | DoSendTest - [DoSendTest : 166] 0 == 2153066735

So this is starting to look promising.
(Assignee)

Comment 42

4 months ago
I've talked to Jesper on IRC, he's fine with me finishing off what's needed.
Assignee: jesper → jorgk
Severity: blocker → critical
Flags: needinfo?(jesper)

Updated

4 months ago
Blocks: 1334779
(Assignee)

Comment 43

4 months ago
I need some help here. The code works fine, the Xpcshell proxy test failures for SMTP, POP and NNTP are solved with the first patch here. I'll do IMAP later.

However, now I get a failing test, test_sendMailAddressIDN.js.

What happens is that some test cases in that test trigger an error here:
https://dxr.mozilla.org/comm-central/rev/03dfa060aa19752c83105fe63eed57230555d1b1/mailnews/compose/src/nsSmtpProtocol.cpp#1914
This used to run *sync* inside nsSmtpProtocol::LoadUrl(). With the patch, it runs *async* in the proxy resolve callback.

Kent, you don't need weeks to get familiar with proxies. All you need to do is tell me what I need to do to raise the exception, so the test will get it. Or do I need to change the test? I have the impression that the test is expecting a sync result which doesn't come. How do I get the async result?
Test here:
https://dxr.mozilla.org/comm-central/rev/03dfa060aa19752c83105fe63eed57230555d1b1/mailnews/compose/test/unit/test_sendMailAddressIDN.js#166
Test parts failing are 3 and 4, well, I never got beyond 3.

Do I hear promises?

This bug is currently blocked on the issue.

NI'ing a few other people who may or may not know or won't reply ;-)
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(Pidgeot18)
(Assignee)

Updated

4 months ago
Whiteboard: [m-c sync][not strictly dependent on tests from bug 1208087][patchlove] → [Thunderbird-testfailure]
(Assignee)

Updated

4 months ago
Whiteboard: [Thunderbird-testfailure] → [Thunderbird-testfailure: X all]
(Assignee)

Comment 44

4 months ago
I discussed with Kent on IRC how this needs to be restructured.
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(Pidgeot18)
(Assignee)

Comment 45

4 months ago
Let me capture some of this discussion:

The NS_ERROR_BUT_DONT_SHOW_ALERT is no longer returned from the sync call to msgSend.sendMessageFile(). So the fact that something went wrong needs to be communicated to the nsIMsgSendListener, or else it hangs like the test does currently. Kent said:
  so an error needs to get propagated to a onStopSending callback
  I also see a onSendNotPerformed.  I am not sure of the definition here,
  if that is considered an alternate terminating callback.

So the code needs to change to pass the error to the send listener and the test needs to change so it checks for the error differently.
(Assignee)

Comment 46

3 months ago
Created attachment 8836200 [details] [diff] [review]
791645-DeprecatedBlockingResolve.patch (JK v5), POP, SMTP, NNTP only [checked-in: comment 48]

Moved this upfront checks for SMTP back from the proxy callback to the sync part where they belong. Result: Test passes ;-) Forget comments 43-45.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=73a3ef2611398ca76224730559aa812338c602c8
Attachment #8834280 - Attachment is obsolete: true
(Assignee)

Comment 47

3 months ago
Created attachment 8836229 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v1) IMAP

IMAP part, refreshed after landing of bug 1335638.
Still need to remove MsgExamineForProxy().
Attachment #8833824 - Attachment is obsolete: true
(Assignee)

Comment 48

3 months ago
Comment on attachment 8836200 [details] [diff] [review]
791645-DeprecatedBlockingResolve.patch (JK v5), POP, SMTP, NNTP only [checked-in: comment 48]

https://hg.mozilla.org/comm-central/rev/5a3b041cbe1615d68c25d5f9ec8a98f5d0516b4f

I've landed this part to fix at least some of the Xpcshell test failures and to expose it to some testing in Daily. r=jcranmer,jorgk.
Attachment #8836200 - Attachment description: 791645-DeprecatedBlockingResolve.patch (JK v5), POP, SMTP, NNTP only → 791645-DeprecatedBlockingResolve.patch (JK v5), POP, SMTP, NNTP only [checked-in: comment 48]
Attachment #8836200 - Flags: review+
(Assignee)

Comment 49

3 months ago
Comment on attachment 8836229 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v1) IMAP

Very unsuccessful try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0b72dfbc0a324707e1d05e05e1415da572d52b72
Too many failures to list.
(Assignee)

Comment 50

3 months ago
(In reply to Jorg K (GMT+1) from comment #48)
> I've landed this part to fix at least some of the Xpcshell test failures and
> to expose it to some testing in Daily. r=jcranmer,jorgk.
I'm using this Daily now and one negative effect already observed is that when sending a large message, the progress bar jumps to 100% straight away saying "Delivering mail...". Before there was a visible progress.
(Assignee)

Comment 51

3 months ago
Created attachment 8836409 [details] [diff] [review]
791645-fix-progress.patch [checked-in: comment 51]

https://hg.mozilla.org/comm-central/rev/5c33fa867d03e8099ab69b795f9967382eba80d2
Small tweak in SMTP to landed POP, SMTP, NNTP patch from comment #48.
(Assignee)

Comment 52

3 months ago
Created attachment 8837343 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v2) IMAP

The IMAP part needed some more reshuffling ;-)
The tests I tried passed locally now, so this is starting to look promising.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=417b1383537c9a43dd6528f39a75ffb2ed9ba21e
Attachment #8836229 - Attachment is obsolete: true
(Assignee)

Comment 53

3 months ago
That's starting to look good, one failed test:
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_listSubscribed.js
(Assignee)

Comment 54

3 months ago
Created attachment 8837705 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v2b) IMAP with debug

OK here is the latest code. As I said in comment #53, all Xpcshell test pass, except one:
mozilla/mach xpcshell-test mailnews/imap/test/unit/test_listSubscribed.js

I've spent hours with this in the debugger and I can now demonstrate the problem with a few debug prints:

"=== SetupWithUrl: m_runningUrl=0000000008BE2800 channel=000000000BE39870"
"=== releasing m_runningUrl=0000000008BE2800"
"=== SetupWithUrl: m_runningUrl=0000000008BE5000 channel=000000000BE39A10"
"=== SetupWithUrlCallback: m_runningUrl=0000000000000000 channel=000000000BE39870"
"=== m_runningUrl=0000000000000000"
"[6876] ###!!! ASSERTION: missing channel or running url: 'false', file c:/mozilla-source/comm-central/mailnews/imap/src/nsImapProtocol.cpp, line 2219"

So what happens? SetupWithUrl gets called with URL 1 (8BE2800). That calls the async proxy resolution. Sadly URL 1 gets released *before* the async callback arrives. Then SetupWithUrl gets called for URL 2 (8BE5000) and then the callback arrives for the first URL. You can see this looking at the channel.

Since I don't know how this async and threading stuff hangs together, it's very hard for me to fix it.

The release happens after ImapThreadMainLoop() returns in nsImapProtocol::Run(). To the layman's eye is doesn't appear to be a good idea to get out of that main loop while we're still waiting for something asynchronous to happen.

Kent, once again, you don't need to know anything about proxies. All you need to know is that the function SetupWithUrl() called by LoadImapUrl() got refactored into a part that runs sync and a part that runs async later. That async part is called SetupWithUrlCallback() Once it has executed code that was previously in SetupWithUrl() it continues to execute LoadImapUrlInternal() which contains code previously in LoadImapUrl().

So picture:

Before:
LoadImapUrl()
part 1
             --> SetupWithUrl()
                 part1
                 sync proxy resolution
                 part2
             <--
part2.

Now:
Sync part:
LoadImapUrl()
part 1       --> SetupWithUrl()
                 part 1
                 async proxy resolution
             <--
done.

Async part in callback:
                 SetupWithUrlCallback()
                 part2
             <--
LoadImapUrlInternal()
part2
done.

All works fine, only of course if between the sync part and the arrival of the async part the URL is released, then we have a problem.
Attachment #8837343 - Attachment is obsolete: true
Flags: needinfo?(rkent)
(Assignee)

Comment 55

3 months ago
Hmm, I don't know why it decides to release the URL, so I tried:
  if (m_runningUrl)
  {
    if (m_proxyRequest) {
      printf("=== releasing cancel proxy\n");
      m_proxyRequest->Cancel(NS_BINDING_ABORTED);
    }
    printf("=== releasing m_runningUrl=%p\n", (void*)m_runningUrl);
    NS_ReleaseOnMainThread(m_runningUrl.forget());
  }

That Cancel() gets called, and I would imagine the callback shouldn't fire any more, but it does:
=== SetupWithUrl: m_runningUrl=000000000BC09400 channel=00000000075A8870"
=== releasing cancel proxy"
=== releasing m_runningUrl=000000000BC09400"
=== SetupWithUrlCallback: m_runningUrl=0000000000000000 channel=00000000075A8870"
=== SetupWithUrl: m_runningUrl=000000000BC0BC00 channel=00000000075A8A10"
=== SetupWithUrlCallback: m_runningUrl=0000000000000000 channel=00000000075A8870"
=== m_runningUrl=0000000000000000"
Actually, that made things worse :-(

Comment 56

3 months ago
Comment on attachment 8837705 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v2b) IMAP with debug

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +828,5 @@
> +        printf("=== SetupWithUrl: m_runningUrl=%p channel=%p\n", (void*)m_runningUrl, (void*)m_mockChannel);
> +        rv = MsgExamineForProxyAsync(m_mockChannel, this, getter_AddRefs(m_proxyRequest));
> +        if (NS_FAILED(rv))
> +        {
> +          rv = SetupWithUrlCallback(false, nullptr);

I could not find any evidence looking at other code that resorting to a fallback setup of the url (without a proxy I assume) is common practice with an error return from MsgExamineForProxyAsync or pps->AsyncResolve. So I would consider this odd code. At the very least it deserves a comment about why you are doing this. My first impulse, if there was no supporting reason for this, would be to not do a fallback.

@@ +2184,5 @@
> +  rv = SetupSinkProxy(); // generate proxies for all of the event sinks in the url
> +  if (NS_FAILED(rv)) // URL can be invalid.
> +    return rv;
> +
> +  m_lastActiveTime = PR_Now();

It is the movement of this line that is causing the crash. Here is why. The first IMAP call:

IMAPPump.incomingServer.performExpand(null); 

creates the imap protocol, then waits for the proxy callback. Before that completes, the second call is done:

IMAPPump.inbox.updateFolderWithListener(null, asyncUrlListener);

This call when looking for an available connection tries the previous one. But because m_lastActiveTime is 0, when it checks the connection timeout in nsImapIncomingServer::GetImapConnection, the running time in incorrect and it kills the protocol.

The immediate fix is to move that line back to nsImapProtocol::LoadImapUrl so that it gets called immediately.

But the goal of a test is to discover problems. We have discovered a problem: when the protocol is destroyed, you are not cancelling the proxy callback. You should make sure that the protocol does not crash if the protocol is destroyed before the proxy callback occurs. Might as well test it too, maybe by adding a test where you explicitly tell the thread to die while waiting for the proxy callback.

I have not reviewed the rest of the code, as I understand what you wanted was a second pair of eyes to understand the crash.

Updated

3 months ago
Flags: needinfo?(rkent)
(Assignee)

Comment 57

3 months ago
Oh, Kent just commented, but I'll post this anyway without reading his comment first.

===

I don't want to attach a new patch for everything I try. Here I tried to get the URL from the channel:

nsresult nsImapProtocol::SetupWithUrlCallback(bool proxyCallback, nsIProxyInfo* proxyInfo)
{
  m_proxyRequest = nullptr;

  nsresult rv;
  printf("=== SetupWithUrlCallback: m_runningUrl=%p channel=%p\n", (void*)m_runningUrl, (void*)m_mockChannel);
  // Check channel m_url
  nsCOMPtr<nsIURI> curi;
  m_mockChannel->GetURI(getter_AddRefs(curi));
  printf("=== SetupWithUrlCallback: ChannelURI=%p\n", (void*)curi);
  if (!m_runningUrl)
    m_runningUrl = do_QueryInterface(curi);
  printf("=== SetupWithUrlCallback: m_runningUrl(new)=%p\n", (void*)m_runningUrl);

That gives:

"=== SetupWithUrl: m_runningUrl=000000000DEE9C00 channel=00000000072F0870"
"=== releasing m_runningUrl=000000000DEE9C00"
"=== SetupWithUrl: m_runningUrl=000000000DEEC400 channel=00000000072F0A10"
"=== SetupWithUrlCallback: m_runningUrl=0000000000000000 channel=00000000072F0870"
"=== SetupWithUrlCallback: ChannelURI=000000000DEE9C08"
"=== SetupWithUrlCallback: m_runningUrl(new)=000000000DEE9C00" <===
"=== SetupWithUrlCallback: m_runningUrl=000000000DEEC400 channel=00000000072F0A10"
"=== SetupWithUrlCallback: ChannelURI=000000000DEEC408"
"=== SetupWithUrlCallback: m_runningUrl(new)=000000000DEEC400"

So at the position marked <=== we successfully restored the URL, but the test still fails.
(Assignee)

Comment 58

3 months ago
(In reply to Kent James (:rkent) from comment #56)
> I could not find any evidence looking at other code that resorting to a
> fallback setup of the url (without a proxy I assume) is common practice with
> an error return from MsgExamineForProxyAsync or pps->AsyncResolve. So I
> would consider this odd code. At the very least it deserves a comment about
> why you are doing this. My first impulse, if there was no supporting reason
> for this, would be to not do a fallback.
In all other protocols we do so:
https://dxr.mozilla.org/comm-central/search?q=MsgExamineForProxyAsync&redirect=false
That at least passed Joshua's f?-->f+ at the time.

> The immediate fix is to move that line back to nsImapProtocol::LoadImapUrl
> so that it gets called immediately.
Thanks!

> But the goal of a test is to discover problems. We have discovered a
> problem: when the protocol is destroyed, you are not cancelling the proxy
> callback. You should make sure that the protocol does not crash if the
> protocol is destroyed before the proxy callback occurs. Might as well test
> it too, maybe by adding a test where you explicitly tell the thread to die
> while waiting for the proxy callback.
Will do.

> I have not reviewed the rest of the code, as I understand what you wanted
> was a second pair of eyes to understand the crash.
OK, I'll r? you. In comment #38 you said I should do it by myself (and you would need to (quote) "invest several days of effort coming up to speed on proxies"). I don't think that's the case. One simply needs to understand that there was a refactoring and look at it this way.

So, keen to do a post-landing review for the other three?
(Assignee)

Comment 59

3 months ago
Created attachment 8837964 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v3) IMAP

Magically, moving that one line makes the test pass: Yay!! \o/
Attachment #8837705 - Attachment is obsolete: true
(Assignee)

Comment 60

3 months ago
Comment on attachment 8837964 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v3) IMAP

Try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aff7fd20ff081a836c1d26bf110ffea449440c2d

Kent, can you give this a rough review please. It's really just a reshuffling of code (which when done badly doesn't work).

There are a few things marked XXX in the patch. I could lose the hack with the m_url and just cast m_runningUrl where I need to retrieve the port.

Also, maybe you can contribute a snipped to solve "when the protocol is destroyed, you are not cancelling the proxy callback". You can pinpoint this better than I can. Examples here:
https://dxr.mozilla.org/comm-central/search?q=m_proxyRequest-%3ECancel&redirect=false

Thanks a million for your help!
Attachment #8837964 - Flags: review?(rkent)
(Assignee)

Comment 61

3 months ago
Green try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aff7fd20ff081a836c1d26bf110ffea449440c2d
(Assignee)

Comment 62

3 months ago
Created attachment 8838296 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v4) IMAP [checked in: comment 64]

Some more reshuffling, removed kludge to get port, added proxy cancel. Should be good to go finally:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ef679c07b1fbf34baefb0dcfb0050bc5bec8a704
Attachment #8837964 - Attachment is obsolete: true
Attachment #8837964 - Flags: review?(rkent)
(Assignee)

Comment 63

3 months ago
Oops, should have included my patch in the try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bffb1bd85d9d42ae1ac14b68fd4f48926aa9159b
(Assignee)

Comment 64

3 months ago
Comment on attachment 8838296 [details] [diff] [review]
791645-DeprecatedBlockingResolve-IMAP.patch (JK v4) IMAP [checked in: comment 64]

https://hg.mozilla.org/comm-central/rev/45293a726fc3b519b1081c393c95908c361c077b

Checked in (with some white-space changes) according to comment #38 to give this some exposure.

A debug try run at
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=03b9d3f541b207d4f1a231c5c96edaa6707f8c99
"only" shows the six crashes due to bug 1337865

I addressed Kent's comment #56 "when the protocol is destroyed, you are not cancelling the proxy callback" as well as I could. Sadly, I couldn't make much sense of "Might as well test it too, maybe by adding a test where you explicitly tell the thread to die while waiting for the proxy callback". It would be good for him to check this and provide a follow-up patch to rectify any remaining issues.
Attachment #8838296 - Attachment description: 791645-DeprecatedBlockingResolve-IMAP.patch (JK v4) IMAP → 791645-DeprecatedBlockingResolve-IMAP.patch (JK v4) IMAP [checked in: comment 64]
Flags: needinfo?(rkent)
(Assignee)

Updated

3 months ago
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: X all]

Comment 65

3 months ago
This bug is the reason why Thunderbird with foxyproxy crashed, see it's forum post: FoxyProxy 4.5.6 crashes Tbird 38.6.0 - https://forums.getfoxyproxy.org/viewtopic.php?f=4&t=1798
(Assignee)

Comment 66

3 months ago
I assumed that much since FoxyProxy's (co-?)author Jesper Hansen came here to fix the bug. The fixes have landed now, only pending some minor issues to be looked at before the bug will be resolved as fixed. So does FoxyProxy work now with a Daily build of TB?

Comment 67

3 months ago
Hi, Jorg K, thanks for the hint. I just download a nightly build from this folder: https://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central/, and the file thunderbird-54.0a1.en-US.win64.zip which is build on 23-Feb-2017 11:54. I have foxyproxy add on installed, and I see no crash of TB, but it looks like foxyproxy does not works quite well, when I click on the "add proxy" button of the foxyproxy setting dialog, nothing prompt.

Comment 68

3 months ago
I'm not sure, but is the foxyproxy add on get disabled automatically?

I see a page in the TB, which said:
----------------------------------------------------------------
FoxyProxy Standard for Thunderbird has been blocked for your protection.

Why was it blocked?
    This add-on is causing consistent startup crashes on Thunderbird 38 and above.
Who is affected?
    All users who have this add-on installed on Thunderbird 38 and above.
What does this mean?

    The problematic add-on or plugin will be automatically disabled and no longer usable. 
----------------------------------------------------------------
When I looked at the status of the add on, it looks like this add on is still enabled.
(Assignee)

Comment 69

3 months ago
Kent, repeating my PM:

===
Since the proxy bug has landed on TB 54 it would be good to fix any outstanding issues before the branch date next Monday or else this will make for a confusing bug and additional uplifts.

I've added a |m_proxyRequest->Cancel(NS_BINDING_ABORTED);| here:
https://dxr.mozilla.org/comm-central/rev/c9ecf72b2ee3cfdbdcae751ff7ccfb05431d098d/mailnews/imap/src/nsImapProtocol.cpp#1050

It you want it elsewhere, please send a patch to do so (*).

Also, you made this somewhat cryptic here (comment #56):
https://bugzilla.mozilla.org/show_bug.cgi?id=791645#c56
  We have discovered a problem: when the protocol is destroyed,
  you are not cancelling the proxy callback. You should make
  sure that the protocol does not crash if the protocol is
  destroyed before the proxy callback occurs.
I thinks that's covered, but might not be in the right spot.

  Might as well test it too, maybe by adding a test where
  you explicitly tell the thread to die while waiting for
  the proxy callback.
I don't understand this, so please send a patch (*).
===

OK, if you don't want to write a patch, please just give very precise instructions ;-)

Comment 70

3 months ago
Jorg asked that I spend a few minutes clarifying my comment:

"But the goal of a test is to discover problems. We have discovered a problem: when the protocol is destroyed, you are not cancelling the proxy callback. You should make sure that the protocol does not crash if the protocol is destroyed before the proxy callback occurs. Might as well test it too, maybe by adding a test where you explicitly tell the thread to die while waiting for the proxy callback."

OK now you are cancelling the proxy callback, that is good. Now, does that prevent the crash in the case where the original problem is put back (moving the one line of code)? No, because of inadequate error checking.

If you abort the protocol, OnProxyAvailable gives a aStatus of NS_BINDING_ABORTED. But that is never checked:

nsImapProtocol::OnProxyAvailable(nsICancelable *aRequest, nsIChannel *aChannel,
                                 nsIProxyInfo *aProxyInfo, nsresult aStatus)
{
  nsresult rv = SetupWithUrlCallback(aProxyInfo);
  NS_ENSURE_SUCCESS(rv, rv);

so we end up back in SetupWithUrlCallback with the same issue that caused the abort. So that is one problem, I don't think we should be continuing if the proxy shows an error.

Second, the crash itself should never have been allowed, it only happens because of inadequate error checking here:

      nsCOMPtr<nsIURI> uri = do_QueryInterface(m_runningUrl);
      uri->GetPort(&port);

so that is a second problem. The crash occurred because there are two different places where the code did not check an error return. I think those should be added.

The second is no risk, because with null uri you would crash. The first is more problematic. I THINK you should cancel, but I don't understand the operation of the protocol well enough to be sure. Also, the NS_ENSURE_SUCCESS(rv, rv); is not adequate in OnProxyAvailable, because this is an async callback. The code really should do whatever steps are needed for the protocol to return an error. The proxy code is not going to know what to do with an error return from the callback, it is the responsibility of the imap protocol to process the error, and then to send whatever error notifications that are expected. Unfortunately I do not know what those are, but in the grander scheme there is often an nsIUrlListener which should be getting an error callback on OnStopRunningUrl().
Flags: needinfo?(rkent)
(Assignee)

Comment 71

3 months ago
Thanks Kent. You pointed out various problems:

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

I'll land a patch to add error checking to this:
  nsCOMPtr<nsIURI> uri = do_QueryInterface(m_runningUrl);
  uri->GetPort(&port);
and I'll move the remaining issue to bug 1344538.
(Assignee)

Comment 72

3 months ago
Created attachment 8843682 [details] [diff] [review]
791645-imap-error-check.patch [checked in: comment 73]

This patch adds an error check to
  nsCOMPtr<nsIURI> uri = do_QueryInterface(m_runningUrl);

It looks terrible since I changed the indentation level of an entire block.

Assuming r=rkent according to comment #70, last paragraph ("second problem"). As I said, the "first problem" is now in bug 1344538.
Attachment #8843682 - Flags: review+
(Assignee)

Comment 73

3 months ago
https://hg.mozilla.org/comm-central/rev/cbc772c7b1b4f0608a6fddc315e0fff65503abfb

Summarising landings in this bug:
https://hg.mozilla.org/comm-central/rev/cd505091d791 (comment #20 in January of 2015)
https://hg.mozilla.org/comm-central/rev/5a3b041cbe1615d68c25d5f9ec8a98f5d0516b4f (comment #48: NNTP, POP, SMTP)
https://hg.mozilla.org/comm-central/rev/5c33fa867d03e8099ab69b795f9967382eba80d2 (comment #51: small SMTP tweak)
https://hg.mozilla.org/comm-central/rev/45293a726fc3b519b1081c393c95908c361c077b (comment #64: IMAP)
https://hg.mozilla.org/comm-central/rev/cbc772c7b1b4f0608a6fddc315e0fff65503abfb (IMAP: additional error check).

See you all in bug 1344538!
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
(Assignee)

Comment 74

3 months ago
Created attachment 8843690 [details] [diff] [review]
verification-patch.patch - Testing only, not for check-in

OK, just to bring this matter to a close, I moved the line |m_lastActiveTime = PR_Now();| back to where it was causing test_listSubscribed.js to fail.

Without the additional check I added in the last patch landed, it crashes indeed. With the check, it doesn't crash, but it still fails big time, this time on an assert in the proxy handling:

"Hit MOZ_CRASH(nsProtocolProxyService not thread-safe) at c:/mozilla-source/comm-central/mozilla/netwerk/base/nsProtocolProxyService.cpp:398"
"#01: mozilla::net::nsAsyncResolveRequest::DoCallback (c:\\mozilla-source\\comm-central\\mozilla\\netwerk\\base\\nsprotocolproxyservice.cpp:289)"
"#02: mozilla::net::nsAsyncResolveRequest::Run (c:\\mozilla-source\\comm-central\\mozilla\\netwerk\\base\\nsprotocolproxyservice.cpp:157)"

To be continued in bug 1344538.
(In reply to asmwarrior from comment #68)
> I'm not sure, but is the foxyproxy add on get disabled automatically?

yes. bug 1137274 comment 23

Comment 76

3 months ago
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #75)
> (In reply to asmwarrior from comment #68)
> > I'm not sure, but is the foxyproxy add on get disabled automatically?
> 
> yes. bug 1137274 comment 23

OK, but it looks like this bug is fixed, right? I'm not quite follow your discussion about the code changes, so is there any chance to re-enable this add on?
At least people can test it on the new builds to see whether it crashes again. Thanks.
(Assignee)

Comment 77

3 months ago
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #75)
> (In reply to asmwarrior from comment #68)
> > I'm not sure, but is the foxyproxy add on get disabled automatically?
> yes. bug 1137274 comment 23

So maybe we should disable the disablement and check whether FoxyProxy works now that this bug here is fixed. I don't see a patch in bug 1137274, so I don't know how it got disabled in the first place and how we would go about re-enabling it.
unless a hard block was put on foxyproxy, you should be able to override the blocklist and test the addon.
https://wiki.mozilla.org/Blocklisting
see the last few bugs of https://mzl.la/2mTfnLx - sort by bug#

Comment 80

3 months ago
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #78)
> unless a hard block was put on foxyproxy, you should be able to override the
> blocklist and test the addon.
> https://wiki.mozilla.org/Blocklisting

Hi, Wayne Mery, thanks for the info. I have just download a nightly build TB 2017-03-06 for testing. From the page https://wiki.mozilla.org/Blocklisting I don't know how to override the blocklist. I paste the new TB binaries over the old install, and start TB I still see the same message, the option dialog of foxyproxy can be opened, but nothing can be added or changed there.

Can you help on this? Thanks. Do I have to re-install foxyproxy add on?

Comment 81

3 months ago
(In reply to asmwarrior from comment #80)
> I paste the new TB binaries over
> the old install, and start TB I still see the same message, the option
> dialog of foxyproxy can be opened, but nothing can be added or changed there.
> 
> Do I have to re-install foxyproxy add on?

OK, I just download a new foxyproxy_standard-4.6.5-fx+sm+tb.xpi, and then reinstall it to the testing TB. Now, the add-on works fine, great! Thanks.

Comment 82

2 months ago
Glad to see movement on this bug! (From the author of FoxyProxy)
(Assignee)

Updated

22 hours ago
Depends on: 1367707
You need to log in before you can comment on or make changes to this bug.