Closed Bug 1315076 Opened 3 years ago Closed 3 years ago

Strict mode violation during favicon download

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 53
Iteration:
1.9
Tracking Status
firefox53 --- fixed

People

(Reporter: Grisha, Assigned: sebastian)

Details

(Whiteboard: [MobileAS])

Attachments

(1 file)

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)
Thanks for filing this! I'll have a look.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
I can reproduce this when restoring a session (fresh process) with a website without a favicon (we try to load from the default URL).
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
In addition to that if getResponseCode() throws an IOException then we will never close the connection.
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.
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.
Iteration: --- → 1.8
Priority: -- → P1
Whiteboard: [MobileAS]
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 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."
(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 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.
Attachment #8807596 - Flags: review?(gkruglov)
Iteration: 1.8 → 1.9
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?
Attachment #8807596 - Flags: review?(gkruglov) → review+
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 :)
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e5192843bc07
IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed. r=Grisha
https://hg.mozilla.org/mozilla-central/rev/e5192843bc07
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.