Last Comment Bug 791457 - MailNews calls synchronous nsIProtocolProxyService::Resolve which doesn't exist any more since Bug 769764 removed it.
: MailNews calls synchronous nsIProtocolProxyService::Resolve which doesn't exi...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: unspecified
: All All
: -- critical (vote)
: Thunderbird 18.0
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 791645 769764
  Show dependency treegraph
 
Reported: 2012-09-15 06:20 PDT by Philip Chee
Modified: 2012-09-28 15:14 PDT (History)
16 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP1 Awful Hack to get builds working again (4.72 KB, patch)
2012-09-15 10:27 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Hack (4.05 KB, patch)
2012-09-15 16:30 PDT, neil@parkwaycc.co.uk
sid.bugzilla: review+
Pidgeot18: feedback-
Details | Diff | Splinter Review
Addressed review comments (8.05 KB, patch)
2012-09-16 14:13 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Addressed review comments (9.97 KB, patch)
2012-09-16 14:39 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Correct patch (9.10 KB, patch)
2012-09-16 14:42 PDT, neil@parkwaycc.co.uk
sid.bugzilla: review+
Details | Diff | Splinter Review

Description Philip Chee 2012-09-15 06:20:51 PDT
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 2012-09-15 10:27:43 PDT
Created attachment 661510 [details] [diff] [review]
WIP1 Awful Hack to get builds working again

This builds but I haven't tested this on a proxy connection.
Comment 2 neil@parkwaycc.co.uk 2012-09-15 12:37:31 PDT
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 neil@parkwaycc.co.uk 2012-09-15 16:30:13 PDT
Created attachment 661539 [details] [diff] [review]
Hack
Comment 4 Joshua Cranmer [:jcranmer] 2012-09-15 17:14:30 PDT
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 Siddharth Agarwal [:sid0] (inactive) 2012-09-15 19:31:55 PDT
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 Siddharth Agarwal [:sid0] (inactive) 2012-09-15 19:39:33 PDT
(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 neil@parkwaycc.co.uk 2012-09-16 02:28:52 PDT
(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 Siddharth Agarwal [:sid0] (inactive) 2012-09-16 04:12:44 PDT
We don't.
Comment 9 neil@parkwaycc.co.uk 2012-09-16 14:13:59 PDT
Created attachment 661632 [details] [diff] [review]
Addressed review comments
Comment 10 neil@parkwaycc.co.uk 2012-09-16 14:39:06 PDT
Created attachment 661633 [details] [diff] [review]
Addressed review comments
Comment 11 neil@parkwaycc.co.uk 2012-09-16 14:42:59 PDT
Created attachment 661635 [details] [diff] [review]
Correct patch
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-09-16 16:12:08 PDT
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] 2012-09-16 17:59:19 PDT
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 2012-09-16 23:49:42 PDT
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] 2012-09-17 17:28:57 PDT
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) 2012-09-18 11:34:53 PDT
(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) 2012-09-28 01:32:01 PDT
Re-landed: https://hg.mozilla.org/comm-central/rev/9991035a6713
Comment 18 Joshua Cranmer [:jcranmer] 2012-09-28 09:03:28 PDT
(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) 2012-09-28 15:14:29 PDT
Oh, I see, I didn't understand your comment before. You're right, that is likely the case.

Note You need to log in before you can comment on or make changes to this bug.