[PM] Fix memory leak when WebMemoryAggregator gets no result

Make WebMemoryAggregator owned by its callback instead of by itself so
that it is also deleted if the callback is dropped.

R=ulan

Bug: 1085129
Change-Id: I4a7dcdd208c1b02933cf9e94cfcc6bd84502b3ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2515042Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823547}

components/performance_manager/v8_memory/web_memory_aggregator.cc

@@ -10,7 +10,6 @@
 #include "base/callback.h"
 #include "components/performance_manager/public/graph/frame_node.h"
 #include "components/performance_manager/public/graph/process_node.h"
-#include "components/performance_manager/public/v8_memory/v8_detailed_memory.h"
 #include "components/performance_manager/public/v8_memory/web_memory.h"
 #include "url/gurl.h"
 
@@ -85,21 +84,28 @@ void WebMeasureMemory(
     mojom::WebMemoryMeasurement::Mode mode,
     base::OnceCallback<void(mojom::WebMemoryMeasurementPtr)> callback) {
   auto web_memory_aggregator = std::make_unique<WebMemoryAggregator>(
-      frame_node->GetFrameToken(), std::move(callback));
-  // Start memory measurementfor the process of the given frame.
-  auto request = std::make_unique<V8DetailedMemoryRequestOneShot>(
-      frame_node->GetProcessNode(),
+      frame_node->GetFrameToken(),
+      WebMeasurementModeToRequestMeasurementMode(mode), std::move(callback));
+
+  // Create a measurement complete callback to own |web_memory_aggregator|. It
+  // will be deleted when the callback is executed or dropped.
+  V8DetailedMemoryRequestOneShot* request = web_memory_aggregator->request();
+  auto measurement_complete_callback =
       base::BindOnce(&WebMemoryAggregator::MeasurementComplete,
-                     base::Unretained(web_memory_aggregator.get())),
-      WebMeasurementModeToRequestMeasurementMode(mode));
-  WebMemoryAggregator::MakeSelfOwned(std::move(web_memory_aggregator),
-                                     std::move(request));
+                     std::move(web_memory_aggregator));
+
+  // Start memory measurement for the process of the given frame.
+  request->StartMeasurement(frame_node->GetProcessNode(),
+                            std::move(measurement_complete_callback));
 }
 
 WebMemoryAggregator::WebMemoryAggregator(
     const blink::LocalFrameToken& frame_token,
+    V8DetailedMemoryRequest::MeasurementMode mode,
     MeasurementCallback callback)
-    : frame_token_(frame_token), callback_(std::move(callback)) {}
+    : frame_token_(frame_token),
+      callback_(std::move(callback)),
+      request_(std::make_unique<V8DetailedMemoryRequestOneShot>(mode)) {}
 
 WebMemoryAggregator::~WebMemoryAggregator() = default;
 
@@ -107,17 +113,6 @@ void WebMemoryAggregator::MeasurementComplete(
     const ProcessNode* process_node,
     const V8DetailedMemoryProcessData*) {
   std::move(callback_).Run(BuildMemoryUsageResult(frame_token_, process_node));
-  self_reference_.reset();
-}
-
-void WebMemoryAggregator::MakeSelfOwned(
-    std::unique_ptr<WebMemoryAggregator> web_memory_aggregator,
-    std::unique_ptr<V8DetailedMemoryRequestOneShot> request) {
-  // Stash the request to ensure that it lives until the measurement is done.
-  web_memory_aggregator->request_ = std::move(request);
-  // Transfer the ownership to itself to make it self-owned. This reference will
-  // be reset in MeasurementCompleted.
-  web_memory_aggregator->self_reference_ = std::move(web_memory_aggregator);
 }
 
 }  // namespace v8_memory

components/performance_manager/v8_memory/web_memory_aggregator.h

@@ -9,6 +9,7 @@
 
 #include "base/callback.h"
 #include "components/performance_manager/public/mojom/web_memory.mojom.h"
