1254089 - Consider using HttpsUrlConnection for LoadFaviconTask

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r=rnewman,nalexander

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/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 3

[Richard Newman [:rnewman]]

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 4

[Nick Alexander :nalexander [he/him]]

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?

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1254089 - LoadFaviconTask: Add proxy support. r?rnewman

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

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

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/

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

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/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(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 9

[Richard Newman [:rnewman]]

Comment on attachment 8727502 [details]
MozReview Request: Bug 1254089 - LoadFaviconTask: Add proxy support. r?rnewman

https://reviewboard.mozilla.org/r/38495/#review35075

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/d8c0657ff5d34dd1a62d63830f967e0d215b9e2b
Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r=rnewman,nalexander

https://hg.mozilla.org/integration/fx-team/rev/c8d1f83532a7fe4289a473e3528c04b8b2f85a64
Bug 1254089 - LoadFaviconTask: Add proxy support. r=rnewman

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/d8c0657ff5d3
https://hg.mozilla.org/mozilla-central/rev/c8d1f83532a7