Replace map<int64, PerFrameV8MemoryUsageData> with an array

The move of V8PerFrameMemoryReporterImpl from content to blink
introduced a bug where the PerFrameV8MemoryUsageData::associated_memory
mojo map started to use WTF::HashMap as the backing, which makes key=0
invalid because the hash traits for int64_t reserve that value for the
deleted sentinel.

This CL also fixes another bug of dereferencing a non-existent entry
in the |frames| HashMap.

Additionally this CL adds a test for V8PerFrameMemoryReporterImpl to
make sure that the fixes work.

Bug: 1085129
Change-Id: I04b0b7230fde191571f407b4931d6dba543bd2d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339317
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795937}

components/performance_manager/v8_memory/v8_per_frame_memory_decorator.cc

@@ -237,6 +237,7 @@ void NodeAttachedProcessData::StartMeasurement() {
   // NodeAttachedProcessData when the last V8PerFrameMemoryRequest is deleted,
   // which could happen at any time.
   resource_usage_reporter_->GetPerFrameV8MemoryUsageData(
+      blink::mojom::V8PerFrameMemoryReporter::Mode::DEFAULT,
       base::BindOnce(&NodeAttachedProcessData::OnPerFrameV8MemoryUsageData,
                      weak_factory_.GetWeakPtr()));
 }
