an http channel looks up the proxy information via protocolproxyservice->AsyncResolve(), which as we know needs to be async to prevent jank in the general case (especially on windows). This inherently means a callback that goes through the main thread event queue. On android that can add a lot of latency processing events in the work queue that we really should be processing while the request is out on the network.. commonly this slows things down by 25+ms, I've seen >50 without looking too hard. Conveniently on android the default configuration is to use no proxy and we actually know that without going to another thread (unlike windows).. so there is no reason to take the round trip there. This patch adds a AsyncResolve method to the already existing nsIProtocolProxyService2.idl .. if version of the interface is invoked it signals that the caller is willing to have the callback exectued re-entrantly on the stack of AsyncResolve(). (it of course doesn't require that it happen that way, that would be janktastic.) I did that rather than changing the semantics of the existing AsyncResolve out of fear of breaking add-on code in too subtle of a way. (even with an iid bump, if it compiled people wouldn't read the fine print.)

Is bug 887767 just a dupe?

(In reply to Honza Bambas (:mayhemer) from comment #2) > Is bug 887767 just a dupe? sort of.. but we certainly don't need them both. I hope you don't mind that I dup'd them in reverse.. just because there was already a r? and attachment here.

Comment on attachment 768564 [details] [diff] [review] allow nsiprotocolproxyservice::asyncresolve() to be called re-entrantly Review of attachment 768564 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsProtocolProxyService.cpp @@ +1119,5 @@ > // we can do it locally > ApplyFilters(uri, info, pi); > ctx->SetResult(NS_OK, pi); > + if (isSyncOK) { > + ctx->Run(); your new IDL function promises *result would be null, so I'd suggest setting it to that here in case client passes in address to non-nulled memory. @@ +1125,5 @@ > + } > + > + rv = ctx->DispatchCallback(); > + if (NS_SUCCEEDED(rv)) > + ctx.forget(result); So this is different from the existing code. Looks like we were failing to set *result in the existing non-PACMan async path here, and you've fixed it? ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1780,5 @@ > + if (pps2) { > + rv = pps2->AsyncResolve2(mProxyURI ? mProxyURI : mURI, mProxyResolveFlags, > + this, getter_AddRefs(mProxyRequest)); > + } > + else { I personally loathe the extra carriage return style, vs just "} else {". But you're the module owner and thus head code stylist :) The file has a mix of both, so...

(In reply to Patrick McManus [:mcmanus] from comment #4) > sort of.. but we certainly don't need them both. I hope you don't mind that > I dup'd them in reverse.. just because there was already a r? and attachment > here. Sure, glad this is fixed :)

(In reply to Jason Duell (:jduell) from comment #5) > Comment on attachment 768564 [details] [diff] [review] > > So this is different from the existing code. Looks like we were failing to > set *result in the existing non-PACMan async path here, and you've fixed it? > yes; thanks!

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

Turns out (surprisingly) that this fixes a DNS leak introduced with 769764. You can reproduce it with a local proxy (using a remote one is fine) and a FTP request. Thus, could we please get this backported to aurora and beta?

Georg, this likely doesn't fix a leak. it might bypass code that does leak under some conditions, but probably not all of them. can you file a bug on how to repro the leak?

(In reply to Patrick McManus [:mcmanus] from comment #11) > Georg, this likely doesn't fix a leak. it might bypass code that does leak > under some conditions, but probably not all of them. That's why the surprise! Makes sense... > can you file a bug on how to repro the leak? Done: https://bugzilla.mozilla.org/show_bug.cgi?id=896206