Fix flakiness in Android renderer OOPHP test.

The current flakiness occurs in the form of a timeout waiting for profiling to
start for renderers. On Android, there's a warm-up renderer that could start
before ProfilingProcessHost gets a chance to start up - as such, it will never
be profiled. This CL adds logic to force profiling to start for all renderers,
which should fix the flakiness.

This CL also fixes a bug in ProfilingClient, which was only keeping the most
recent binding.

Bug: 804412
Change-Id: I836e55650b30a6360e0bc0f9189aacb7a75ea0c9
Reviewed-on: https://chromium-review.googlesource.com/889999Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532544}

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

@@ -13,7 +13,6 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import org.chromium.base.test.util.CommandLineFlags;
-import org.chromium.base.test.util.DisabledTest;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.test.ChromeActivityTestRule;
@@ -64,8 +63,6 @@ public class ProfilingProcessHostAndroidTest {
     @Test
     @MediumTest
     @CommandLineFlags.Add({"memlog=all-renderers", "memlog-stack-mode=pseudo"})
-    // Disabled: https://crbug.com/804412
-    @DisabledTest
     public void testModeRendererPseudo() throws Exception {
         TestAndroidShim profilingProcessHost = new TestAndroidShim();
         Assert.assertTrue(profilingProcessHost.runTestForMode("all-renderers", false, "pseudo"));

chrome/browser/profiling_host/profiling_process_host.cc

@@ -676,6 +676,14 @@ void ProfilingProcessHost::StartManualProfiling(base::ProcessId pid) {
                          base::Unretained(this), pid));
 }
 
+void ProfilingProcessHost::StartProfilingRenderersForTesting() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  for (auto iter = content::RenderProcessHost::AllHostsIterator();
+       !iter.IsAtEnd(); iter.Advance()) {
+    StartProfilingRenderer(iter.GetCurrentValue());
+  }
+}
+
 void ProfilingProcessHost::StartProfilingPidOnIOThread(base::ProcessId pid) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
 

chrome/browser/profiling_host/profiling_process_host.h

@@ -159,6 +159,11 @@ class ProfilingProcessHost : public content::BrowserChildProcessObserver,
   // Starts profiling the process with the given id.
   void StartManualProfiling(base::ProcessId pid);
 
+  // This function starts profiling all renderers. Attempting to start profiling
+  // a renderer that is already being profiled is a no-op [the new request is
+  // dropped by the profiling service].
+  void StartProfilingRenderersForTesting();
+
  private:
   friend struct base::DefaultSingletonTraits<ProfilingProcessHost>;
   friend class BackgroundProfilingTriggersTest;

chrome/browser/profiling_host/profiling_test_driver.cc

@@ -429,6 +429,15 @@ bool ProfilingTestDriver::RunTest(const Options& options) {
     if (!initialization_success_)
       return false;
     if (ShouldProfileRenderer()) {
+      // On Android, there is a warm-up renderer that is sometimes started
+      // before the ProfilingProcessHost can be started. This renderer will
+      // therefore not be profiled, even if Chrome is started with the
+      // --memlog=all-renderers switch.
+      content::BrowserThread::PostTask(
+          content::BrowserThread::UI, FROM_HERE,
+          base::Bind(&ProfilingProcessHost::StartProfilingRenderersForTesting,
+                     base::Unretained(ProfilingProcessHost::GetInstance())));
+
       content::BrowserThread::PostTask(
           content::BrowserThread::UI, FROM_HERE,
           base::Bind(

chrome/common/profiling/profiling_client.cc

@@ -21,7 +21,7 @@ const int kTimeoutDurationMs = 10000;
 }  // namespace
 
 ProfilingClient::ProfilingClient()
-    : binding_(this), started_profiling_(false) {}
+    : started_profiling_(false) {}
 
 ProfilingClient::~ProfilingClient() {
   StopAllocatorShimDangerous();
@@ -46,7 +46,7 @@ void ProfilingClient::OnServiceManagerConnected(
 }
 
 void ProfilingClient::BindToInterface(mojom::ProfilingClientRequest request) {
-  binding_.Bind(std::move(request));
+  bindings_.AddBinding(this, std::move(request));
 }
 
 void ProfilingClient::StartProfiling(mojo::ScopedHandle memlog_sender_pipe,

chrome/common/profiling/profiling_client.h

@@ -6,7 +6,7 @@
 #define CHROME_COMMON_PROFILING_PROFILING_CLIENT_H_
 
 #include "chrome/common/profiling/profiling_client.mojom.h"
-#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "mojo/public/cpp/system/handle.h"
 
 namespace content {
@@ -36,8 +36,13 @@ class ProfilingClient : public mojom::ProfilingClient {
   void BindToInterface(profiling::mojom::ProfilingClientRequest request);
 
  private:
-  // The most recent client request is bound and kept alive.
-  mojo::Binding<mojom::ProfilingClient> binding_;
+  // Ideally, this would be a mojo::Binding that would only keep alive one
+  // client request. However, the service that makes the client requests
+  // [content_browser] is different from the service that dedupes the client
+  // requests [profiling service]. This means that there may be a brief
+  // intervals where there are two active bindings, until the profiling service
+  // has a chance to figure out which one to keep.
+  mojo::BindingSet<mojom::ProfilingClient> bindings_;
 
   bool started_profiling_;