Closed
Bug 1315076
Opened 8 years ago
Closed 8 years ago
Strict mode violation during favicon download
Categories
(Firefox for Android Graveyard :: Favicon Handling, defect, P1)
Firefox for Android Graveyard
Favicon Handling
Tracking
(firefox53 fixed)
RESOLVED
FIXED
Firefox 53
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)
Assignee | ||
Comment 1•8 years ago
|
||
Thanks for filing this! I'll have a look.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
I can reproduce this when restoring a session (fresh process) with a website without a favicon (we try to load from the default URL).
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
In addition to that if getResponseCode() throws an IOException then we will never close the connection.
Assignee | ||
Comment 5•8 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 1.8
Priority: -- → P1
Whiteboard: [MobileAS]
Reporter | ||
Comment 8•8 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review-reply |
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."
Reporter | ||
Comment 10•8 years ago
|
||
(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!
Assignee | ||
Comment 11•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8807596 -
Flags: review?(gkruglov)
Updated•8 years ago
|
Iteration: 1.8 → 1.9
Comment hidden (mozreview-request) |
Reporter | ||
Comment 13•8 years ago
|
||
mozreview-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 ::: 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+
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 16•8 years ago
|
||
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
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e5192843bc07
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•