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

android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java

Index: android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
diff --git a/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java b/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
index f3b994ef35a72f4493662f589bd7f807a26eb0f6..f80d94e46e979afcabfb8e6885b07acd4f3e46a4 100644
--- a/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
+++ b/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
@@ -15,7 +15,11 @@ import android.os.Binder;
 import android.os.Build;
 import android.os.IBinder;
 import android.os.ParcelFileDescriptor;
+import android.webkit.ValueCallback;
 
+import org.chromium.android_webview.PlatformServiceBridge;
+import org.chromium.android_webview.command_line.CommandLineUtil;
+import org.chromium.base.CommandLine;
 import org.chromium.base.Log;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.components.minidump_uploader.CrashFileManager;
@@ -44,26 +48,80 @@ public class CrashReceiverService extends Service {
     private Object mCopyingLock = new Object();
     private boolean mIsCopying = false;
 
-    // same switch as kEnableCrashReporterForTesting in //base/base_switches.h
-    static final String CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH =
-            "enable-crash-reporter-for-testing";
+    private volatile boolean mIsCrashCopyingEnabledForTesting;
+
+    private ConsentManager mConsentManager = new ConsentManager();
+
+    private static class ConsentManager {
+        private static final int CONSENT_NOT_RETURNED = 0;
+        private static final int CONSENT_OK = 1;
+        private static final int CONSENT_NOT_OK = 2;
+
+        private Object mLock = new Object();
+        private int mConsentValue = CONSENT_NOT_RETURNED;
+
+        private void resetConsentState() {
+            synchronized (mLock) {
+                mConsentValue = CONSENT_NOT_RETURNED;
+            }
+        }
+
+        private void setConsentValue(boolean ok) {
+            synchronized (mLock) {
+                assert mConsentValue == CONSENT_NOT_RETURNED;
+                mConsentValue = ok ? CONSENT_OK : CONSENT_NOT_OK;
+                mLock.notifyAll();
+            }
+        }
+
+        /**
+         * Wait until a consent value was received, and return that value.
+         */
+        private boolean getConsentValue() {
+            synchronized (mLock) {
+                while (mConsentValue == CONSENT_NOT_RETURNED) {
+                    try {
+                        mLock.wait();

[sgurun-gerrit only, 2017/01/25 19:42:06]

I think we may have a max waiting time here. Do not trust gmscore or any process
to come back to you. 

Alternatively, we can change the platformservicebridgegoogle such that it
retries after some time, and if retry does not work within a certain time,
returns consent not ok.

I am tending towards the latter. I will file a bug to paul for it, so no change
needed here.

+                    } catch (InterruptedException e) {
+                        break;
+                    }
+                }
+                return mConsentValue == CONSENT_OK;
+            }
+        }
+    }
 
     @Override
     public void onCreate() {
         super.onCreate();
-        SynchronizedWebViewCommandLine.initOnSeparateThread();
+        CommandLineUtil.initCommandLine();
+        mIsCrashCopyingEnabledForTesting = CommandLine.getInstance().hasSwitch(

[sgurun-gerrit only, 2017/01/25 19:42:06]

if command line flag has switch, call setConsentValue(true)
then you can remove mIsCrashXX variable

+                CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH);
+
+        mConsentManager.resetConsentState();

[sgurun-gerrit only, 2017/01/25 19:42:06]

I don't think you need that. You already set the variable to
CONSENT_NOT_RETURNED.
you can also remove the method above.

[gsennton, 2017/01/26 10:24:50]

Right, I was under the assumption that onCreate could be called several times
for the same Service instance, but that doesn't seem to be the case ;)
I'll remove this code anyway given that we don't really need to check the U&D
toggle here.

+        PlatformServiceBridge.getInstance(this).queryMetricsSetting(new ValueCallback<Boolean>() {

[sgurun-gerrit only, 2017/01/25 19:42:06]

do this in the else, if command line does not have switch,

[paulmiller, 2017/01/25 23:24:05]

Was talking about this with Selim - why do we need another queryMetricsSetting()
in the service? Are we sending any dumps to the service before consent is
determined? If not, then doesn't receiving dumps indicate to the service that
consent was given?

[gsennton, 2017/01/26 10:24:50]

We don't strictly need it - and no we aren't sending any dumps unless the U&D
flag (or enableForTesting) are ON, so when only WebView code uses these services
we don't need to check the U&D flag within these services.

The only possible scenario I'm wondering about is if some app decides to use the
minidump-copying API (CrashReceiverService.transmitCrashes) to trigger data
uploading to Google servers. Then we would be uploading data because of an app
requesting it, without us checking the U&D checkbox.

Anyway, I'll remove the U&D + enableForTesting check in this CL for now, and if
we feel that we need it anyway I can add it later (so most of the comments here
are obsolete now).

+            @Override
+            public void onReceiveValue(Boolean enabled) {
+                mConsentManager.setConsentValue(enabled);
+            }
+        });
+    }
+
+    private boolean isCrashCopyingEnabled() {

[sgurun-gerrit only, 2017/01/25 19:42:06]

you can then remove this method

+        if (mIsCrashCopyingEnabledForTesting) {
+            return true;
+        }
+        return mConsentManager.getConsentValue();
     }
 
     private final ICrashReceiverService.Stub mBinder = new ICrashReceiverService.Stub() {
         @Override
         public void transmitCrashes(ParcelFileDescriptor[] fileDescriptors) {
-            // TODO(gsennton): replace this check with a check for Android Checkbox when we have
-            // access to that value through GmsCore.
-            if (!SynchronizedWebViewCommandLine.hasSwitch(
-                        CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH)) {
+            if (!isCrashCopyingEnabled()) {

[sgurun-gerrit only, 2017/01/25 19:42:06]

and simply call mContentManager.getConsentValue() here

                 Log.i(TAG, "Crash reporting is not enabled, bailing!");
                 return;
             }
+
             int uid = Binder.getCallingUid();
             performMinidumpCopyingSerially(
                     CrashReceiverService.this, uid, fileDescriptors, true /* scheduleUploads */);