*** DO NOT FREEZE nsIProtocolProxyFilter *** The signature of nsIProtocolProxyFilter::applyFilter(nsIProtocolProxyService, nsIURI, nsIProxyInfo) should be changed so that applyFilter() implementations are handed a channel (or a request in the case where there is no channel). Something like: nsIProxyInfo applyFilter(nsIChannel, nsIRequest, nsIProxyInfo) Note I've also removed the first argument, nsIProtocolProxyService, because a reference to that service can easily be obtained with Cc["@mozilla.org/network/protocol-proxy-service;1"].getService(Ci.nsIProtocolProxyService) by the object implementing nsIProtocolProxyFilter. I've no objection to keeping that argument, but it seems strange to me that it's there. [29-05-2008 14:59] <ericjung> timelyx: but applyFilter() isn't given a channel [29-05-2008 15:00] <timelyx> it's given a url?!? [29-05-2008 15:00] <ericjung> timelyx: yep [29-05-2008 15:00] <ericjung> timelyx: that's the problem [29-05-2008 15:03] <ericjung> timelyx: nsIProtocolProxyFilter.idl isn't frozen :) [29-05-2008 15:03] <timelyx> file a bug [29-05-2008 15:03] <timelyx> it should have a channel [29-05-2008 15:03] <ericjung> timelyx: right away, sir [29-05-2008 15:03] <timelyx> or perhaps a request [29-05-2008 15:03] <timelyx> in case there isn't a channel [29-05-2008 15:04] <timelyx> not sure exactly what [29-05-2008 15:04] <timelyx> but make sure it's very clear that it can't go freezing :)

One use case for this is for the FoxyProxy extension (author: me) to provide different proxies by tab or browser. From the channel handed to applyFilter, the callee can get the load group and from that the tab--if one exists for the channel.

Seems this is a flaw in the Firefox architecture. After 16 months it is still not fixed?

