[PM] Make WebMemoryAggregator report 0 bytes for cross-process frames.

This conflicts with the use of 0 bytes as a sentinel for frames that
hadn't been measured and should be dropped from the result. So this also
makes the bytes optional in WebMemoryBreakdownEntry, so that frames with
a byte count of 0 are reported and frames without a byte count are
dropped from the result.

R=ulan

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

components/performance_manager/public/mojom/web_memory.mojom

@@ -47,13 +47,21 @@ struct WebMemoryAttribution {
   string? id;
 };
 
+// The amount of memory used by a breakdown.
+struct WebMemoryUsage {
+  uint64 bytes;
+};
+
 // Describes a memory region and attributes it to a set of contexts.
 // Usually the set consists of a single context. If there are multiple
 // contexts then this means that the memory may be owned by any of them.
 struct WebMemoryBreakdownEntry {
-  uint64 bytes;
-  array<WebMemoryAttribution> attribution;
+  // The memory used in this breakdown. It is null for breakdowns that did not
+  // have a memory measurement (for example a frame that was added after the
+  // measurement started).
+  WebMemoryUsage? memory;
 
+  array<WebMemoryAttribution> attribution;
   // TODO(1085129): Add memory types once they are implemented.
 };
 

components/performance_manager/v8_memory/v8_memory_test_helpers.cc

@@ -19,6 +19,7 @@
 #include "components/performance_manager/graph/process_node_impl.h"
 #include "components/performance_manager/public/mojom/v8_contexts.mojom.h"
 #include "components/performance_manager/public/performance_manager.h"
+#include "components/performance_manager/v8_memory/v8_context_tracker.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
@@ -226,6 +227,7 @@ void WebMemoryTestHarness::SetUp() {
   GetGraphFeaturesHelper().EnableV8ContextTracker();
   Super::SetUp();
   process_ = CreateNode<ProcessNodeImpl>();
+  other_process_ = CreateNode<ProcessNodeImpl>();
   pages_.push_back(CreateNode<PageNodeImpl>());
 }
 
