Closed Bug 1357997 Opened 3 years ago Closed 2 years ago

Replace url.openConnection with ProxySelector.openConnectionWithProxy

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jhao, Assigned: jhao)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor-mobile])

Attachments

(1 file)

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
Assignee: nobody → jhao
Priority: -- → P1
Whiteboard: [tor-mobile]
Attachment #8861283 - Flags: review?(rnewman)
Hi Richard,

Would you please review this patch?

Thanks.
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?
Attachment #8861283 - Flags: review?(rnewman) → review?(s.kaspari)
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.
Attachment #8861283 - Flags: review?(s.kaspari)
Comment on attachment 8861283 [details]
Bug 1357997 - Use ProxySelector.openConnection instead of url.openConnection.

https://reviewboard.mozilla.org/r/133234/#review138396
Attachment #8861283 - Flags: review+
(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?
Flags: needinfo?(s.kaspari)
Some of our code (at least Sync) is using ch.boye.httpclientandroidlib (HttpClient).
Flags: needinfo?(s.kaspari)
Summary: Proxy all http connections in Fennec code → Replace url.openConnection with ProxySelector.openConnectionWithProxy
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/676720d93349
Use ProxySelector.openConnection instead of url.openConnection. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/676720d93349
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.