DownloadContentService: Implement proxy support

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: sebastian, Assigned: k.krish, Mentored)

Tracking

(Depends on 1 bug)

unspecified
Firefox 49
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.61 KB, patch
sebastian
: review+
Details | Diff | Splinter Review
Follow-up from bug 1197720.

Implement proxy support for DownloadContentService.
Richard: I filed this bug after your comment here: bug 1197720 comment 15. Do I need to manually configure a proxy to be used by HttpClient in some situations? Is there some code in sync I can look at?
Flags: needinfo?(rnewman)
We have some code to help:

java/org/mozilla/gecko/util/ProxySelector.java

but if you're using HttpClient, you're basically implementing Bug 942652. Not too hard, but it hasn't been done yet.
Depends on: 942652
Flags: needinfo?(rnewman)
Hey Krishna!

This is a first simple bug.

We are creating the HttpUrlConnection for the DownloadContentService here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java#146

And we have a helper method to add the proxies to an URLConnection here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/util/ProxySelector.java#31

You basically just need to connect the both parts. :)
Assignee: nobody → k.krish
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
Hey Sebastian,

I will look into the code and come up with the patch real soon :)
(Assignee)

Comment 5

3 years ago
Posted file Bug1209496.diff (obsolete) —
Hey Sebastian, Please review the patch I have submitted. Please let me know if anything is not correct.
Attachment #8747441 - Flags: review?(s.kaspari)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8747441 [details]
Bug1209496.diff

>diff --git a/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java b/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java
>--- a/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java
>+++ b/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java
>@@ -11,25 +11,27 @@ import android.util.Log;
> 
> import org.mozilla.gecko.AppConstants;
> import org.mozilla.gecko.background.nativecode.NativeCrypto;
> import org.mozilla.gecko.dlc.catalog.DownloadContent;
> import org.mozilla.gecko.dlc.catalog.DownloadContentCatalog;
> import org.mozilla.gecko.sync.Utils;
> import org.mozilla.gecko.util.HardwareUtils;
> import org.mozilla.gecko.util.IOUtils;
>+import org.mozilla.gecko.util.ProxySelector;
> 
> import java.io.BufferedInputStream;
> import java.io.File;
> import java.io.FileInputStream;
> import java.io.IOException;
> import java.io.InputStream;
> import java.net.HttpURLConnection;
> import java.net.MalformedURLException;
>-import java.net.URL;
>+import java.net.URI;
>+import java.net.URISyntaxException;
> 
> public abstract class BaseAction {
>     private static final String LOGTAG = "GeckoDLCBaseAction";
> 
>     /**
>      * Exception indicating a recoverable error has happened. Download of the content will be retried later.
>      */
>     /* package-private */ static class RecoverableDownloadContentException extends Exception {
>@@ -140,24 +142,26 @@ public abstract class BaseAction {
>             throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO, e);
>         } finally {
>             IOUtils.safeStreamClose(inputStream);
>         }
>     }
> 
>     protected HttpURLConnection buildHttpURLConnection(String url)
>             throws UnrecoverableDownloadContentException, IOException {
>-        // TODO: Implement proxy support (Bug 1209496)
>+        
>         try {
>             System.setProperty("http.keepAlive", "true");
> 
>-            HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection();
>+            HttpURLConnection connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(new URI(url));
>             connection.setRequestProperty("User-Agent", HardwareUtils.isTablet() ?
>                     AppConstants.USER_AGENT_FENNEC_TABLET :
>                     AppConstants.USER_AGENT_FENNEC_MOBILE);
>             connection.setRequestMethod("GET");
>             connection.setInstanceFollowRedirects(true);
>             return connection;
>         } catch (MalformedURLException e) {
>             throw new UnrecoverableDownloadContentException(e);
>+        } catch (URISyntaxException e) {
>+            throw new UnrecoverableDownloadContentException(e);
>         }
>     }
> }
Attachment #8747441 - Attachment description: Using ProxySelector Class for creating a HttpURLConnection → Bug1209496.diff
Comment on attachment 8747441 [details]
Bug1209496.diff

Review of attachment 8747441 [details]:
-----------------------------------------------------------------

This looks good.

Can you upload a patch with the nit fixed? I'll push it to the try server afterwards.

::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java
@@ +146,5 @@
>      }
>  
>      protected HttpURLConnection buildHttpURLConnection(String url)
>              throws UnrecoverableDownloadContentException, IOException {
> +        

nit: Remove this empty line. :)
Attachment #8747441 - Flags: review?(s.kaspari) → review+
(Assignee)

Updated

3 years ago
Attachment #8747441 - Attachment is obsolete: true
Attachment #8747441 - Attachment is patch: false
Attachment #8747441 - Flags: review+
(Assignee)

Comment 8

3 years ago
Posted patch Bug1209496Splinter Review
Updated code. Removed the empty line. :)
Attachment #8749947 - Flags: review?(s.kaspari)
Comment on attachment 8749947 [details] [diff] [review]
Bug1209496

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

LGTM.

For the next patches: Are you exporting your patches using Mercurial? They do not contain a header (No author, no commit message etc.).
Attachment #8749947 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #11)
> https://hg.mozilla.org/integration/fx-team/rev/
> e9a379142b334234d148ea4c3b89edf259a1668b
> Bug 1209496 - DownloadContentService: Add proxy support. r=sebastian

I added commit message and author and landed the patch.

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e9a379142b33
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.