From 385dbda50c642d722cfd7854dbfca231c59bd6d1 Mon Sep 17 00:00:00 2001 From: Nate Fischer Date: Fri, 16 Oct 2020 19:47:03 +0000 Subject: [PATCH] AW UMA: test for renderer histograms No change to production logic. This adds a test to verify we collect renderer process histograms and merge them with browser process histograms (at which point we can be confident they'll be included in the next UMA log). Fixed: 1112018 Test: run_webview_instrumentation_test_apk -f AwMetricsIntegrationTest.* Change-Id: I2a4cd8328061fea6e67f9f4fb43f1fdd07669525 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2473807 Commit-Queue: Nate Fischer Auto-Submit: Nate Fischer Reviewed-by: Bo Cr-Commit-Position: refs/heads/master@{#818037} --- .../metrics/aw_metrics_service_client.cc | 11 ++++ .../metrics/AwMetricsServiceClient.java | 9 +++ .../test/AwMetricsIntegrationTest.java | 58 +++++++++++++++++++ .../metrics/android_metrics_service_client.cc | 26 ++++++++- .../metrics/android_metrics_service_client.h | 8 ++- 5 files changed, 109 insertions(+), 3 deletions(-) diff --git a/android_webview/browser/metrics/aw_metrics_service_client.cc b/android_webview/browser/metrics/aw_metrics_service_client.cc index 63a8a07f6e2cea..d16ef6a02e3f2a 100644 --- a/android_webview/browser/metrics/aw_metrics_service_client.cc +++ b/android_webview/browser/metrics/aw_metrics_service_client.cc @@ -9,6 +9,7 @@ #include "android_webview/browser/metrics/aw_stability_metrics_provider.h" #include "android_webview/browser_jni_headers/AwMetricsServiceClient_jni.h" +#include "base/android/callback_android.h" #include "base/android/jni_android.h" #include "base/metrics/histogram_functions.h" #include "base/metrics/persistent_histogram_allocator.h" @@ -161,4 +162,14 @@ void JNI_AwMetricsServiceClient_SetUploadIntervalForTesting( base::TimeDelta::FromMilliseconds(upload_interval_ms)); } +// static +void JNI_AwMetricsServiceClient_SetOnFinalMetricsCollectedListenerForTesting( + JNIEnv* env, + const base::android::JavaParamRef& listener) { + AwMetricsServiceClient::GetInstance() + ->SetOnFinalMetricsCollectedListenerForTesting(base::BindRepeating( + base::android::RunRunnableAndroid, + base::android::ScopedJavaGlobalRef(listener))); +} + } // namespace android_webview diff --git a/android_webview/java/src/org/chromium/android_webview/metrics/AwMetricsServiceClient.java b/android_webview/java/src/org/chromium/android_webview/metrics/AwMetricsServiceClient.java index 0f49c219764233..9c6cbb4d645d16 100644 --- a/android_webview/java/src/org/chromium/android_webview/metrics/AwMetricsServiceClient.java +++ b/android_webview/java/src/org/chromium/android_webview/metrics/AwMetricsServiceClient.java @@ -72,10 +72,19 @@ public static void setUploadIntervalForTesting(long uploadIntervalMs) { AwMetricsServiceClientJni.get().setUploadIntervalForTesting(uploadIntervalMs); } + /** + * Sets a callback to run each time after final metrics have been collected. + */ + @VisibleForTesting + public static void setOnFinalMetricsCollectedListenerForTesting(Runnable listener) { + AwMetricsServiceClientJni.get().setOnFinalMetricsCollectedListenerForTesting(listener); + } + @NativeMethods interface Natives { void setHaveMetricsConsent(boolean userConsent, boolean appConsent); void setFastStartupForTesting(boolean fastStartupForTesting); void setUploadIntervalForTesting(long uploadIntervalMs); + void setOnFinalMetricsCollectedListenerForTesting(Runnable listener); } } diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/AwMetricsIntegrationTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/AwMetricsIntegrationTest.java index d730abd1f0c315..c26d4218e55c4c 100644 --- a/android_webview/javatests/src/org/chromium/android_webview/test/AwMetricsIntegrationTest.java +++ b/android_webview/javatests/src/org/chromium/android_webview/test/AwMetricsIntegrationTest.java @@ -4,6 +4,10 @@ package org.chromium.android_webview.test; +import static org.chromium.android_webview.test.OnlyRunIn.ProcessMode.MULTI_PROCESS; + +import android.support.test.InstrumentationRegistry; + import androidx.test.filters.MediumTest; import org.junit.Assert; @@ -19,12 +23,14 @@ import org.chromium.base.Callback; import org.chromium.base.ThreadUtils; import org.chromium.base.metrics.RecordHistogram; +import org.chromium.base.test.util.CallbackHelper; import org.chromium.base.test.util.CommandLineFlags; import org.chromium.base.test.util.Feature; import org.chromium.components.metrics.ChromeUserMetricsExtensionProtos.ChromeUserMetricsExtension; import org.chromium.components.metrics.MetricsSwitches; import org.chromium.components.metrics.SystemProfileProtos.SystemProfileProto; import org.chromium.content_public.browser.test.util.TestThreadUtils; +import org.chromium.net.test.EmbeddedTestServer; import java.util.concurrent.BlockingQueue; import java.util.concurrent.LinkedBlockingQueue; @@ -305,4 +311,56 @@ public void testPageLoadsEnableMultipleUploads() throws Throwable { // 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. } + + @Test + @MediumTest + @Feature({"AndroidWebView"}) + @OnlyRunIn(MULTI_PROCESS) // This test is specific to the OOP-renderer + public void testRendererHistograms() throws Throwable { + EmbeddedTestServer embeddedTestServer = EmbeddedTestServer.createAndStartServer( + InstrumentationRegistry.getInstrumentation().getContext()); + try { + // Discard initial log since the renderer process hasn't been created yet. + mPlatformServiceBridge.waitForNextMetricsLog(); + + final CallbackHelper helper = new CallbackHelper(); + int finalMetricsCollectedCount = helper.getCallCount(); + + // Load a page and wait for final metrics collection. + TestThreadUtils.runOnUiThreadBlocking(() -> { + AwMetricsServiceClient.setOnFinalMetricsCollectedListenerForTesting( + () -> { helper.notifyCalled(); }); + }); + + // Load a page to ensure the renderer process is created. + mRule.loadUrlSync(mAwContents, mContentsClient.getOnPageFinishedHelper(), + embeddedTestServer.getURL("/simple_page.html")); + helper.waitForCallback(finalMetricsCollectedCount, 1); + + // At this point we know one of two things must be true: + // + // 1. The renderer process completed startup (logging the expected histogram) before + // subprocess histograms were collected. In this case, we know the desired histogram + // has been copied into the browser process. + // 2. Subprocess histograms were collected before the renderer process completed + // startup. While we don't know if our histogram was copied over, we do know the + // page load has finished and this woke up the metrics service, so MetricsService + // will collect subprocess metrics again. + // + // Load a page and wait for another final log collection. We know this log collection + // must be triggered by either the second page load start (scenario 1) or the first page + // load finish (scenario 2), either of which ensures the renderer startup histogram must + // have been copied into the browser process. + + mRule.loadUrlSync(mAwContents, mContentsClient.getOnPageFinishedHelper(), + embeddedTestServer.getURL("/simple_page.html")); + helper.waitForCallback(finalMetricsCollectedCount, 2); + + Assert.assertEquals(1, + RecordHistogram.getHistogramTotalCountForTesting( + "Android.SeccompStatus.RendererSandbox")); + } finally { + embeddedTestServer.stopAndDestroyServer(); + } + } } diff --git a/components/embedder_support/android/metrics/android_metrics_service_client.cc b/components/embedder_support/android/metrics/android_metrics_service_client.cc index c4508daef9ac02..b59d1ce2883f9b 100644 --- a/components/embedder_support/android/metrics/android_metrics_service_client.cc +++ b/components/embedder_support/android/metrics/android_metrics_service_client.cc @@ -190,6 +190,20 @@ std::unique_ptr CreateFileMetricsProvider( return file_metrics_provider; } +base::OnceClosure CreateChainedClosure(base::OnceClosure cb1, + base::OnceClosure cb2) { + return base::BindOnce( + [](base::OnceClosure cb1, base::OnceClosure cb2) { + if (cb1) { + std::move(cb1).Run(); + } + if (cb2) { + std::move(cb2).Run(); + } + }, + std::move(cb1), std::move(cb2)); +} + } // namespace // Needs to be kept in sync with the writer in @@ -442,8 +456,11 @@ void AndroidMetricsServiceClient::CollectFinalMetricsForLog( // Set up the callback task to call after we receive histograms from all // child processes. |timeout| specifies how long to wait before absolutely // calling us back on the task. - content::FetchHistogramsAsynchronously(base::ThreadTaskRunnerHandle::Get(), - std::move(done_callback), timeout); + content::FetchHistogramsAsynchronously( + base::ThreadTaskRunnerHandle::Get(), + CreateChainedClosure(std::move(done_callback), + on_final_metrics_collected_listener_), + timeout); if (collect_final_metrics_for_log_closure_) std::move(collect_final_metrics_for_log_closure_).Run(); @@ -519,6 +536,11 @@ void AndroidMetricsServiceClient::SetCollectFinalMetricsForLogClosureForTesting( collect_final_metrics_for_log_closure_ = std::move(closure); } +void AndroidMetricsServiceClient::SetOnFinalMetricsCollectedListenerForTesting( + base::RepeatingClosure listener) { + on_final_metrics_collected_listener_ = std::move(listener); +} + int AndroidMetricsServiceClient::GetSampleBucketValue() { return UintToPerMille(base::PersistentHash(metrics_service_->GetClientId())); } diff --git a/components/embedder_support/android/metrics/android_metrics_service_client.h b/components/embedder_support/android/metrics/android_metrics_service_client.h index d0edeeaea09c38..7d6a9f6fb63cd5 100644 --- a/components/embedder_support/android/metrics/android_metrics_service_client.h +++ b/components/embedder_support/android/metrics/android_metrics_service_client.h @@ -154,9 +154,14 @@ class AndroidMetricsServiceClient : public MetricsServiceClient, const content::NotificationSource& source, const content::NotificationDetails& details) override; - // Runs |closure| when CollectFinalMetricsForLog() is called. + // Runs |closure| when CollectFinalMetricsForLog() is called, when we begin + // collecting final metrics. void SetCollectFinalMetricsForLogClosureForTesting(base::OnceClosure closure); + // Runs |listener| after all final metrics have been collected. + void SetOnFinalMetricsCollectedListenerForTesting( + base::RepeatingClosure listener); + metrics::MetricsStateManager* metrics_state_manager() const { return metrics_state_manager_.get(); } @@ -256,6 +261,7 @@ class AndroidMetricsServiceClient : public MetricsServiceClient, base::TimeDelta overridden_upload_interval_; base::OnceClosure collect_final_metrics_for_log_closure_; + base::RepeatingClosure on_final_metrics_collected_listener_; // MetricsServiceClient may be created before the UI thread is promoted to // BrowserThread::UI. Use |sequence_checker_| to enforce that the