+#include "components/performance_manager/public/v8_memory/v8_detailed_memory.h"
 #include "third_party/blink/public/common/tokens/tokens.h"
 
 namespace performance_manager {
@@ -17,34 +18,28 @@ class ProcessNode;
 
 namespace v8_memory {
 
-class V8DetailedMemoryRequestOneShot;
-class V8DetailedMemoryProcessData;
 // A helper class for implementing WebMeasureMemory().
 class WebMemoryAggregator {
  public:
   using MeasurementCallback =
       base::OnceCallback<void(mojom::WebMemoryMeasurementPtr)>;
 
-  WebMemoryAggregator(const blink::LocalFrameToken&, MeasurementCallback);
+  WebMemoryAggregator(const blink::LocalFrameToken&,
+                      V8DetailedMemoryRequest::MeasurementMode,
+                      MeasurementCallback);
 
   ~WebMemoryAggregator();
 
+  V8DetailedMemoryRequestOneShot* request() const { return request_.get(); }
+
   // A callback for V8DetailedMemoryRequestOneShot.
   void MeasurementComplete(const ProcessNode*,
                            const V8DetailedMemoryProcessData*);
 
-  // Transfer ownership of the given request to the given WebMemoryAggregator
-  // and makes the latter self-owned.
-  static void MakeSelfOwned(std::unique_ptr<WebMemoryAggregator>,
-                            std::unique_ptr<V8DetailedMemoryRequestOneShot>);
-
  private:
   blink::LocalFrameToken frame_token_;
   MeasurementCallback callback_;
   std::unique_ptr<V8DetailedMemoryRequestOneShot> request_;
-  // WebMemory is self-owned and lives until the measurement request
-  // is completed or failed.
-  std::unique_ptr<WebMemoryAggregator> self_reference_;
 };
 
 }  // namespace v8_memory

components/performance_manager/v8_memory/web_memory_aggregator_unittest.cc

@@ -97,7 +97,7 @@ void WebMemoryAggregatorTest::MeasureAndVerify(
     base::flat_map<std::string, Bytes> expected) {
   bool measurement_done = false;
   WebMemoryAggregator web_memory(
-      frame->frame_token(),
+      frame->frame_token(), V8DetailedMemoryRequest::MeasurementMode::kDefault,
       base::BindLambdaForTesting([&measurement_done, &expected](
                                      mojom::WebMemoryMeasurementPtr result) {
         base::flat_map<std::string, Bytes> actual;
@@ -146,8 +146,8 @@ TEST_F(WebMemoryAggregatorPMTest, WebMeasureMemory) {
   blink::LocalFrameToken frame_token =
       blink::LocalFrameToken(main_frame()->GetFrameToken());
 
-  // Call WebMemory::Measure on the performance manager sequence and verify
-  // that the result matches the data provided by the mock reporter.
+  // Call WebMeasureMemory on the performance manager sequence and verify that
+  // the result matches the data provided by the mock reporter.
   base::RunLoop run_loop;
   auto measurement_callback =
       base::BindLambdaForTesting([&](mojom::WebMemoryMeasurementPtr result) {
@@ -184,6 +184,53 @@ TEST_F(WebMemoryAggregatorPMTest, WebMeasureMemory) {
   run_loop.Run();
 }
 
+TEST_F(WebMemoryAggregatorPMTest, MeasurementInterrupted) {
+  CreateCrossProcessChildFrame();
+
+  blink::LocalFrameToken frame_token =
+      blink::LocalFrameToken(child_frame()->GetFrameToken());
+
+  // Call WebMeasureMemory on the performance manager sequence but delete the
+  // process being measured before the result arrives.
+  auto measurement_callback =
+      base::BindOnce([](mojom::WebMemoryMeasurementPtr result) {
+        FAIL() << "Measurement callback ran unexpectedly";
+      });
+
+  base::WeakPtr<FrameNode> frame_node_wrapper =
+      PerformanceManager::GetFrameNodeForRenderFrameHost(child_frame());
+  PerformanceManager::CallOnGraph(
+      FROM_HERE, base::BindLambdaForTesting([&]() {
+        ASSERT_TRUE(frame_node_wrapper);
+        FrameNode* frame_node = frame_node_wrapper.get();
+        WebMeasureMemory(frame_node,
+                         mojom::WebMemoryMeasurement::Mode::kDefault,
+                         std::move(measurement_callback));
+      }));
+
+  // Set up and bind the mock reporter.
+  MockV8DetailedMemoryReporter mock_reporter;
+  {
+    ::testing::InSequence seq;
+    ExpectBindReceiver(&mock_reporter, child_process_id());
+
+    auto data = NewPerProcessV8MemoryUsage(1);
+    AddIsolateMemoryUsage(frame_token, 1001u, data->isolates[0].get());
+    ExpectQueryAndDelayReply(&mock_reporter, base::TimeDelta::FromSeconds(10),
+                             std::move(data));
+  }
+
+  // Verify that requests are sent but reply is not yet received.
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(5));
+  ::testing::Mock::VerifyAndClearExpectations(&mock_reporter);
+
+  // Remove the child frame, which will destroy the child process.
+  content::RenderFrameHostTester::For(child_frame())->Detach();
+
+  // Advance until the reply is expected to make sure nothing explodes.
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(5));
+}
+
 }  // namespace v8_memory
 
 }  // namespace performance_manager