@@ -239,6 +241,7 @@ FrameNodeImpl* WebMemoryTestHarness::AddFrameNodeImpl(
     Bytes memory_usage,
     FrameNodeImpl* parent,
     FrameNodeImpl* opener,
+    ProcessNodeImpl* process,
     base::Optional<std::string> id_attribute,
     base::Optional<std::string> src_attribute) {
   // If there's an opener, the new frame is also a new page.
@@ -252,14 +255,16 @@ FrameNodeImpl* WebMemoryTestHarness::AddFrameNodeImpl(
   int frame_tree_node_id = GetNextUniqueId();
   int frame_routing_id = GetNextUniqueId();
   auto frame_token = blink::LocalFrameToken();
-  auto frame = CreateNode<FrameNodeImpl>(process_.get(), page, parent,
+  auto frame = CreateNode<FrameNodeImpl>(process, page, parent,
                                          frame_tree_node_id, frame_routing_id,
                                          frame_token, browsing_instance_id);
   if (url) {
     frame->OnNavigationCommitted(GURL(*url), /*same document*/ true);
   }
-  V8DetailedMemoryExecutionContextData::CreateForTesting(frame.get())
-      ->set_v8_bytes_used(memory_usage.bytes);
+  if (memory_usage) {
+    V8DetailedMemoryExecutionContextData::CreateForTesting(frame.get())
+        ->set_v8_bytes_used(memory_usage.value());
+  }
   frames_.push_back(std::move(frame));
   FrameNodeImpl* frame_impl = frames_.back().get();
 
@@ -284,9 +289,20 @@ FrameNodeImpl* WebMemoryTestHarness::AddFrameNodeImpl(
     DCHECK(!id_attribute);
     DCHECK(!src_attribute);
   }
+
+  // If the frame is in the same process as its parent include the attribution
+  // in OnV8ContextCreated, otherwise it must be attached separately with
+  // OnRemoteIframeAttached.
   DCHECK(frame_impl->process_node());
-  frame_impl->process_node()->OnV8ContextCreated(std::move(description),
-                                                 std::move(attribution));
+  if (parent && parent->process_node() != frame_impl->process_node()) {
+    frame_impl->process_node()->OnV8ContextCreated(
+        std::move(description), mojom::IframeAttributionDataPtr());
+    V8ContextTracker::GetFromGraph(graph())->OnRemoteIframeAttachedForTesting(
+        frame_impl, parent, blink::RemoteFrameToken(), std::move(attribution));
+  } else {
+    frame_impl->process_node()->OnV8ContextCreated(std::move(description),
+                                                   std::move(attribution));
+  }
 
   return frame_impl;
 }

components/performance_manager/v8_memory/v8_memory_test_helpers.h

@@ -225,10 +225,7 @@ class WebMemoryTestHarness : public GraphTestHarness {
   using Super = GraphTestHarness;
 
   // Wrapper for memory usage bytes to improve test readability.
-  struct Bytes {
-    uint64_t bytes;
-    bool operator==(const Bytes& other) const { return bytes == other.bytes; }
-  };
+  using Bytes = base::Optional<uint64_t>;
 
   WebMemoryTestHarness();
   ~WebMemoryTestHarness() override;
@@ -243,7 +240,8 @@ class WebMemoryTestHarness : public GraphTestHarness {
       base::Optional<std::string> id_attribute = base::nullopt,
       base::Optional<std::string> src_attribute = base::nullopt) {
     return AddFrameNodeImpl(url, kDefaultBrowsingInstanceId, bytes, parent,
-                            /*opener=*/nullptr, id_attribute, src_attribute);
+                            /*opener=*/nullptr, process_.get(), id_attribute,
+                            src_attribute);
   }
 
   // Creates a frame node as if from window.open and adds it to the graph.
@@ -251,7 +249,9 @@ class WebMemoryTestHarness : public GraphTestHarness {
                                         Bytes bytes,
                                         FrameNodeImpl* opener) {
     return AddFrameNodeImpl(url, kDefaultBrowsingInstanceId, bytes,
-                            /*parent=*/nullptr, opener);
+                            /*parent=*/nullptr, opener, process_.get(),
+                            /*id_attribute=*/base::nullopt,
+                            /*src_attribute=*/base::nullopt);
   }
 
   // Creates a frame node in a different browsing instance and adds it to the
@@ -263,7 +263,8 @@ class WebMemoryTestHarness : public GraphTestHarness {
       base::Optional<std::string> id_attribute = base::nullopt,
       base::Optional<std::string> src_attribute = base::nullopt) {
     return AddFrameNodeImpl(url, kDefaultBrowsingInstanceId + 1, bytes, parent,
-                            /*opener=*/nullptr, id_attribute, src_attribute);
+                            /*opener=*/nullptr, process_.get(), id_attribute,
+                            src_attribute);
   }
 
   // Creates a frame node in a different browsing instance as if from
@@ -273,7 +274,21 @@ class WebMemoryTestHarness : public GraphTestHarness {
       Bytes bytes,
       FrameNodeImpl* opener) {
     return AddFrameNodeImpl(url, kDefaultBrowsingInstanceId + 1, bytes,
-                            /*parent=*/nullptr, opener);
+                            /*parent=*/nullptr, opener, process_.get(),
+                            /*id_attribute=*/base::nullopt,
+                            /*src_attribute=*/base::nullopt);
+  }
+
+  // Creates a frame node in a different process and adds it to the graph.
+  FrameNodeImpl* AddCrossProcessFrameNode(
+      std::string url,
+      Bytes bytes,
+      FrameNodeImpl* parent,
+      base::Optional<std::string> id_attribute = base::nullopt,
+      base::Optional<std::string> src_attribute = base::nullopt) {
+    return AddFrameNodeImpl(url, kDefaultBrowsingInstanceId, bytes, parent,
+                            /*opener=*/nullptr, other_process_.get(),
+                            id_attribute, src_attribute);
   }
 
   ProcessNode* process_node() const { return process_.get(); }
