Revert "cppgc-js: heap snapshot: Add logic for querying detachedness"

This reverts commit e68285e2.

Reason for revert: ASAN test failing:
https://ci.chromium.org/p/v8/builders/ci/V8%20Mac64%20ASAN/29838?

Original change's description:
> cppgc-js: heap snapshot: Add logic for querying detachedness
>
> Adds infrastructure to allow embedders specifying a detachedness state
> that is queried when encountering an object with a TraceReference that
> has a non-zero wrapper class id set.
>
> Change-Id: Ie7f2f253544ee25a25565eb08d82e9df5f0a74d2
> Bug: chromium:1056170
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2502345
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#70841}

TBR=ulan@chromium.org,mlippautz@chromium.org,omerkatz@chromium.org

Change-Id: Ic13337b9c5b336a81efa5f2672f5a501084b5326
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:1056170
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2505613Reviewed-by: Francis McCabe <fgm@chromium.org>
Commit-Queue: Francis McCabe <fgm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70843}

include/v8-profiler.h

@@ -817,18 +817,6 @@ class V8_EXPORT HeapProfiler {
                                              v8::EmbedderGraph* graph,
                                              void* data);
 
-  /**
-   * Callback function invoked during heap snapshot generation to retrieve
-   * the detachedness state of an object referenced by a TracedReference.
-   *
-   * The callback takes Local<Value> as parameter to allow the embedder to
-   * unpack the TracedReference into a Local and reuse that Local for different
-   * purposes.
-   */
-  using GetDetachednessCallback = EmbedderGraph::Node::Detachedness (*)(
-      v8::Isolate* isolate, const v8::Local<v8::Value>& v8_value,
-      uint16_t class_id, void* data);
-
   /** Returns the number of snapshots taken. */
   int GetSnapshotCount();
 
