Bug 1356693 - infer: fix RESOURCE_LEAK's in base draft
authorAndrzej Hunt <ahunt@mozilla.com>
Wed, 19 Apr 2017 08:26:38 -0700
changeset 569895 68b43a5b065ef82b7d9e0144f98cc94308e7b532
parent 569894 bf3b3797097cfda7e418e38be419650b6de11b9a
child 569896 68b8cb64a7d6ccf41fbf4dcf5ff387581cac048c
push id56302
push userahunt@mozilla.com
push dateFri, 28 Apr 2017 00:45:18 +0000
bugs1356693
milestone55.0a1
Bug 1356693 - infer: fix RESOURCE_LEAK's in base MozReview-Commit-ID: Gm9GqOk37UZ
mobile/android/base/java/org/mozilla/gecko/ANRReporter.java
mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java
mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java
mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java
--- a/mobile/android/base/java/org/mozilla/gecko/ANRReporter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/ANRReporter.java
@@ -17,16 +17,17 @@ import java.io.OutputStream;
 import java.io.Reader;
 import java.util.Locale;
 import java.util.UUID;
 import java.util.regex.Pattern;
 
 import org.json.JSONObject;
 import org.mozilla.gecko.annotation.WrapForJNI;
 import org.mozilla.gecko.AppConstants.Versions;
+import org.mozilla.gecko.util.IOUtils;
 import org.mozilla.gecko.util.StringUtils;
 import org.mozilla.gecko.util.ThreadUtils;
 
 import android.content.BroadcastReceiver;
 import android.content.Context;
 import android.content.Intent;
 import android.content.IntentFilter;
 import android.os.Handler;
@@ -149,18 +150,20 @@ public final class ANRReporter extends B
 
         // Find the traces file name if we can.
         try {
             // getprop [prop-name [default-value]]
             Process propProc = (new ProcessBuilder())
                 .command("/system/bin/getprop", "dalvik.vm.stack-trace-file")
                 .redirectErrorStream(true)
                 .start();
+
+            BufferedReader buf = null;
             try {
-                BufferedReader buf = new BufferedReader(
+                buf = new BufferedReader(
                     new InputStreamReader(
                             propProc.getInputStream(), StringUtils.UTF_8), TRACES_LINE_SIZE);
                 String propVal = buf.readLine();
                 if (DEBUG) {
                     Log.d(LOGTAG, "getprop returned " + String.valueOf(propVal));
                 }
                 // getprop can return empty string when the prop value is empty
                 // or prop is undefined, treat both cases the same way
@@ -171,16 +174,18 @@ public final class ANRReporter extends B
                     } else if (DEBUG) {
                         Log.d(LOGTAG, "cannot access traces file");
                     }
                 } else if (DEBUG) {
                     Log.d(LOGTAG, "empty getprop result");
                 }
             } finally {
                 propProc.destroy();
+
+                IOUtils.safeStreamClose(buf);
             }
         } catch (IOException e) {
             Log.w(LOGTAG, e);
         } catch (ClassCastException e) {
             Log.w(LOGTAG, e); // Bug 975436
         }
         return null;
     }
--- a/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java
@@ -36,17 +36,19 @@ import org.json.JSONException;
 import org.json.JSONObject;
 import org.mozilla.gecko.annotation.RobocopTarget;
 import org.mozilla.gecko.GeckoSharedPrefs;
 import org.mozilla.gecko.GeckoProfile;
 import org.mozilla.gecko.Locales;
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.distribution.Distribution;
 import org.mozilla.gecko.restrictions.Restrictions;
+import org.mozilla.gecko.util.IOUtils;
 import org.mozilla.gecko.util.RawResource;
