Closed Bug 791645 Opened 12 years ago Closed 7 years ago

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

Categories

(MailNews Core :: Networking, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 54.0

People

(Reporter: philip.chee, Assigned: jorgk-bmo)

References

Details

Attachments

(7 files, 12 obsolete files)

16.89 KB, patch
neil
: review+
Details | Diff | Splinter Review
56.60 KB, patch
jcranmer
: feedback+
Details | Diff | Splinter Review
43.89 KB, patch
jorgk-bmo
: 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
jorgk-bmo
: review+
Details | Diff | Splinter Review
2.29 KB, patch
Details | Diff | Splinter Review
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]
Attached patch WIP patch (obsolete) — Splinter Review
I couldn't make the call to OnProxyAvailable() to get the nsIProxyInfo.
So, how should I proceed?
Attachment #813561 - Flags: feedback?(neil)
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?
Blocks: cc-backlog
Blocks: 436344
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
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
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]
Depends on: 1124841
> 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.
FYI I've filed a new bug:
Bug 1124841 - nsMsgUtils.cpp(2086) : error C2039: 'DeprecatedBlockingResolve' : is not a member of 'nsIProtocolProxyService2'
Attached patch async_proxy_resolve.diff (obsolete) — Splinter Review
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.
Attached patch synchronous hack (obsolete) — Splinter Review
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.
(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)
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+
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
Depends on: 1125672
Blocks: 1137274
Depends on: 1159448
given the crashes in bug 1159448, we may want to make this a priority.
Severity: normal → major
Giving imap a shot in this bug.
Assignee: nobody → jesper
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)
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.
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-
Addressed the issues the best I could.
Attachment #8632349 - Attachment is obsolete: true
Attachment #8638539 - Flags: feedback?(Pidgeot18)
Flags: needinfo?(mkmelin+mozilla)
See Also: → 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]
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]
Refreshing the patch now.
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.
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)
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)
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.
Attachment #8833824 - Flags: review?(rkent)
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 :-(
Severity: major → blocker
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
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.
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)
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)
Whiteboard: [m-c sync][not strictly dependent on tests from bug 1208087][patchlove] → [Thunderbird-testfailure]
Whiteboard: [Thunderbird-testfailure] → [Thunderbird-testfailure: X all]
I discussed with Kent on IRC how this needs to be restructured.
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(Pidgeot18)
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.
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
IMAP part, refreshed after landing of bug 1335638.
Still need to remove MsgExamineForProxy().
Attachment #8833824 - Attachment is obsolete: true
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+
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.
(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.
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
That's starting to look good, one failed test:
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_listSubscribed.js
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)
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 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.
Flags: needinfo?(rkent)
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.
(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?
Magically, moving that one line makes the test pass: Yay!! \o/
Attachment #8837705 - Attachment is obsolete: true
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)
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)
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)
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: X all]
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
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?
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.
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.
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 ;-)
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)
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.
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+
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
(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.
(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#
(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?
(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.
Glad to see movement on this bug! (From the author of FoxyProxy)
Depends on: 1367707
Depends on: 1385375
You need to log in before you can comment on or make changes to this bug.