[PM] Add off-sequence wrapper for single-process V8DetailedMemoryRequest

Also adds a GetProcessNodeForRenderProcessHostId accessor to
PerformanceManager.

R=fdoray

Bug: 1080672
Change-Id: I3c2ceb14643833867d836c06c8d409889e24ba46
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2456667
Commit-Queue: Joe Mason <joenotcharles@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816895}

components/performance_manager/performance_manager.cc

@@ -13,6 +13,7 @@
 #include "components/performance_manager/performance_manager_registry_impl.h"
 #include "components/performance_manager/performance_manager_tab_helper.h"
 #include "components/performance_manager/public/performance_manager_owned.h"
+#include "content/public/browser/render_process_host.h"
 
 namespace performance_manager {
 
@@ -101,6 +102,17 @@ PerformanceManager::GetProcessNodeForRenderProcessHost(
   return user_data->process_node()->GetWeakPtr();
 }
 
+// static
+base::WeakPtr<ProcessNode>
+PerformanceManager::GetProcessNodeForRenderProcessHostId(
+    RenderProcessHostId id) {
+  DCHECK(id);
+  auto* rph = content::RenderProcessHost::FromID(id.value());
+  if (!rph)
+    return nullptr;
+  return GetProcessNodeForRenderProcessHost(rph);
+}
+
 // static
 void PerformanceManager::AddObserver(
     PerformanceManagerMainThreadObserver* observer) {

components/performance_manager/public/performance_manager.h

@@ -12,6 +12,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequenced_task_runner.h"
+#include "components/performance_manager/public/render_process_host_id.h"
 
 namespace content {
 class RenderFrameHost;
@@ -96,6 +97,16 @@ class PerformanceManager {
   static base::WeakPtr<ProcessNode> GetProcessNodeForRenderProcessHost(
       content::RenderProcessHost* rph);
 
+  // Returns a WeakPtr to the ProcessNode associated with a given
+  // RenderProcessHostId (which must be valid), or a null WeakPtr if there's no
+  // ProcessNode for this ID. (There may be no RenderProcessHost for this ID,
+  // or it may be during a brief window after the RPH is created but before the
+  // ProcessNode is added.) Valid to call from the main thread only, the
+  // returned WeakPtr should only be dereferenced on the PM sequence (e.g. it
+  // can be used in a CallOnGraph callback).
+  static base::WeakPtr<ProcessNode> GetProcessNodeForRenderProcessHostId(
+      RenderProcessHostId id);
+
   // Adds / removes an observer that is notified of PerformanceManager events
   // that happen on the main thread. Can only be called on the main thread.
   static void AddObserver(PerformanceManagerMainThreadObserver* observer);

components/performance_manager/public/v8_memory/v8_detailed_memory.h

@@ -11,6 +11,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
 #include "base/time/time.h"
@@ -232,8 +233,8 @@ class V8DetailedMemoryDecorator
   // Implementation details below this point.
 
   // V8DetailedMemoryRequest objects register themselves with the decorator.
-  // If |process_node| is null, the request will be sent to every process,
-  // otherwise it will be sent only to |process_node|.
+  // If |process_node| is null, the request will be sent to every renderer
+  // process, otherwise it will be sent only to |process_node|.
   void AddMeasurementRequest(util::PassKey<V8DetailedMemoryRequest>,
                              V8DetailedMemoryRequest* request,
                              const ProcessNode* process_node = nullptr);
@@ -411,11 +412,14 @@ class V8DetailedMemoryRequest {
   // Private constructor for V8DetailedMemoryRequestAnySeq. Saves
   // |off_sequence_request| as a pointer to the off-sequence object that
   // triggered the request and starts measurements with frequency
-  // |min_time_between_requests|.
+  // |min_time_between_requests|. If |process_to_measure| is nullopt, the
+  // request will be sent to every renderer process, otherwise it will be sent
+  // only to |process_to_measure|.
   V8DetailedMemoryRequest(
       util::PassKey<V8DetailedMemoryRequestAnySeq>,
       const base::TimeDelta& min_time_between_requests,
       MeasurementMode mode,
+      base::Optional<base::WeakPtr<ProcessNode>> process_to_measure,
       base::WeakPtr<V8DetailedMemoryRequestAnySeq> off_sequence_request);
 
   // Private constructor for V8DetailedMemoryRequestOneShot. Sets
@@ -437,6 +441,9 @@ class V8DetailedMemoryRequest {
       const ProcessNode* process_node) const;
 
  private:
+  void StartMeasurementFromOffSequence(
+      base::Optional<base::WeakPtr<ProcessNode>> process_to_measure,
+      Graph* graph);
   void StartMeasurementImpl(Graph* graph, const ProcessNode* process_node);
 
   base::TimeDelta min_time_between_requests_;
@@ -534,9 +541,16 @@ class V8DetailedMemoryRequestAnySeq {
  public:
   using MeasurementMode = V8DetailedMemoryRequest::MeasurementMode;
 
+  // Creates a memory measurement request that will be sent repeatedly with at
+  // least |min_time_between_requests| between each measurement. The request
+  // will be sent to the process with ID |process_to_measure|, which must be a
+  // renderer process, or to all renderer processes if |process_to_measure| is
+  // nullopt. The process will perform the measurement during a GC as determined
+  // by |mode|.
   explicit V8DetailedMemoryRequestAnySeq(
       const base::TimeDelta& min_time_between_requests,
-      MeasurementMode mode = MeasurementMode::kDefault);
+      MeasurementMode mode = MeasurementMode::kDefault,
+      base::Optional<RenderProcessHostId> process_to_measure = base::nullopt);
   ~V8DetailedMemoryRequestAnySeq();
 
   V8DetailedMemoryRequestAnySeq(const V8DetailedMemoryRequestAnySeq&) = delete;
@@ -564,6 +578,11 @@ class V8DetailedMemoryRequestAnySeq {
       const V8DetailedMemoryObserverAnySeq::FrameDataMap& frame_data) const;
 
  private:
+  void InitializeWrappedRequest(
+      const base::TimeDelta& min_time_between_requests,
+      MeasurementMode mode,
+      base::Optional<base::WeakPtr<ProcessNode>> process_to_measure);
+
   std::unique_ptr<V8DetailedMemoryRequest> request_;
   base::ObserverList<V8DetailedMemoryObserverAnySeq, /*check_empty=*/true>
       observers_;

components/performance_manager/v8_memory/v8_detailed_memory.cc

@@ -592,15 +592,18 @@ V8DetailedMemoryRequest::V8DetailedMemoryRequest(
     util::PassKey<V8DetailedMemoryRequestAnySeq>,
     const base::TimeDelta& min_time_between_requests,
     MeasurementMode mode,
+    base::Optional<base::WeakPtr<ProcessNode>> process_to_measure,
     base::WeakPtr<V8DetailedMemoryRequestAnySeq> off_sequence_request)
     : V8DetailedMemoryRequest(min_time_between_requests, mode) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
   off_sequence_request_ = std::move(off_sequence_request);
   off_sequence_request_sequence_ = base::SequencedTaskRunnerHandle::Get();
-  // Unretained is safe since |this| will be destroyed on the graph sequence.
+  // Unretained is safe since |this| will be destroyed on the graph sequence
+  // from an async task posted after this.
   PerformanceManager::CallOnGraph(
-      FROM_HERE, base::BindOnce(&V8DetailedMemoryRequest::StartMeasurement,
-                                base::Unretained(this)));
+      FROM_HERE,
+      base::BindOnce(&V8DetailedMemoryRequest::StartMeasurementFromOffSequence,
+                     base::Unretained(this), std::move(process_to_measure)));
 }
 
 V8DetailedMemoryRequest::V8DetailedMemoryRequest(
@@ -700,6 +703,22 @@ void V8DetailedMemoryRequest::NotifyObserversOnMeasurementAvailable(
     observer.OnV8MemoryMeasurementAvailable(process_node, process_data);
 }
 
+void V8DetailedMemoryRequest::StartMeasurementFromOffSequence(
+    base::Optional<base::WeakPtr<ProcessNode>> process_to_measure,
+    Graph* graph) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (!process_to_measure) {
+    // No process was given so measure all renderers in the graph.
+    StartMeasurement(graph);
+  } else if (!process_to_measure.value()) {
+    // V8DetailedMemoryRequestAnySeq was called with a process ID that wasn't
+    // found in the graph, or has already been destroyed. Do nothing.
+  } else {
+    DCHECK_EQ(graph, process_to_measure.value()->GetGraph());
+    StartMeasurementForProcess(process_to_measure.value().get());
+  }
+}
+
 void V8DetailedMemoryRequest::StartMeasurementImpl(
     Graph* graph,
     const ProcessNode* process_node) {
@@ -1069,16 +1088,28 @@ void V8DetailedMemoryDecorator::MeasurementRequestQueue::Validate() {
 
 V8DetailedMemoryRequestAnySeq::V8DetailedMemoryRequestAnySeq(
     const base::TimeDelta& min_time_between_requests,
-    MeasurementMode mode) {
-  // |request_| must be initialized in the constructor body so that
-  // |weak_factory_| is completely constructed.
-  //
-  // Can't use make_unique since this calls the private any-sequence
-  // constructor. After construction the V8DetailedMemoryRequest must only be
-  // accessed on the graph sequence.
-  request_ = base::WrapUnique(new V8DetailedMemoryRequest(
-      util::PassKey<V8DetailedMemoryRequestAnySeq>(), min_time_between_requests,
-      mode, weak_factory_.GetWeakPtr()));
+    MeasurementMode mode,
+    base::Optional<RenderProcessHostId> process_to_measure) {
+  base::Optional<base::WeakPtr<ProcessNode>> process_node;
+  if (process_to_measure) {
+    // GetProcessNodeForRenderProcessHostId must be called from the UI thread.
+    auto ui_task_runner = content::GetUIThreadTaskRunner({});
+    if (!ui_task_runner->RunsTasksInCurrentSequence()) {
+      ui_task_runner->PostTaskAndReplyWithResult(
+          FROM_HERE,
+          base::BindOnce(
+              &PerformanceManager::GetProcessNodeForRenderProcessHostId,
+              process_to_measure.value()),
+          base::BindOnce(
+              &V8DetailedMemoryRequestAnySeq::InitializeWrappedRequest,
+              weak_factory_.GetWeakPtr(), min_time_between_requests, mode));
+      return;
+    }
+    process_node = PerformanceManager::GetProcessNodeForRenderProcessHostId(
+        process_to_measure.value());
+  }
+  InitializeWrappedRequest(min_time_between_requests, mode,
+                           std::move(process_node));
 }
 
 V8DetailedMemoryRequestAnySeq::~V8DetailedMemoryRequestAnySeq() {
@@ -1121,6 +1152,19 @@ void V8DetailedMemoryRequestAnySeq::NotifyObserversOnMeasurementAvailable(
                                             process_data, frame_data);
 }
 
+void V8DetailedMemoryRequestAnySeq::InitializeWrappedRequest(
+    const base::TimeDelta& min_time_between_requests,
+    MeasurementMode mode,
+    base::Optional<base::WeakPtr<ProcessNode>> process_to_measure) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Can't use make_unique since this calls the private any-sequence
+  // constructor. After construction the V8DetailedMemoryRequest must only be
+  // accessed on the graph sequence.
+  request_ = base::WrapUnique(new V8DetailedMemoryRequest(
+      util::PassKey<V8DetailedMemoryRequestAnySeq>(), min_time_between_requests,
+      mode, std::move(process_to_measure), weak_factory_.GetWeakPtr()));
+}
+
 }  // namespace v8_memory
 
 }  // namespace performance_manager

components/performance_manager/v8_memory/v8_detailed_memory_unittest.cc

@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
@@ -28,6 +29,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/tokens/tokens.h"
@@ -312,6 +314,23 @@ class V8DetailedMemoryRequestAnySeqTest
     : public PerformanceManagerTestHarness,
       public V8DetailedMemoryDecoratorTestBase {
  public:
+  void SetUp() override {
+    PerformanceManagerTestHarness::SetUp();
+
+    // Precondition: CallOnGraph must run on a different sequence. Note that
+    // all tasks passed to CallOnGraph will only run when run_loop.Run() is
+    // called.
+    ASSERT_TRUE(GetMainThreadTaskRunner()->RunsTasksInCurrentSequence());
+    base::RunLoop run_loop;
+    PerformanceManager::CallOnGraph(
+        FROM_HERE, base::BindLambdaForTesting([&] {
+          EXPECT_FALSE(
+              this->GetMainThreadTaskRunner()->RunsTasksInCurrentSequence());
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+
   scoped_refptr<base::SingleThreadTaskRunner> GetMainThreadTaskRunner()
       override {
     return task_environment()->GetMainThreadTaskRunner();
@@ -1626,16 +1645,6 @@ TEST_F(V8DetailedMemoryDecoratorDeathTest, InvalidParameters) {
 }
 
 TEST_F(V8DetailedMemoryRequestAnySeqTest, RequestIsSequenceSafe) {
-  // Precondition: CallOnGraph must run on a different sequence.  Note that all
-  // tasks passed to CallOnGraph will only run when run_loop.Run() is called
-  // below.
-  ASSERT_TRUE(GetMainThreadTaskRunner()->RunsTasksInCurrentSequence());
-  PerformanceManager::CallOnGraph(
-      FROM_HERE, base::BindLambdaForTesting([this] {
-        EXPECT_FALSE(
-            this->GetMainThreadTaskRunner()->RunsTasksInCurrentSequence());
-      }));
-
   // Set the active contents and simulate a navigation, which adds nodes to the
   // graph.
   SetContents(CreateTestWebContents());
@@ -1696,7 +1705,7 @@ TEST_F(V8DetailedMemoryRequestAnySeqTest, RequestIsSequenceSafe) {
   EXPECT_CALL(observer,
               OnV8MemoryMeasurementAvailable(process_id, expected_process_data,
                                              expected_frame_data))
-      .WillOnce([this, &run_loop, &process_id, &expected_frame_data]() {
+      .WillOnce([&]() {
         run_loop.Quit();
         ASSERT_TRUE(
             this->GetMainThreadTaskRunner()->RunsTasksInCurrentSequence())
@@ -1747,6 +1756,86 @@ TEST_F(V8DetailedMemoryRequestAnySeqTest, RequestIsSequenceSafe) {
   run_loop2.Run();
 }
 
+TEST_F(V8DetailedMemoryRequestAnySeqTest, SingleProcessRequest) {
+  content::IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
+  SetContents(CreateTestWebContents());
+
+  // Set up a page at a.com with a subframe at b.com. These should be in
+  // different processes.
+  const GURL kUrlA("http://a.com/");
+  const GURL kUrlB("http://b.com/");
+  content::RenderFrameHost* main_frame =
+      content::NavigationSimulator::NavigateAndCommitFromBrowser(
+          web_contents(), GURL("http://a.com"));
+  content::RenderFrameHost* child_frame =
+      content::RenderFrameHostTester::For(main_frame)->AppendChild("frame1");
+  child_frame = content::NavigationSimulator::NavigateAndCommitFromDocument(
+      GURL("http://b.com"), child_frame);
+
+  const RenderProcessHostId process_id1(main_frame->GetProcess()->GetID());
+  const RenderProcessHostId process_id2(child_frame->GetProcess()->GetID());
+  ASSERT_NE(process_id1, process_id2);
+
+  V8DetailedMemoryProcessData expected_process_data1;
+  expected_process_data1.set_unassociated_v8_bytes_used(1U);
+  V8DetailedMemoryProcessData expected_process_data2;
+  expected_process_data2.set_unassociated_v8_bytes_used(2U);
+
+  MockV8DetailedMemoryReporter mock_reporter1;
+  MockV8DetailedMemoryReporter mock_reporter2;
+  {
+    auto data = NewPerProcessV8MemoryUsage(1);
+    data->isolates[0]->unassociated_bytes_used = 1U;
+    ExpectBindAndRespondToQuery(&mock_reporter1, std::move(data), process_id1);
+
+    data = NewPerProcessV8MemoryUsage(1);
+    data->isolates[0]->unassociated_bytes_used = 2U;
+    ExpectBindAndRespondToQuery(&mock_reporter2, std::move(data), process_id2);
+  }
+
+  // Create one request that measures both processes, and one request that
+  // measures only one.
+  V8DetailedMemoryRequestAnySeq all_process_request(
+      V8DetailedMemoryDecoratorTest::kMinTimeBetweenRequests);
+  MockV8DetailedMemoryObserverAnySeq all_process_observer;
+  all_process_request.AddObserver(&all_process_observer);
+
+  V8DetailedMemoryRequestAnySeq single_process_request(
+      V8DetailedMemoryDecoratorTest::kMinTimeBetweenRequests,
+      MeasurementMode::kBounded, process_id1);
+  MockV8DetailedMemoryObserverAnySeq single_process_observer;
+  single_process_request.AddObserver(&single_process_observer);
+
+  // When a measurement is available the all process observer should be invoked
+  // for both processes, and the single process observer only for process 1.
+  EXPECT_CALL(
+      all_process_observer,
+      OnV8MemoryMeasurementAvailable(process_id1, expected_process_data1, _));
+  EXPECT_CALL(
+      all_process_observer,
+      OnV8MemoryMeasurementAvailable(process_id2, expected_process_data2, _));
+  EXPECT_CALL(
+      single_process_observer,
+      OnV8MemoryMeasurementAvailable(process_id1, expected_process_data1, _));
+
+  // Now execute all the above tasks.
+  task_environment()->RunUntilIdle();
+  Mock::VerifyAndClearExpectations(&mock_reporter1);
+  Mock::VerifyAndClearExpectations(&mock_reporter2);
+  Mock::VerifyAndClearExpectations(&all_process_observer);
+  Mock::VerifyAndClearExpectations(&single_process_observer);
+
+  // Must remove the observer before destroying the request to avoid a DCHECK
+  // from ObserverList.
+  all_process_request.RemoveObserver(&all_process_observer);
+  single_process_request.RemoveObserver(&single_process_observer);
+
+  // Execute the above tasks and exit.
+  base::RunLoop run_loop;
+  PerformanceManager::CallOnGraph(FROM_HERE, run_loop.QuitClosure());
+  run_loop.Run();
+}
+
 }  // namespace v8_memory
 
 }  // namespace performance_manager