@@ -282,16 +297,17 @@ class WebMemoryTestHarness : public GraphTestHarness {
   static constexpr int kDefaultBrowsingInstanceId = 0;
 
   // Creates and adds a new frame node to the graph.
-  FrameNodeImpl* AddFrameNodeImpl(
-      base::Optional<std::string> url,
-      int browsing_instance_id,
-      Bytes bytes,
-      FrameNodeImpl* parent = nullptr,
-      FrameNodeImpl* opener = nullptr,
-      base::Optional<std::string> id_attribute = base::nullopt,
-      base::Optional<std::string> src_attribute = base::nullopt);
+  FrameNodeImpl* AddFrameNodeImpl(base::Optional<std::string> url,
+                                  int browsing_instance_id,
+                                  Bytes bytes,
+                                  FrameNodeImpl* parent,
+                                  FrameNodeImpl* opener,
+                                  ProcessNodeImpl* process,
+                                  base::Optional<std::string> id_attribute,
+                                  base::Optional<std::string> src_attribute);
   int GetNextUniqueId();
   TestNodeWrapper<ProcessNodeImpl> process_;
+  TestNodeWrapper<ProcessNodeImpl> other_process_;
   std::vector<TestNodeWrapper<PageNodeImpl>> pages_;
   std::vector<TestNodeWrapper<FrameNodeImpl>> frames_;
   int next_unique_id_ = 0;

components/performance_manager/v8_memory/web_memory_aggregator.cc

@@ -141,13 +141,6 @@ WebMemoryAggregator::AggregateMeasureMemoryResult() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   aggregation_result_ = mojom::WebMemoryMeasurement::New();
   VisitFrame(nullptr, aggregation_start_node_);
-
-  // Don't report breakdowns without any memory use.
-  base::EraseIf(aggregation_result_->breakdown,
-                [](const mojom::WebMemoryBreakdownEntryPtr& entry) {
-                  return entry->bytes == 0;
-                });
-
   return std::move(aggregation_result_);
 }
 
