[PM] Implement V8ContextTracker business logic.

BUG=1080672

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

components/performance_manager/BUILD.gn

@@ -234,6 +234,7 @@ source_set("unit_tests") {
     "render_process_host_id_unittest.cc",
     "v8_memory/v8_context_tracker_helpers_unittest.cc",
     "v8_memory/v8_context_tracker_internal_unittest.cc",
+    "v8_memory/v8_context_tracker_unittest.cc",
     "v8_memory/v8_detailed_memory_unittest.cc",
     "web_contents_proxy_unittest.cc",
     "worker_watcher_unittest.cc",

components/performance_manager/graph/frame_node_impl.h

@@ -59,6 +59,8 @@ class FrameNodeImpl
       public TypedNodeBase<FrameNodeImpl, FrameNode, FrameNodeObserver>,
       public mojom::DocumentCoordinationUnit {
  public:
+  using PassKey = util::PassKey<FrameNodeImpl>;
+
   static const char kDefaultPriorityReason[];
   static constexpr NodeTypeEnum Type() { return NodeTypeEnum::kFrame; }
 
@@ -161,6 +163,8 @@ class FrameNodeImpl
     return &execution_context_;
   }
 
+  static PassKey CreatePassKeyForTesting() { return PassKey(); }
+
  private:
   friend class ExecutionContextPriorityAccess;
   friend class FrameNodeImplDescriber;

components/performance_manager/graph/process_node_impl.h

@@ -14,6 +14,7 @@
 #include "base/process/process.h"
 #include "base/process/process_handle.h"
 #include "base/time/time.h"
+#include "base/util/type_safety/pass_key.h"
 #include "components/performance_manager/graph/node_attached_data.h"
 #include "components/performance_manager/graph/node_base.h"
 #include "components/performance_manager/graph/properties.h"
@@ -44,6 +45,8 @@ class ProcessNodeImpl
       public TypedNodeBase<ProcessNodeImpl, ProcessNode, ProcessNodeObserver>,
       public mojom::ProcessCoordinationUnit {
  public:
+  using PassKey = util::PassKey<ProcessNodeImpl>;
+
   static constexpr NodeTypeEnum Type() { return NodeTypeEnum::kProcess; }
 
   ProcessNodeImpl(content::ProcessType process_type,
@@ -118,6 +121,8 @@ class ProcessNodeImpl
     return weak_factory_.GetWeakPtr();
   }
 
+  static PassKey CreatePassKeyForTesting() { return PassKey(); }
+
  protected:
   void SetProcessImpl(base::Process process,
                       base::ProcessId process_id,

components/performance_manager/v8_memory/v8_context_tracker.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/optional.h"
+#include "base/util/type_safety/pass_key.h"
 #include "components/performance_manager/public/execution_context/execution_context.h"
 #include "components/performance_manager/public/graph/graph.h"
 #include "components/performance_manager/public/graph/graph_registered.h"
@@ -17,6 +18,10 @@
 #include "third_party/blink/public/common/tokens/tokens.h"
 
 namespace performance_manager {
+
+class FrameNodeImpl;
+class ProcessNodeImpl;
+
 namespace v8_memory {
 
 // Forward declaration.
@@ -117,6 +122,87 @@ class V8ContextTracker
   const V8ContextState* GetV8ContextState(
       const blink::V8ContextToken& token) const;
 
+  //////////////////////////////////////////////////////////////////////////////
+  // The following functions handle inbound IPC, and are only meant to be
+  // called from ProcessNodeImpl and FrameNodeImpl (hence the use of PassKey).
+
+  // Notifies the context tracker of a V8Context being created in a renderer
+  // process. If the context is associated with an ExecutionContext (EC) then
+  // |description.execution_context_token| will be provided. If the EC is a
+  // frame, and the parent of that frame is also in the same process, then
+  // |iframe_attribution_data| will be provided, otherwise these will be empty.
+  // In the case where they are empty the iframe data will be provided by a
+  // separate call to OnIframeAttached() from the process hosting the
+  // parent frame. See the V8ContextWorldType enum for a description of the
+  // relationship between world types, world names and execution contexts.
+  void OnV8ContextCreated(
+      util::PassKey<ProcessNodeImpl> key,
+      ProcessNodeImpl* process_node,
+      const V8ContextDescription& description,
+      const base::Optional<IframeAttributionData>& iframe_attribution_data);
+
+  // Notifies the tracker that a V8Context is now detached from its associated
+  // ExecutionContext (if one was provided during OnV8ContextCreated). If the
+  // context stays detached for a long time this is indicative of a Javascript
+  // leak, with the context being kept alive by a stray reference from another
+  // context. All ExecutionContext-associated V8Contexts will have this method
+  // called before they are destroyed, and it will not be called for other
+  // V8Contexts (they are never considered detached).
+  void OnV8ContextDetached(util::PassKey<ProcessNodeImpl> key,
+                           ProcessNodeImpl* process_node,
+                           const blink::V8ContextToken& v8_context_token);
+
+  // Notifies the tracker that a V8Context has been garbage collected. This will
+  // only be called after OnV8ContextDetached if the OnV8ContextCreated had a
+  // non-empty |execution_context_token|.
+  void OnV8ContextDestroyed(util::PassKey<ProcessNodeImpl> key,
+                            ProcessNodeImpl* process_node,
+                            const blink::V8ContextToken& v8_context_token);
+
+  // Notifies the tracker that a RemoteFrame child with a LocalFrame parent was
+  // created in a renderer, providing the iframe.id and iframe.src from the
+  // parent point of view. This will decorate the ExecutionContextData of the
+  // appropriate child frame. We require the matching OnRemoteIframeDetached to
+  // be called for bookkeeping. This should only be called once for a given
+  // |remote_frame_token|.
+  void OnRemoteIframeAttached(
+      util::PassKey<FrameNodeImpl> key,
+      FrameNodeImpl* parent_frame_node,
+      const blink::RemoteFrameToken& remote_frame_token,
+      const IframeAttributionData& iframe_attribution_data);
+
+  // TODO(chrisha): Add OnRemoteIframeAttributesChanged support.
+
+  // Notifies the tracker that a RemoteFrame child with a LocalFrame parent was
+  // detached from an iframe element in a renderer. This is used to cleanup
+  // iframe data that is being tracked due to a previous call to
+  // OnIframeAttached, unless the data was adopted by a call to
+  // OnV8ContextCreated. Should only be called once for a given
+  // |remote_frame_token|, and only after a matching "OnRemoteIframeAttached"
+  // call.
+  void OnRemoteIframeDetached(
+      util::PassKey<FrameNodeImpl> key,
+      FrameNodeImpl* parent_frame_node,
+      const blink::RemoteFrameToken& remote_frame_token);
+
+  //////////////////////////////////////////////////////////////////////////////
+  // The following functions are for testing only.
+
+  void OnRemoteIframeAttachedForTesting(
+      FrameNodeImpl* frame_node,
+      const blink::RemoteFrameToken& remote_frame_token,
+      const IframeAttributionData& iframe_attribution_data);
+
+  void OnRemoteIframeDetachedForTesting(
+      FrameNodeImpl* parent_frame_node,
+      const blink::RemoteFrameToken& remote_frame_token);
+
+  // System wide metrics.
+  size_t GetExecutionContextCountForTesting() const;
+  size_t GetV8ContextCountForTesting() const;
+  size_t GetDestroyedExecutionContextCountForTesting() const;
+  size_t GetDetachedV8ContextCountForTesting() const;
+
  private:
   // Implementation of execution_context::ExecutionContextObserverDefaultImpl.
   void OnBeforeExecutionContextRemoved(
@@ -138,6 +224,21 @@ class V8ContextTracker
   // Implementation of ProcessNode::ObserverDefaultImpl.
   void OnBeforeProcessNodeRemoved(const ProcessNode* node) final;
 
+  // OnIframeAttached bounces over to the UI thread to
+  // lookup the RenderFrameHost* associated with a given RemoteFrameToken,
+  // landing here.
+  void OnRemoteIframeAttachedImpl(
+      mojo::ReportBadMessageCallback bad_message_callback,
+      FrameNodeImpl* frame_node,
+      const blink::RemoteFrameToken& remote_frame_token,
+      const IframeAttributionData& iframe_attribution_data);
+
+  // To maintain strict ordering with OnRemoteIframeAttached events, detached
+  // events also detour through the UI thread to arrive here.
+  void OnRemoteIframeDetachedImpl(
+      FrameNodeImpl* parent_frame_node,
+      const blink::RemoteFrameToken& remote_frame_token);
+
   // Stores Chrome-wide data store used by the tracking.
   std::unique_ptr<DataStore> data_store_;
 };

components/performance_manager/v8_memory/v8_context_tracker_helpers.cc

@@ -76,6 +76,12 @@ bool IsWorkletToken(const blink::ExecutionContextToken& token) {
          token.Is<blink::PaintWorkletToken>();
 }
 
+bool IsWorkerToken(const blink::ExecutionContextToken& token) {
+  return token.Is<blink::DedicatedWorkerToken>() ||
+         token.Is<blink::ServiceWorkerToken>() ||
+         token.Is<blink::SharedWorkerToken>();
+}
+
 const execution_context::ExecutionContext* GetExecutionContext(
     const blink::ExecutionContextToken& token,
     Graph* graph) {

components/performance_manager/v8_memory/v8_context_tracker_helpers.h

@@ -43,6 +43,10 @@ bool IsValidExtensionId(const std::string& s) WARN_UNUSED_RESULT;
 bool IsWorkletToken(const blink::ExecutionContextToken& token)
     WARN_UNUSED_RESULT;
 
+// Returns true if an ExecutionContextToken corresponds to a worklet.
+bool IsWorkerToken(const blink::ExecutionContextToken& token)
+    WARN_UNUSED_RESULT;
+
 // Looks up the execution context corresponding to the given token. Note that
 // the ExecutionContextRegistry must be installed on the graph.
 const execution_context::ExecutionContext* GetExecutionContext(

components/performance_manager/v8_memory/v8_context_tracker_internal.cc

@@ -5,6 +5,7 @@
 #include "components/performance_manager/v8_memory/v8_context_tracker_internal.h"
 
 #include "base/check.h"
+#include "components/performance_manager/v8_memory/v8_context_tracker_helpers.h"
 
 namespace performance_manager {
 namespace v8_memory {
@@ -16,7 +17,7 @@ namespace internal {
 ExecutionContextData::ExecutionContextData(
     ProcessData* process_data,
     const blink::ExecutionContextToken& token,
-    const base::Optional<IframeAttributionData> iframe_attribution_data)
+    const base::Optional<IframeAttributionData>& iframe_attribution_data)
     : ExecutionContextState(token, iframe_attribution_data),
       process_data_(process_data) {}
 
@@ -67,6 +68,14 @@ bool ExecutionContextData::MarkDestroyed(util::PassKey<ProcessData>) {
   return true;
 }
 
+bool ExecutionContextData::MarkMainWorldSeen(
+    util::PassKey<V8ContextTrackerDataStore>) {
+  if (main_world_seen_)
+    return false;
+  main_world_seen_ = true;
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // RemoteFrameData implementation:
 
@@ -108,7 +117,12 @@ V8ContextData::V8ContextData(ProcessData* process_data,
     : V8ContextState(description, execution_context_data),
       process_data_(process_data) {
   DCHECK(process_data);
+  DCHECK_EQ(static_cast<bool>(execution_context_data),
+            static_cast<bool>(description.execution_context_token));
   if (execution_context_data) {
+    DCHECK_EQ(execution_context_data->GetToken(),
+              description.execution_context_token.value());
+
     // These must be same process.
     DCHECK_EQ(process_data, execution_context_data->process_data());
     execution_context_data->IncrementV8ContextCount(PassKey());
@@ -142,6 +156,25 @@ bool V8ContextData::MarkDetached(util::PassKey<ProcessData>) {
   return true;
 }
 
+bool V8ContextData::IsMainV8Context() const {
+  auto* ec_data = GetExecutionContextData();
+  if (!ec_data)
+    return false;
+  // ExecutionContexts hosting worklets have no main world (there can be many
+  // worklets sharing an ExecutionContext).
+  if (IsWorkletToken(ec_data->GetToken()))
+    return false;
+
+  // We've already checked sane combinations of ExecutionContextToken types and
+  // world types in ValidateV8ContextDescription, so don't need to be overly
+  // thorough here.
+
+  // Only main frames and workers can be "main" contexts.
+  auto world_type = description.world_type;
+  return world_type == V8ContextWorldType::kMain ||
+         world_type == V8ContextWorldType::kWorkerOrWorklet;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // ProcessData implementation:
 
@@ -284,11 +317,17 @@ void V8ContextTrackerDataStore::Pass(std::unique_ptr<RemoteFrameData> rf_data) {
   DCHECK(result.second);
 }
 
-void V8ContextTrackerDataStore::Pass(std::unique_ptr<V8ContextData> v8_data) {
+bool V8ContextTrackerDataStore::Pass(std::unique_ptr<V8ContextData> v8_data) {
   DCHECK(v8_data.get());
+  auto* ec_data = v8_data->GetExecutionContextData();
+  if (ec_data && v8_data->IsMainV8Context()) {
+    if (!ec_data->MarkMainWorldSeen(PassKey()))
+      return false;
+  }
   v8_data->process_data()->Add(PassKey(), v8_data.get());
   auto result = global_v8_context_datas_.insert(std::move(v8_data));
   DCHECK(result.second);
+  return true;
 }
 
 ExecutionContextData* V8ContextTrackerDataStore::Get(
@@ -324,12 +363,14 @@ void V8ContextTrackerDataStore::MarkDestroyed(ExecutionContextData* ec_data) {
   }
 }
 
-void V8ContextTrackerDataStore::MarkDetached(V8ContextData* v8_data) {
+bool V8ContextTrackerDataStore::MarkDetached(V8ContextData* v8_data) {
   DCHECK(v8_data);
   if (v8_data->process_data()->MarkDetached(PassKey(), v8_data)) {
     DCHECK_LT(detached_v8_context_count_, global_v8_context_datas_.size());
     ++detached_v8_context_count_;
+    return true;
   }
+  return false;
 }
 
 void V8ContextTrackerDataStore::Destroy(

components/performance_manager/v8_memory/v8_context_tracker_internal.h

@@ -36,6 +36,7 @@ class ExecutionContextData;
 class ProcessData;
 class RemoteFrameData;
 class V8ContextData;
+class V8ContextTrackerDataStore;
 
 // A comparator for "Data" objects that compares by token.
 template <typename DataType, typename TokenType>
@@ -68,7 +69,7 @@ class ExecutionContextData : public base::LinkNode<ExecutionContextData>,
   ExecutionContextData(
       ProcessData* process_data,
       const blink::ExecutionContextToken& token,
-      const base::Optional<IframeAttributionData> iframe_attribution_data);
+      const base::Optional<IframeAttributionData>& iframe_attribution_data);
   ExecutionContextData& operator=(const ExecutionContextData&) = delete;
   ~ExecutionContextData() override;
 
@@ -76,6 +77,7 @@ class ExecutionContextData : public base::LinkNode<ExecutionContextData>,
   ProcessData* process_data() const { return process_data_; }
   RemoteFrameData* remote_frame_data() { return remote_frame_data_; }
   size_t v8_context_count() const { return v8_context_count_; }
+  bool main_world_seen() const { return main_world_seen_; }
 
   // For consistency, all Data objects have a GetToken() function.
   const blink::ExecutionContextToken& GetToken() const { return token; }
@@ -105,6 +107,13 @@ class ExecutionContextData : public base::LinkNode<ExecutionContextData>,
   // if it was already destroyed.
   WARN_UNUSED_RESULT bool MarkDestroyed(util::PassKey<ProcessData>);
 
+  // Marks the main world as having been seen. Returns true if the state changed
+  // and false if this had already occurred. This is called when the
+  // V8ContextData is passed to the data store and can prevent it from
+  // succeeding.
+  WARN_UNUSED_RESULT bool MarkMainWorldSeen(
+      util::PassKey<V8ContextTrackerDataStore>);
+
  private:
   ProcessData* const process_data_;
 
@@ -112,6 +121,10 @@ class ExecutionContextData : public base::LinkNode<ExecutionContextData>,
 
   // The count of V8ContextDatas keeping this object alive.
   size_t v8_context_count_ = 0;
+
+  // True if a main world V8Context has been seen for this EC. Can only ever
+  // toggle from false to true.
+  bool main_world_seen_ = false;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -189,6 +202,12 @@ class V8ContextData : public base::LinkNode<V8ContextData>,
   // if it was already detached.
   WARN_UNUSED_RESULT bool MarkDetached(util::PassKey<ProcessData>);
 
+  // Returns true if this is the "main" V8Context for an ExecutionContext.
+  // This will return true if |GetExecutionContextData()| is a frame and
+  // |description.world_type| is kMain, or if |GetExecutionContextData()| is a
+  // worker and |description.world_type| is a kWorkerOrWorklet.
+  bool IsMainV8Context() const;
+
  private:
   ProcessData* const process_data_;
 };
@@ -290,16 +309,17 @@ class V8ContextTrackerDataStore {
   // |ec_data| to the impl that "ShouldDestroy" should return false.
   void Pass(std::unique_ptr<ExecutionContextData> ec_data);
   void Pass(std::unique_ptr<RemoteFrameData> rf_data);
-  void Pass(std::unique_ptr<V8ContextData> v8_data);
+  WARN_UNUSED_RESULT bool Pass(std::unique_ptr<V8ContextData> v8_data);
 
   // Looks up owned objects by token.
   ExecutionContextData* Get(const blink::ExecutionContextToken& token);
   RemoteFrameData* Get(const blink::RemoteFrameToken& token);
   V8ContextData* Get(const blink::V8ContextToken& token);
 
-  // For marking objects as detached/destroyed.
+  // For marking objects as detached/destroyed. "MarkDetached" returns true if
+  // the object was not previously detached, false otherwise.
   void MarkDestroyed(ExecutionContextData* ec_data);
-  void MarkDetached(V8ContextData* v8_data);
+  WARN_UNUSED_RESULT bool MarkDetached(V8ContextData* v8_data);
 
   // Destroys objects by token. They must exist ("Get" should return non
   // nullptr).

components/performance_manager/v8_memory/v8_context_tracker_internal_unittest.cc

@@ -42,6 +42,14 @@ class V8ContextTrackerInternalTest : public GraphTestHarness {
   MockSinglePageWithMultipleProcessesGraph mock_graph_;
 };
 
+V8ContextDescription MakeMatchingV8ContextDescription(
+    ExecutionContextData* ec_data) {
+  DCHECK(ec_data);
+  V8ContextDescription v8_desc;
+  v8_desc.execution_context_token = ec_data->GetToken();
+  return v8_desc;
+}
+
 using V8ContextTrackerInternalDeathTest = V8ContextTrackerInternalTest;
 
 }  // namespace
@@ -57,6 +65,35 @@ TEST_F(V8ContextTrackerInternalDeathTest,
   EXPECT_DCHECK_DEATH(data_store()->Pass(std::move(ec_data)));
 }
 
+TEST_F(V8ContextTrackerInternalDeathTest,
+       MultipleMainWorldsForExecutionContextFails) {
+  auto* process_data = ProcessData::GetOrCreate(
+      static_cast<ProcessNodeImpl*>(mock_graph_.process.get()));
+  std::unique_ptr<ExecutionContextData> ec_data =
+      std::make_unique<ExecutionContextData>(
+          process_data, mock_graph_.frame->frame_token(), base::nullopt);
+  EXPECT_TRUE(ec_data->ShouldDestroy());
+  EXPECT_FALSE(ec_data->main_world_seen());
+
+  V8ContextDescription v8_desc;
+  v8_desc.world_type = V8ContextWorldType::kMain;
+  v8_desc.execution_context_token = ec_data->GetToken();
+
+  std::unique_ptr<V8ContextData> v8_data =
+      std::make_unique<V8ContextData>(process_data, v8_desc, ec_data.get());
+  EXPECT_TRUE(v8_data->IsMainV8Context());
+  EXPECT_TRUE(data_store()->Pass(std::move(v8_data)));
+  EXPECT_TRUE(ec_data->main_world_seen());
+
+  v8_desc.token = blink::V8ContextToken();
+  v8_data =
+      std::make_unique<V8ContextData>(process_data, v8_desc, ec_data.get());
+  EXPECT_TRUE(v8_data->IsMainV8Context());
+  EXPECT_FALSE(data_store()->Pass(std::move(v8_data)));
+
+  data_store()->Pass(std::move(ec_data));
+}
+
 TEST_F(V8ContextTrackerInternalDeathTest, SameProcessRemoteFrameDataExplodes) {
   auto* process_data = ProcessData::GetOrCreate(
       static_cast<ProcessNodeImpl*>(mock_graph_.process.get()));
@@ -78,9 +115,10 @@ TEST_F(V8ContextTrackerInternalDeathTest, CrossProcessV8ContextDataExplodes) {
       std::make_unique<ExecutionContextData>(
           process_data, mock_graph_.frame->frame_token(), base::nullopt);
   std::unique_ptr<V8ContextData> v8_data;
-  EXPECT_DCHECK_DEATH(
-      v8_data = std::make_unique<V8ContextData>(
-          other_process_data, V8ContextDescription(), ec_data.get()));
+  EXPECT_DCHECK_DEATH(v8_data = std::make_unique<V8ContextData>(
+                          other_process_data,
+                          MakeMatchingV8ContextDescription(ec_data.get()),
+                          ec_data.get()));
 }
 
 TEST_F(V8ContextTrackerInternalTest, ExecutionContextDataShouldDestroy) {
@@ -106,14 +144,16 @@ TEST_F(V8ContextTrackerInternalTest, ExecutionContextDataShouldDestroy) {
 
   // Adding a V8ContextData should also keep the object alive.
   std::unique_ptr<V8ContextData> v8_data1 = std::make_unique<V8ContextData>(
-      process_data, V8ContextDescription(), ec_data.get());
+      process_data, MakeMatchingV8ContextDescription(ec_data.get()),
+      ec_data.get());
   EXPECT_TRUE(ec_data->remote_frame_data());
   EXPECT_EQ(1u, ec_data->v8_context_count());
   EXPECT_FALSE(ec_data->ShouldDestroy());
 
   // Add another V8ContextData.
   std::unique_ptr<V8ContextData> v8_data2 = std::make_unique<V8ContextData>(
-      process_data, V8ContextDescription(), ec_data.get());
+      process_data, MakeMatchingV8ContextDescription(ec_data.get()),
+      ec_data.get());
   EXPECT_TRUE(ec_data->remote_frame_data());
   EXPECT_EQ(2u, ec_data->v8_context_count());
   EXPECT_FALSE(ec_data->ShouldDestroy());
@@ -194,13 +234,14 @@ TEST_F(V8ContextTrackerInternalTest,
 
   // Create a V8ContextData.
   std::unique_ptr<V8ContextData> v8_data = std::make_unique<V8ContextData>(
-      process_data, V8ContextDescription(), ec_data.get());
+      process_data, MakeMatchingV8ContextDescription(ec_data.get()),
+      ec_data.get());
   auto* raw_v8_data = v8_data.get();
   EXPECT_FALSE(v8_data->IsTracked());
 
   // Pass both of these to the Impl.
   data_store()->Pass(std::move(ec_data));
-  data_store()->Pass(std::move(v8_data));
+  EXPECT_TRUE(data_store()->Pass(std::move(v8_data)));
   EXPECT_TRUE(raw_ec_data->IsTracked());
   EXPECT_TRUE(raw_v8_data->IsTracked());
   EXPECT_EQ(1u, data_store()->GetExecutionContextDataCount());
@@ -231,15 +272,17 @@ TEST_F(V8ContextTrackerInternalTest, ContextCounts) {
   auto* raw_ec_data = ec_data.get();
 
   std::unique_ptr<V8ContextData> v8_data1 = std::make_unique<V8ContextData>(
-      process_data, V8ContextDescription(), ec_data.get());
+      process_data, MakeMatchingV8ContextDescription(ec_data.get()),
+      ec_data.get());
   auto* raw_v8_data1 = v8_data1.get();
 
   std::unique_ptr<V8ContextData> v8_data2 = std::make_unique<V8ContextData>(
-      process_data, V8ContextDescription(), ec_data.get());
+      process_data, MakeMatchingV8ContextDescription(ec_data.get()),
+      ec_data.get());
 
   data_store()->Pass(std::move(ec_data));
-  data_store()->Pass(std::move(v8_data1));
-  data_store()->Pass(std::move(v8_data2));
+  EXPECT_TRUE(data_store()->Pass(std::move(v8_data1)));
+  EXPECT_TRUE(data_store()->Pass(std::move(v8_data2)));
 
   EXPECT_EQ(1u, data_store()->GetExecutionContextDataCount());
   EXPECT_EQ(0u, data_store()->GetDestroyedExecutionContextDataCount());
@@ -255,8 +298,9 @@ TEST_F(V8ContextTrackerInternalTest, ContextCounts) {
   EXPECT_TRUE(raw_ec_data->destroyed);
 
   EXPECT_FALSE(raw_v8_data1->detached);
-  data_store()->MarkDetached(raw_v8_data1);
+  EXPECT_TRUE(data_store()->MarkDetached(raw_v8_data1));
   EXPECT_TRUE(raw_v8_data1->detached);
+  EXPECT_FALSE(data_store()->MarkDetached(raw_v8_data1));
 
   EXPECT_EQ(1u, data_store()->GetExecutionContextDataCount());
   EXPECT_EQ(1u, data_store()->GetDestroyedExecutionContextDataCount());
@@ -326,11 +370,11 @@ class V8ContextTrackerInternalTearDownOrderTest
 
     // Create a couple V8ContextDatas.
     std::unique_ptr<V8ContextData> v8_data = std::make_unique<V8ContextData>(
-        process_data_, V8ContextDescription(), ec_data_);
-    data_store()->Pass(std::move(v8_data));
-    v8_data = std::make_unique<V8ContextData>(process_data_,
-                                              V8ContextDescription(), ec_data_);
-    data_store()->Pass(std::move(v8_data));
+        process_data_, MakeMatchingV8ContextDescription(ec_data_), ec_data_);
+    EXPECT_TRUE(data_store()->Pass(std::move(v8_data)));
+    v8_data = std::make_unique<V8ContextData>(
+        process_data_, MakeMatchingV8ContextDescription(ec_data_), ec_data_);
+    EXPECT_TRUE(data_store()->Pass(std::move(v8_data)));
 
     EXPECT_EQ(1u, data_store()->GetExecutionContextDataCount());
     EXPECT_EQ(1u, data_store()->GetRemoteFrameDataCount());