[AW] Bind once to metrics service in AwNonembeddedUmaRecorder

Make AwNonembeddedUmaRecorder bind once MetricsBridgeService and never
unbind, i.e keep the service binding for the app context lifetime. This
should save the overhead of binding and then unbinding for each
histogram record.

Never unbinding the service should be okay, while the non-embedded
process is alive, there is expected to be a consistent stream of
histogram records that need to be sent to the service.

AwNonembeddedUmaRecorder can be accessed concurrently from many threads
making it tricky to maintain one instance of ServiceConnection given
possible accidental changes in connection status by the system. In
order to avoid losing histogram records and not to block the calling
thread, we add incoming histogram records to a thread-safe list and
when the service connection is ready we send those records to the
service. If the connection is ready we send records directly to the
service.

Fixed: 1081262
Test: bin/run_webview_instrumentation_test_apk -f "*AwNonembeddedUmaRecorderTest*"
Change-Id: If61c6cdc25c61dacbbd747179e4625acf8a98a50
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207531Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Commit-Queue: Hazem Ashmawy <hazems@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772624}

android_webview/java/src/org/chromium/android_webview/common/metrics/AwNonembeddedUmaRecorder.java

@@ -10,6 +10,7 @@ import android.content.ServiceConnection;
 import android.os.IBinder;
 import android.os.RemoteException;
 
+import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 
 import org.chromium.android_webview.common.services.IMetricsBridgeService;
@@ -20,6 +21,11 @@ import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
 import org.chromium.base.metrics.UmaRecorder;
 
+import java.util.ArrayList;
+import java.util.List;
+
+import javax.annotation.concurrent.GuardedBy;
+
 /**
  * {@link UmaRecorder} for nonembedded WebView processes.
  * Can be used as a delegate in {@link org.chromium.base.metrics.UmaRecorderHolder}. This may only
@@ -28,6 +34,11 @@ import org.chromium.base.metrics.UmaRecorder;
 public class AwNonembeddedUmaRecorder implements UmaRecorder {
     private static final String TAG = "AwNonembedUmaRecord";
 
+    // Arbitrary limit to avoid adding records indefinitely if there is a problem connecting to the
+    // service.
+    @VisibleForTesting
+    public static final int MAX_PENDING_RECORDS_COUNT = 512;
+
     private final String mServiceName;
 
     @VisibleForTesting
@@ -135,35 +146,95 @@ public class AwNonembeddedUmaRecorder implements UmaRecorder {
         assert false : "Recording UserAction in non-embedded WebView processes isn't supported yet";
     }
 
-    // Connects to the MetricsBridgeService to record the histogram call.
-    private void recordHistogram(HistogramRecord methodCall) {
+    private final Object mLock = new Object();
+    // Service stub object
+    @GuardedBy("mLock")
+    @Nullable
+    private IMetricsBridgeService mServiceStub;
+
+    // List of HistogramRecords that are pending to be sent to the service because the connection
+    // isn't ready yet.
+    @GuardedBy("mLock")
+    private final List<HistogramRecord> mPendingRecordsList = new ArrayList<>();
+
+    private final ServiceConnection mServiceConnection = new ServiceConnection() {
+        @Override
+        public void onServiceConnected(ComponentName className, IBinder service) {
+            synchronized (mLock) {
+                mServiceStub = IMetricsBridgeService.Stub.asInterface(service);
+                for (HistogramRecord record : mPendingRecordsList) {
+                    sendToServiceLocked(record);
+                }
+                mPendingRecordsList.clear();
+            }
+        }
+
+        @Override
+        public void onServiceDisconnected(ComponentName className) {
+            synchronized (mLock) {
+                mServiceStub = null;
+            }
+        }
+    };
+
+    /**
+     * Send a record to the metrics service, assumes that {@code mLock} is held by the current
+     * thread.
+     */
+    @GuardedBy("mLock")
+    private void sendToServiceLocked(HistogramRecord record) {
+        try {
+            // We are not punting this to a background thread since the cost of IPC itself
+            // should be relatively cheap, and the remote method does its work
+            // asynchronously.
+            mServiceStub.recordMetrics(record.toByteArray());
+        } catch (RemoteException e) {
+            Log.e(TAG, "Remote Exception calling IMetricsBridgeService#recordMetrics", e);
+        }
+    }
+
+    // Is app context bound to the MetricsBridgeService.
+    @GuardedBy("mLock")
+    private boolean mIsBound = false;
+
+    /**
+     * Bind the service only once and keep the service binding for the lifetime of the app process.
+     * Assumes that {@code mLock} is held by the current thread.
+     *
+     * We never unbind because it's fine to keep the service alive while the app is running, since
+     * it's most likely that there will be consistent stream of histograms while the app/process is
+     * running.
+     */
+    @GuardedBy("mLock")
+    private void maybeBindServiceLocked() {
+        if (mIsBound) return;
+
         final Context appContext = ContextUtils.getApplicationContext();
         final Intent intent = new Intent();
         intent.setClassName(appContext, mServiceName);
+        mIsBound = appContext.bindService(intent, mServiceConnection, Context.BIND_AUTO_CREATE);
+        if (!mIsBound) {
+            Log.w(TAG, "Could not bind to MetricsBridgeService " + intent);
+        }
+    }
 
