AW UMA: wake up MetricsService like Chrome does

This configures AwMetricsServiceClient to "wake up" the MetricsService
the same way Chrome's client does. This listens to the content-layer
events for page load start/stop, renderer hangs, and closed renderer
processes.

This also adds an integration test to verify this works as expected (for
the page load start case), and adds plumbing to configure the upload
interval in the test (because the commandline switch is rate-limited to
a 20 second minimum to avoid DOS'ing the server).

Bug: 1003204
Test: run_webview_instrumentation_test_apk -f AwMetricsIntegrationTest.*
Test: Manual - verify renderer hang case by using the AndroidX demo app
Test: (which supports triggering and recovering from renderer hangs)
Change-Id: I1f19f89cbc3625d4911755a2b17dd82908c2bb62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900613
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715132}

android_webview/browser/metrics/aw_metrics_service_client.cc

@@ -19,6 +19,7 @@
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/strings/string16.h"
+#include "base/time/time.h"
 #include "components/metrics/android_metrics_provider.h"
 #include "components/metrics/call_stack_profile_metrics_provider.h"
 #include "components/metrics/cpu_metrics_provider.h"
@@ -36,6 +37,8 @@
 #include "components/version_info/android/channel_getter.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/network_service_instance.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
 
 namespace android_webview {
 
@@ -182,6 +185,10 @@ void AwMetricsServiceClient::MaybeStartMetrics() {
     if (app_consent_ && user_consent_or_flag) {
       metrics_service_ = CreateMetricsService(metrics_state_manager_.get(),
                                               this, pref_service_);
+      // Register for notifications so we can detect when the user or app are
+      // interacting with WebView. We use these as signals to wake up the
+      // MetricsService.
+      RegisterForNotifications();
       metrics_state_manager_->ForceClientIdCreation();
       is_in_sample_ = IsInSample();
       if (IsReportingEnabled()) {
@@ -195,6 +202,17 @@ void AwMetricsServiceClient::MaybeStartMetrics() {
   }
 }
 
+void AwMetricsServiceClient::RegisterForNotifications() {
+  registrar_.Add(this, content::NOTIFICATION_LOAD_START,
+                 content::NotificationService::AllSources());
+  registrar_.Add(this, content::NOTIFICATION_LOAD_STOP,
+                 content::NotificationService::AllSources());
+  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                 content::NotificationService::AllSources());
+  registrar_.Add(this, content::NOTIFICATION_RENDER_WIDGET_HOST_HANG,
+                 content::NotificationService::AllSources());
+}
+
 void AwMetricsServiceClient::SetHaveMetricsConsent(bool user_consent,
                                                    bool app_consent) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -210,6 +228,12 @@ void AwMetricsServiceClient::SetFastStartupForTesting(
   fast_startup_for_testing_ = fast_startup_for_testing;
 }
 
+void AwMetricsServiceClient::SetUploadIntervalForTesting(
+    const base::TimeDelta& upload_interval) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  overridden_upload_interval_ = upload_interval;
+}
+
 std::unique_ptr<const base::FieldTrial::EntropyProvider>
 AwMetricsServiceClient::CreateLowEntropyProvider() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -292,6 +316,9 @@ base::TimeDelta AwMetricsServiceClient::GetStandardUploadInterval() {
   // controlled by the platform logging mechanism. Since this mechanism has its
   // own logic for rate-limiting on cellular connections, we disable the
   // component-layer logic.
+  if (!overridden_upload_interval_.is_zero()) {
+    return overridden_upload_interval_;
+  }
   return metrics::GetUploadInterval(false /* use_cellular_upload_interval */);
 }
 
@@ -311,6 +338,24 @@ std::string AwMetricsServiceClient::GetAppPackageName() {
   return std::string();
 }
 
+void AwMetricsServiceClient::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  switch (type) {
+    case content::NOTIFICATION_LOAD_STOP:
+    case content::NOTIFICATION_LOAD_START:
+    case content::NOTIFICATION_RENDERER_PROCESS_CLOSED:
+    case content::NOTIFICATION_RENDER_WIDGET_HOST_HANG:
+      metrics_service_->OnApplicationNotIdle();
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
 // WebView metrics are sampled at (possibly) different rates depending on
 // channel, based on the client ID. Sampling is hard-coded (rather than
 // controlled via variations, as in Chrome) because:
@@ -378,4 +423,12 @@ void JNI_AwMetricsServiceClient_SetFastStartupForTesting(
       fast_startup_for_testing);
 }
 
+// static
+void JNI_AwMetricsServiceClient_SetUploadIntervalForTesting(
+    JNIEnv* env,
+    jlong upload_interval_ms) {
+  AwMetricsServiceClient::GetInstance()->SetUploadIntervalForTesting(
+      base::TimeDelta::FromMilliseconds(upload_interval_ms));
+}
+
 }  // namespace android_webview

