Closed Bug 1254089 Opened 4 years ago Closed 4 years ago

Consider using HttpsUrlConnection for LoadFaviconTask

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(2 files)

Currently we can't load many favicons (bug 1250997) because our HttpClient version does not support SNI (765064).

I started to look into how we can support SNI but a quick win could be to refactor LoadFaviconTask to use HttpsUrlConnection.
HttpUrlConnection supports SNI and LoadFaviconTask does not require fancy HTTP features.

Review commit: https://reviewboard.mozilla.org/r/38459/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38459/
Attachment #8727359 - Flags: review?(rnewman)
Attachment #8727359 - Flags: review?(nalexander)
This was pretty straight-forward so I pushed it to review. This would allow us to load favicons from hosts that use SNI - without support SNI everywhere yet.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Attachment #8727359 - Flags: review?(rnewman) → review+
Comment on attachment 8727359 [details]
MozReview Request: Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r=rnewman,nalexander

https://reviewboard.mozilla.org/r/38459/#review35039

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:140
(Diff revision 1)
> -        HttpGet request = new HttpGet(faviconURI);
> +        HttpURLConnection connection = (HttpURLConnection) faviconURI.toURL().openConnection();

Take this opportunity to also add proxy support:

https://hg.mozilla.org/mozilla-central/rev/34428badfa6a

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:165
(Diff revision 1)
> -                    // Consume the entity before recurse or exit.
> +                connection.disconnect();

Does `disconnect` fully consume the socket before returning it to the connection pool? Docs still suggest you should read the input stream before disconnecting.
Comment on attachment 8727359 [details]
MozReview Request: Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r=rnewman,nalexander

https://reviewboard.mozilla.org/r/38459/#review35049

lgtm.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:150
(Diff revision 1)
> -                Header header = response.getFirstHeader("Location");
> +            String newURI = connection.getHeaderField("Location");

nit: final.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:266
(Diff revision 1)
> -        if (entityReportedLength > 0) {
> +        if (response.contentLength > 0) {

While we're here, let's upper bound this.  `2**24`?

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:552
(Diff revision 1)
> +            //noinspection SuspiciousNameCombination (We are always assuming a square favicon)

Hmm.  Worth extracting a `targetHeight` to make this more clear?
Attachment #8727359 - Flags: review?(nalexander) → review+
Comment on attachment 8727359 [details]
MozReview Request: Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r=rnewman,nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38459/diff/1-2/
Attachment #8727359 - Attachment description: MozReview Request: Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r?rnewman,nalexander → MozReview Request: Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r=rnewman,nalexander
Comment on attachment 8727502 [details]
MozReview Request: Bug 1254089 - LoadFaviconTask: Add proxy support. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38495/diff/1-2/
(In reply to Richard Newman [:rnewman] from comment #3)
> Does `disconnect` fully consume the socket before returning it to the
> connection pool? Docs still suggest you should read the input stream before
> disconnecting.

This might not close the socket connection instantly because HttpUrlConnection tries to reuse the connections (keepAlive). But this should close all resources for this request. This here is just the error case. Whenever I actually ask for and read the stream then I always close it. Here getInputStream() might not even return anything, because there's getErrorStream(). It doesn't look like I need to request both streams just to close them.


(In reply to Nick Alexander :nalexander from comment #4)
> mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:266
> (Diff revision 1)
> > -        if (entityReportedLength > 0) {
> > +        if (response.contentLength > 0) {
> 
> While we're here, let's upper bound this.  `2**24`?

But if the buffer is not big enough then readFully() will create a bigger array and copy the bytes to the new array. So wouldn't creating a smaller buffer here only add additional memory churn?


(In reply to Nick Alexander :nalexander from comment #4)
> mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:552
> (Diff revision 1)
> > +            //noinspection SuspiciousNameCombination (We are always assuming a square favicon)
> 
> Hmm.  Worth extracting a `targetHeight` to make this more clear?

I renamed targetWidth to targetWidthAndHeight. Hopefully this is more clear and does not create an additional member.
Comment on attachment 8727502 [details]
MozReview Request: Bug 1254089 - LoadFaviconTask: Add proxy support. r?rnewman

https://reviewboard.mozilla.org/r/38495/#review35075
Attachment #8727502 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/d8c0657ff5d3
https://hg.mozilla.org/mozilla-central/rev/c8d1f83532a7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.