+import org.mozilla.gecko.util.StringUtils;
 import org.mozilla.gecko.util.ThreadUtils;
 import org.mozilla.gecko.preferences.GeckoPreferences;
 
 /**
  * {@code SuggestedSites} provides API to get a list of locale-specific
  * suggested sites to be used in Fennec's top sites panel. It provides
  * only a single method to fetch the list as a {@code Cursor}. This cursor
  * will then be wrapped by {@code TopSitesCursorWrapper} to blend top,
@@ -263,38 +265,32 @@ public class SuggestedSites {
      */
     static void saveSites(File f, Map<String, Site> sites) {
         ThreadUtils.assertNotOnUiThread();
 
         if (sites == null || sites.isEmpty()) {
             return;
         }
 
-        OutputStreamWriter osw = null;
+        OutputStreamWriter outputStreamWriter = null;
 
         try {
             final JSONArray jsonSites = new JSONArray();
             for (Site site : sites.values()) {
                 jsonSites.put(site.toJSON());
             }
 
-            osw = new OutputStreamWriter(new FileOutputStream(f), "UTF-8");
+            outputStreamWriter = new OutputStreamWriter(new FileOutputStream(f), StringUtils.UTF_8);
 
             final String jsonString = jsonSites.toString();
-            osw.write(jsonString, 0, jsonString.length());
+            outputStreamWriter.write(jsonString, 0, jsonString.length());
         } catch (Exception e) {
             Log.e(LOGTAG, "Failed to save suggested sites", e);
         } finally {
-            if (osw != null) {
-                try {
-                    osw.close();
-                } catch (IOException e) {
-                    // Ignore.
-                }
-            }
+            IOUtils.safeStreamClose(outputStreamWriter);
         }
     }
 
     private void maybeWaitForDistribution() {
         if (distribution == null) {
             return;
         }
 
--- a/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
+++ b/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
@@ -644,17 +644,26 @@ public class Distribution {
 
         final String contentType = connection.getContentType();
         if (contentType == null || !contentType.startsWith(EXPECTED_CONTENT_TYPE)) {
             Log.w(LOGTAG, "Malformed response: invalid Content-Type.");
             Telemetry.addToHistogram(HISTOGRAM_CODE_CATEGORY, CODE_CATEGORY_FETCH_INVALID_CONTENT_TYPE);
             return null;
         }
 
-        return new JarInputStream(new BufferedInputStream(connection.getInputStream()), true);
+        final BufferedInputStream bufferedInputStream = new BufferedInputStream(connection.getInputStream());
+        try {
+            return new JarInputStream(bufferedInputStream, true);
+        } catch (IOException e) {
+            // Thrown e.g. if JarInputStream can't parse the input as a valid Zip.
+            // In that case we need to ensure the bufferedInputStream gets closed since it won't
+            // be used anywhere (while still passing the Exception up the stack).
+            bufferedInputStream.close();
+            throw e;
+        }
     }
 
     private static void recordFetchTelemetry(final Exception exception) {
         if (exception == null) {
             // Should never happen.
             Telemetry.addToHistogram(HISTOGRAM_CODE_CATEGORY, CODE_CATEGORY_FETCH_EXCEPTION);
             return;
         }
--- a/mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java
+++ b/mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java
@@ -29,16 +29,17 @@ import java.util.Locale;
 import java.util.MissingResourceException;
 import java.util.zip.CRC32;
 
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.json.JSONArray;
 import org.mozilla.gecko.AppConstants;
 import org.mozilla.gecko.util.HardwareUtils;
+import org.mozilla.gecko.util.IOUtils;
 import org.mozilla.gecko.util.ProxySelector;
 
 import android.content.Context;
 import android.content.pm.PackageManager.NameNotFoundException;
 import android.os.Build;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.text.TextUtils;
@@ -373,37 +374,46 @@ public class SwitchBoard {
     }
 
     /**
      * Returns a String containing the server response from a GET request
      * @param url URL for GET request.
      * @return Returns String from server or null when failed.
      */
     @Nullable private static String readFromUrlGET(URL url) {
+        HttpURLConnection connection = null;
+        InputStreamReader inputStreamReader = null;
+        BufferedReader bufferReader = null;
         try {
-            HttpURLConnection connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(url.toURI());
+            connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(url.toURI());
             connection.setRequestProperty("User-Agent", HardwareUtils.isTablet() ?
                     AppConstants.USER_AGENT_FENNEC_TABLET :
                     AppConstants.USER_AGENT_FENNEC_MOBILE);
             connection.setRequestMethod("GET");
             connection.setUseCaches(false);
 
-            InputStream is = connection.getInputStream();
-            InputStreamReader inputStreamReader = new InputStreamReader(is);
-            BufferedReader bufferReader = new BufferedReader(inputStreamReader, 8192);
+            // BufferedReader(Reader, int) can throw, hence we need to keep a separate reference
+            // to the InputStreamReader in order to always be able to close it:
+            inputStreamReader = new InputStreamReader(connection.getInputStream());
+            bufferReader = new BufferedReader(inputStreamReader, 8192);
             String line;
             StringBuilder resultContent = new StringBuilder();
             while ((line = bufferReader.readLine()) != null) {
                 resultContent.append(line);
             }
-            bufferReader.close();
 
             return resultContent.toString();
         } catch (IOException | URISyntaxException e) {
             e.printStackTrace();
+        } finally {
+            IOUtils.safeStreamClose(bufferReader);
+            IOUtils.safeStreamClose(inputStreamReader);
+            if (connection != null) {
+                connection.disconnect();
+            }
         }
 
         return null;
     }
 
     /**
      * Return the bucket number of the user. There are 100 possible buckets.
      */
--- a/mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java
+++ b/mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java
@@ -7,16 +7,17 @@ package org.mozilla.gecko.updater;
 
 import org.mozilla.gecko.AppConstants;
 import org.mozilla.gecko.CrashHandler;
 import org.mozilla.gecko.R;
 
 import org.mozilla.apache.commons.codec.binary.Hex;
 
 import org.mozilla.gecko.permissions.Permissions;
+import org.mozilla.gecko.util.IOUtils;
 import org.mozilla.gecko.util.ProxySelector;
 import org.w3c.dom.Document;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
 
 import android.Manifest;
 import android.app.AlarmManager;
 import android.app.IntentService;
@@ -43,16 +44,17 @@ import android.util.Log;
 
 import java.io.BufferedInputStream;
 import java.io.BufferedOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.net.HttpURLConnection;
 import java.net.URI;
 import java.net.URL;
 import java.net.URLConnection;
 import java.security.MessageDigest;
 import java.util.Calendar;
 import java.util.GregorianCalendar;
 import java.util.List;
 import java.util.TimeZone;
@@ -368,26 +370,28 @@ public class UpdateService extends Inten
             builder.setContentText(getString(R.string.updater_apply_select));
             builder.setContentIntent(contentIntent);
 
             mNotificationManager.notify(NOTIFICATION_ID, builder.build());
         }
     }
 
     private UpdateInfo findUpdate(boolean force) {
+        URLConnection conn = null;
         try {
             URI uri = getUpdateURI(force);
 
             if (uri == null) {
               Log.e(LOGTAG, "failed to get update URI");
               return null;
             }
 
             DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
-            Document dom = builder.parse(ProxySelector.openConnectionWithProxy(uri).getInputStream());
+            conn = ProxySelector.openConnectionWithProxy(uri);
+            Document dom = builder.parse(conn.getInputStream());
 
             NodeList nodes = dom.getElementsByTagName("update");
             if (nodes == null || nodes.getLength() == 0)
                 return null;
 
             Node updateNode = nodes.item(0);
             Node buildIdNode = updateNode.getAttributes().getNamedItem("buildID");
             if (buildIdNode == null)
@@ -427,16 +431,24 @@ public class UpdateService extends Inten
                 Log.e(LOGTAG, "missing some required update information, have: " + info);
                 return null;
             }
 
             return info;
         } catch (Exception e) {
             Log.e(LOGTAG, "failed to check for update: ", e);
             return null;
+        } finally {
+            // conn isn't guaranteed to be an HttpURLConnection, hence we don't want to cast earlier
+            // in this method. However in our current implementation it usually is, so we need to
+            // make sure we close it in that case:
+            final HttpURLConnection httpConn = (HttpURLConnection) conn;
+            if (httpConn != null) {
+                httpConn.disconnect();
+            }
         }
     }
 
     private MessageDigest createMessageDigest(String hashFunction) {
         String javaHashFunction = null;
 
         if ("sha512".equalsIgnoreCase(hashFunction)) {
             javaHashFunction = "SHA-512";
@@ -546,29 +558,30 @@ public class UpdateService extends Inten
             deleteUpdatePackage(getLastFileName());
         }
 
         Log.i(LOGTAG, "downloading update package");
         sendCheckUpdateResult(CheckUpdateResult.DOWNLOADING);
 
         OutputStream output = null;
         InputStream input = null;
+        URLConnection conn = null;
 
         mDownloading = true;
         mCancelDownload = false;
         showDownloadNotification(downloadFile);
 
         try {
             NetworkInfo netInfo = mConnectivityManager.getActiveNetworkInfo();
             if (netInfo != null && netInfo.isConnected() &&
                 netInfo.getType() == ConnectivityManager.TYPE_WIFI) {
                 mWifiLock.acquire();
             }
 
-            URLConnection conn = ProxySelector.openConnectionWithProxy(info.uri);
+            conn = ProxySelector.openConnectionWithProxy(info.uri);
             int length = conn.getContentLength();
 
             output = new BufferedOutputStream(new FileOutputStream(downloadFile));
             input = new BufferedInputStream(conn.getInputStream());
 
             byte[] buf = new byte[BUFSIZE];
             int len = 0;
 
@@ -601,31 +614,32 @@ public class UpdateService extends Inten
             }
         } catch (Exception e) {
             downloadFile.delete();
             showDownloadFailure();
 
             Log.e(LOGTAG, "failed to download update: ", e);
             return null;
         } finally {
-            try {
-                if (input != null)
-                    input.close();
-            } catch (java.io.IOException e) { }
-
-            try {
-                if (output != null)
-                    output.close();
-            } catch (java.io.IOException e) { }
+            IOUtils.safeStreamClose(input);
+            IOUtils.safeStreamClose(output);
 
             mDownloading = false;
 
             if (mWifiLock.isHeld()) {
                 mWifiLock.release();
             }
+
+            // conn isn't guaranteed to be an HttpURLConnection, hence we don't want to cast earlier
+            // in this method. However in our current implementation it usually is, so we need to
+            // make sure we close it in that case:
+            final HttpURLConnection httpConn = (HttpURLConnection) conn;
+            if (httpConn != null) {
+                httpConn.disconnect();
+            }
         }
     }
 
     private boolean verifyDownloadedPackage(File updateFile) {
         MessageDigest digest = createMessageDigest(getLastHashFunction());
         if (digest == null)
             return false;