1315076 - Strict mode violation during favicon download

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect, P1)

Description

[:Grisha Kruglov]

Seeing this on startup in logcat:


11-01 13:52:21.223 3089-3097/? E/StrictMode: A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
                                             java.lang.Throwable: Explicit termination method 'close' not called
                                                 at dalvik.system.CloseGuard.open(CloseGuard.java:180)
                                                 at com.android.org.conscrypt.OpenSSLSocketImpl.startHandshake(OpenSSLSocketImpl.java:288)
                                                 at com.android.okhttp.internal.http.SocketConnector.connectTls(SocketConnector.java:103)
                                                 at com.android.okhttp.Connection.connect(Connection.java:143)
                                                 at com.android.okhttp.Connection.connectAndSetOwner(Connection.java:185)
                                                 at com.android.okhttp.OkHttpClient$1.connectAndSetOwner(OkHttpClient.java:128)
                                                 at com.android.okhttp.internal.http.HttpEngine.nextConnection(HttpEngine.java:341)
                                                 at com.android.okhttp.internal.http.HttpEngine.connect(HttpEngine.java:330)
                                                 at com.android.okhttp.internal.http.HttpEngine.sendRequest(HttpEngine.java:248)
                                                 at com.android.okhttp.internal.huc.HttpURLConnectionImpl.execute(HttpURLConnectionImpl.java:433)
                                                 at com.android.okhttp.internal.huc.HttpURLConnectionImpl.connect(HttpURLConnectionImpl.java:114)
                                                 at com.android.okhttp.internal.huc.DelegatingHttpsURLConnection.connect(DelegatingHttpsURLConnection.java:89)
                                                 at com.android.okhttp.internal.huc.HttpsURLConnectionImpl.connect(HttpsURLConnectionImpl.java)
                                                 at org.mozilla.gecko.icons.loader.IconDownloader.connectTo(IconDownloader.java:182)
                                                 at org.mozilla.gecko.icons.loader.IconDownloader.tryDownloadRecurse(IconDownloader.java:131)
                                                 at org.mozilla.gecko.icons.loader.IconDownloader.tryDownload(IconDownloader.java:120)
                                                 at org.mozilla.gecko.icons.loader.IconDownloader.downloadAndDecodeImage(IconDownloader.java:93)
                                                 at org.mozilla.gecko.icons.loader.IconDownloader.load(IconDownloader.java:59)
                                                 at org.mozilla.gecko.icons.IconTask.loadIcon(IconTask.java:128)
                                                 at org.mozilla.gecko.icons.IconTask.call(IconTask.java:59)
                                                 at org.mozilla.gecko.icons.IconTask.call(IconTask.java:24)
                                                 at java.util.concurrent.FutureTask.run(FutureTask.java:237)
                                                 at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
                                                 at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
                                                 at java.lang.Thread.run(Thread.java:818)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Thanks for filing this! I'll have a look.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

I can reproduce this when restoring a session (fresh process) with a website without a favicon (we try to load from the default URL).

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

My stacktrace is slightly different, is triggered by just calling getResponse() and looks a lot like this Android / okhttp bug, even though it should be fixed:
https://github.com/square/okhttp/issues/441

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

In addition to that if getResponseCode() throws an IOException then we will never close the connection.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

All this code would be much easier if we let HttpUrlConnection handle the redirects - our own redirect handling was needed back when we were using HttpClient.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed.

As a side effect this fixes a resource leak in case getResponseCode() throws an IOException.

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

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

In rare cases I still see a StrictMode warning but this seems to be a false positive. We are now always closing the connection and the stream.

Comment 8

[:Grisha Kruglov]

Comment on attachment 8807596 [details]
Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed.

