Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Thunderbird 18.0

Status

MailNews Core
Backend
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Philip Chee, Assigned: neil@parkwaycc.co.uk)

Tracking

unspecified
Thunderbird 18.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 769764
(Reporter)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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");
(Assignee)

Comment 3

5 years ago
Created attachment 661539 [details] [diff] [review]
Hack
Attachment #661539 - Flags: review?(sagarwal)
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...
Attachment #661539 - Flags: feedback-
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.
Attachment #661539 - Flags: review?(sagarwal) → review+
(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.)
(Assignee)

Comment 7

5 years ago
(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?
We don't.
(Assignee)

Comment 9

5 years ago
Created attachment 661632 [details] [diff] [review]
Addressed review comments
Attachment #661539 - Attachment is obsolete: true
Attachment #661632 - Flags: review?(sagarwal)
(Assignee)

Comment 10

5 years ago
Created attachment 661633 [details] [diff] [review]
Addressed review comments
Attachment #661632 - Attachment is obsolete: true
Attachment #661632 - Flags: review?(sagarwal)
Attachment #661633 - Flags: review?(sagarwal)
(Assignee)

Comment 11

5 years ago
Created attachment 661635 [details] [diff] [review]
Correct patch
Attachment #661633 - Attachment is obsolete: true
Attachment #661633 - Flags: review?(sagarwal)
Attachment #661635 - Flags: review?(sagarwal)
Attachment #661635 - Flags: review?(sagarwal) → review+
https://hg.mozilla.org/comm-central/rev/b1bafd9591ab

I'll re-open the tree if everything goes green. Thanks, Neil!
Assignee: nobody → neil
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
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

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

I assume this one is not needed now.
Attachment #661510 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Blocks: 791645
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 18.0 → ---
(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?
Re-landed: https://hg.mozilla.org/comm-central/rev/9991035a6713
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
(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.
Oh, I see, I didn't understand your comment before. You're right, that is likely the case.
You need to log in before you can comment on or make changes to this bug.