-        ServiceConnection connection = new ServiceConnection() {
-            @Override
-            public void onServiceConnected(ComponentName className, IBinder service) {
-                IMetricsBridgeService metricsService =
-                        IMetricsBridgeService.Stub.asInterface(service);
-                try {
-                    // We are not punting this to a background thread since the cost of IPC itself
-                    // should be relatively cheap, and the remote method does its work
-                    // asynchronously.
-                    metricsService.recordMetrics(methodCall.toByteArray());
-                } catch (RemoteException e) {
-                    Log.e(TAG, "Remote Exception calling IMetricsBridgeService#recordMetrics", e);
-                } finally {
-                    appContext.unbindService(this);
-                }
+    /**
+     * Record a histogram by sending it to metrics service or add it to a pending list if the
+     * connection isn't ready yet. Bind the service only once on the first record to arrive.
+     */
+    private void recordHistogram(HistogramRecord record) {
+        synchronized (mLock) {
+            if (mServiceStub != null) {
+                sendToServiceLocked(record);
+                return;
             }
 
-            @Override
-            public void onServiceDisconnected(ComponentName className) {}
-        };
-        // TODO(https://crbug.com/1081262) bind to the service once
-        if (!appContext.bindService(intent, connection, Context.BIND_AUTO_CREATE)) {
-            Log.w(TAG, "Could not bind to MetricsBridgeService " + intent);
+            maybeBindServiceLocked();
+            if (mPendingRecordsList.size() < MAX_PENDING_RECORDS_COUNT) {
+                mPendingRecordsList.add(record);
+            } else {
+                Log.w(TAG, "Number of pending records has reached max capacity, dropping record");
+            }
         }
     }
 }
\ No newline at end of file

android_webview/javatests/src/org/chromium/android_webview/test/common/metrics/AwNonembeddedUmaRecorderTest.java

@@ -28,18 +28,20 @@ import org.chromium.base.metrics.UmaRecorderHolder;
 @RunWith(AwJUnit4ClassRunner.class)
 @OnlyRunIn(SINGLE_PROCESS)
 public class AwNonembeddedUmaRecorderTest {
-    private static final long BINDER_TIMEOUT_MILLIS = 10000;
+    private static final String TEST_SERVICE_NAME = MockMetricsBridgeService.class.getName();
 
     private AwNonembeddedUmaRecorder mUmaRecorder;
 
     @Before
     public void setUp() {
-        mUmaRecorder = new AwNonembeddedUmaRecorder(MockMetricsBridgeService.class.getName());
+        mUmaRecorder = new AwNonembeddedUmaRecorder(TEST_SERVICE_NAME);
+        // Reset static variables in case when the service is still running from a previous test.
+        MockMetricsBridgeService.reset();
     }
 
     @Test
     @MediumTest
-    public void testRecordTrueBooleanHistogram() {
+    public void testRecordTrueBooleanHistogram() throws Throwable {
         String histogramName = "testRecordBooleanHistogram";
         HistogramRecord recordProto = HistogramRecord.newBuilder()
                                               .setRecordType(RecordType.HISTOGRAM_BOOLEAN)
@@ -47,13 +49,13 @@ public class AwNonembeddedUmaRecorderTest {
                                               .setSample(1)
                                               .build();
         mUmaRecorder.recordBooleanHistogram(histogramName, true);
-        byte[] recordedData = MockMetricsBridgeService.getRecordedData();
+        byte[] recordedData = MockMetricsBridgeService.getReceivedDataAfter(1);
         Assert.assertArrayEquals(recordProto.toByteArray(), recordedData);
     }
 
     @Test
     @MediumTest
-    public void testRecordFalseBooleanHistogram() {
+    public void testRecordFalseBooleanHistogram() throws Throwable {
         String histogramName = "testRecordBooleanHistogram";
         HistogramRecord recordProto = HistogramRecord.newBuilder()
                                               .setRecordType(RecordType.HISTOGRAM_BOOLEAN)
@@ -61,13 +63,13 @@ public class AwNonembeddedUmaRecorderTest {
                                               .setSample(0)
                                               .build();
         mUmaRecorder.recordBooleanHistogram(histogramName, false);
-        byte[] recordedData = MockMetricsBridgeService.getRecordedData();
+        byte[] recordedData = MockMetricsBridgeService.getReceivedDataAfter(1);
         Assert.assertArrayEquals(recordProto.toByteArray(), recordedData);
     }
 
     @Test
     @MediumTest
-    public void testRecordExponentialHistogram() {
+    public void testRecordExponentialHistogram() throws Throwable {
         String histogramName = "recordExponentialHistogram";
         int sample = 100;
         int min = 5;
@@ -82,13 +84,13 @@ public class AwNonembeddedUmaRecorderTest {
                                               .setNumBuckets(numBuckets)
                                               .build();
         mUmaRecorder.recordExponentialHistogram(histogramName, sample, min, max, numBuckets);
-        byte[] recordedData = MockMetricsBridgeService.getRecordedData();
+        byte[] recordedData = MockMetricsBridgeService.getReceivedDataAfter(1);
         Assert.assertArrayEquals(recordProto.toByteArray(), recordedData);
     }
 
     @Test
     @MediumTest
-    public void testRecordLinearHistogram() {
+    public void testRecordLinearHistogram() throws Throwable {
         String histogramName = "testRecordLinearHistogram";
         int sample = 100;
         int min = 5;
@@ -103,13 +105,13 @@ public class AwNonembeddedUmaRecorderTest {
                                               .setNumBuckets(numBuckets)
                                               .build();
         mUmaRecorder.recordLinearHistogram(histogramName, sample, min, max, numBuckets);
-        byte[] recordedData = MockMetricsBridgeService.getRecordedData();
+        byte[] recordedData = MockMetricsBridgeService.getReceivedDataAfter(1);
         Assert.assertArrayEquals(recordProto.toByteArray(), recordedData);
     }
 
     @Test
     @MediumTest
-    public void testRecordSparseHistogram() {
+    public void testRecordSparseHistogram() throws Throwable {
         String histogramName = "testRecordSparseHistogram";
         int sample = 10;
         HistogramRecord recordProto = HistogramRecord.newBuilder()
@@ -118,14 +120,56 @@ public class AwNonembeddedUmaRecorderTest {
                                               .setSample(sample)
                                               .build();
         mUmaRecorder.recordSparseHistogram(histogramName, sample);
-        byte[] recordedData = MockMetricsBridgeService.getRecordedData();
+        byte[] recordedData = MockMetricsBridgeService.getReceivedDataAfter(1);
         Assert.assertArrayEquals(recordProto.toByteArray(), recordedData);
     }
 
+    @Test
+    @MediumTest
+    public void testRecordMultipleHistograms() throws Throwable {
+        String histogramName = "testRecordMultipleSparseHistograms";
+        HistogramRecord recordProto = HistogramRecord.newBuilder()
+                                              .setRecordType(RecordType.HISTOGRAM_SPARSE)
+                                              .setHistogramName(histogramName)
+                                              .setSample(3)
+                                              .build();
+
+        mUmaRecorder.recordSparseHistogram(histogramName, 1);
+        mUmaRecorder.recordSparseHistogram(histogramName, 2);
+        mUmaRecorder.recordSparseHistogram(histogramName, 3);
+
+        byte[] recordedData = MockMetricsBridgeService.getReceivedDataAfter(3);
+        Assert.assertArrayEquals(recordProto.toByteArray(), recordedData);
+    }
+
+    @Test
+    @MediumTest
+    @SuppressWarnings("ThreadPriorityCheck")
+    public void testStressParallelHistograms() throws Exception {
+        final int numThreads = 16;
+        final int numSamples = AwNonembeddedUmaRecorder.MAX_PENDING_RECORDS_COUNT / numThreads;
+
+        Thread[] threads = new Thread[numThreads];
+        for (int i = 0; i < numThreads; i++) {
+            threads[i] = new Thread(() -> {
+                for (int j = 0; j < numSamples; j++) {
+                    mUmaRecorder.recordSparseHistogram("StressTest", 10);
+                    // Make it more likely this thread will be preempted.
+                    Thread.yield();
+                }
+            });
+            threads[i].start();
+        }
+        for (Thread thread : threads) {
+            thread.join();
+        }
+        MockMetricsBridgeService.getReceivedDataAfter(numSamples * numThreads);
+    }
+
     @Test
     @MediumTest
     // Test calling RecordHistogram class methods to make sure a record is delegated as expected.
-    public void testRecordHistogram() {
+    public void testRecordHistogram() throws Throwable {
         String histogramName = "testRecordHistogram.testRecordSparseHistogram";
         int sample = 10;
         HistogramRecord recordProto = HistogramRecord.newBuilder()
@@ -135,7 +179,7 @@ public class AwNonembeddedUmaRecorderTest {
                                               .build();
         UmaRecorderHolder.setNonNativeDelegate(mUmaRecorder);
         RecordHistogram.recordSparseHistogram(histogramName, sample);
-        byte[] recordedData = MockMetricsBridgeService.getRecordedData();
+        byte[] recordedData = MockMetricsBridgeService.getReceivedDataAfter(1);
         Assert.assertArrayEquals(recordProto.toByteArray(), recordedData);
     }
 }
\ No newline at end of file

android_webview/javatests/src/org/chromium/android_webview/test/services/MockMetricsBridgeService.java

@@ -13,22 +13,27 @@ import org.junit.Assert;
 
 import org.chromium.android_webview.common.services.IMetricsBridgeService;
 
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+
 /**
  * Mock implementation for MetricsBridgeService that record metrics data and provide methods
  * for testing this data. Used in
  * {@link org.chromium.android_webview.test.common.metrics.AwNonembeddedUmaRecorderTest}.
  */
 public class MockMetricsBridgeService extends Service {
-    public static final long BINDER_TIMEOUT_MILLIS = 10000;
+    public static final long TIMEOUT_MILLIS = 10000;
 
     private static byte[] sRecordedData;
-    private static final ConditionVariable sMethodCalled = new ConditionVariable();
+    private static final AtomicInteger sRecordsCount = new AtomicInteger(0);
+    private static final ConditionVariable sReceivedRecord = new ConditionVariable();
 
     private final IMetricsBridgeService.Stub mMockBinder = new IMetricsBridgeService.Stub() {
         @Override
         public void recordMetrics(byte[] data) {
             sRecordedData = data;
-            sMethodCalled.open();
+            sRecordsCount.incrementAndGet();
+            sReceivedRecord.open();
         }
 
         @Override
@@ -43,14 +48,32 @@ public class MockMetricsBridgeService extends Service {
     }
 
     /**
-     * Block until the recordMetrics is called and data is set is fail after
-     * {@link BINDER_TIMEOUT_MILLIS}.
+     * Block until the recordMetrics is called exactly for {@code times} number of times. Fail
+     * after {@link TIMEOUT_MILLIS} milliseconds.
      *
-     * @return byte date received by recordMetrics.
+     * @param times the number of times recordMetrics() is expected to be called
+     * @return the byte data received by recordMetrics on the {@code times} call.
      */
-    public static byte[] getRecordedData() {
-        Assert.assertTrue("Timed out waiting for recordMetrics() to be called",
-                sMethodCalled.block(BINDER_TIMEOUT_MILLIS));
+    public static byte[] getReceivedDataAfter(int times) throws TimeoutException {
+        while (sRecordsCount.get() < times) {
+            sReceivedRecord.close();
+            if (!sReceivedRecord.block(TIMEOUT_MILLIS)) {
+                throw new TimeoutException(
+                        "Timedout waiting for recordMetrics() to be called for the ("
+                        + (sRecordsCount.get() + 1) + ") time");
+            }
+        }
+        Assert.assertEquals("recordMetrics() should be called (" + times + ") times", times,
+                sRecordsCount.get());
         return sRecordedData;
     }
+
+    /**
+     * Reset static variables between tests.
+     */
+    public static void reset() {
+        sRecordedData = null;
+        sRecordsCount.set(0);
+        sReceivedRecord.close();
+    }
 }
\ No newline at end of file