@@ -221,7 +214,18 @@ bool WebMemoryAggregator::VisitFrame(
   DCHECK(aggregation_point);
   if (auto* frame_data =
           V8DetailedMemoryExecutionContextData::ForFrameNode(frame_node)) {
-    aggregation_point->bytes += frame_data->v8_bytes_used();
+    if (!aggregation_point->memory) {
+      aggregation_point->memory = mojom::WebMemoryUsage::New();
+    }
+
+    // Ensure this frame is actually in the same process as the requesting
+    // frame. If not it should be considered to have 0 bytes.
+    // (https://github.com/WICG/performance-measure-memory/issues/20).
+    uint64_t bytes_used = (frame_node->GetProcessNode() ==
+                           aggregation_start_node_->GetProcessNode())
+                              ? frame_data->v8_bytes_used()
+                              : 0;
+    aggregation_point->memory->bytes += bytes_used;
   }
 
   // Recurse into children and opened pages. This node's aggregation point
@@ -269,10 +273,16 @@ const FrameNode* FindAggregationStartNode(const FrameNode* requesting_node) {
 
   // Follow parent and opener links to find the most general same-site node to
   // start the aggregation traversal from.
-  const FrameNode* start_node = requesting_node;
-  while (auto* parent_or_opener =
-             GetSameOriginParentOrOpener(start_node, requesting_origin)) {
-    start_node = parent_or_opener;
+  const FrameNode* start_node = nullptr;
+  for (auto* parent_or_opener = requesting_node; parent_or_opener;
+       parent_or_opener =
+           GetSameOriginParentOrOpener(parent_or_opener, requesting_origin)) {
+    // Only consider nodes in the same process as potential start nodes.
+    // (https://github.com/WICG/performance-measure-memory/issues/20).
+    if (parent_or_opener->GetProcessNode() ==
+        requesting_node->GetProcessNode()) {
+      start_node = parent_or_opener;
+    }
   }
 
   DCHECK(start_node);

components/performance_manager/v8_memory/web_memory_aggregator_unittest.cc

@@ -30,7 +30,7 @@ using NodeAggregationType = WebMemoryAggregator::NodeAggregationType;
 using WebMemoryAggregatorTest = WebMemoryTestHarness;
 
 struct ExpectedMemoryBreakdown {
-  uint64_t bytes = 0U;
+  WebMemoryTestHarness::Bytes bytes;
   AttributionScope scope = AttributionScope::kWindow;
   base::Optional<std::string> url;
   base::Optional<std::string> id;
@@ -38,7 +38,7 @@ struct ExpectedMemoryBreakdown {
 
   ExpectedMemoryBreakdown() = default;
   ExpectedMemoryBreakdown(
-      uint64_t expected_bytes,
+      WebMemoryTestHarness::Bytes expected_bytes,
       AttributionScope expected_scope,
       base::Optional<std::string> expected_url = base::nullopt,
       base::Optional<std::string> expected_id = base::nullopt,
@@ -59,7 +59,10 @@ mojom::WebMemoryMeasurementPtr CreateExpectedMemoryMeasurement(
   auto expected_measurement = mojom::WebMemoryMeasurement::New();
   for (const auto& breakdown : breakdowns) {
     auto expected_breakdown = mojom::WebMemoryBreakdownEntry::New();
-    expected_breakdown->bytes = breakdown.bytes;
+    if (breakdown.bytes) {
+      expected_breakdown->memory = mojom::WebMemoryUsage::New();
+      expected_breakdown->memory->bytes = breakdown.bytes.value();
+    }
 
     auto attribution = mojom::WebMemoryAttribution::New();
     attribution->scope = breakdown.scope;
@@ -129,15 +132,18 @@ TEST_F(WebMemoryAggregatorTest, CreateBreakdownEntry) {
     internal::CopyBreakdownAttribution(breakdown_with_url,
                                        breakdown_with_empty_url);
 
-    // All measurements should be created with 0 bytes.
+    // All measurements should be created without measurement results.
     auto expected_result = CreateExpectedMemoryMeasurement({
-        ExpectedMemoryBreakdown(0, AttributionScope::kCrossOriginAggregated,
+        ExpectedMemoryBreakdown(/*bytes=*/base::nullopt,
+                                AttributionScope::kCrossOriginAggregated,
                                 /*expected_url=*/base::nullopt,
                                 /*expected_id=*/base::nullopt,
                                 /*expected_src=*/base::nullopt),
-        ExpectedMemoryBreakdown(0, AttributionScope::kWindow,
+        ExpectedMemoryBreakdown(/*bytes=*/base::nullopt,
+                                AttributionScope::kWindow,
                                 "https://example.com", attribute, attribute),
-        ExpectedMemoryBreakdown(0, AttributionScope::kWindow,
+        ExpectedMemoryBreakdown(/*bytes=*/base::nullopt,
+                                AttributionScope::kWindow,
                                 /*expected_url=*/"", attribute, attribute),
     });
     EXPECT_EQ(MeasurementToJSON(measurement),
@@ -293,10 +299,21 @@ TEST_F(WebMemoryAggregatorTest, AggregateNestedCrossOrigin) {
       AddFrameNode("https://example.com/iframe3", Bytes{6}, subframe3,
                    "example-id6", "https://example.com/iframe3");
 
-  // A frame with 0 bytes of memory use (eg. a frame that's added to the frame
-  // tree during the measurement) should not appear in the result.
+  // To test aggregation all the frames above are in the same process, even
+  // though in production frames with different origins will be in different
+  // processes whenever possible. Frames in a different process from the
+  // requesting frame should all have 0 bytes reported.
+  FrameNodeImpl* cross_process_frame =
+      AddCrossProcessFrameNode("https://example.com/cross_process", Bytes{100},
+                               subframe3, "cross-process-id1");
+  FrameNodeImpl* cross_process_frame2 =
+      AddCrossProcessFrameNode("https://foo.com/cross_process", Bytes{200},
+                               subframe3, "cross-process-id2");
+
+  // A frame without a memory measurement (eg. a frame that's added to the frame
+  // tree during the measurement) should not have a memory entry in the result.
   FrameNodeImpl* empty_frame =
-      AddFrameNode("https://example.com/empty_frame", Bytes{0}, subframe3);
+      AddFrameNode("https://example.com/empty_frame", base::nullopt, subframe3);
 
   EXPECT_EQ(internal::FindAggregationStartNode(main_frame), main_frame);
   WebMemoryAggregator aggregator(main_frame);
@@ -339,6 +356,16 @@ TEST_F(WebMemoryAggregatorTest, AggregateNestedCrossOrigin) {
   EXPECT_EQ(internal::GetSameOriginParentOrOpener(
                 empty_frame, aggregator.requesting_origin()),
             subframe3);
+  EXPECT_EQ(aggregator.FindNodeAggregationType(cross_process_frame),
+            NodeAggregationType::kSameOriginAggregationPoint);
+  EXPECT_EQ(internal::GetSameOriginParentOrOpener(
+                cross_process_frame, aggregator.requesting_origin()),
+            subframe3);
+  EXPECT_EQ(aggregator.FindNodeAggregationType(cross_process_frame2),
+            NodeAggregationType::kCrossOriginAggregationPoint);
+  EXPECT_EQ(internal::GetSameOriginParentOrOpener(
+                cross_process_frame2, aggregator.requesting_origin()),
+            subframe3);
 
   auto expected_result = CreateExpectedMemoryMeasurement({
       ExpectedMemoryBreakdown(10, AttributionScope::kWindow,
@@ -358,6 +385,13 @@ TEST_F(WebMemoryAggregatorTest, AggregateNestedCrossOrigin) {
       ExpectedMemoryBreakdown(6, AttributionScope::kWindow,
                               "https://example.com/iframe3", "example-id6",
                               "https://example.com/iframe3"),
+      ExpectedMemoryBreakdown(0, AttributionScope::kWindow,
+                              "https://example.com/cross_process",
+                              "cross-process-id1"),
+      ExpectedMemoryBreakdown(0, AttributionScope::kCrossOriginAggregated,
+                              base::nullopt, "cross-process-id2"),
+      ExpectedMemoryBreakdown(base::nullopt, AttributionScope::kWindow,
+                              "https://example.com/empty_frame"),
   });
   auto result = aggregator.AggregateMeasureMemoryResult();
   EXPECT_EQ(MeasurementToJSON(result), MeasurementToJSON(expected_result));
@@ -407,6 +441,29 @@ TEST_F(WebMemoryAggregatorTest, FindAggregationStartNode) {
             MeasurementToJSON(main_frame_expected_result));
 }
 
+TEST_F(WebMemoryAggregatorTest, FindCrossProcessAggregationStartNode) {
+  FrameNodeImpl* main_frame = AddFrameNode("https://example.com/", Bytes{1});
+  FrameNodeImpl* cross_process_child = AddCrossProcessFrameNode(
+      "https://example.com/cross_process.html", Bytes{2}, main_frame);
+  FrameNodeImpl* same_process_child = AddFrameNode(
+      "https://example.com/same_process.html", Bytes{3}, cross_process_child);
+
+  auto origin = url::Origin::Create(GURL("https://example.com"));
+  ASSERT_EQ(internal::GetSameOriginParentOrOpener(cross_process_child, origin),
+            main_frame);
+  ASSERT_EQ(internal::GetSameOriginParentOrOpener(same_process_child, origin),
+            cross_process_child);
+
+  // |cross_process_child| has no ancestor in the same process as it.
+  EXPECT_EQ(internal::FindAggregationStartNode(cross_process_child),
+            cross_process_child);
+
+  // The search starting from |same_process_child| should skip over
+  // |cross_process_child|, which is in a different process, and find
+  // |main_frame| which is in the same process.
+  EXPECT_EQ(internal::FindAggregationStartNode(same_process_child), main_frame);
+}
+
 TEST_F(WebMemoryAggregatorTest, AggregateWindowOpener) {
   FrameNodeImpl* main_frame = AddFrameNode("https://example.com/", Bytes{10});
   FrameNodeImpl* child_frame = AddFrameNode("https://example.com/iframe.html",

components/performance_manager/v8_memory/web_memory_impl_unittest.cc

@@ -70,12 +70,13 @@ void WebMemoryImplTest::MeasureAndVerify(
         base::flat_map<std::string, Bytes> actual;
         for (const auto& entry : result->breakdown) {
           EXPECT_EQ(1u, entry->attribution.size());
-          if (mojom::WebMemoryAttribution::Scope::kWindow ==
-              entry->attribution[0]->scope) {
-            actual[*entry->attribution[0]->url] = Bytes{entry->bytes};
-          } else {
-            actual[*entry->attribution[0]->src] = Bytes{entry->bytes};
-          }
+          std::string attribution_tag =
+              (mojom::WebMemoryAttribution::Scope::kWindow ==
+               entry->attribution[0]->scope)
+                  ? *entry->attribution[0]->url
+                  : *entry->attribution[0]->src;
+          actual[attribution_tag] =
+              entry->memory ? Bytes{entry->memory->bytes} : base::nullopt;
         }
         EXPECT_EQ(expected, actual);
         measurement_done = true;
@@ -130,7 +131,8 @@ TEST_F(WebMemoryImplPMTest, WebMeasureMemory) {
         const auto& entry = result->breakdown[0];
         EXPECT_EQ(1u, entry->attribution.size());
         EXPECT_EQ(kMainFrameUrl, *(entry->attribution[0]->url));
-        EXPECT_EQ(1001u, entry->bytes);
+        ASSERT_TRUE(entry->memory);
+        EXPECT_EQ(1001u, entry->memory->bytes);
         run_loop.Quit();
       });
   auto bad_message_callback =

third_party/blink/renderer/core/timing/measure_memory/measure_memory_controller.cc

@@ -166,7 +166,8 @@ MemoryAttribution* ConvertAttribution(
 MemoryBreakdownEntry* ConvertBreakdown(
     const WebMemoryBreakdownEntryPtr& breakdown_entry) {
   auto* result = MemoryBreakdownEntry::Create();
-  result->setBytes(breakdown_entry->bytes);
+  DCHECK(breakdown_entry->memory);
+  result->setBytes(breakdown_entry->memory->bytes);
   HeapVector<Member<MemoryAttribution>> attribution;
   for (const auto& entry : breakdown_entry->attribution) {
     attribution.push_back(ConvertAttribution(entry));
@@ -187,7 +188,9 @@ MemoryBreakdownEntry* EmptyBreakdown() {
 MemoryMeasurement* ConvertResult(const WebMemoryMeasurementPtr& measurement) {
   HeapVector<Member<MemoryBreakdownEntry>> breakdown;
   for (const auto& entry : measurement->breakdown) {
-    breakdown.push_back(ConvertBreakdown(entry));
+    // Skip breakdowns that didn't get a measurement.
+    if (entry->memory)
+      breakdown.push_back(ConvertBreakdown(entry));
   }
   // Add an empty breakdown entry as required by the spec.
   // See https://github.com/WICG/performance-measure-memory/issues/10.