1357997 - Replace url.openConnection with ProxySelector.openConnectionWithProxy

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Jonathan Hao (inactive) [:jhao]]

We need to make sure all http connections in Fennec goes through a centralized proxy.

The connections don't have to go through Necko as argued in bug 507641.  We could do it like this commit: https://github.com/guardianproject/tor-browser/commit/4b68807942c7bce90760ea1f9450f6b4ab9e8db9

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1357997 - Use ProxySelector.openConnection instead of url.openConnection.

Review commit: https://reviewboard.mozilla.org/r/133234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133234/

Comment 2

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8861283 [details]
Bug 1357997 - Use ProxySelector.openConnection instead of url.openConnection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133234/diff/1-2/

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8861283 [details]
Bug 1357997 - Use ProxySelector.openConnection instead of url.openConnection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133234/diff/2-3/

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Hi Richard,

Would you please review this patch?

Thanks.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8861283 [details]
Bug 1357997 - Use ProxySelector.openConnection instead of url.openConnection.

This looks fine to me, but Sebastian has more recent context than me.

I'm also confused by omg.util.ProxySelector, even though I reviewed the patch that added `openConnectionWithProxy`!

`openConnectionWithProxy` uses `java.net.ProxySelector`, not `omg.util.ProxySelector`.

Callers through `GeckoAppShell` that use `ProxySelector().select` -- Gecko callers trying to use `getProxyForURI` -- will use our code.

Java callers that use `omg.util.ProxySelector.openConnectionWithProxy` will use the system `ProxySelector`!

I expect the intent is that these two classes behave identically, reading system properties, but I don't see how we could guarantee that. Sebastian, what are your thoughts?

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8861283 [details]
Bug 1357997 - Use ProxySelector.openConnection instead of url.openConnection.

https://reviewboard.mozilla.org/r/133234/#review138390

Sorry, this took so long. The code changes look good to me.

Our helper openConnectionWithProxy() indeed only cares about the system proxy and doesn't use whatever is setup in Gecko (There's no UI for that, but I guess add-ons could set one). Let's file a follow-up bug if we want to add support for this.

Also note that we still have some code that does not use HttpUrlConnection (yet?). So even after this lands we do not proxy *all* connections yet.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8861283 [details]
Bug 1357997 - Use ProxySelector.openConnection instead of url.openConnection.

https://reviewboard.mozilla.org/r/133234/#review138396

Comment 8

[Jonathan Hao (inactive) [:jhao]]

(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> Comment on attachment 8861283 [details]
> Bug 1357997 - Use ProxySelector.openConnection instead of url.openConnection.
> 
> https://reviewboard.mozilla.org/r/133234/#review138390
> 
> Sorry, this took so long. The code changes look good to me.
> 
> Our helper openConnectionWithProxy() indeed only cares about the system
> proxy and doesn't use whatever is setup in Gecko (There's no UI for that,
> but I guess add-ons could set one). Let's file a follow-up bug if we want to
> add support for this.
> 
> Also note that we still have some code that does not use HttpUrlConnection
> (yet?). So even after this lands we do not proxy *all* connections yet.

Thanks, Sebastian.  So what connections do we have beside HttpUrlConnection?

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Some of our code (at least Sync) is using ch.boye.httpclientandroidlib (HttpClient).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/676720d93349
Use ProxySelector.openConnection instead of url.openConnection. r=sebastian

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/676720d93349