Prepare heap profiling for integration with Android WebView.

This CL is a refactor with no intended behavior change outside of tests.

Test behavior change:
* TestDriver::CheckOrStartProfiling now waits for the allocator shim to be
  initialized even if profiling was enabled for the current process via command
  line flags. This is because initialization is asynchronous.
* TestDriver::ValidateRendererAllocations no longer makes any checks if the
  renderer is not being profiled, since on Android WebView there may not even be
  a renderer process in this case.

Refactor:
* Moves the C++ side of the android test shim into components/heap_profiling, so
  that the logic can be shared with Android Webview.
* Renames the test shim from TestAndroidShim to HeapProfilingTestShim.
* Removes redundant checks in TestDriver::ValidateRendererAllocations.

Change-Id: I4755f6169b0f9c556a86005916aa9150dec6853b
BUG: 827545
Reviewed-on: https://chromium-review.googlesource.com/1012542Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551242}

chrome/android/BUILD.gn

@@ -848,10 +848,10 @@ chrome_shared_library("libchrome") {
 chrome_shared_library("libchromefortest") {
   testonly = true
   sources = [
+    "../../components/heap_profiling/heap_profiling_test_shim.cc",
+    "../../components/heap_profiling/heap_profiling_test_shim.h",
     "../app/android/chrome_main_delegate_android_initializer.cc",
     "../browser/android/chrome_entry_point_for_test.cc",
-    "../browser/profiling_host/test_android_shim.cc",
-    "../browser/profiling_host/test_android_shim.h",
   ]
   deps = [
     ":chrome_jni_for_test_registration($default_toolchain)",

chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/TestAndroidShim.java → chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/HeapProfilingTestShim.java

@@ -7,13 +7,13 @@ package org.chromium.chrome.browser.profiling_host;
 import org.chromium.base.annotations.MainDex;
 
 /**
- * Provides direct access to test_android_shim, which in turn forwards to
- * ProfilingTestDriver. Only used for testing.
+ * Provides direct access to heap_profiling_test_shim, which in turn forwards to
+ * heap_profiling::TestDriver. Only used for testing.
  */
 @MainDex
-public class TestAndroidShim {
-    public TestAndroidShim() {
-        mNativeTestAndroidShim = nativeInit();
+public class HeapProfilingTestShim {
+    public HeapProfilingTestShim() {
+        mNativeHeapProfilingTestShim = nativeInit();
     }
 
     /**
@@ -24,7 +24,7 @@ public class TestAndroidShim {
      */
     public boolean runTestForMode(String mode, boolean dynamicallyStartProfiling, String stackMode,
             boolean shouldSample, boolean sampleEverything) {
-        return nativeRunTestForMode(mNativeTestAndroidShim, mode, dynamicallyStartProfiling,
+        return nativeRunTestForMode(mNativeHeapProfilingTestShim, mode, dynamicallyStartProfiling,
                 stackMode, shouldSample, sampleEverything);
     }
 
@@ -33,16 +33,16 @@ public class TestAndroidShim {
      * After the call, this class instance shouldn't be used.
      */
     public void destroy() {
-        if (mNativeTestAndroidShim != 0) {
-            nativeDestroy(mNativeTestAndroidShim);
-            mNativeTestAndroidShim = 0;
+        if (mNativeHeapProfilingTestShim != 0) {
+            nativeDestroy(mNativeHeapProfilingTestShim);
+            mNativeHeapProfilingTestShim = 0;
         }
     }
 
-    private long mNativeTestAndroidShim;
+    private long mNativeHeapProfilingTestShim;
     private native long nativeInit();
-    private native void nativeDestroy(long nativeTestAndroidShim);
-    private native boolean nativeRunTestForMode(long nativeTestAndroidShim, String mode,
+    private native void nativeDestroy(long nativeHeapProfilingTestShim);
+    private native boolean nativeRunTestForMode(long nativeHeapProfilingTestShim, String mode,
             boolean dynamicallyStartProfiling, String stackMode, boolean shouldSample,
             boolean sampleEverything);
 }

chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/ProfilingProcessHostAndroidTest.java

@@ -38,25 +38,23 @@ public class ProfilingProcessHostAndroidTest {
     @MediumTest
     @CommandLineFlags.Add({"memlog=browser", "memlog-stack-mode=native-include-thread-names"})
     public void testModeBrowser() throws Exception {
-        TestAndroidShim profilingProcessHost = new TestAndroidShim();
-        Assert.assertTrue(profilingProcessHost.runTestForMode(
-                "browser", false, "native-include-thread-names", false, false));
+        HeapProfilingTestShim shim = new HeapProfilingTestShim();
+        Assert.assertTrue(
+                shim.runTestForMode("browser", false, "native-include-thread-names", false, false));
     }
 
     @Test
     @MediumTest
     public void testModeBrowserDynamic() throws Exception {
-        TestAndroidShim profilingProcessHost = new TestAndroidShim();
-        Assert.assertTrue(
-                profilingProcessHost.runTestForMode("browser", true, "native", false, false));
+        HeapProfilingTestShim shim = new HeapProfilingTestShim();
+        Assert.assertTrue(shim.runTestForMode("browser", true, "native", false, false));
     }
 
     @Test
     @MediumTest
     public void testModeBrowserDynamicPseudo() throws Exception {
-        TestAndroidShim profilingProcessHost = new TestAndroidShim();
-        Assert.assertTrue(
-                profilingProcessHost.runTestForMode("browser", true, "pseudo", false, false));
+        HeapProfilingTestShim shim = new HeapProfilingTestShim();
+        Assert.assertTrue(shim.runTestForMode("browser", true, "pseudo", false, false));
     }
 
     // Non-browser processes must be profiled with a command line flag, since
@@ -66,33 +64,29 @@ public class ProfilingProcessHostAndroidTest {
     @MediumTest
     @CommandLineFlags.Add({"memlog=all-renderers", "memlog-stack-mode=pseudo"})
     public void testModeRendererPseudo() throws Exception {
-        TestAndroidShim profilingProcessHost = new TestAndroidShim();
-        Assert.assertTrue(profilingProcessHost.runTestForMode(
-                "all-renderers", false, "pseudo", false, false));
+        HeapProfilingTestShim shim = new HeapProfilingTestShim();
+        Assert.assertTrue(shim.runTestForMode("all-renderers", false, "pseudo", false, false));
     }
 
     @Test
     @MediumTest
     @CommandLineFlags.Add({"memlog=gpu", "memlog-stack-mode=pseudo"})
     public void testModeGpuPseudo() throws Exception {
-        TestAndroidShim profilingProcessHost = new TestAndroidShim();
-        Assert.assertTrue(
-                profilingProcessHost.runTestForMode("gpu", false, "native", false, false));
+        HeapProfilingTestShim shim = new HeapProfilingTestShim();
+        Assert.assertTrue(shim.runTestForMode("gpu", false, "native", false, false));
     }
 
     @Test
     @MediumTest
     public void testModeBrowserDynamicPseudoSampleEverything() throws Exception {
-        TestAndroidShim profilingProcessHost = new TestAndroidShim();
-        Assert.assertTrue(
-                profilingProcessHost.runTestForMode("browser", true, "pseudo", true, true));
+        HeapProfilingTestShim shim = new HeapProfilingTestShim();
+        Assert.assertTrue(shim.runTestForMode("browser", true, "pseudo", true, true));
     }
 
     @Test
     @MediumTest
     public void testModeBrowserDynamicPseudoSamplePartial() throws Exception {
-        TestAndroidShim profilingProcessHost = new TestAndroidShim();
-        Assert.assertTrue(
-                profilingProcessHost.runTestForMode("browser", true, "pseudo", true, false));
+        HeapProfilingTestShim shim = new HeapProfilingTestShim();
+        Assert.assertTrue(shim.runTestForMode("browser", true, "pseudo", true, false));
     }
 }

chrome/browser/profiling_host/BUILD.gn

@@ -52,7 +52,7 @@ if (!is_android) {
 
   generate_jni("jni_headers") {
     sources = [
-      "../../android/javatests/src/org/chromium/chrome/browser/profiling_host/TestAndroidShim.java",
+      "../../android/javatests/src/org/chromium/chrome/browser/profiling_host/HeapProfilingTestShim.java",
     ]
     jni_package = "chrome_profiling_host"
   }
@@ -61,7 +61,7 @@ if (!is_android) {
   # shim to function correctly.
   android_library("profiling_host_java_test_support") {
     testonly = true
-    java_files = [ "../../android/javatests/src/org/chromium/chrome/browser/profiling_host/TestAndroidShim.java" ]
+    java_files = [ "../../android/javatests/src/org/chromium/chrome/browser/profiling_host/HeapProfilingTestShim.java" ]
     deps = [
       "//base:base_java",
     ]

components/heap_profiling/DEPS

 include_rules = [
   "+components/services/heap_profiling/public",
   "+content/public",
+  "+jni",
   "+services/resource_coordinator/public/cpp",
   "+services/service_manager/public/cpp",
 ]

chrome/browser/profiling_host/test_android_shim.cc → components/heap_profiling/heap_profiling_test_shim.cc

@@ -2,31 +2,32 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/profiling_host/test_android_shim.h"
+#include "components/heap_profiling/heap_profiling_test_shim.h"
 
 #include "base/android/jni_string.h"
 #include "components/heap_profiling/test_driver.h"
 #include "components/services/heap_profiling/public/cpp/settings.h"
-#include "jni/TestAndroidShim_jni.h"
+#include "jni/HeapProfilingTestShim_jni.h"
 
 using base::android::JavaParamRef;
 using base::android::ScopedJavaLocalRef;
 
-static jlong JNI_TestAndroidShim_Init(JNIEnv* env,
-                                      const JavaParamRef<jobject>& obj) {
-  TestAndroidShim* profiler = new TestAndroidShim(env, obj);
+static jlong JNI_HeapProfilingTestShim_Init(JNIEnv* env,
+                                            const JavaParamRef<jobject>& obj) {
+  HeapProfilingTestShim* profiler = new HeapProfilingTestShim(env, obj);
   return reinterpret_cast<intptr_t>(profiler);
 }
 
-TestAndroidShim::TestAndroidShim(JNIEnv* env, jobject obj) {}
+HeapProfilingTestShim::HeapProfilingTestShim(JNIEnv* env, jobject obj) {}
 
-TestAndroidShim::~TestAndroidShim() {}
+HeapProfilingTestShim::~HeapProfilingTestShim() {}
 
-void TestAndroidShim::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
+void HeapProfilingTestShim::Destroy(JNIEnv* env,
+                                    const JavaParamRef<jobject>& obj) {
   delete this;
 }
 
-jboolean TestAndroidShim::RunTestForMode(
+jboolean HeapProfilingTestShim::RunTestForMode(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& obj,
     const base::android::JavaParamRef<jstring>& mode,

chrome/browser/profiling_host/test_android_shim.h → components/heap_profiling/heap_profiling_test_shim.h

@@ -2,19 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_PROFILING_HOST_TEST_ANDROID_SHIM_H_
-#define CHROME_BROWSER_PROFILING_HOST_TEST_ANDROID_SHIM_H_
+#ifndef COMPONENTS_HEAP_PROFILING_HEAP_PROFILING_TEST_SHIM_H_
+#define COMPONENTS_HEAP_PROFILING_HEAP_PROFILING_TEST_SHIM_H_
 
 #include "base/android/jni_android.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/macros.h"
 
-// This class implements the native methods of TestAndroidShim.java,
-// and acts as a bridge to ProfilingProcessHost. Note that this class is only
-// used for testing.
-class TestAndroidShim {
+// This class implements the native methods of HeapProfilingTestShim.java, and
+// acts as a bridge to TestDriver. Note that this class is only used for
+// testing.
+class HeapProfilingTestShim {
  public:
-  TestAndroidShim(JNIEnv* env, jobject obj);
+  HeapProfilingTestShim(JNIEnv* env, jobject obj);
   void Destroy(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
 
   jboolean RunTestForMode(
@@ -27,9 +27,9 @@ class TestAndroidShim {
       jboolean sample_everything);
 
  private:
-  ~TestAndroidShim();
+  ~HeapProfilingTestShim();
 
-  DISALLOW_COPY_AND_ASSIGN(TestAndroidShim);
+  DISALLOW_COPY_AND_ASSIGN(HeapProfilingTestShim);
 };
 
-#endif  // CHROME_BROWSER_PROFILING_HOST_TEST_ANDROID_SHIM_H_
+#endif  // COMPONENTS_HEAP_PROFILING_HEAP_PROFILING_TEST_SHIM_H_

components/heap_profiling/test_driver.cc

@@ -647,8 +647,31 @@ bool TestDriver::CheckOrStartProfiling() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   if (options_.profiling_already_started) {
-    if (Supervisor::GetInstance()->HasStarted())
+    if (Supervisor::GetInstance()->HasStarted()) {
+      // Even if profiling has started, it's possible that the allocator shim
+      // has not yet been initialized. Wait for it.
+      if (ShouldProfileBrowser()) {
+        bool already_initialized = false;
+        std::unique_ptr<base::RunLoop> run_loop;
+        if (running_on_ui_thread_) {
+          run_loop.reset(new base::RunLoop);
+          already_initialized = SetOnInitAllocatorShimCallbackForTesting(
+              run_loop->QuitClosure(), base::ThreadTaskRunnerHandle::Get());
+
+          if (!already_initialized)
+            run_loop->Run();
+        } else {
+          already_initialized = SetOnInitAllocatorShimCallbackForTesting(
+              base::Bind(&base::WaitableEvent::Signal,
+                         base::Unretained(&wait_for_ui_thread_)),
+              base::ThreadTaskRunnerHandle::Get());
+          if (!already_initialized) {
+            wait_for_profiling_to_start_ = true;
+          }
+        }
+      }
       return true;
+    }
     LOG(ERROR) << "Profiling should have been started, but wasn't";
     return false;
   }
@@ -880,6 +903,11 @@ bool TestDriver::ValidateBrowserAllocations(base::Value* dump_json) {
 }
 
 bool TestDriver::ValidateRendererAllocations(base::Value* dump_json) {
+  // On Android Webview, there is may not be a separate Renderer process. If we
+  // are not asked to profile the Renderer, do not perform any Renderer checks.
+  if (!ShouldProfileRenderer())
+    return true;
+
   std::vector<int> pids;
   bool result = NumProcessesWithName(dump_json, "Renderer", &pids) >= 1;
   if (!result) {
@@ -890,41 +918,16 @@ bool TestDriver::ValidateRendererAllocations(base::Value* dump_json) {
   for (int pid : pids) {
     base::ProcessId renderer_pid = static_cast<base::ProcessId>(pid);
     base::Value* heaps_v2 = FindArgDump(renderer_pid, dump_json, "heaps_v2");
-    if (ShouldProfileRenderer()) {
-      if (!heaps_v2) {
-        LOG(ERROR) << "Failed to find heaps v2 for renderer";
-        return false;
-      }
-
-      base::Value* process_mmaps =
-          FindArgDump(renderer_pid, dump_json, "process_mmaps");
-      if (!ValidateProcessMmaps(process_mmaps, HasNativeFrames())) {
-        LOG(ERROR) << "Failed to validate renderer process mmaps.";
-        return false;
-      }
-
-      // ValidateDump doesn't always succeed for the renderer, since we don't do
-      // anything to flush allocations, there are very few allocations recorded
-      // by the heap profiler. When we do a heap dump, we prune small
-      // allocations...and this can cause all allocations to be pruned.
-      // ASSERT_NO_FATAL_FAILURE(ValidateDump(dump_json.get(), 0, 0));
-    } else {
-      if (heaps_v2) {
-        LOG(ERROR) << "There should be no heap dump for the renderer.";
-        return false;
-      }
+    if (!heaps_v2) {
+      LOG(ERROR) << "Failed to find heaps v2 for renderer";
+      return false;
     }
 
-    if (options_.mode == Mode::kAllRenderers) {
-      if (NumProcessesWithName(dump_json, "Renderer", nullptr) == 0) {
-        LOG(ERROR) << "There should be at least 1 renderer dump";
-        return false;
-      }
-    } else {
-      if (NumProcessesWithName(dump_json, "Renderer", nullptr) == 0) {
-        LOG(ERROR) << "There should be more than 1 renderer dump";
-        return false;
-      }
+    base::Value* process_mmaps =
+        FindArgDump(renderer_pid, dump_json, "process_mmaps");
+    if (!ValidateProcessMmaps(process_mmaps, HasNativeFrames())) {
+      LOG(ERROR) << "Failed to validate renderer process mmaps.";
+      return false;
     }
   }
 

components/services/heap_profiling/public/cpp/allocator_shim.cc

@@ -78,6 +78,8 @@ namespace {
 
 using base::allocator::AllocatorDispatch;
 
+bool g_initialized_ = false;
+base::LazyInstance<base::Lock>::Leaky g_on_init_allocator_shim_lock_;
 base::LazyInstance<base::OnceClosure>::Leaky g_on_init_allocator_shim_callback_;
 base::LazyInstance<scoped_refptr<base::TaskRunner>>::Leaky
     g_on_init_allocator_shim_task_runner_;
@@ -246,8 +248,9 @@ class SendBuffer {
   void SendCurrentBuffer() {
     SenderPipe::Result result = g_sender_pipe->Send(buffer_, used_, kTimeoutMs);
     used_ = 0;
-    if (result == SenderPipe::Result::kError)
+    if (result == SenderPipe::Result::kError) {
       StopAllocatorShimDangerous();
+    }
     if (result == SenderPipe::Result::kTimeout) {
       StopAllocatorShimDangerous();
       // TODO(erikchen): Emit a histogram. https://crbug.com/777546.
@@ -694,10 +697,14 @@ void InitAllocatorShim(SenderPipe* sender_pipe,
     g_hook_gc_free(&HookGCFree);
   }
 
-  if (*g_on_init_allocator_shim_callback_.Pointer()) {
-    (*g_on_init_allocator_shim_task_runner_.Pointer())
-        ->PostTask(FROM_HERE,
-                   std::move(*g_on_init_allocator_shim_callback_.Pointer()));
+  {
+    base::AutoLock lock(*g_on_init_allocator_shim_lock_.Pointer());
+    g_initialized_ = true;
+    if (*g_on_init_allocator_shim_callback_.Pointer()) {
+      (*g_on_init_allocator_shim_task_runner_.Pointer())
+          ->PostTask(FROM_HERE,
+                     std::move(*g_on_init_allocator_shim_callback_.Pointer()));
+    }
   }
 }
 
@@ -886,11 +893,15 @@ void SetGCHeapAllocationHookFunctions(SetGCAllocHookFunction hook_alloc,
   }
 }
 
-void SetOnInitAllocatorShimCallbackForTesting(
+bool SetOnInitAllocatorShimCallbackForTesting(
     base::OnceClosure callback,
     scoped_refptr<base::TaskRunner> task_runner) {
+  base::AutoLock lock(*g_on_init_allocator_shim_lock_.Pointer());
+  if (g_initialized_)
+    return true;
   *g_on_init_allocator_shim_callback_.Pointer() = std::move(callback);
   *g_on_init_allocator_shim_task_runner_.Pointer() = task_runner;
+  return false;
 }
 
 }  // namespace heap_profiling

components/services/heap_profiling/public/cpp/allocator_shim.h

@@ -55,9 +55,11 @@ using SetGCFreeHookFunction = void (*)(void (*)(uint8_t*));
 void SetGCHeapAllocationHookFunctions(SetGCAllocHookFunction hook_alloc,
                                       SetGCFreeHookFunction hook_free);
 
-// Exists for testing only. |callback| is called on |task_runner| after the
-// allocator shim is initialized.
-void SetOnInitAllocatorShimCallbackForTesting(
+// Exists for testing only.
+// A return value of |true| means that the allocator shim was already
+// initialized and |callback| will never be called. Otherwise, |callback| will
+// be called on |task_runner| after the allocator shim is initialized.
+bool SetOnInitAllocatorShimCallbackForTesting(
     base::OnceClosure callback,
     scoped_refptr<base::TaskRunner> task_runner);
 

components/services/heap_profiling/public/cpp/client.cc

@@ -43,15 +43,17 @@ void EnsureCFIInitializedOnBackgroundThread(
 Client::Client() : started_profiling_(false), weak_factory_(this) {}
 
 Client::~Client() {
-  StopAllocatorShimDangerous();
+  if (started_profiling_) {
+    StopAllocatorShimDangerous();
 
-  base::trace_event::MallocDumpProvider::GetInstance()->EnableMetrics();
+    base::trace_event::MallocDumpProvider::GetInstance()->EnableMetrics();
 
-  // The allocator shim cannot be synchronously, consistently stopped. We leak
-  // the sender_pipe_, with the idea that very few future messages will
-  // be sent to it. This happens at shutdown, so resources will be reclaimed by
-  // the OS after the process is terminated.
-  sender_pipe_.release();
+    // The allocator shim cannot be synchronously, consistently stopped. We leak
+    // the sender_pipe_, with the idea that very few future messages will
+    // be sent to it. This happens at shutdown, so resources will be reclaimed
+    // by the OS after the process is terminated.
+    sender_pipe_.release();
+  }
 }
 
 void Client::BindToInterface(mojom::ProfilingClientRequest request) {