[Android WebView] Trigger minidump uploading on renderer crash.

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

 // Copyright 2012 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.android_webview;
 
 import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
 import android.content.ServiceConnection;
 import android.os.AsyncTask;
 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;
 import org.chromium.base.CommandLine;
 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;
 import org.chromium.base.process_launcher.ChildProcessCreationParams;
 import org.chromium.components.minidump_uploader.CrashFileManager;
 import org.chromium.content.browser.BrowserStartupController;
 import org.chromium.content.browser.ChildProcessLauncher;
 import org.chromium.policy.CombinedPolicyProvider;
 
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.RandomAccessFile;
 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";
 
     private static final String TAG = "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;
 
     /**
      * Loads the native library, and performs basic static construction of objects needed
      * to run webview in this process. Does not create threads; safe to call from zygote.
      * Note: it is up to the caller to ensure this is only called once.
      */
     public static void loadLibrary() {
         PathUtils.setPrivateDataDirectorySuffix(PRIVATE_DATA_DIRECTORY_SUFFIX);
@@ skipping 82 matching lines @@
                     throw new RuntimeException(error);
                 } else {
                     Log.w(TAG, error);
                 }
             }
         } finally {
             StrictMode.setThreadPolicy(oldPolicy);
         }
     }
 
+    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
      * our copies in the app directory.
      * @param userApproved whether we have user consent to upload crash data - if we do, copy the
      * minidumps, if we don't, delete them.
      */
     public static void handleMinidumps(
             final String webViewPackageName, final boolean userApproved) {
         new AsyncTask<Void, Void, Void>() {
@@ skipping 68 matching lines @@
                     public void onServiceDisconnected(ComponentName className) {}
                 };
                 if (!appContext.bindService(intent, connection, Context.BIND_AUTO_CREATE)) {
                     Log.w(TAG, "Could not bind to Minidump-copying Service " + intent);
                 }
                 return null;
             }
         }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
     }
 }