@@ -979,10 +967,6 @@ class V8_EXPORT HeapProfiler {
   void RemoveBuildEmbedderGraphCallback(BuildEmbedderGraphCallback callback,
                                         void* data);
 
-  void SetGetDetachednessCallback(GetDetachednessCallback callback, void* data);
-  void ClearGetDetachednessCallback(GetDetachednessCallback callback,
-                                    void* data);
-
   /**
    * Default value of persistent handle class ID. Must not be used to
    * define a class. Can be used to reset a class of a persistent

src/api/api.cc

@@ -11027,18 +11027,6 @@ void HeapProfiler::RemoveBuildEmbedderGraphCallback(
       callback, data);
 }
 
-void HeapProfiler::SetGetDetachednessCallback(GetDetachednessCallback callback,
-                                              void* data) {
-  reinterpret_cast<i::HeapProfiler*>(this)->SetGetDetachednessCallback(callback,
-                                                                       data);
-}
-
-void HeapProfiler::ClearGetDetachednessCallback(
-    GetDetachednessCallback callback, void* data) {
-  reinterpret_cast<i::HeapProfiler*>(this)->ClearGetDetachednessCallback(
-      callback, data);
-}
-
 void EmbedderHeapTracer::SetStackStart(void* stack_start) {
   CHECK(isolate_);
   reinterpret_cast<i::Isolate*>(isolate_)->global_handles()->SetStackStart(

src/heap/cppgc-js/cpp-snapshot.cc

@@ -45,15 +45,9 @@ class EmbedderNode : public v8::EmbedderGraph::Node {
   }
   Node* WrapperNode() final { return wrapper_node_; }
 
-  void SetDetachedness(Detachedness detachedness) {
-    detachedness_ = detachedness;
-  }
-  Detachedness GetDetachedness() final { return detachedness_; }
-
  private:
   const char* name_;
   Node* wrapper_node_ = nullptr;
-  Detachedness detachedness_ = Detachedness::kUnknown;
 };
 
 // Node representing an artificial root group, e.g., set of Persistent handles.
@@ -411,13 +405,6 @@ class CppGraphBuilderImpl final {
               reinterpret_cast<v8::internal::Isolate*>(cpp_heap_.isolate()),
               v8_value, parent.header()->Payload())) {
         parent.get_node()->SetWrapperNode(v8_node);
-
-        auto* profiler =
-            reinterpret_cast<Isolate*>(cpp_heap_.isolate())->heap_profiler();
-        if (profiler->HasGetDetachednessCallback()) {
-          parent.get_node()->SetDetachedness(
-              profiler->GetDetachedness(v8_value, ref.WrapperClassId()));
-        }
       }
     }
   }

src/profiler/heap-profiler.cc

@@ -64,24 +64,6 @@ void HeapProfiler::BuildEmbedderGraph(Isolate* isolate,
   }
 }
 
-void HeapProfiler::SetGetDetachednessCallback(
-    v8::HeapProfiler::GetDetachednessCallback callback, void* data) {
-  get_detachedness_callback_ = {callback, data};
-}
-
-void HeapProfiler::ClearGetDetachednessCallback(
-    v8::HeapProfiler::GetDetachednessCallback callback, void* data) {
-  get_detachedness_callback_ = {nullptr, nullptr};
-}
-
-v8::EmbedderGraph::Node::Detachedness HeapProfiler::GetDetachedness(
-    const v8::Local<v8::Value> v8_value, uint16_t class_id) {
-  DCHECK(HasGetDetachednessCallback());
-  return get_detachedness_callback_.first(
-      reinterpret_cast<v8::Isolate*>(heap()->isolate()), v8_value, class_id,
-      get_detachedness_callback_.second);
-}
-
 HeapSnapshot* HeapProfiler::TakeSnapshot(
     v8::ActivityControl* control,
     v8::HeapProfiler::ObjectNameResolver* resolver,

src/profiler/heap-profiler.h

@@ -72,16 +72,6 @@ class HeapProfiler : public HeapObjectAllocationTracker {
     return !build_embedder_graph_callbacks_.empty();
   }
 
-  void SetGetDetachednessCallback(
-      v8::HeapProfiler::GetDetachednessCallback callback, void* data);
-  void ClearGetDetachednessCallback(
-      v8::HeapProfiler::GetDetachednessCallback callback, void* data);
-  bool HasGetDetachednessCallback() const {
-    return get_detachedness_callback_.first != nullptr;
-  }
-  v8::EmbedderGraph::Node::Detachedness GetDetachedness(
-      const v8::Local<v8::Value> v8_value, uint16_t class_id);
-
   bool is_tracking_object_moves() const { return is_tracking_object_moves_; }
 
   Handle<HeapObject> FindHeapObjectById(SnapshotObjectId id);
@@ -109,8 +99,6 @@ class HeapProfiler : public HeapObjectAllocationTracker {
   std::unique_ptr<SamplingHeapProfiler> sampling_heap_profiler_;
   std::vector<std::pair<v8::HeapProfiler::BuildEmbedderGraphCallback, void*>>
       build_embedder_graph_callbacks_;
-  std::pair<v8::HeapProfiler::GetDetachednessCallback, void*>
-      get_detachedness_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(HeapProfiler);
 };

test/unittests/heap/unified-heap-snapshot-unittest.cc

@@ -14,7 +14,6 @@
 #include "include/v8-profiler.h"
 #include "src/api/api-inl.h"
 #include "src/heap/cppgc-js/cpp-heap.h"
-#include "src/heap/cppgc/object-allocator.h"
 #include "src/objects/objects-inl.h"
 #include "src/profiler/heap-snapshot-generator-inl.h"
 #include "src/profiler/heap-snapshot-generator.h"
@@ -276,52 +275,24 @@ class GCedWithJSRef : public cppgc::GarbageCollected<GCedWithJSRef> {
     v8_object_.SetWrapperClassId(class_id);
   }
 
-  uint16_t WrapperClassId() const { return v8_object_.WrapperClassId(); }
-
-  TracedReference<v8::Object>& wrapper() { return v8_object_; }
-
  private:
   TracedReference<v8::Object> v8_object_;
 };
 constexpr const char GCedWithJSRef::kExpectedName[];
 
-class JsTestingScope {
- public:
-  explicit JsTestingScope(v8::Isolate* isolate)
-      : isolate_(isolate),
-        handle_scope_(isolate),
-        context_(v8::Context::New(isolate)),
-        context_scope_(context_) {}
-
-  v8::Isolate* isolate() const { return isolate_; }
-  v8::Local<v8::Context> context() const { return context_; }
-
- private:
-  v8::Isolate* isolate_;
-  v8::HandleScope handle_scope_;
-  v8::Local<v8::Context> context_;
-  v8::Context::Scope context_scope_;
-};
-
-cppgc::Persistent<GCedWithJSRef> SetupWrapperWrappablePair(
-    JsTestingScope& testing_scope, cppgc::AllocationHandle& allocation_handle,
-    const char* name) {
-  cppgc::Persistent<GCedWithJSRef> gc_w_js_ref =
-      cppgc::MakeGarbageCollected<GCedWithJSRef>(allocation_handle);
-  v8::Local<v8::Object> wrapper_object = WrapperHelper::CreateWrapper(
-      testing_scope.context(), gc_w_js_ref.Get(), name);
-  gc_w_js_ref->SetV8Object(testing_scope.isolate(), wrapper_object);
-  return std::move(gc_w_js_ref);
-}
-
 }  // namespace
 
 TEST_F(UnifiedHeapSnapshotTest, JSReferenceForcesVisibleObject) {
   // Test ensures that a C++->JS reference forces an object to be visible in the
   // snapshot.
-  JsTestingScope testing_scope(v8_isolate());
-  cppgc::Persistent<GCedWithJSRef> gc_w_js_ref = SetupWrapperWrappablePair(
-      testing_scope, allocation_handle(), "LeafJSObject");
+  cppgc::Persistent<GCedWithJSRef> gc_w_js_ref =
+      cppgc::MakeGarbageCollected<GCedWithJSRef>(allocation_handle());
+  v8::HandleScope scope(v8_isolate());
+  v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
+  v8::Context::Scope context_scope(context);
+  v8::Local<v8::Object> api_object =
+      WrapperHelper::CreateWrapper(context, gc_w_js_ref.Get(), "LeafJSObject");
+  gc_w_js_ref->SetV8Object(v8_isolate(), api_object);
   const v8::HeapSnapshot* snapshot = TakeHeapSnapshot();
   EXPECT_TRUE(IsValidSnapshot(snapshot));
   EXPECT_TRUE(
@@ -337,19 +308,22 @@ TEST_F(UnifiedHeapSnapshotTest, MergedWrapperNode) {
   // Test ensures that the snapshot sets a wrapper node for C++->JS references
   // that have a class id set and that object nodes are merged into the C++
   // node, i.e., the directly reachable JS object is merged into the C++ object.
-  JsTestingScope testing_scope(v8_isolate());
-  cppgc::Persistent<GCedWithJSRef> gc_w_js_ref = SetupWrapperWrappablePair(
-      testing_scope, allocation_handle(), "MergedObject");
-  gc_w_js_ref->SetWrapperClassId(1);  // Any class id will do.
-  v8::Local<v8::Object> next_object = WrapperHelper::CreateWrapper(
-      testing_scope.context(), nullptr, "NextObject");
+  cppgc::Persistent<GCedWithJSRef> gc_w_js_ref =
+      cppgc::MakeGarbageCollected<GCedWithJSRef>(allocation_handle());
+  v8::HandleScope scope(v8_isolate());
+  v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
+  v8::Context::Scope context_scope(context);
   v8::Local<v8::Object> wrapper_object =
-      gc_w_js_ref->wrapper().Get(v8_isolate());
+      WrapperHelper::CreateWrapper(context, gc_w_js_ref.Get(), "MergedObject");
+  gc_w_js_ref->SetV8Object(v8_isolate(), wrapper_object);
+  gc_w_js_ref->SetWrapperClassId(1);  // Any class id will do.
   // Chain another object to `wrapper_object`. Since `wrapper_object` should be
   // merged into `GCedWithJSRef`, the additional object must show up as direct
   // child from `GCedWithJSRef`.
+  v8::Local<v8::Object> next_object =
+      WrapperHelper::CreateWrapper(context, nullptr, "NextObject");
   wrapper_object
-      ->Set(testing_scope.context(),
+      ->Set(context,
             v8::String::NewFromUtf8(v8::Isolate::GetCurrent(), "link")
                 .ToLocalChecked(),
             next_object)
@@ -366,126 +340,5 @@ TEST_F(UnifiedHeapSnapshotTest, MergedWrapperNode) {
                             }));
 }
 
-namespace {
-
-constexpr uint16_t kClassIdForAttachedState = 0xAAAA;
-
-class DetachednessHandler {
- public:
-  static size_t callback_count;
-
-  static v8::EmbedderGraph::Node::Detachedness GetDetachedness(
-      v8::Isolate* isolate, const v8::Local<v8::Value>& v8_value,
-      uint16_t class_id, void* data) {
-    callback_count++;
-    return class_id == kClassIdForAttachedState
-               ? v8::EmbedderGraph::Node::Detachedness::kAttached
-               : v8::EmbedderGraph::Node::Detachedness::kDetached;
-  }
-
-  static void Reset() { callback_count = 0; }
-};
-// static
-size_t DetachednessHandler::callback_count = 0;
-
-template <typename Callback>
-void ForEachEntryWithName(const v8::HeapSnapshot* snapshot, const char* needle,
-                          Callback callback) {
-  const HeapSnapshot* heap_snapshot =
-      reinterpret_cast<const HeapSnapshot*>(snapshot);
-  for (const HeapEntry& entry : heap_snapshot->entries()) {
-    if (strcmp(entry.name(), needle) == 0) {
-      callback(entry);
-    }
-  }
-}
-
-constexpr uint8_t kExpectedDetachedValueForUnknown =
-    static_cast<uint8_t>(v8::EmbedderGraph::Node::Detachedness::kUnknown);
-constexpr uint8_t kExpectedDetachedValueForAttached =
-    static_cast<uint8_t>(v8::EmbedderGraph::Node::Detachedness::kAttached);
-constexpr uint8_t kExpectedDetachedValueForDetached =
-    static_cast<uint8_t>(v8::EmbedderGraph::Node::Detachedness::kDetached);
-
-}  // namespace
-
-TEST_F(UnifiedHeapSnapshotTest, NoTriggerForClassIdZero) {
-  // Test ensures that objects with JS references that have no class id set do
-  // not have their detachedness state queried.
-  JsTestingScope testing_scope(v8_isolate());
-  cppgc::Persistent<GCedWithJSRef> gc_w_js_ref = SetupWrapperWrappablePair(
-      testing_scope, allocation_handle(), "MergedObject");
-  DetachednessHandler::Reset();
-  v8_isolate()->GetHeapProfiler()->SetGetDetachednessCallback(
-      DetachednessHandler::GetDetachedness, nullptr);
-  gc_w_js_ref->SetWrapperClassId(0);
-  EXPECT_EQ(0u, gc_w_js_ref->WrapperClassId());
-  const v8::HeapSnapshot* snapshot = TakeHeapSnapshot();
-  EXPECT_EQ(0u, DetachednessHandler::callback_count);
-  EXPECT_TRUE(IsValidSnapshot(snapshot));
-  EXPECT_TRUE(
-      ContainsRetainingPath(*snapshot,
-                            {
-                                kExpectedCppRootsName,             // NOLINT
-                                GetExpectedName<GCedWithJSRef>(),  // NOLINT
-                            }));
-  ForEachEntryWithName(
-      snapshot, GetExpectedName<GCedWithJSRef>(), [](const HeapEntry& entry) {
-        EXPECT_EQ(kExpectedDetachedValueForUnknown, entry.detachedness());
-      });
-}
-
-TEST_F(UnifiedHeapSnapshotTest, TriggerDetachednessCallbackSettingAttached) {
-  // Test ensures that objects with JS references that have a non-zero class id
-  // set do  have their detachedness state queried and set (attached version).
-  JsTestingScope testing_scope(v8_isolate());
-  cppgc::Persistent<GCedWithJSRef> gc_w_js_ref = SetupWrapperWrappablePair(
-      testing_scope, allocation_handle(), "MergedObject");
-  DetachednessHandler::Reset();
-  v8_isolate()->GetHeapProfiler()->SetGetDetachednessCallback(
-      DetachednessHandler::GetDetachedness, nullptr);
-  gc_w_js_ref->SetWrapperClassId(kClassIdForAttachedState);
-  EXPECT_NE(0u, gc_w_js_ref->WrapperClassId());
-  const v8::HeapSnapshot* snapshot = TakeHeapSnapshot();
-  EXPECT_EQ(1u, DetachednessHandler::callback_count);
-  EXPECT_TRUE(IsValidSnapshot(snapshot));
-  EXPECT_TRUE(
-      ContainsRetainingPath(*snapshot,
-                            {
-                                kExpectedCppRootsName,             // NOLINT
-                                GetExpectedName<GCedWithJSRef>(),  // NOLINT
-                            }));
-  ForEachEntryWithName(
-      snapshot, GetExpectedName<GCedWithJSRef>(), [](const HeapEntry& entry) {
-        EXPECT_EQ(kExpectedDetachedValueForAttached, entry.detachedness());
-      });
-}
-
-TEST_F(UnifiedHeapSnapshotTest, TriggerDetachednessCallbackSettingDetached) {
-  // Test ensures that objects with JS references that have a non-zero class id
-  // set do  have their detachedness state queried and set (detached version).
-  JsTestingScope testing_scope(v8_isolate());
-  cppgc::Persistent<GCedWithJSRef> gc_w_js_ref = SetupWrapperWrappablePair(
-      testing_scope, allocation_handle(), "MergedObject");
-  DetachednessHandler::Reset();
-  v8_isolate()->GetHeapProfiler()->SetGetDetachednessCallback(
-      DetachednessHandler::GetDetachedness, nullptr);
-  gc_w_js_ref->SetWrapperClassId(kClassIdForAttachedState - 1);
-  EXPECT_NE(0u, gc_w_js_ref->WrapperClassId());
-  const v8::HeapSnapshot* snapshot = TakeHeapSnapshot();
-  EXPECT_EQ(1u, DetachednessHandler::callback_count);
-  EXPECT_TRUE(IsValidSnapshot(snapshot));
-  EXPECT_TRUE(
-      ContainsRetainingPath(*snapshot,
-                            {
-                                kExpectedCppRootsName,             // NOLINT
-                                GetExpectedName<GCedWithJSRef>(),  // NOLINT
-                            }));
-  ForEachEntryWithName(
-      snapshot, GetExpectedName<GCedWithJSRef>(), [](const HeapEntry& entry) {
-        EXPECT_EQ(kExpectedDetachedValueForDetached, entry.detachedness());
-      });
-}
-
 }  // namespace internal
 }  // namespace v8