https://reviewboard.mozilla.org/r/90718/#review91642

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:119
(Diff revision 1)
>          final HttpURLConnection connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(
>                  new URI(faviconURI));
>  
>          connection.setRequestProperty("User-Agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());
>  
> -        // We implemented or own way of following redirects back when this code was using HttpClient.
> +        // HttpURLConnection will follow up to five HTTP redirects.

How do you know it's just five? Briefly looking through docs I didn't see mention of this.

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:120
(Diff revision 1)
>                  new URI(faviconURI));
>  
>          connection.setRequestProperty("User-Agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());
>  
> -        // We implemented or own way of following redirects back when this code was using HttpClient.
> -        // Nowadays we should let HttpUrlConnection do the work - assuming that it doesn't follow
> +        // HttpURLConnection will follow up to five HTTP redirects.
> +        connection.setInstanceFollowRedirects(true);

IIUC, HttpUrlConnection won't follow cross-protocol redirects (e.g. http to https) [0], whereas previous behaviour in `tryDownloadRecurse` did do that.

https -> http case seems unhelpful, but the opposite might be a fairly common occurance, I would imagine? e.g. the main site is http, but assets are served off of an https cdn.

[0] https://bugs.openjdk.java.net/browse/JDK-4620571

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8807596 [details]
Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed.

https://reviewboard.mozilla.org/r/90718/#review91642

> How do you know it's just five? Briefly looking through docs I didn't see mention of this.

https://developer.android.com/reference/java/net/HttpURLConnection.html

From the "response handling" section: "HttpURLConnection will follow up to five HTTP redirects."

Comment 10

[:Grisha Kruglov]

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Comment on attachment 8807596 [details]
> Bug 1315076 - IconDownloader: Let HttpUrlConnection handle http redirects.
> 
> https://reviewboard.mozilla.org/r/90718/#review91642
> 
> > How do you know it's just five? Briefly looking through docs I didn't see mention of this.
> 
> https://developer.android.com/reference/java/net/HttpURLConnection.html
> 
> From the "response handling" section: "HttpURLConnection will follow up to
> five HTTP redirects."

Of course, I just didn't parse that page properly. Thanks!

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8807596 [details]
Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed.

https://reviewboard.mozilla.org/r/90718/#review91642

> IIUC, HttpUrlConnection won't follow cross-protocol redirects (e.g. http to https) [0], whereas previous behaviour in `tryDownloadRecurse` did do that.
> 
> https -> http case seems unhelpful, but the opposite might be a fairly common occurance, I would imagine? e.g. the main site is http, but assets are served off of an https cdn.
> 
> [0] https://bugs.openjdk.java.net/browse/JDK-4620571

I agree. I can live with dropping https to http redirects (there are some security/privaxy concerns here) but supporting the other way around (http -> https) makes sense. I'll update the patch.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8807596 [details]
Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed.

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

Comment 13

[:Grisha Kruglov]

Comment on attachment 8807596 [details]
Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed.

https://reviewboard.mozilla.org/r/90718/#review95016

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:87
(Diff revision 2)
> -     *                     in the event of a transient connection failure.
> -     * @throws URISyntaxException If the underlying call to tryDownload retries and raises such an
> -     *                            exception trying a fallback URL.
>       */
>      @VisibleForTesting
> -    LoadFaviconResult downloadAndDecodeImage(Context context, String targetFaviconURL) throws IOException, URISyntaxException {
> +    LoadFaviconResult downloadAndDecodeImage(Context context, String targetFaviconURL) {

@Nullable

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:116
(Diff revision 2)
>       * Helper method for trying the download request to grab a Favicon.
>       *
>       * @param faviconURI URL of Favicon to try and download
>       * @return The HttpResponse containing the downloaded Favicon if successful, null otherwise.
>       */
> -    private HttpURLConnection tryDownload(String faviconURI) throws URISyntaxException, IOException {
> +    private HttpURLConnection tryDownload(String faviconURI) {

@Nullable

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:125
(Diff revision 2)
>      }
>  
>      /**
>       * Try to download from the favicon URL and recursively follow redirects.
>       */
> -    private HttpURLConnection tryDownloadRecurse(String faviconURI, HashSet<String> visited) throws URISyntaxException, IOException {
> +    private HttpURLConnection tryDownloadRecurse(String faviconURI, HashSet<String> visited) {

@Nullable

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:170
(Diff revision 2)
> -            connection.disconnect();
> +                connection.disconnect();
> -            return null;
> +                return null;
> -        }
> +            }
> +        } catch (IOException | URISyntaxException e) {
> +            if (connection != null) {
> +                connection.disconnect();

Might be better to call .disconnect in a finally block?

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8807596 [details]
Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed.

https://reviewboard.mozilla.org/r/90718/#review95016

> Might be better to call .disconnect in a finally block?

In case of a successful connection I do not want to close it but use it later on :)

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8807596 [details]
Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5192843bc07
IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed. r=Grisha

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/e5192843bc07