1125372 - Fix asyncResolve and asyncResolve2 to allow both passing a channel and passing a URI (unbreak various add-ons like chatzilla, foxyproxy, flagfox)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[:Gijs (he/him)]

Because proxies, apparently!

Comment 1

[Arthur Edelstein [:arthur]]

Attachment: Bug-1125372.patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8554019 [details] [diff] [review]
Bug-1125372.patch

Review of attachment 8554019 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much for the immediate patch!

Unfortunately we'll need to stay compatible with current stable and/or ESR for a bit, so it's not quite this simple... we'll need to detect the change having happened. I guess that the following would work:

var channelOrURI;
if (Components.interfaces.nsIProtocolProxyChannelFilter)
    channelOrURI = ios.newChannel(uri, null, null);
else
    channelOrURI = ios.newURI(uri, null, null);

and then pass/name channelOrURI instead of uri in the remainder. Is that right?

Comment 3

[Peja Stija]

Most probably this bug is also rendering the proxy addon "FoxyProxy" non-working.
It doesn't proxify anything anymore since the 20150122 nightly.

Comment 4

[:Gijs (he/him)]

(In reply to Peja Stija from comment #3)
> Most probably this bug is also rendering the proxy addon "FoxyProxy"
> non-working.
> It doesn't proxify anything anymore since the 20150122 nightly.

Please file a separate bug for this.

Comment 5

[:Gijs (he/him)]

(note also that you changed the variable name of the argument to 'channel', but not the variable definition...)

Comment 6

[:Gijs (he/him)]

Attachment: use nsISupports as first param of asyncResolve instead of switching wholesale from nsIURI to nsIChannel, for improved add-on compat,

Comment 7

[Dave Garrett]

Thanks for writing a patch for this quickly after issues were reported. This looks like the ideal route to fix.

Comment 8

[Arthur Edelstein [:arthur]]

(In reply to :Gijs Kruitbosch from comment #6)
> Created attachment 8554548 [details] [diff] [review]
> use nsISupports as first param of asyncResolve instead of switching
> wholesale from nsIURI to nsIChannel, for improved add-on compat,

By using nsISupports we lose the safety of compile-time type checking in C++. I wonder if we could overload asyncResolve instead, to have one signature with nsIURI and another with nsIChannel?

Comment 9

[:Gijs (he/him)]

(In reply to Arthur Edelstein from comment #8)
> (In reply to :Gijs Kruitbosch from comment #6)
> > Created attachment 8554548 [details] [diff] [review]
> > use nsISupports as first param of asyncResolve instead of switching
> > wholesale from nsIURI to nsIChannel, for improved add-on compat,
> 
> By using nsISupports we lose the safety of compile-time type checking in
> C++.

Yes, but consumers of asyncResolve should in principle be checking rv anyway, as it can return non-NS_OK, and the method won't crash and burn - it'll just result in an error code. As noted earlier, the alternative would be having two methods (or 4, rather, because asyncResolve2... :-\ ). To distinguish the alternatives.

> I wonder if we could overload asyncResolve instead, to have one
> signature with nsIURI and another with nsIChannel?

I'm pretty sure that's not allowed in Mozilla XPIDL, because xpconnect wouldn't know which method to call for JS callers. If it *were* allowed, I have no idea why e.g. http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIIOService.idl#16 uses a whole bundle of newURI.*-named methods that all take different arguments with different names, rather than all calling them newURI (or newURIFromChannel, in some cases).

Comment 10

[Patrick McManus [:mcmanus]]

I was undecided on whether or not to mark this as WONTFIX.

I don't break interfaces lightly, but interfaces can be broken if the tradeoff is worthwhile - I'm comfortable with that. Its a necessary part of avoiding stagnation.

The whole point of the channel-not-uri patch is to make more information available to the proxy selection service. Certainly forcing everyone to update will help with that - and channels are really the networking primitive.

On the other hand, perhaps most of the work is done by having the core gecko code ported over to it already and fixing most of the additional loose ends (which are currently breakage) will not create meaningful channels anyhow.

As for the 3 addons mentioned here.

* chatzilla has an nsiChannel used by ircprotocolhandler. It seems easy to make that change and we know there are several awkward ways to autodetect the version of the interface required.

* foxyproxy's maintainers were an advocate of the gecko change - they want the additional information a channel provides over a URI so I expect them to make the update without a problem even if this compatibility patch is accepted.

* flagfox is a bit harder. It is an example of an algorithm that is already broken and will be more broken with this change. Yet it probably falls in the "works well enough" category. It hooks the location bar change and does the proxy lookup from that uri - completely independent of the proxy lookup the network stack is doing. Thanks to PAC that uri->proxy mapping might not be constant already.. one would assume that using two different channels will make it worse. Using just the real channel is doable, but admittedly pretty hard compared to the location bar work flow that is implemented now. I expect that any realistic update to flagfox would just create an empty channel like this patch already does - so no new information would be added. It would be nice if something like resource timing gave ip addressing information that was used for the resource - but that's a whole separate endeavor.

Based on that, let's go ahead with the compat patch. I would like a few changes though - I'll mark them in the review.

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8554548 [details] [diff] [review]
use nsISupports as first param of asyncResolve instead of switching wholesale from nsIURI to nsIChannel, for improved add-on compat,

Review of attachment 8554548 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsIProtocolProxyService.idl
@@ +70,5 @@
>       *
> +     * @param aChannelOrURI
> +     *        The channel for which a proxy is to be found, or, if no channel is
> +     *        available, a URI indicating the same. This method will return
> +     *        NS_ERROR_NOINTERFACE if this argument isn't either a URI or a channel.

either a nsIURI or nsIChannel

::: netwerk/base/nsIProtocolProxyService2.idl
@@ +13,1 @@
>  interface nsIProtocolProxyService2 : nsIProtocolProxyService

let's keep this one channel only.

@@ +18,5 @@
>     */
>    void reloadPAC();
>  
>      /**
>       * This method is identical to asyncResolve() except it may execute the

document that it is different in that it is channel only

::: netwerk/base/nsProtocolProxyService.cpp
@@ +1219,5 @@
>      nsCOMPtr<nsIURI> uri;
> +    nsCOMPtr<nsIChannel> channel = do_QueryInterface(channelOrURI);
> +    if (!channel) {
> +        uri = do_QueryInterface(channelOrURI);
> +        if (!uri)

braces please

@@ +1223,5 @@
> +        if (!uri)
> +            return NS_ERROR_NO_INTERFACE;
> +
> +        // make a temporary channel from the argument url
> +        nsCOMPtr<nsIIOService> ios(do_GetIOService(&rv));

nullcheck ios

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +1872,5 @@
>      nsCOMPtr<nsIProtocolProxyService> pps =
>          do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID);
>  
>      if (pps && !mChannel->ProxyInfo()) {
> +        pps->AsyncResolve(static_cast<nsIChannel*>(mChannel), 0, this,

change it to asyncresolve2

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8554548 [details] [diff] [review]
use nsISupports as first param of asyncResolve instead of switching wholesale from nsIURI to nsIChannel, for improved add-on compat,

Review of attachment 8554548 [details] [diff] [review]:
-----------------------------------------------------------------

please also add a simple nsiuri test to test_protocolproxyservice

Comment 13

[:Gijs (he/him)]

Attachment: use nsISupports as first param of asyncResolve instead of switching wholesale from nsIURI to nsIChannel, for improved add-on compat,

I'm going by the rv for the getService call rather than null-checking ios, hope that's OK. Also, you asked to make the FTP channel code use asyncResolve2. That's done, but you didn't mention the HTTP changes, so I've left those - do you want those to use the _2 version of the method, too? I added a test, which passes on my machine... can you verify that it is sane? (it's basically a copy of the channel version, just passing uris rather than channels)

Comment 14

[:Gijs (he/him)]

Comment on attachment 8555375 [details] [diff] [review]
use nsISupports as first param of asyncResolve instead of switching wholesale from nsIURI to nsIChannel, for improved add-on compat,

Review of attachment 8555375 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2035,5 @@
>      // optimization, but if an add-on has only provided the original interface
>      // then it is ok to use that version.
>      nsCOMPtr<nsIProtocolProxyService2> pps2 = do_QueryInterface(pps);
>      if (pps2) {
> +        rv = pps2->AsyncResolve2(static_cast<nsIChannel*>(this), mProxyResolveFlags,

Egh, nevermind the question about the http case here - I see now that this is conditional on getting a pps2 - however, the static_cast is unnecessary for the asyncresolve2 case now.

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8555375 [details] [diff] [review]
use nsISupports as first param of asyncResolve instead of switching wholesale from nsIURI to nsIChannel, for improved add-on compat,

Review of attachment 8555375 [details] [diff] [review]:
-----------------------------------------------------------------

r=mcmanus .. you'll have to undo the ftp channel bits from my last review. my mistake.

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +1873,4 @@
>          do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID);
>  
>      if (pps && !mChannel->ProxyInfo()) {
> +        pps->AsyncResolve2(mChannel, 0, this,

my advice here was wrong in the first review. This would have to be conditional the way it is in httpchannel.. don't do that, just leave it as the not-2 version. sorry.

Comment 16

[:Gijs (he/him)]

Attachment: Patch for checkin

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=67a6f307b9ab

Comment 17

[Dave Garrett]

(In reply to Patrick McManus [:mcmanus] from comment #10)
> * flagfox is a bit harder. It is an example of an algorithm that is already
> broken and will be more broken with this change. Yet it probably falls in
> the "works well enough" category.

Pretty much. The only reason I touch the proxy API at all is because DNS lookups
still send network requests when proxy settings say it shouldn't. (the DNS leaks)
If nsIDNSService just reported an error in this instance I wouldn't need this at all.
It's a little strange that API consumers are expected to enforce this. For example,
any addon that uses DNS can leak a DNS request when it shouldn't, which can be
a privacy leak when used in the Tor Browser, with FoxyProxy, or with the
"network.proxy.socks_remote_dns" pref set and a regular SOCKS proxy.
(the current check is based on discussion with the FoxyProxy developer)

> It would be nice if something like resource timing gave ip addressing
> information that was used for the resource - but that's a whole separate
> endeavor.

Indeed. There are edge cases where this simple route is not ideal. A simple
way to fetch the IP used to fetch a resource would be preferable. Definitely a
different topic, though.

Comment 18

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/feea863c5f24

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/feea863c5f24

Comment 21

[Arthur Edelstein [:arthur]]

Gijs and Patrick -- thanks for your efforts here, and sorry my patch caused so much extra work for you.

Comment 22

[Peja Stija]

Even after incorporating this bugfix, FoxyProxy remains broken.

Comment 23

[Jesper Hansen]

We're fixing another way in FoxyProxy as this bug didn't work in the way I had expected it would