(In reply to comment #2) > Seems this is a flaw in the Firefox architecture. > After 16 months it is still not fixed? Perhaps you can propose a patch?

(In reply to comment #3) > (In reply to comment #2) > > Seems this is a flaw in the Firefox architecture. > > After 16 months it is still not fixed? > > Perhaps you can propose a patch? Unfortunately no. But from what you wrote it *seems* that this could be easily resolved/fixed by the Mozilla developers. Correct me if I'm wrong. Please don't take my former comment as an offence, it should be more an offence to the *paid*(!) Firefox developers.

(In reply to comment #4 > Unfortunately no. > But from what you wrote it *seems* that this could be easily resolved/fixed by > the Mozilla developers. Correct me if I'm wrong. OK, you're wrong :) I looked very closely at the source code, and it does require significant changes.

Would like to add my desire for this change as well. I'm writing an extension where it would be extremely useful to get the referrer of the request. Can't do this with just a URI.

Voting because this is quite important for the Torbutton add-on: https://trac.torproject.org/projects/tor/ticket/986

(In reply to comment #7) > Voting because this is quite important for the Torbutton add-on: > https://trac.torproject.org/projects/tor/ticket/986 FWIW, it was "quite important" to the FoxyProxy add-on long before Torbutton even existed.

I am also writing an Add-On which needs this to be more secure and not confuse the user. (Also assigning Proxys per-Window and not per-URI) nsIObserver callbacks can be queried for nsIChannel, nsIWebProgressListener callbacks can be queried for nsIChannel, but nsIProtocolProxyFilter callbacks not :-( will this ever be fixed?

If this were to be fixed, it would require changing nsIProtocolProxyService::AsyncResolve to take a channel instead of an nsIURI, and presumably nsIProtocolProxyService::Resolve as well. From then on we could presumably just propogate that channel to the various places that want it. Christian, is this something we want to do? Then again, looking at callers like http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1557, it looks like we could just query the channel from the callback object. That would be even easier.

Granted, for the synchronous resolve case there is no callback object, so that's not a catch-all solution.

What I ended up discovering with this proof-of-concept patch is that apparently not a single caller of synchronous resolve has access to a channel, as far as I can tell. They all seem to be checking whether a URL can be proxied, and lots of those checks happen in channel creation functions before a channel has been created. Furthermore, the only user of AsyncResolve is the HTTP channel code (and tests).

(In reply to Josh Matthews [:jdm] (away until Sept 13) from comment #13) > What I ended up discovering with this proof-of-concept patch is that > apparently not a single caller of synchronous resolve has access to a > channel, as far as I can tell. They all seem to be checking whether a URL > can be proxied, and lots of those checks happen in channel creation > functions before a channel has been created. Furthermore, the only user of > AsyncResolve is the HTTP channel code (and tests). Sounds disappointing :)

Here's a new patch that creates an additional interface, nsIProtocolProxyChannelFilter.idl. This strategy supports both the existing nsIProtocolProxyFilter using and the new nsIProtocolProxyChannelFilter.

Comment on attachment 8496315 [details] [diff] [review] 0001-Bug-436344-Allow-filtering-of-proxies-by-channel.patch Review of attachment 8496315 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsIProtocolProxyService2.idl @@ +25,4 @@ > * > * No documentation - it is deprecated! > **/ > + nsIProxyInfo deprecatedBlockingResolve(in nsIChannel aChannel, in unsigned long aFlags); rather than changing the deprecated public interface, can you remove it and fuss with the linking so that nsPluginHost can still use it, but not expose it in a idl - https://bugzilla.mozilla.org/show_bug.cgi?id=778201#c6 ? ::: netwerk/base/public/nsISpeculativeConnect.idl @@ +24,5 @@ > * @param aCallbacks any security callbacks for use with SSL for interfaces > * such as nsIBadCertListener. May be null. > * > */ > + void speculativeConnect(in nsIChannel aChannel, this bit seems wrong. We speculatively connect to a uri (well, really a host and port) - not a channel. can we avoid changing this interface?

Thanks for the review. Both suggested fixes made in this new version of the patch.

Updating attachment -- previous version was missing a created file.

Comment on attachment 8507245 [details] [diff] [review] 0001-Bug-436344-Allow-filtering-of-proxies-by-channel.patch Review of attachment 8507245 [details] [diff] [review]: ----------------------------------------------------------------- Arthur- thanks for taking this on.. i'm only r-'ing it because you need to figure out the websocket interaction bit.. the rest is trivial ::: dom/plugins/base/moz.build @@ +101,4 @@ > '/dom/base', > '/layout/generic', > '/layout/xul', > + '/netwerk/base/src', ask josh aas to r? the plugin bits.. lgtm ::: dom/plugins/base/nsPluginHost.cpp @@ -612,5 @@ > return res; > > nsCOMPtr<nsIProxyInfo> pi; > > - // Remove this with bug 778201 probably want to maintain a comment that this is a sucky thing to do ::: netwerk/base/public/nsIProtocolProxyChannelFilter.idl @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ let's cutdown on the file proliferation here and just add this interface to the nsiprotocolproxyfilter.idl file ::: netwerk/base/src/nsIOService.cpp @@ +1248,5 @@ > return NS_OK; > } > > + nsCOMPtr<nsIURI> uri; > + channel->GetURI(getter_AddRefs(uri)); check return code @@ +1286,5 @@ > return rv; > > + nsCOMPtr<nsIChannel> channel; > + rv = NewChannelFromURI(aURI, getter_AddRefs(channel)); > + NS_ENSURE_SUCCESS(rv, rv); if (NS_FAILED(rv)) { return rv; } [preferred for new code as it doesn't hide flow control] ::: netwerk/base/src/nsPACMan.cpp @@ +340,5 @@ > TimeStamp::Now() > mScheduledReload) > LoadPACFromURI(EmptyCString()); > > + nsCOMPtr<nsIURI> uri; > + channel->GetURI(getter_AddRefs(uri)); check return code ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1829,4 @@ > // then it is ok to use that version. > nsCOMPtr<nsIProtocolProxyService2> pps2 = do_QueryInterface(pps); > if (pps2) { > + rv = pps2->AsyncResolve2(this, mProxyResolveFlags, so this breaks websockets @@ -1829,4 @@ > // then it is ok to use that version. > nsCOMPtr<nsIProtocolProxyService2> pps2 = do_QueryInterface(pps); > if (pps2) { > - rv = pps2->AsyncResolve2(mProxyURI ? mProxyURI : mURI, mProxyResolveFlags, so this breaks websockets.. iirc, basically websockets has to be able to say "I want a http channel to https://foo.com/ws that I will bootstrap websockets from using http and 101, but when you lookup a proxy for that use wss://foo.com/ws".. by reducing this just to the https:// uri you've lost that bit of information.

Thanks again for the review. I think I've fixed everything mentioned so far. In the next day or two I'll be attaching a patch with some fixes to unit tests.

Hi Josh, Patrick suggested I ask you to review the changes to dom/plugins/base/nsPluginHost.cpp in my patch here. Thanks in advance.

(In reply to Arthur Edelstein from comment #20) > Created attachment 8508532 [details] [diff] [review] > 0001-Bug-436344-Allow-filtering-of-proxies-by-channel.patch > > Thanks again for the review. I think I've fixed everything mentioned so far. > In the next day or two I'll be attaching a patch with some fixes to unit > tests. great.. make sure to test the new channel based filter api too. also if you've got l1 access for a try run please include that. thanks!

Comment on attachment 8508532 [details] [diff] [review] 0001-Bug-436344-Allow-filtering-of-proxies-by-channel.patch Review of attachment 8508532 [details] [diff] [review]: ----------------------------------------------------------------- i'm just going to drop the review flag here until the tests are included.. but it was promising based on the last round

Comment on attachment 8508532 [details] [diff] [review] 0001-Bug-436344-Allow-filtering-of-proxies-by-channel.patch Review of attachment 8508532 [details] [diff] [review]: ----------------------------------------------------------------- Plugin changes look fine to me.

I haven't posted a final version because I ran into a tricky unit test failure in `dom/workers/test/test_websocket.html` . If anyone has a suggestion on how to fix this, it would be much appreciated. In any case I'll work on it more when I get some time.

I've fixed the last problem that caused the broken unit tests (posting patch). Here's the results from the try server: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69bee055aeb2

Comment on attachment 8540819 [details] [diff] [review] 0001-Bug-436344-Allow-filtering-of-proxies-by-channel.patch Review of attachment 8540819 [details] [diff] [review]: ----------------------------------------------------------------- thanks for this arthur - sorry I was afk for the holiday and thus the delay. just a few pretty minor things left to sort out ::: netwerk/base/src/nsPACMan.cpp @@ +340,4 @@ > TimeStamp::Now() > mScheduledReload) > LoadPACFromURI(EmptyCString()); > > + nsCOMPtr<nsIURI> uri; this code is a repeat of the protocolproxyservices::getproxyuri() code, right? Let's just expose that somehow and call the same code if possible. ::: netwerk/base/src/nsProtocolProxyService.cpp @@ +71,5 @@ > > +// Return the channel's proxy URI, or if it doesn't exist, the > +// channel's main URI. > +static nsresult > +GetProxyURI(nsIChannel* channel, nsIURI** aURI) necko uses type *var, not type* var @@ +76,5 @@ > +{ > + nsresult rv; > + nsCOMPtr<nsIURI> uri; > + rv = channel->GetURI(getter_AddRefs(uri)); > + if (NS_FAILED(rv)) return rv; multi line with braces please.. there are a couple other instances of this in the diff - please track them down @@ +77,5 @@ > + nsresult rv; > + nsCOMPtr<nsIURI> uri; > + rv = channel->GetURI(getter_AddRefs(uri)); > + if (NS_FAILED(rv)) return rv; > + nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel); httpchannel(do_QueryInterface(channel)) is a tiny bit more efficient. (there are lots of examples of doing it the "wrong way" admittedly. @@ +78,5 @@ > + nsCOMPtr<nsIURI> uri; > + rv = channel->GetURI(getter_AddRefs(uri)); > + if (NS_FAILED(rv)) return rv; > + nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel); > + nsCOMPtr<nsIURI> proxyURI = nullptr; skip the assignment.. a comptr is ctor'd to null @@ +79,5 @@ > + rv = channel->GetURI(getter_AddRefs(uri)); > + if (NS_FAILED(rv)) return rv; > + nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel); > + nsCOMPtr<nsIURI> proxyURI = nullptr; > + if (!!httpChannel) { just "if (httpchannel) pls @@ +83,5 @@ > + if (!!httpChannel) { > + rv = httpChannel->GetProxyURI(getter_AddRefs(proxyURI)); > + if (NS_FAILED(rv)) return rv; > + } > + NS_IF_ADDREF(*aURI = proxyURI ? proxyURI : uri); if (proxyURI) { proxyURI.forget(aURI); } else { uri.forget(aURI); } @@ +1391,3 @@ > > + FilterLink *link = new FilterLink(position, channelFilter); > + if (!link) braces on the conditional.. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1193,4 @@ > } > > NS_IMETHODIMP > +HttpBaseChannel::GetProxyURI(nsIURI** proxyURI) HttpBaseChannel::GetProxyURI(nsIURI **aOut) { NS_ENSURE_ARG_POINTER(aOut); nsCOMPtr<nsIURI> temp(mProxyURI); temp.forget(aOut); return NS_OK; } ::: netwerk/protocol/http/nsIHttpChannel.idl @@ +95,4 @@ > ACString getRequestHeader(in ACString aHeader); > > /** > + * Read the proxy URI, which, if non-null, will be used to resolve let's move this to nsIHttpChannelInternal and don't forget to bump uuid ::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp @@ +117,4 @@ > } > > NS_IMETHODIMP > +nsViewSourceChannel::GetProxyURI(nsIURI** proxyURI) type *var @@ +118,5 @@ > > NS_IMETHODIMP > +nsViewSourceChannel::GetProxyURI(nsIURI** proxyURI) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; I actually think this should be NS_OK.. null is the right value for this channel type

Thanks, Patrick! I think I have fixed all issues in your last review. I was able to refactor a little and get rid of changes to nsPACMan.* and nsViewSourceChannel.cpp. Try server results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31a0563b4aff

Comment on attachment 8546967 [details] [diff] [review] 0001-Bug-436344-Allow-filtering-of-proxies-by-channel.patch Review of attachment 8546967 [details] [diff] [review]: ----------------------------------------------------------------- sorry it took me a week to get to this. r=mcmanus with nits below changed thanks for the patch! ::: netwerk/base/src/nsProtocolProxyService.cpp @@ +1853,4 @@ > > for (FilterLink *iter = mFilters; iter; iter = iter->next) { > PruneProxyInfo(info, list); > + if (!!iter->filter) { iter->filter please @@ +1855,5 @@ > PruneProxyInfo(info, list); > + if (!!iter->filter) { > + nsCOMPtr<nsIURI> uri; > + rv = GetProxyURI(channel, getter_AddRefs(uri)); > + if (!!uri) { uri please @@ +1859,5 @@ > + if (!!uri) { > + rv = iter->filter->ApplyFilter(this, uri, *list, > + getter_AddRefs(result)); > + } > + } else if (!!iter->channelFilter) { iter->channelFilter please

Thank you, Patrick! :)

Patch with nits fixed.

Updating the patch again to take into account changes in mozilla-central.

Another slight fix to patch to make it compatible with mozilla-central. Try server results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0196e32fed3a

FYI, this broke Chatzilla. Per McManus, it appears it's the asyncResolve(uri... call in Chatzilla; it needs to provide a channel now.

This fix also broken the "FoxyProxy" addon, and perhaps all proxy related addons.

Comment on attachment 8552878 [details] [diff] [review] 0001-Bug-436344-Allow-filtering-of-proxies-by-channel.patch Review of attachment 8552878 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsProtocolProxyService.h @@ +42,4 @@ > NS_DECL_NSIPROTOCOLPROXYSERVICE > NS_DECL_NSIOBSERVER > > + NS_DECLARE_STATIC_IID_ACCESSOR(NS_PROTOCOL_PROXY_SERVICE_IMPL_CID) You provide an IID for the class, but you didn't add it to the QI implementation... is this intentional?

Is it too late to have this patch changed slightly so that the changed function signature either goes by a new name, or provides callers some way of detecting whether they're using the old or new flavour? It's quite tricky for us to maintain compatibility cleanly in bug 1125372.

Flagfox is also broken in latest nightly because of this. I don't see any way to actually detect this in addon code. Don't change ancient APIs without a way to detect which you're using. Near as I can tell, you're either expecting manual version checks or developers to not be able to support both. In the case of Flagfox, I have no channel or request, nor do I think I should have to make a fake one to use this API. All I need is to know if the TRANSPARENT_PROXY_RESOLVES_HOST flag is set on the proxy info object, so I don't leak DNS requests when users don't want them. Please allow nsIProtocolProxyService.asyncResolve to continue to accept an nsIURI in addition to an nsIChannel. Also, please mark bugs that change major APIs as "addon-compat" early, instead of a couple days after making the breaking change. needinfoing assignee. This bug probably needs backout and revision as it breaks quite a few major addons with no apparent method to deal with it properly.

(In reply to Dave Garrett from comment #40) > Also, please mark bugs that change major APIs as "addon-compat" early, > instead of a couple days after making the breaking change. To be clear, I'm adding the flag because I am an add-on developer affected by the change (ChatZilla, bug 1125372), which is certainly less than ideal.

Patrick, can you suggest what to do here? Can we put the new implementation under asyncResolveWithChannel or something, and mark the old version deprecated (and make it just do a channel creation like most of the other code changes in here do) ? That would solve the add-on compatibility issues here.

Hi all - This interface was intentionally changed- the idea is that the service can do proxy selection based on the channel with the new api. Its more of a benefit to the service than it is to the callers, in the same way that we ask many callers to provide principals now than we did in the past. If the callers just used a backwards compatible interface with just a uri that wouldn't be meaningfully providing more information to the service. doesn't chatzilla use channels?

(In reply to Patrick McManus [:mcmanus] from comment #43) > Hi all - > > This interface was intentionally changed- the idea is that the service can > do proxy selection based on the channel with the new api. Its more of a > benefit to the service than it is to the callers, in the same way that we > ask many callers to provide principals now than we did in the past. If the > callers just used a backwards compatible interface with just a uri that > wouldn't be meaningfully providing more information to the service. > > doesn't chatzilla use channels? It uses sockets and TCP, not HTTP, and at this point in the code, it doesn't have a channel, no (I forget if it gets to using channels later, but it doesn't seem important here). This really isn't about chatzilla specifically, though - it's about the fact that, from JS, it's impossible to detect which version of the API you're looking at, unless you could making a list of all the previous IIDs of the interface. In other words, you can't *know* what the first argument to the asyncResolve call is, if you have software (add-ons) that needs to work with both the uri and the channel variant. That's the most important thing to get fixed. The easiest way to do this is to rename the function, and if we're doing that, we might as well leave the old one and create the channel ourselves, and then pass it to the new thing.

FWIW, it's possible to infer which API it is by the existence or otherwise of the nsIProtocolProxyChannelFilter interface, but that seems like a pretty terrible crutch.

Thinking about this more, we can just use nsISupports here, I think. Putting together a patch right now.

(In reply to :Gijs Kruitbosch from comment #46) > Thinking about this more, we can just use nsISupports here, I think. Putting > together a patch right now. I was going down this path.. but wanted to try it out before replying. look forward to the patch

Patch in bug 1125372. Clearing needinfo here; let's use bug 1125372 to improve the add-on compat here so as not to confuse this bug even more.

(In reply to Peja Stija from comment #37) > This fix also broken the "FoxyProxy" addon, and perhaps all proxy related > addons. Fixed in FoxyProxy Standard 4.5.1 and FoxyProxy Basic 3.5.1

(In reply to Eric H. Jung from comment #49) > (In reply to Peja Stija from comment #37) > > This fix also broken the "FoxyProxy" addon, and perhaps all proxy related > > addons. > > Fixed in FoxyProxy Standard 4.5.1 and FoxyProxy Basic 3.5.1 Thanks a lot Eric !

Is it possible this broke the Java plugin as well? See bug 1165286