[Android Crash Reporting] Allow uploading minidumps via the JobScheduler

chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java

Index: chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java b/chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java
new file mode 100644
index 0000000000000000000000000000000000000000..7bbe5bc2e25d2bc50e2cdf25a0b676b28f4ac983
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java
@@ -0,0 +1,107 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+package org.chromium.chrome.browser.crash;
+
+import android.annotation.TargetApi;
+import android.content.Context;
+import android.net.ConnectivityManager;
+import android.net.NetworkInfo;
+import android.os.Build;
+import android.os.PersistableBundle;
+
+import org.chromium.components.minidump_uploader.MinidumpUploaderDelegate;
+import org.chromium.components.minidump_uploader.util.CrashReportingPermissionManager;
+
+import java.io.File;
+
+/**
+ * Chrome-specific implementations for minidump uploading logic.
+ */
+@TargetApi(Build.VERSION_CODES.LOLLIPOP)
+public class ChromeMinidumpUploaderDelegate implements MinidumpUploaderDelegate {
+    // PersistableBundle keys:
+    static final String IS_CLIENT_IN_METRICS_SAMPLE = "isClientInMetricsSample";
+    static final String IS_CRASH_UPLOAD_DISABLED_BY_COMMAND_LINE =
+            "isCrashUploadDisabledByCommandLine";
+    static final String IS_USAGE_AND_CRASH_REPORTING_PERMITTED_BY_USER =
+            "isUsageAndCrashReportingPermittedByUser";
+    static final String IS_UPLOAD_ENABLED_FOR_TESTS = "isUploadEnabledForTests";
+
+    /**
+     * The application context in which minidump uploads are running.
+     */
+    private final Context mContext;
+
+    /**
+     * The cached crash reporting permissions. These are cached because the upload job might run
+     * outside of a context in which the original permissions are easily accessible.
+     */
+    private final PersistableBundle mPermissions;
+
+    /**
+     * The system connectivity manager service, used to determine the network state.
+     */
+    private final ConnectivityManager mConnectivityManager;
+
+    /**
+     * Constructs a new Chrome-specific minidump uploader delegate.
+     * @param context The application context in which minidump uploads are running.
+     * @param permissions The cached crash reporting permissions.
+     */
+    ChromeMinidumpUploaderDelegate(Context context, PersistableBundle permissions) {
+        mContext = context;
+        mPermissions = permissions;
+        mConnectivityManager =
+                (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
+    }
+
+    @Override
+    public File getCacheDir() {
+        return mContext.getCacheDir();
+    }
+
+    @Override
+    public CrashReportingPermissionManager createCrashReportingPermissionManager() {
+        return new CrashReportingPermissionManager() {
+            // Note: getBoolean is only available in API level 22, so the data is serialized via
+            // ints instead.
+            @Override
+            public boolean isClientInMetricsSample() {
+                return mPermissions.getInt(IS_CLIENT_IN_METRICS_SAMPLE, 1) == 1;
+            }
+            @Override
+            public boolean isNetworkAvailableForCrashUploads() {
+                // TODO(isherman): This code should really be shared with the Android Webview
+                // implementation, which tests whether the connection is metered, rather than
+                // testing the type of the connection. Implement this change in M59 -- for M58, it's
+                // more important to maintain consistency with the previous implementation. When
+                // changing this, note that forced uploads do *not* require unmetered connections.
+                NetworkInfo networkInfo = mConnectivityManager.getActiveNetworkInfo();
+                if (networkInfo == null || !networkInfo.isConnected()) return false;
+                return networkInfo.getType() == ConnectivityManager.TYPE_WIFI
+                        || networkInfo.getType() == ConnectivityManager.TYPE_ETHERNET;
+            }
+            @Override
+            public boolean isCrashUploadDisabledByCommandLine() {
+                return mPermissions.getInt(IS_CRASH_UPLOAD_DISABLED_BY_COMMAND_LINE, 0) == 1;
+            }
+            @Override
+            public boolean isUsageAndCrashReportingPermittedByUser() {
+                // TODO(isherman): Is it okay to use a cached value here? Ideally, we'd only use the
+                // cached value if Chrome is no longer running -- and update the cached value
+                // periodically. How hard would that be to implement?

[Ilya Sherman, 2017/03/13 03:12:09]

Maria and Gustav, WDYT?  Do either of you know how we could check whether Chrome
is running?

[gsennton, 2017/03/13 17:57:17]

When implementing the usage-and-diagnostics check for WebView there was a
request for us (WebView) to ensure there was user-consent at the time of the
upload (since that could be long after the jobservice job is scheduled.
I would guess we should do the same in Chrome.

I don't know if there's a way to check whether Chrome is running.


It looks like PrivacyPreferencesManager is using a Shared Preference to store
the value of the consent
(PrivacyPreferencesManager.isUsageAndCrashReportingPermittedByUser()) though,
could you just read that value? (if you are still running in the browser process
I see no harm in doing so).

[Ilya Sherman, 2017/03/14 02:18:55]

Okay, it does seem like it's safe to directly call
PrivacyPreferencesManager.isUsageAndCrashReprotingPermittedByUser() -- whew!  I
was worried because I saw that sometimes the PrivacyPreferencesManager was
instantiated twice, which prevented me from using e.g.
PrivacyPreferencesManager.isClientInMetricsSample(), when following these steps:

1. Launch Chrome
2. Navigate to chrome://crash
3. Wait for the minidump upload to be scheduled
4. Force quit Chrome
5. Wait for the upload to run.

I'm actually still not entirely sure why the PrivacyPreferencesManager is being
created twice, since the return value of getInstance() is cached in a global
singleton.  I'm therefore still a bit worried that there might be some subtle
issue around calling into the PrivacyPreferencesManager from this job. 
Thoughts?

[gsennton, 2017/03/14 18:17:28]

When is PrivacyPreferencesManager being created (twice)? I would assume that
when you force-quit Chrome the browser process is killed and when the upload-job
is later run the browser process would be re-started (thus again initializing
PrivacyPreferencesManager if you are using that class inside the
uploading-code).

[Maria, 2017/03/14 19:03:32]

I went and read the documentation. Jobs do run within the application's process
(browser process for us).

Here's what I would expect to happen:
1. Launch Chrome. PreferenceManager gets created as an in-memory static
2. chrome://crash will crash a renderer, browser process is alive
3. minidump upload is scheduled using the same preference manager value to set
isPermittedByUser
4. Force quit chrome. Browser process is dead. Bye bye everything.
5. Upload is scheduled and run. Browser process is started. PreferenceManager is
created. If ChromeTabbedActivity were started, it would use the same object.

So if you see something different, please comment.

[Ilya Sherman, 2017/03/15 02:13:34]

I think you're both correct, and that makes a lot of sense.  Thank you!  I feel
much less wary of this code now =)  I'm still not really used to the Android
process model where processes can be killed and started again, with different
bits of initialization code running depending on ... stuff =P

+                return mPermissions.getInt(IS_USAGE_AND_CRASH_REPORTING_PERMITTED_BY_USER, 0) == 1;
+            }
+            @Override
+            public boolean isUploadEnabledForTests() {
+                return mPermissions.getInt(IS_UPLOAD_ENABLED_FOR_TESTS, 0) == 1;
+            }
+        };
+    }
+
+    @Override
+    public void prepareToUploadMinidumps(final Runnable startUploads) {
+        startUploads.run();
+    }
+}