@@ -274,20 +275,21 @@ void NodeAttachedProcessData::OnPerFrameV8MemoryUsageData(
       // No data for this node, clear any data associated with it.
       NodeAttachedFrameData::Destroy(frame_node);
     } else {
-      // There should always be data for the main isolated world for each frame.
-      DCHECK(base::Contains(it->second->associated_bytes, 0));
-
+      bool found_main_world = false;
       NodeAttachedFrameData* frame_data =
           NodeAttachedFrameData::GetOrCreate(frame_node);
-      for (const auto& kv : it->second->associated_bytes) {
-        if (kv.first == 0) {
+      for (const auto& entry : it->second->associated_bytes) {
+        if (entry->world_id ==
+            blink::mojom::V8IsolatedWorldMemoryUsage::kMainWorldId) {
           frame_data->data_available_ = true;
-          frame_data->data_.set_v8_bytes_used(kv.second->bytes_used);
+          frame_data->data_.set_v8_bytes_used(entry->bytes_used);
+          found_main_world = true;
         } else {
           // TODO(crbug.com/1080672): Where to stash the rest of the data?
         }
       }
-
+      // There should always be data for the main isolated world for each frame.
+      DCHECK(found_main_world);
       // Clear this datum as its usage has been consumed.
       associated_memory.erase(it);
     }

components/performance_manager/v8_memory/v8_per_frame_memory_decorator_unittest.cc

@@ -51,7 +51,7 @@ class LenientMockV8PerFrameMemoryReporter
 
   MOCK_METHOD(void,
               GetPerFrameV8MemoryUsageData,
-              (GetPerFrameV8MemoryUsageDataCallback callback),
+              (Mode mode, GetPerFrameV8MemoryUsageDataCallback callback),
               (override));
 
   void Bind(mojo::PendingReceiver<blink::mojom::V8PerFrameMemoryReporter>
@@ -142,8 +142,9 @@ class V8PerFrameMemoryDecoratorTestBase {
       base::RepeatingCallback<void(
           MockV8PerFrameMemoryReporter::GetPerFrameV8MemoryUsageDataCallback
               callback)> responder) {
-    EXPECT_CALL(*mock_reporter, GetPerFrameV8MemoryUsageData(_))
+    EXPECT_CALL(*mock_reporter, GetPerFrameV8MemoryUsageData(_, _))
         .WillOnce([this, responder](
+                      MockV8PerFrameMemoryReporter::Mode mode,
                       MockV8PerFrameMemoryReporter::
                           GetPerFrameV8MemoryUsageDataCallback callback) {
           this->last_query_time_ = base::TimeTicks::Now();
@@ -230,11 +231,14 @@ void AddPerFrameIsolateMemoryUsage(
     per_frame_data = datum.get();
     data->associated_memory.push_back(std::move(datum));
   }
-  ASSERT_FALSE(base::Contains(per_frame_data->associated_bytes, world_id));
+  for (const auto& entry : per_frame_data->associated_bytes) {
+    EXPECT_NE(world_id, entry->world_id);
+  }
 
   auto isolated_world_usage = blink::mojom::V8IsolatedWorldMemoryUsage::New();
   isolated_world_usage->bytes_used = bytes_used;
-  per_frame_data->associated_bytes[world_id] = std::move(isolated_world_usage);
+  isolated_world_usage->world_id = world_id;
+  per_frame_data->associated_bytes.push_back(std::move(isolated_world_usage));
 }
 
 }  // namespace

third_party/blink/public/mojom/performance_manager/v8_per_frame_memory.mojom

@@ -8,6 +8,9 @@ import "mojo/public/mojom/base/unguessable_token.mojom";
 
 // The amount of heap memory used by V8 in the context of a frame.
 struct V8IsolatedWorldMemoryUsage {
+  const int64 kMainWorldId = 0;
+  int64 world_id;
+
   // The number of v8 heap bytes used by a V8 isolated world.
   uint64 bytes_used = 0;
 
@@ -18,11 +21,11 @@ struct V8IsolatedWorldMemoryUsage {
   // For example Chrome extensions use the host ID, as per
   // extensions::ScriptInjection::GetHostIdForIsolatedWorld. Some types of
   // isolated world will not have a suitable tag so will leave this empty.
-  string stable_id;
+  string? stable_id;
 
   // An optional human readable name for the world, for debugging. Unlike
   // stable_id this might not be unique.
-  string human_readable_name;
+  string? human_readable_name;
 };
 
 // Returns the number of bytes used by the v8 heap per frame.
@@ -30,9 +33,9 @@ struct PerFrameV8MemoryUsageData {
   // The frame-unique token.
   mojo_base.mojom.UnguessableToken frame_token;
 
-  // The resources used by this frame, mapped on the isolated world ID.
-  // World ID 0 is the main world.
-  map<int64, V8IsolatedWorldMemoryUsage> associated_bytes;
+  // This should actually be a map with world ID keys, but due to limitations
+  // of WTF::HashMap around keys with value = 0, we have to use an array.
+  array<V8IsolatedWorldMemoryUsage> associated_bytes;
 };
 
 // Returns the number of bytes used by the v8 heap in a process.
@@ -54,9 +57,18 @@ struct PerProcessV8MemoryUsageData {
 
 // Allows a browser to query the resource usage of sub-processes.
 interface V8PerFrameMemoryReporter {
+  // The mode for performing memory measurement.
+  enum Mode {
+    DEFAULT, // Perform memory measurement on the next garbage collection
+             // and force garbage collection after some timeout.
+
+    EAGER,   // Force immediate garbage collection and memory measurement.
+
+    LAZY,    // Perform memory measurement on the next garbage collection.
+  };
   // Requests a per-frame estimate of v8 heap byte usage on the next garbage
   // collection. Note that this causes extra cost for the next garbage
   // collection, which can be on the order of 10-20%.
-  GetPerFrameV8MemoryUsageData() => (PerProcessV8MemoryUsageData data);
+  GetPerFrameV8MemoryUsageData(Mode mode) => (PerProcessV8MemoryUsageData data);
 };
 

third_party/blink/renderer/controller/BUILD.gn

@@ -179,6 +179,9 @@ source_set("blink_unittests_sources") {
       "oom_intervention_impl_test.cc",
       "user_level_memory_pressure_signal_generator_test.cc",
     ]
+  } else {
+    sources +=
+        [ "performance_manager/v8_per_frame_memory_reporter_impl_test.cc" ]
   }
 
   if (is_linux || is_chromeos || is_android || is_mac || is_win) {

third_party/blink/renderer/controller/performance_manager/v8_per_frame_memory_reporter_impl.cc

@@ -21,6 +21,7 @@ struct HashTraits<::blink::mojom::blink::PerFrameV8MemoryUsageDataPtr>
   typedef const ::blink::mojom::blink::PerFrameV8MemoryUsageDataPtr&
       PeekOutType;
 };
+
 }  // namespace WTF
 
 namespace blink {
@@ -76,9 +77,11 @@ class FrameAssociatedMeasurementDelegate : public v8::MeasureMemoryDelegate {
         ++(result->num_unassociated_contexts);
         result->unassociated_context_bytes_used += size;
       } else {
-        mojom::blink::PerFrameV8MemoryUsageData* per_frame_resources =
-            frames.at(frame).get();
-        if (!per_frame_resources) {
+        auto it = frames.find(frame);
+        mojom::blink::PerFrameV8MemoryUsageData* per_frame_resources;
+        if (it != frames.end()) {
+          per_frame_resources = it->value.get();
+        } else {
 #if DCHECK_IS_ON()
           // Check that the frame token didn't already occur.
           for (const auto& entry : frames) {
@@ -97,19 +100,29 @@ class FrameAssociatedMeasurementDelegate : public v8::MeasureMemoryDelegate {
         mojom::blink::V8IsolatedWorldMemoryUsagePtr isolated_world_usage =
             mojom::blink::V8IsolatedWorldMemoryUsage::New();
         isolated_world_usage->bytes_used = size;
-        int32_t world_id = frame->GetScriptContextWorldId(context);
-
-        if (world_id != DOMWrapperWorld::WorldId::kMainWorldId) {
+        isolated_world_usage->world_id =
+            frame->GetScriptContextWorldId(context);
+        static_assert(
+            DOMWrapperWorld::WorldId::kMainWorldId ==
+                mojom::blink::V8IsolatedWorldMemoryUsage::kMainWorldId,
+            "The main world IDs must match.");
+
+        if (isolated_world_usage->world_id !=
+            DOMWrapperWorld::WorldId::kMainWorldId) {
           isolated_world_usage->stable_id =
               blink::GetIsolatedWorldStableId(context);
           isolated_world_usage->human_readable_name =
               blink::GetIsolatedWorldHumanReadableName(context);
         }
+#if DCHECK_IS_ON()
+        // Check that the world id didn't already occur.
+        for (const auto& entry : per_frame_resources->associated_bytes) {
+          DCHECK_NE(entry->world_id, isolated_world_usage->world_id);
+        }
+#endif
 
-        DCHECK(
-            !base::Contains(per_frame_resources->associated_bytes, world_id));
-        per_frame_resources->associated_bytes.Set(
-            world_id, std::move(isolated_world_usage));
+        per_frame_resources->associated_bytes.push_back(
+            std::move(isolated_world_usage));
       }
     }
     // Move the per-frame memory values to the result.
@@ -122,6 +135,19 @@ class FrameAssociatedMeasurementDelegate : public v8::MeasureMemoryDelegate {
   GetPerFrameV8MemoryUsageDataCallback callback_;
 };
 
+v8::MeasureMemoryExecution ToV8MeasureMemoryExecution(
+    V8PerFrameMemoryReporterImpl::Mode mode) {
+  switch (mode) {
+    case V8PerFrameMemoryReporterImpl::Mode::DEFAULT:
+      return v8::MeasureMemoryExecution::kDefault;
+    case V8PerFrameMemoryReporterImpl::Mode::EAGER:
+      return v8::MeasureMemoryExecution::kEager;
+    case V8PerFrameMemoryReporterImpl::Mode::LAZY:
+      return v8::MeasureMemoryExecution::kLazy;
+  }
+  NOTREACHED();
+}
+
 }  // namespace
 
 // static
@@ -132,6 +158,7 @@ void V8PerFrameMemoryReporterImpl::Create(
 }
 
 void V8PerFrameMemoryReporterImpl::GetPerFrameV8MemoryUsageData(
+    V8PerFrameMemoryReporterImpl::Mode mode,
     GetPerFrameV8MemoryUsageDataCallback callback) {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   if (!isolate) {
@@ -141,7 +168,8 @@ void V8PerFrameMemoryReporterImpl::GetPerFrameV8MemoryUsageData(
         std::make_unique<FrameAssociatedMeasurementDelegate>(
             std::move(callback));
 
-    isolate->MeasureMemory(std::move(delegate));
+    isolate->MeasureMemory(std::move(delegate),
+                           ToV8MeasureMemoryExecution(mode));
   }
 }
 

third_party/blink/renderer/controller/performance_manager/v8_per_frame_memory_reporter_impl.h

@@ -6,17 +6,19 @@
 #define THIRD_PARTY_BLINK_RENDERER_CONTROLLER_PERFORMANCE_MANAGER_V8_PER_FRAME_MEMORY_REPORTER_IMPL_H_
 
 #include "third_party/blink/public/mojom/performance_manager/v8_per_frame_memory.mojom-blink.h"
+#include "third_party/blink/renderer/controller/controller_export.h"
 
 namespace blink {
 
 // Exposes V8 per-frame associated memory metrics to the browser.
-class V8PerFrameMemoryReporterImpl
+class CONTROLLER_EXPORT V8PerFrameMemoryReporterImpl
     : public mojom::blink::V8PerFrameMemoryReporter {
  public:
   static void Create(
       mojo::PendingReceiver<mojom::blink::V8PerFrameMemoryReporter> receiver);
 
   void GetPerFrameV8MemoryUsageData(
+      Mode mode,
       GetPerFrameV8MemoryUsageDataCallback callback) override;
 };
 

third_party/blink/renderer/controller/performance_manager/v8_per_frame_memory_reporter_impl_test.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/controller/performance_manager/v8_per_frame_memory_reporter_impl.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/testing/sim/sim_compositor.h"
+#include "third_party/blink/renderer/core/testing/sim/sim_request.h"
+#include "third_party/blink/renderer/core/testing/sim/sim_test.h"
+#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
+#include "v8/include/v8.h"
+
+namespace blink {
+
+class V8PerFrameMemoryReporterImplTest : public SimTest {};
+
+namespace {
+
+class MemoryUsageChecker {
+ public:
+  void Callback(mojom::blink::PerProcessV8MemoryUsageDataPtr result) {
+    EXPECT_EQ(2u, result->associated_memory.size());
+    for (const auto& frame_memory : result->associated_memory) {
+      for (const auto& entry : frame_memory->associated_bytes) {
+        EXPECT_EQ(0u, entry->world_id);
+        EXPECT_LT(4000000u, entry->bytes_used);
+      }
+    }
+    called_ = true;
+  }
+  bool IsCalled() { return called_; }
+
+ private:
+  bool called_ = false;
+};
+
+}  // anonymous namespace
+
+TEST_F(V8PerFrameMemoryReporterImplTest, GetPerFrameV8MemoryUsageData) {
+  SimRequest main_resource("https://example.com/", "text/html");
+  SimRequest child_frame_resource("https://example.com/subframe.html",
+                                  "text/html");
+
+  LoadURL("https://example.com/");
+
+  main_resource.Complete(String::Format(R"HTML(
+      <script>
+        window.onload = function () {
+          globalThis.array = new Array(1000000).fill(0);
+          console.log("main loaded");
+        }
+      </script>
+      <body>
+        <iframe src='https://example.com/subframe.html'></iframe>
+      </body>)HTML"));
+
+  test::RunPendingTasks();
+
+  child_frame_resource.Complete(String::Format(R"HTML(
+      <script>
+        window.onload = function () {
+          globalThis.array = new Array(1000000).fill(0);
+          console.log("iframe loaded");
+        }
+      </script>
+      <body>
+      </body>)HTML"));
+
+  test::RunPendingTasks();
+
+  // Ensure that main frame and subframe are loaded before measuring memory
+  // usage.
+  EXPECT_TRUE(ConsoleMessages().Contains("main loaded"));
+  EXPECT_TRUE(ConsoleMessages().Contains("iframe loaded"));
+
+  V8PerFrameMemoryReporterImpl reporter;
+  MemoryUsageChecker checker;
+  reporter.GetPerFrameV8MemoryUsageData(
+      V8PerFrameMemoryReporterImpl::Mode::EAGER,
+      WTF::Bind(&MemoryUsageChecker::Callback, WTF::Unretained(&checker)));
+
+  test::RunPendingTasks();
+
+  EXPECT_TRUE(checker.IsCalled());
+}
+}  // namespace blink