android_webview/browser/metrics/aw_metrics_service_client.h

@@ -12,9 +12,12 @@
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
 #include "base/sequence_checker.h"
+#include "base/time/time.h"
 #include "components/metrics/enabled_state_provider.h"
 #include "components/metrics/metrics_log_uploader.h"
 #include "components/metrics/metrics_service_client.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
 
 class PrefService;
 
@@ -90,7 +93,8 @@ enum class BackfillInstallDate {
 // the sample, it then calls MetricsService::Start(). If consent was not
 // granted, MaybeStartMetrics() instead clears the client ID, if any.
 class AwMetricsServiceClient : public metrics::MetricsServiceClient,
-                               public metrics::EnabledStateProvider {
+                               public metrics::EnabledStateProvider,
+                               public content::NotificationObserver {
   friend struct base::LazyInstanceTraitsBase<AwMetricsServiceClient>;
 
  public:
@@ -102,6 +106,7 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient,
   void Initialize(PrefService* pref_service);
   void SetHaveMetricsConsent(bool user_consent, bool app_consent);
   void SetFastStartupForTesting(bool fast_startup_for_testing);
+  void SetUploadIntervalForTesting(const base::TimeDelta& upload_interval);
   std::unique_ptr<const base::FieldTrial::EntropyProvider>
   CreateLowEntropyProvider();
 
@@ -132,6 +137,11 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient,
   // returns the empty string.
   std::string GetAppPackageName() override;
 
+  // content::NotificationObserver
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override;
+
  protected:
   // Returns the metrics sampling rate, to be used by IsInSample(). This is a
   // double in the non-inclusive range (0.00, 1.00). Virtual for testing.
@@ -164,9 +174,11 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient,
 
  private:
   void MaybeStartMetrics();
+  void RegisterForNotifications();
 
   std::unique_ptr<metrics::MetricsStateManager> metrics_state_manager_;
   std::unique_ptr<metrics::MetricsService> metrics_service_;
+  content::NotificationRegistrar registrar_;
   PrefService* pref_service_ = nullptr;
   bool init_finished_ = false;
   bool set_consent_finished_ = false;
@@ -175,6 +187,10 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient,
   bool is_in_sample_ = false;
   bool fast_startup_for_testing_ = false;
 
+  // When non-zero, this overrides the default value in
+  // GetStandardUploadInterval().
+  base::TimeDelta overridden_upload_interval_;
+
   // AwMetricsServiceClient may be created before the UI thread is promoted to
   // BrowserThread::UI. Use |sequence_checker_| to enforce that the
   // AwMetricsServiceClient is used on a single thread.

android_webview/browser/metrics/aw_metrics_service_client_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "android_webview/browser/metrics/aw_metrics_service_client.h"
 
+#include <memory>
+
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
@@ -15,6 +17,7 @@
 #include "components/metrics/metrics_service.h"
 #include "components/metrics/metrics_switches.h"
 #include "components/prefs/testing_pref_service.h"
+#include "content/public/browser/notification_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace android_webview {
@@ -96,7 +99,9 @@ std::unique_ptr<TestClient> CreateAndInitTestClient(PrefService* prefs) {
 
 class AwMetricsServiceClientTest : public testing::Test {
  public:
-  AwMetricsServiceClientTest() : task_runner_(new base::TestSimpleTaskRunner) {
+  AwMetricsServiceClientTest()
+      : task_runner_(new base::TestSimpleTaskRunner),
+        notification_service_(content::NotificationService::Create()) {
     // Required by MetricsService.
     base::SetRecordActionTaskRunner(task_runner_);
   }
@@ -108,6 +113,11 @@ class AwMetricsServiceClientTest : public testing::Test {
   base::test::TaskEnvironment task_environment_;
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
 
+  // AwMetricsServiceClient::RegisterForNotifications() requires the
+  // NotificationService to be up and running. Initialize it here, throw away
+  // the value because we don't need it directly.
+  std::unique_ptr<content::NotificationService> notification_service_;
+
   DISALLOW_COPY_AND_ASSIGN(AwMetricsServiceClientTest);
 };
 

android_webview/java/src/org/chromium/android_webview/metrics/AwMetricsServiceClient.java

@@ -69,6 +69,11 @@ public class AwMetricsServiceClient {
         AwMetricsServiceClientJni.get().setFastStartupForTesting(fastStartupForTesting);
     }
 
+    @VisibleForTesting
+    public static void setUploadIntervalForTesting(long uploadIntervalMs) {
+        AwMetricsServiceClientJni.get().setUploadIntervalForTesting(uploadIntervalMs);
+    }
+
     @CalledByNative
     private static String getAppPackageName() {
         // Return this unconditionally; let native code enforce whether or not it's OK to include
@@ -103,5 +108,6 @@ public class AwMetricsServiceClient {
     interface Natives {
         void setHaveMetricsConsent(boolean userConsent, boolean appConsent);
         void setFastStartupForTesting(boolean fastStartupForTesting);
+        void setUploadIntervalForTesting(long uploadIntervalMs);
     }
 }

android_webview/javatests/src/org/chromium/android_webview/test/AwMetricsMetadataTest.java → android_webview/javatests/src/org/chromium/android_webview/test/AwMetricsIntegrationTest.java

@@ -29,19 +29,20 @@ import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.LinkedBlockingQueue;
 
 /**
- * Integration test to verify WebView's metrics metadata. This isn't a great spot to verify WebView
- * reports metadata correctly; that should be done with unittests on individual MetricsProviders.
- * This is an opportunity to verify these MetricsProviders are hooked up in WebView's
- * implementation.
+ * Integration test to verify WebView's metrics implementation. This isn't a great spot to verify
+ * WebView reports metadata correctly; that should be done with unittests on individual
+ * MetricsProviders. This is an opportunity to verify these MetricsProviders (or other components
+ * integrating with the MetricsService) are hooked up in WebView's implementation.
  *
  * <p>This configures the initial metrics upload to happen very quickly (so tests don't need to run
- * multiple seconds). This does not touch the upload interval between subsequent uploads, because
- * each test runs in a separate browser process, so we'll never reach subsequent uploads. See
- * https://crbug.com/932582.
+ * multiple seconds). This also configures subsequent uploads to happen very frequently (see
+ * UPLOAD_INTERVAL_MS), although many test cases won't require this (and since each test case runs
+ * in a separate browser process, often we'll never reach subsequent uploads, see
+ * https://crbug.com/932582).
  */
 @RunWith(AwJUnit4ClassRunner.class)
 @CommandLineFlags.Add({MetricsSwitches.FORCE_ENABLE_METRICS_REPORTING}) // Override sampling logic
-public class AwMetricsMetadataTest {
+public class AwMetricsIntegrationTest {
     @Rule
     public AwActivityTestRule mRule = new AwActivityTestRule();
 
@@ -50,6 +51,9 @@ public class AwMetricsMetadataTest {
     private TestAwContentsClient mContentsClient;
     private TestPlatformServiceBridge mPlatformServiceBridge;
 
+    // Some short interval, arbitrarily chosen.
+    private static final long UPLOAD_INTERVAL_MS = 10;
+
     private static class TestPlatformServiceBridge extends PlatformServiceBridge {
         private final BlockingQueue<byte[]> mQueue;
 
@@ -80,6 +84,15 @@ public class AwMetricsMetadataTest {
             byte[] data = AwActivityTestRule.waitForNextQueueElement(mQueue);
             return ChromeUserMetricsExtension.parseFrom(data);
         }
+
+        /**
+         * Asserts there are no more metrics logs queued up.
+         */
+        public void assertNoMetricsLogs() throws Exception {
+            // Assert the size is zero (rather than the queue is empty), so if this fails we have
+            // some hint as to how many logs were queued up.
+            Assert.assertEquals("Expected no metrics logs to be in the queue", 0, mQueue.size());
+        }
     }
 
     @Before
@@ -94,8 +107,20 @@ public class AwMetricsMetadataTest {
         PlatformServiceBridge.injectInstance(mPlatformServiceBridge);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             // Need to configure the metrics delay first, because
-            // handleMinidumpsAndSetMetricsConsent() triggers MetricsService initialization.
+            // handleMinidumpsAndSetMetricsConsent() triggers MetricsService initialization. The
+            // first upload for each test case will be triggered with minimal latency, and
+            // subsequent uploads (for tests cases which need them) will be scheduled every
+            // UPLOAD_INTERVAL_MS. We use programmatic hooks (instead of commandline flags) because:
+            //  * We don't want users in the wild to upload reports to Google which are recorded
+            //    immediately after startup: these records would be very unusual in that they don't
+            //    contain (many) histograms.
+            //  * The interval for subsequent uploads is rate-limited to mitigate accidentally
+            //    DOS'ing the metrics server. We want to keep that protection for clients in the
+            //    wild, but don't need the same protection for the test because it doesn't upload
+            //    reports.
             AwMetricsServiceClient.setFastStartupForTesting(true);
+            AwMetricsServiceClient.setUploadIntervalForTesting(UPLOAD_INTERVAL_MS);
+
             AwBrowserProcess.handleMinidumpsAndSetMetricsConsent(true /* updateMetricsConsent */);
         });
     }
@@ -222,4 +247,30 @@ public class AwMetricsMetadataTest {
                 "Should not log hardware.bluetooth", systemProfile.getHardware().hasBluetooth());
         Assert.assertFalse("Should not log hardware.usb", systemProfile.getHardware().hasUsb());
     }
+
+    @Test
+    @MediumTest
+    @Feature({"AndroidWebView"})
+    public void testPageLoadsEnableMultipleUploads() throws Throwable {
+        mPlatformServiceBridge.waitForNextMetricsLog();
+
+        // At this point, the MetricsService should be asleep, and should not have created any more
+        // metrics logs.
+        mPlatformServiceBridge.assertNoMetricsLogs();
+
+        // The start of a page load should be enough to indicate to the MetricsService that the app
+        // is "in use" and it's OK to upload the next record. No need to wait for onPageFinished,
+        // since this behavior should be gated on NOTIFICATION_LOAD_START.
+        mRule.loadUrlAsync(mAwContents, "about:blank");
+
+        // This may take slightly longer than UPLOAD_INTERVAL_MS, due to the time spent processing
+        // the metrics log, but should be well within the timeout (unless something is broken).
+        mPlatformServiceBridge.waitForNextMetricsLog();
+
+        // If we get here, we got a second metrics log (and the test may pass). If there was no
+        // second metrics log, then the above call will fail with TimeoutException. We should not
+        // assertNoMetricsLogs() however, because it's possible we got a metrics log between
+        // onPageStarted & onPageFinished, in which case onPageFinished would *also* wake up the
+        // metrics service, and we might potentially have a third metrics log in the queue.
+    }
 }

android_webview/test/BUILD.gn

@@ -229,7 +229,7 @@ instrumentation_test_apk("webview_instrumentation_test_apk") {
     "../javatests/src/org/chromium/android_webview/test/AwImeTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwJavaBridgeTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwLegacyQuirksTest.java",
-    "../javatests/src/org/chromium/android_webview/test/AwMetricsMetadataTest.java",
+    "../javatests/src/org/chromium/android_webview/test/AwMetricsIntegrationTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwNetworkConfigurationTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwPageLoadMetricsTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwPermissionManagerTest.java",