791457 - MailNews calls synchronous nsIProtocolProxyService::Resolve which doesn't exist any more since Bug 769764 removed it.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Philip Chee]

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.

Comment 1

[Philip Chee]

Attachment: WIP1 Awful Hack to get builds working again

This builds but I haven't tested this on a proxy connection.

Comment 2

[[e-mail]]

Comment on attachment 661510 [details] [diff] [review]
WIP1 Awful Hack to get builds working again

>+  nsCOMPtr<nsIProtocolProxyService> pps;
>+  nsCOMPtr<nsIProtocolProxyService2> pps2;
No point having both. Just write
nsCOMPtr<nsIProtocolProxyService2> pps2 =
  do_GetService("@mozilla.org/network/protocol-proxy-service;1");

Comment 3

[[e-mail]]

Attachment: Hack

Comment 4

[Joshua Cranmer [:jcranmer]]

Comment on attachment 661539 [details] [diff] [review]
Hack

Looking briefly at bug 769764 suggests that we have some package-manifest/removed-files changes necessary as well.

Comment 5 on that bug also scares me slightly:
2] nsiioservice::newchannel has to remain synchronous - there are far too many callers. So instead of having nsIIOService figure out the proxy for the channel, change to having the channels themselves work it out after ::asyncopen(). For HTTP this involves rearranging a little of the earliest code that relied on the connection info, but its not a big deal. For Websockets it was pretty easy, the IO service just needs to tell the unproxied channel what flags newChannel was called with so that the proxy lookup can be done correctly later on.. the most disruptive change is probably to FTP.. if FTP finds a SOCKS proxy then it can just install it after asyncopen() in the anticipated way, but if it finds a HTTP proxy that it will use it basically needs to throw away the FTP handler and instantiate an HTTP one instead. That was a lot easier when it was done in NewChannel(), but I implemented it with a redirect and that works very well too.

This means that we very well could have to modify our major channel implementations to account for proxy settings as well...

Comment 5

[Rain [:rain1] (inactive)]

Comment on attachment 661539 [details] [diff] [review]
Hack

Looking at the source, it seems like DeprecatedBlockingResolve does precisely what Resolve used to do. I think this patch is fine if the removed-files and package-manifest changes are added to both Tb and SM.

Comment 6

[Rain [:rain1] (inactive)]

(I mean we'll eventually need to get around to fixing point 2 jcranmer mentioned, but that can be done when we switch over to async proxy resolution.)

Comment 7

[[e-mail]]

(In reply to Joshua Cranmer from comment #4)
> This means that we very well could have to modify our major channel
> implementations to account for proxy settings as well...
Do we support IMAP/POP/SMTP over an HTTP proxy?

Comment 8

[Rain [:rain1] (inactive)]

We don't.

Comment 9

[[e-mail]]

Attachment: Addressed review comments

Comment 10

[[e-mail]]

Attachment: Addressed review comments

Comment 11

[[e-mail]]

Attachment: Correct patch

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/b1bafd9591ab

I'll re-open the tree if everything goes green. Thanks, Neil!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Looks like there's still some issues. Not sure if it's still this bug or something else that piled on in the mean time. Please re-open if there's more to do here.

Comment 14

[:aceman]

Comment on attachment 661510 [details] [diff] [review]
WIP1 Awful Hack to get builds working again

I assume this one is not needed now.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Bug 769764 was backed out of m-c due to suspicion of causing intermittent leaks on OSX. That backout obviously re-broke c-c. I've backed this out from c-c for now.
https://hg.mozilla.org/comm-central/rev/3fa8e609454a

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> This means that we very well could have to modify our major channel
> implementations to account for proxy settings as well...

No, why would you have to account for proxy settings?

Comment 17

[Mark Banner (:standard8)]

Re-landed: https://hg.mozilla.org/comm-central/rev/9991035a6713

Comment 18

[Joshua Cranmer [:jcranmer]]

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #16)
> (In reply to Joshua Cranmer [:jcranmer] from comment #4)
> > This means that we very well could have to modify our major channel
> > implementations to account for proxy settings as well...
> 
> No, why would you have to account for proxy settings?

I don't pretend to understand the implementation of our proxy code, especially how it works in our mailnews channels. From speed-reading the async proxy cache, it seems possible that we would have to add states to our state machines to account for async proxy eventually; this is what I meant.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Oh, I see, I didn't understand your comment before. You're right, that is likely the case.