[Android WebView] Ensure we have user consent before uploading minidumps

android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java

Index: android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
diff --git a/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java b/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
index b7fb06b95604bf6e1b142f2741731eb1ae1a8c55..279e0b922748483a2e2c7b2c5c1eb2ba502d2d6b 100644
--- a/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
+++ b/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
@@ -49,6 +49,7 @@ import org.chromium.android_webview.AwSettings;
 import org.chromium.android_webview.HttpAuthDatabase;
 import org.chromium.android_webview.PlatformServiceBridge;
 import org.chromium.android_webview.ResourcesContextWrapperFactory;
+import org.chromium.android_webview.command_line.CommandLineUtil;
 import org.chromium.base.BuildConfig;
 import org.chromium.base.CommandLine;
 import org.chromium.base.ContextUtils;
@@ -83,7 +84,6 @@ public class WebViewChromiumFactoryProvider implements WebViewFactoryProvider {
 
     private static final String CHROMIUM_PREFS_NAME = "WebViewChromiumPrefs";
     private static final String VERSION_CODE_PREF = "lastVersionCodeUsed";
-    private static final String COMMAND_LINE_FILE = "/data/local/tmp/webview-command-line";
     private static final String HTTP_AUTH_DATABASE_FILE = "http_auth.db";
 
     private class WebViewChromiumRunQueue {
@@ -215,10 +215,10 @@ public class WebViewChromiumFactoryProvider implements WebViewFactoryProvider {
         // WebView needs to make sure to always use the wrapped application context.
         ContextUtils.initApplicationContext(ResourcesContextWrapperFactory.get(ctx));
 
-        if (isBuildDebuggable()) {
+        if (CommandLineUtil.isBuildDebuggable()) {
             // Suppress the StrictMode violation as this codepath is only hit on debuggable builds.
             StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads();
-            CommandLine.initFromFile(COMMAND_LINE_FILE);
+            CommandLine.initFromFile(CommandLineUtil.WEBVIEW_COMMAND_LINE_FILE);
             StrictMode.setThreadPolicy(oldPolicy);
         } else {
             CommandLine.init(null);
@@ -270,10 +270,6 @@ public class WebViewChromiumFactoryProvider implements WebViewFactoryProvider {
         }
     }
 
-    private static boolean isBuildDebuggable() {
-        return !Build.TYPE.equals("user");
-    }
-
     /**
      * Both versionCodes should be from a WebView provider package implemented by Chromium.
      * VersionCodes from other kinds of packages won't make any sense in this method.
@@ -406,7 +402,27 @@ public class WebViewChromiumFactoryProvider implements WebViewFactoryProvider {
         final boolean isExternalService = true;
         AwBrowserProcess.configureChildProcessLauncher(webViewPackageName, isExternalService);
         AwBrowserProcess.start();
-        AwBrowserProcess.handleMinidumps(webViewPackageName);
+
+        boolean enableMinidumpUploadingForTesting = CommandLine.getInstance().hasSwitch(
+                CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH);

[sgurun-gerrit only, 2017/01/24 08:58:49]

Is this only for convenience, to ease the manual testing so you don't have to go
to Google settings to set the switch?  I am a little bit skeptical of adding a
switch for that purpose. 

Assuming the switch is useful, which I am not convinced yet, I think it might be
better to have a switch to overwrite user consent with a false/true.  Depending
on the goal you may move this logic to PlatformServiceBridgeGoogle (if switch
exists return the value of switch, rather than GMS) 

this can also allow merging the logic below in a simple way.

[gsennton, 2017/01/24 10:41:47]

It's also useful for devices that do not have the newest gms-core apk - where it
doesn't matter whether we turn the checkbox on or off since we can't fetch its
value from WebView either way.

Right now the switch is also used within MinidumpUploaderImpl, though that seems
unnecessary given that the switch is checked in CrashReceiverService (i.e.
before copying minidumps).


Given that we could probably settle with reading the command-line flag from
within CrashReceiverService (and WebViewFactoryProvider), and given that the
flag will only be read on debuggable builds - maybe we should do just as in
WebViewFactoryProvider, i.e. initialize and use the command only on the main
thread (since we will only read from a command-line-file in debuggable builds)
and enable strictmode if initializing the command-line from a file. (this would
mean removing the entire SynchronizedWebViewCommandLine class) WDYT?


Maybe, as you say, we want a flag for turning on user consent (instead of
depending on the current flag). We are currently depending on the flag
'enable-crash-reporter-for-testing' which is also used for enabling breakpad on
debug builds.

[sgurun-gerrit only, 2017/01/24 18:55:27]

I think we should be a little bit stingy while adding flags. I am ok with the
idea of having the flag for now, but once GMSv10 is released and goes to stable
100% we should remove the flag. Let's add a simple flag in
WebViewFactoryProvider, drop synchronizedwebviewcommandline, and get rid of the
whole thing when time comes, as I mentioned above.

Every once in a while there is a big effort to cleanup flags, see
crbug.com/343941.  

[gsennton, 2017/01/25 10:35:13]

Which flag are we talking about here now? Are you still for using a separate
flag for overwriting user consent? The enable-crash-reporter-for-testing flag
will remain since it's used to generate minidumps on debug builds (in Chrome).

If we add a flag in WebViewFactoryProvider it should be read in
CrashReceiverService as well (with the current PS I'm just reading the
enable-crash-reporting-for-testing flag, I'm still not sure whether we would
want to add a override-user-consent-for-testing flag instead?).

+        if (enableMinidumpUploadingForTesting) {
+            AwBrowserProcess.handleMinidumps(webViewPackageName, true /* enabled */);
+        } else {
+            // Copy minidumps if Android Checkbox is toggled on.

[gsennton, 2017/01/23 18:33:53]

TODO merge this with the queryMetricsSetting call below.

[sgurun-gerrit only, 2017/01/24 08:58:49]

+1

[gsennton, 2017/01/24 10:41:47]

Why aren't we checking canUseGms() in the call below?

[sgurun-gerrit only, 2017/01/24 18:55:27]

you need to check for canUseGms() only if you don't know whether your WebView is
Google WebView or not. For usermetrics and safebrowsing, there are internal code
pieces, and they know they are google webview.

[gsennton, 2017/01/25 10:35:13]

Done.

+            PlatformServiceBridge platformServiceBridge =
+                    PlatformServiceBridge.getInstance(context);
+            if (platformServiceBridge.canUseGms()) {
+                platformServiceBridge.queryMetricsSetting(new ValueCallback<Boolean>() {
+                    @Override
+                    public void onReceiveValue(Boolean enabled) {
+                        AwBrowserProcess.handleMinidumps(webViewPackageName, enabled);
+                    }
+                });
+            } else {
+                // Gms is not available - don't copy minidumps.
+                AwBrowserProcess.handleMinidumps(webViewPackageName, false /* enabled */);
+            }
+        }
 
         PlatformServiceBridge.getInstance(context)
                 .queryMetricsSetting(new ValueCallback<Boolean>() {
@@ -416,7 +432,7 @@ public class WebViewChromiumFactoryProvider implements WebViewFactoryProvider {
                     }
                 });
 
-        if (isBuildDebuggable()) {
+        if (CommandLineUtil.isBuildDebuggable()) {
             setWebContentsDebuggingEnabled(true);
         }
 
@@ -512,7 +528,7 @@ public class WebViewChromiumFactoryProvider implements WebViewFactoryProvider {
                     @Override
                     public void setWebContentsDebuggingEnabled(boolean enable) {
                         // Web Contents debugging is always enabled on debug builds.
-                        if (!isBuildDebuggable()) {
+                        if (!CommandLineUtil.isBuildDebuggable()) {
                             WebViewChromiumFactoryProvider.this.setWebContentsDebuggingEnabled(
                                     enable);
                         }