[Android WebView] Trigger minidump uploading on renderer crash.

android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java

Index: android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
diff --git a/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java b/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
index 64b7c6fc210b41fafd6bb68e92d2d23e182bbe30..02a279516dffdb35d74832b1204c5d9705cdae1a 100644
--- a/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
+++ b/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
@@ -13,7 +13,9 @@ import android.os.IBinder;
 import android.os.ParcelFileDescriptor;
 import android.os.RemoteException;
 import android.os.StrictMode;
+import android.webkit.ValueCallback;
 
+import org.chromium.android_webview.command_line.CommandLineUtil;
 import org.chromium.android_webview.crash.CrashReceiverService;
 import org.chromium.android_webview.crash.ICrashReceiverService;
 import org.chromium.android_webview.policy.AwPolicyProvider;
@@ -22,6 +24,8 @@ import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
 import org.chromium.base.PathUtils;
 import org.chromium.base.ThreadUtils;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.library_loader.LibraryLoader;
 import org.chromium.base.library_loader.LibraryProcessType;
 import org.chromium.base.library_loader.ProcessInitException;
@@ -40,6 +44,7 @@ import java.nio.channels.FileLock;
 /**
  * Wrapper for the steps needed to initialize the java and native sides of webview chromium.
  */
+@JNINamespace("android_webview")
 public abstract class AwBrowserProcess {
     public static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "webview";
 
@@ -47,6 +52,7 @@ public abstract class AwBrowserProcess {
     private static final String EXCLUSIVE_LOCK_FILE = "webview_data.lock";
     private static RandomAccessFile sLockFile;
     private static FileLock sExclusiveFileLock;
+    private static String sWebViewPackageName;
 
     private static final int MAX_MINIDUMP_UPLOAD_TRIES = 3;
 
@@ -149,6 +155,49 @@ public abstract class AwBrowserProcess {
         }
     }
 
+    public static void setWebViewPackageName(String webViewPackageName) {

[gsennton, 2017/04/20 20:32:46]

Having this is a bit ugly... but we fetch the WebView package name from the glue
- so we won't be able to fetch it when calling from native ->
triggerMinidumpUploading() otherwise.

[paulmiller, 2017/04/20 23:38:51]

I was going to do something like this anyway. The metrics code needs to check
the package name in order to determine channel. Better to put the ugliness in
your change than mine! :)

[gsennton, 2017/04/21 15:46:33]

Haha, damn ;)

+        assert sWebViewPackageName == null || sWebViewPackageName.equals(webViewPackageName);
+        sWebViewPackageName = webViewPackageName;
+    }
+
+    /**
+     * Trigger minidump copying, which in turn triggers minidump uploading.
+     */
+    @CalledByNative
+    private static void triggerMinidumpUploading() {
+        handleMinidumpsAndSetMetricsConsent(sWebViewPackageName, false /* updateMetricsConsent */);

[Tobias Sargeant, 2017/04/21 16:18:04]

I think now we could get into a situation where we have more than one AsyncTask
posted by handleMinidumps running concurrently.

[gsennton, 2017/04/24 08:27:16]

Right, that might be undesirable ;)
I think any alternative solution would make this code more complex (through
synchronisation), though I guess even now we would have to argue about what
might happen if two of these AsyncTasks interleave (which is a complex
argument).
I guess we could have a variable with three states on which to synchronize:
states:

Nothing_going_on
Thread_copying_minidumps
Thread_should_copy_minidumps_again

code:

AwBrowserProcess.handleMinidumps():
   if (state == Nothing_going_on):
      post new AsyncTask
      set state = Thread_copying_minidumps
   else if (state == Thread_copying_minidumps):
      set state = Thread_should_copy_minidumps_again

copyMinidumps() (in the AsyncTask):
   copy minidumps...
   before exiting:
      if (state == Thread_should_copy_minidumps_again):
         state = Thread_copying_minidumps
         rerun minidump-copying


WDYT?
(this logic is becoming complex enough that I would like some tests for it
though... so would probably want to create some independent class for this logic
itself, and then test that separately).

[gsennton, 2017/04/24 12:39:54]

As Toby pointed out offline, if we just post these copy-tasks to the same thread
we won't ever have any problem with synchronization (so I'm changing the
handleMinidumps() call to post these tasks to AsyncTask.SERIAL_EXECUTOR).

+    }
+
+    /**
+     * Trigger minidump uploading, and optionaly also update the metrics-consent value depending on
+     * whether the Android Checkbox is toggled on.
+     * @param updateMetricsConsent whether to update the metrics-consent value to represent the

[paulmiller, 2017/04/20 23:38:51]

This flag feels hacky. Why not have 2 queryMetricsSetting() calls, 1 in the
original location for both crashes and metrics, and 1 here for just crashes?

[gsennton, 2017/04/21 15:46:33]

We could do that, I just don't like duplicating the whole statement of
if(enableMinidumpUploadingForTesting) ... and also the
queryMetricsSetting-statement. It feels like we could easily miss updating one
of the two locations in an upcoming fix. WDYT?

[paulmiller, 2017/04/21 17:50:01]

I don't think that calling queryMetricsSetting twice counts as duplicating code.
That's sort of the point of functions, to call them from multiple places, to
avoid duplicating code. But, up to you.

[gsennton, 2017/04/24 08:27:16]

I'm more concerned about the whole enableMinidumpUploadingForTesting-logic:

if (enableMinidumpUploadingForTesting) {
    handleMinidumps();
}

queryMetricsSetting() -> {
    if (!enableMinidumpUploadingForTesting) {
        handleMinidumps();
    }
}

it seems like duplicating that could cause divergence in the future :)
(if this would only have been a call to queryMetricsSetting() I wouldn't mind
duplicating that).

+     * Android Checkbox toggle.
+     */
+    public static void handleMinidumpsAndSetMetricsConsent(
+            final String webViewPackageName, final boolean updateMetricsConsent) {
+        final boolean enableMinidumpUploadingForTesting = CommandLine.getInstance().hasSwitch(
+                CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH);
+        if (enableMinidumpUploadingForTesting) {
+            AwBrowserProcess.handleMinidumps(webViewPackageName, true /* enabled */);
+        }
+
+        PlatformServiceBridge.getInstance().queryMetricsSetting(new ValueCallback<Boolean>() {
+            // Actions conditioned on whether the Android Checkbox is toggled on
+            public void onReceiveValue(Boolean enabled) {
+                ThreadUtils.assertOnUiThread();
+                if (updateMetricsConsent) {
+                    AwMetricsServiceClient.setConsentSetting(
+                            ContextUtils.getApplicationContext(), enabled);
+                }
+
+                if (!enableMinidumpUploadingForTesting) {
+                    AwBrowserProcess.handleMinidumps(webViewPackageName, enabled);
+                }
+            }
+        });
+    }
+
     /**
      * Pass Minidumps to a separate Service declared in the WebView provider package.
      * That Service will copy the Minidumps to its own data directory - at which point we can delete