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

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

 // Copyright 2014 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 com.android.webview.chromium;
 
 import android.Manifest;
 import android.app.ActivityManager;
 import android.content.ComponentCallbacks2;
 import android.content.Context;
@@ skipping 31 matching lines @@
 import org.chromium.android_webview.AwCookieManager;
 import org.chromium.android_webview.AwDevToolsServer;
 import org.chromium.android_webview.AwMetricsServiceClient;
 import org.chromium.android_webview.AwNetworkChangeNotifierRegistrationPolicy;
 import org.chromium.android_webview.AwQuotaManagerBridge;
 import org.chromium.android_webview.AwResource;
 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;
 import org.chromium.base.MemoryPressureListener;
 import org.chromium.base.PackageUtils;
 import org.chromium.base.PathService;
 import org.chromium.base.PathUtils;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.TraceEvent;
 import org.chromium.base.annotations.SuppressFBWarnings;
@@ skipping 14 matching lines @@
 /**
  * Entry point to the WebView. The system framework talks to this class to get instances of the
  * implementation classes.
  */
 @SuppressWarnings("deprecation")
 public class WebViewChromiumFactoryProvider implements WebViewFactoryProvider {
     private static final String TAG = "WebViewChromiumFactoryProvider";
 
     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 {
         public WebViewChromiumRunQueue() {
             mQueue = new ConcurrentLinkedQueue<Runnable>();
         }
 
         public void addTask(Runnable task) {
             mQueue.add(task);
             if (WebViewChromiumFactoryProvider.this.hasStarted()) {
@@ skipping 111 matching lines @@
         } catch (IllegalArgumentException e) {
             if (!ctx.getSystemService(UserManager.class).isUserUnlocked()) {
                 throw e;
             }
             ctx = ctx.createCredentialProtectedStorageContext();
         }
 
         // 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);
         }
 
         if (Settings.Global.getInt(ContextUtils.getApplicationContext().getContentResolver(),
                     Settings.Global.WEBVIEW_MULTIPROCESS, 0)
                 == 1) {
             CommandLine cl = CommandLine.getInstance();
             cl.appendSwitch("webview-sandboxed-renderer");
@@ skipping 31 matching lines @@
     }
 
     static void checkStorageIsNotDeviceProtected(Context context) {
         if ((Build.VERSION.CODENAME.equals("N") || Build.VERSION.SDK_INT > Build.VERSION_CODES.M)
                 && context.isDeviceProtectedStorage()) {
             throw new IllegalArgumentException(
                     "WebView cannot be used with device protected storage");
         }
     }
 
-    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.
      *
      * An introduction to Chromium versionCode scheme:
      * "BBBBPPPAX"
      * BBBB: 4 digit branch number. It monotonically increases over time.
      * PPP: patch number in the branch. It is padded with zeroes to the left. These three digits may
      * change their meaning in the future.
      * A: architecture digit.
@@ skipping 112 matching lines @@
 
         // Make sure that ResourceProvider is initialized before starting the browser process.
         final String webViewPackageName = WebViewFactory.getLoadedPackageInfo().packageName;
         final Context context = ContextUtils.getApplicationContext();
         setUpResources(webViewPackageName, context);
         initPlatSupportLibrary();
         initNetworkChangeNotifier(context);
         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>() {
                     public void onReceiveValue(Boolean enabled) {
                         ThreadUtils.assertOnUiThread();
                         AwMetricsServiceClient.setConsentSetting(context, enabled);
                     }
                 });
 
-        if (isBuildDebuggable()) {
+        if (CommandLineUtil.isBuildDebuggable()) {
             setWebContentsDebuggingEnabled(true);
         }
 
         TraceEvent.setATraceEnabled(mWebViewDelegate.isTraceTagEnabled());
         mWebViewDelegate.setOnTraceEnabledChangeListener(
                 new WebViewDelegate.OnTraceEnabledChangeListener() {
                     @Override
                     public void onTraceEnabledChange(boolean enabled) {
                         TraceEvent.setATraceEnabled(enabled);
                     }
@@ skipping 75 matching lines @@
                     }
 
                     @Override
                     public String getDefaultUserAgent(Context context) {
                         return AwSettings.getDefaultUserAgent();
                     }
 
                     @Override
                     public void setWebContentsDebuggingEnabled(boolean enable) {
                         // Web Contents debugging is always enabled on debug builds.
-                        if (!isBuildDebuggable()) {
+                        if (!CommandLineUtil.isBuildDebuggable()) {
                             WebViewChromiumFactoryProvider.this.setWebContentsDebuggingEnabled(
                                     enable);
                         }
                     }
 
                     @Override
                     public void clearClientCertPreferences(Runnable onCleared) {
                         AwContentsStatics.clearClientCertPreferences(onCleared);
                     }
 
@@ skipping 143 matching lines @@
     WebViewDelegate getWebViewDelegate() {
         return mWebViewDelegate;
     }
 
     // The method to support unreleased Android.
     WebViewContentsClientAdapter createWebViewContentsClientAdapter(WebView webView,
             Context context) {
         return new WebViewContentsClientAdapter(webView, context, mWebViewDelegate);
     }
 }