From 50683aa68d3649ac27c6931fe4a95cfb24325805 Mon Sep 17 00:00:00 2001
From: Michael Lippautz <mlippautz@chromium.org>
Date: Thu, 9 Jun 2022 09:45:35 +0200
Subject: [PATCH] [api] Deprecate PersistentValueVector

Users can just use std::vector<Global<T>>.

Bug: v8:12915
Change-Id: I59fc8458e336df0dfaa3524f1197d4423482530e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3695578
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81023}
---
 include/v8-util.h                      |  6 ++--
 src/api/api.cc                         | 27 ++++++++---------
 src/debug/debug-interface.cc           | 11 ++++---
 src/debug/debug-interface.h            |  8 ++---
 src/inspector/v8-debugger.cc           | 16 +++++-----
 src/inspector/v8-runtime-agent-impl.cc |  8 ++---
 src/profiler/heap-profiler.cc          |  4 +--
 src/profiler/heap-profiler.h           |  2 +-
 test/cctest/test-api.cc                | 42 ++++++++++++--------------
 test/cctest/test-debug.cc              |  6 ++--
 10 files changed, 62 insertions(+), 68 deletions(-)

diff --git a/include/v8-util.h b/include/v8-util.h
index c54418aa25..159027d317 100644
--- a/include/v8-util.h
+++ b/include/v8-util.h
@@ -537,7 +537,6 @@ class StdGlobalValueMap : public GlobalValueMap<K, V, Traits> {
       : GlobalValueMap<K, V, Traits>(isolate) {}
 };
 
-
 class DefaultPersistentValueVectorTraits {
  public:
   typedef std::vector<PersistentContainerValue> Impl;
@@ -562,7 +561,6 @@ class DefaultPersistentValueVectorTraits {
   }
 };
 
-
 /**
  * A vector wrapper that safely stores Global values.
  * C++11 embedders don't need this class, as they can use Global
@@ -573,8 +571,8 @@ class DefaultPersistentValueVectorTraits {
  * PersistentContainerValue, with all conversion into and out of V8
  * handles being transparently handled by this class.
  */
-template<typename V, typename Traits = DefaultPersistentValueVectorTraits>
-class PersistentValueVector {
+template <typename V, typename Traits = DefaultPersistentValueVectorTraits>
+class V8_DEPRECATE_SOON("Use std::vector<Global<V>>.") PersistentValueVector {
  public:
   explicit PersistentValueVector(Isolate* isolate) : isolate_(isolate) { }
 
diff --git a/src/api/api.cc b/src/api/api.cc
index 06977d5507..e5433eb749 100644
--- a/src/api/api.cc
+++ b/src/api/api.cc
@@ -449,11 +449,7 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
 #endif  // V8_SANDBOXED_POINTERS
 
 struct SnapshotCreatorData {
-  explicit SnapshotCreatorData(Isolate* v8_isolate)
-      : isolate_(v8_isolate),
-        default_context_(),
-        contexts_(v8_isolate),
-        created_(false) {}
+  explicit SnapshotCreatorData(Isolate* v8_isolate) : isolate_(v8_isolate) {}
 
   static SnapshotCreatorData* cast(void* data) {
     return reinterpret_cast<SnapshotCreatorData*>(data);
@@ -463,9 +459,9 @@ struct SnapshotCreatorData {
   Isolate* isolate_;
   Persistent<Context> default_context_;
   SerializeInternalFieldsCallback default_embedder_fields_serializer_;
-  PersistentValueVector<Context> contexts_;
+  std::vector<Global<Context>> contexts_;
   std::vector<SerializeInternalFieldsCallback> embedder_fields_serializers_;
-  bool created_;
+  bool created_ = false;
 };
 
 }  // namespace
@@ -530,8 +526,8 @@ size_t SnapshotCreator::AddContext(Local<Context> context,
   DCHECK(!data->created_);
   Isolate* v8_isolate = data->isolate_;
   CHECK_EQ(v8_isolate, context->GetIsolate());
-  size_t index = data->contexts_.Size();
-  data->contexts_.Append(context);
+  size_t index = data->contexts_.size();
+  data->contexts_.emplace_back(v8_isolate, context);
   data->embedder_fields_serializers_.push_back(callback);
   return index;
 }
@@ -607,11 +603,12 @@ void ConvertSerializedObjectsToFixedArray(i::Isolate* i_isolate) {
 StartupData SnapshotCreator::CreateBlob(
     SnapshotCreator::FunctionCodeHandling function_code_handling) {
   SnapshotCreatorData* data = SnapshotCreatorData::cast(data_);
-  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(data->isolate_);
+  Isolate* isolate = data->isolate_;
+  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   DCHECK(!data->created_);
   DCHECK(!data->default_context_.IsEmpty());
 
-  const int num_additional_contexts = static_cast<int>(data->contexts_.Size());
+  const int num_additional_contexts = static_cast<int>(data->contexts_.size());
   const int num_contexts = num_additional_contexts + 1;  // The default context.
 
   // Create and store lists of embedder-provided data needed during
@@ -625,7 +622,7 @@ StartupData SnapshotCreator::CreateBlob(
     ConvertSerializedObjectsToFixedArray(
         data->default_context_.Get(data->isolate_));
     for (int i = 0; i < num_additional_contexts; i++) {
-      ConvertSerializedObjectsToFixedArray(data->contexts_.Get(i));
+      ConvertSerializedObjectsToFixedArray(data->contexts_[i].Get(isolate));
     }
 
     // We need to store the global proxy size upfront in case we need the
@@ -635,7 +632,7 @@ StartupData SnapshotCreator::CreateBlob(
                                             i::AllocationType::kOld);
     for (int i = 0; i < num_additional_contexts; i++) {
       i::Handle<i::Context> context =
-          v8::Utils::OpenHandle(*data->contexts_.Get(i));
+          v8::Utils::OpenHandle(*data->contexts_[i].Get(isolate));
       global_proxy_sizes->set(i,
                               i::Smi::FromInt(context->global_proxy().Size()));
     }
@@ -673,10 +670,10 @@ StartupData SnapshotCreator::CreateBlob(
     data->default_context_.Reset();
     for (int i = 0; i < num_additional_contexts; i++) {
       i::Handle<i::Context> context =
-          v8::Utils::OpenHandle(*data->contexts_.Get(i));
+          v8::Utils::OpenHandle(*data->contexts_[i].Get(isolate));
       contexts.push_back(*context);
     }
-    data->contexts_.Clear();
+    data->contexts_.clear();
   }
 
   // Check that values referenced by global/eternal handles are accounted for.
diff --git a/src/debug/debug-interface.cc b/src/debug/debug-interface.cc
index d1fada5aa1..599c6ab89a 100644
--- a/src/debug/debug-interface.cc
+++ b/src/debug/debug-interface.cc
@@ -872,7 +872,7 @@ int Location::GetColumnNumber() const {
 bool Location::IsEmpty() const { return is_empty_; }
 
 void GetLoadedScripts(Isolate* v8_isolate,
-                      PersistentValueVector<Script>& scripts) {
+                      std::vector<v8::Global<Script>>& scripts) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
   ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
   {
@@ -891,7 +891,7 @@ void GetLoadedScripts(Isolate* v8_isolate,
       if (!script.HasValidSource()) continue;
       i::HandleScope handle_scope(isolate);
       i::Handle<i::Script> script_handle(script, isolate);
-      scripts.Append(ToApiHandle<Script>(script_handle));
+      scripts.emplace_back(v8_isolate, ToApiHandle<Script>(script_handle));
     }
   }
 }
@@ -1140,7 +1140,7 @@ v8::MaybeLocal<v8::Value> EvaluateGlobalForTesting(
 
 void QueryObjects(v8::Local<v8::Context> v8_context,
                   QueryObjectPredicate* predicate,
-                  PersistentValueVector<v8::Object>* objects) {
+                  std::vector<v8::Global<v8::Object>>* objects) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_context->GetIsolate());
   ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
   isolate->heap_profiler()->QueryObjects(Utils::OpenHandle(*v8_context),
@@ -1148,7 +1148,7 @@ void QueryObjects(v8::Local<v8::Context> v8_context,
 }
 
 void GlobalLexicalScopeNames(v8::Local<v8::Context> v8_context,
-                             v8::PersistentValueVector<v8::String>* names) {
+                             std::vector<v8::Global<v8::String>>* names) {
   i::Handle<i::Context> context = Utils::OpenHandle(*v8_context);
   i::Isolate* isolate = context->GetIsolate();
   i::Handle<i::ScriptContextTable> table(
@@ -1161,7 +1161,8 @@ void GlobalLexicalScopeNames(v8::Local<v8::Context> v8_context,
     i::Handle<i::ScopeInfo> scope_info(script_context->scope_info(), isolate);
     for (auto it : i::ScopeInfo::IterateLocalNames(scope_info)) {
       if (i::ScopeInfo::VariableIsSynthetic(it->name())) continue;
-      names->Append(Utils::ToLocal(handle(it->name(), isolate)));
+      names->emplace_back(reinterpret_cast<Isolate*>(isolate),
+                          Utils::ToLocal(handle(it->name(), isolate)));
     }
   }
 }
diff --git a/src/debug/debug-interface.h b/src/debug/debug-interface.h
index cd600e9141..1f1effe938 100644
--- a/src/debug/debug-interface.h
+++ b/src/debug/debug-interface.h
@@ -254,8 +254,8 @@ class WasmScript : public Script {
 };
 #endif  // V8_ENABLE_WEBASSEMBLY
 
-V8_EXPORT_PRIVATE void GetLoadedScripts(Isolate* isolate,
-                                        PersistentValueVector<Script>& scripts);
+V8_EXPORT_PRIVATE void GetLoadedScripts(
+    Isolate* isolate, std::vector<v8::Global<Script>>& scripts);
 
 MaybeLocal<UnboundScript> CompileInspectorScript(Isolate* isolate,
                                                  Local<String> source);
@@ -553,10 +553,10 @@ class QueryObjectPredicate {
 
 void QueryObjects(v8::Local<v8::Context> context,
                   QueryObjectPredicate* predicate,
-                  v8::PersistentValueVector<v8::Object>* objects);
+                  std::vector<v8::Global<v8::Object>>* objects);
 
 void GlobalLexicalScopeNames(v8::Local<v8::Context> context,
-                             v8::PersistentValueVector<v8::String>* names);
+                             std::vector<v8::Global<v8::String>>* names);
 
 void SetReturnValue(v8::Isolate* isolate, v8::Local<v8::Value> value);
 
diff --git a/src/inspector/v8-debugger.cc b/src/inspector/v8-debugger.cc
index 82e5307359..9d3d70ff0d 100644
--- a/src/inspector/v8-debugger.cc
+++ b/src/inspector/v8-debugger.cc
@@ -139,10 +139,10 @@ std::vector<std::unique_ptr<V8DebuggerScript>> V8Debugger::getCompiledScripts(
     int contextGroupId, V8DebuggerAgentImpl* agent) {
   std::vector<std::unique_ptr<V8DebuggerScript>> result;
   v8::HandleScope scope(m_isolate);
-  v8::PersistentValueVector<v8::debug::Script> scripts(m_isolate);
+  std::vector<v8::Global<v8::debug::Script>> scripts;
   v8::debug::GetLoadedScripts(m_isolate, scripts);
-  for (size_t i = 0; i < scripts.Size(); ++i) {
-    v8::Local<v8::debug::Script> script = scripts.Get(i);
+  for (size_t i = 0; i < scripts.size(); ++i) {
+    v8::Local<v8::debug::Script> script = scripts[i].Get(m_isolate);
     if (!script->WasCompiled()) continue;
     if (!script->IsEmbedded()) {
       int contextId;
@@ -809,17 +809,17 @@ v8::MaybeLocal<v8::Array> V8Debugger::internalProperties(
 v8::Local<v8::Array> V8Debugger::queryObjects(v8::Local<v8::Context> context,
                                               v8::Local<v8::Object> prototype) {
   v8::Isolate* isolate = context->GetIsolate();
-  v8::PersistentValueVector<v8::Object> v8Objects(isolate);
+  std::vector<v8::Global<v8::Object>> v8_objects;
   MatchPrototypePredicate predicate(m_inspector, context, prototype);
-  v8::debug::QueryObjects(context, &predicate, &v8Objects);
+  v8::debug::QueryObjects(context, &predicate, &v8_objects);
 
   v8::MicrotasksScope microtasksScope(isolate,
                                       v8::MicrotasksScope::kDoNotRunMicrotasks);
   v8::Local<v8::Array> resultArray = v8::Array::New(
-      m_inspector->isolate(), static_cast<int>(v8Objects.Size()));
-  for (size_t i = 0; i < v8Objects.Size(); ++i) {
+      m_inspector->isolate(), static_cast<int>(v8_objects.size()));
+  for (size_t i = 0; i < v8_objects.size(); ++i) {
     createDataProperty(context, resultArray, static_cast<int>(i),
-                       v8Objects.Get(i));
+                       v8_objects[i].Get(isolate));
   }
   return resultArray;
 }
diff --git a/src/inspector/v8-runtime-agent-impl.cc b/src/inspector/v8-runtime-agent-impl.cc
index f4820f6678..a598af680a 100644
--- a/src/inspector/v8-runtime-agent-impl.cc
+++ b/src/inspector/v8-runtime-agent-impl.cc
@@ -672,12 +672,12 @@ Response V8RuntimeAgentImpl::globalLexicalScopeNames(
   response = scope.initialize();
   if (!response.IsSuccess()) return response;
 
-  v8::PersistentValueVector<v8::String> names(m_inspector->isolate());
+  std::vector<v8::Global<v8::String>> names;
   v8::debug::GlobalLexicalScopeNames(scope.context(), &names);
   *outNames = std::make_unique<protocol::Array<String16>>();
-  for (size_t i = 0; i < names.Size(); ++i) {
-    (*outNames)->emplace_back(
-        toProtocolString(m_inspector->isolate(), names.Get(i)));
+  for (size_t i = 0; i < names.size(); ++i) {
+    (*outNames)->emplace_back(toProtocolString(
+        m_inspector->isolate(), names[i].Get(m_inspector->isolate())));
   }
   return Response::Success();
 }
diff --git a/src/profiler/heap-profiler.cc b/src/profiler/heap-profiler.cc
index dfe37b9911..89b39311cb 100644
--- a/src/profiler/heap-profiler.cc
+++ b/src/profiler/heap-profiler.cc
@@ -241,7 +241,7 @@ Isolate* HeapProfiler::isolate() const { return heap()->isolate(); }
 
 void HeapProfiler::QueryObjects(Handle<Context> context,
                                 debug::QueryObjectPredicate* predicate,
-                                PersistentValueVector<v8::Object>* objects) {
+                                std::vector<v8::Global<v8::Object>>* objects) {
   {
     HandleScope handle_scope(isolate());
     std::vector<Handle<JSTypedArray>> on_heap_typed_arrays;
@@ -279,7 +279,7 @@ void HeapProfiler::QueryObjects(Handle<Context> context,
     v8::Local<v8::Object> v8_obj(
         Utils::ToLocal(handle(JSObject::cast(heap_obj), isolate())));
     if (!predicate->Filter(v8_obj)) continue;
-    objects->Append(v8_obj);
+    objects->emplace_back(reinterpret_cast<v8::Isolate*>(isolate()), v8_obj);
   }
 }
 
diff --git a/src/profiler/heap-profiler.h b/src/profiler/heap-profiler.h
index 0b8fe97516..6a919dbddf 100644
--- a/src/profiler/heap-profiler.h
+++ b/src/profiler/heap-profiler.h
@@ -90,7 +90,7 @@ class HeapProfiler : public HeapObjectAllocationTracker {
 
   void QueryObjects(Handle<Context> context,
                     debug::QueryObjectPredicate* predicate,
-                    v8::PersistentValueVector<v8::Object>* objects);
+                    std::vector<v8::Global<v8::Object>>* objects);
 
  private:
   void MaybeClearStringsStorage();
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index ec0a95da3b..c1e82ae02d 100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -4395,8 +4395,7 @@ TEST(GlobalValueMap) {
   TestGlobalValueMap<WeakMap>();
 }
 
-
-TEST(PersistentValueVector) {
+TEST(VectorOfGlobals) {
   LocalContext env;
   v8::Isolate* isolate = env->GetIsolate();
   v8::internal::GlobalHandles* global_handles =
@@ -4404,41 +4403,40 @@ TEST(PersistentValueVector) {
   size_t handle_count = global_handles->handles_count();
   HandleScope scope(isolate);
 
-  v8::PersistentValueVector<v8::Object> vector(isolate);
+  std::vector<v8::Global<v8::Object>> vector;
 
   Local<v8::Object> obj1 = v8::Object::New(isolate);
   Local<v8::Object> obj2 = v8::Object::New(isolate);
   v8::Global<v8::Object> obj3(isolate, v8::Object::New(isolate));
 
-  CHECK(vector.IsEmpty());
-  CHECK_EQ(0, static_cast<int>(vector.Size()));
+  CHECK(vector.empty());
+  CHECK_EQ(0, static_cast<int>(vector.size()));
 
-  vector.ReserveCapacity(3);
-  CHECK(vector.IsEmpty());
+  vector.reserve(3);
+  CHECK(vector.empty());
 
-  vector.Append(obj1);
-  vector.Append(obj2);
-  vector.Append(obj1);
-  vector.Append(obj3.Pass());
-  vector.Append(obj1);
+  vector.emplace_back(isolate, obj1);
+  vector.emplace_back(isolate, obj2);
+  vector.emplace_back(isolate, obj1);
+  vector.emplace_back(obj3.Pass());
+  vector.emplace_back(isolate, obj1);
 
-  CHECK(!vector.IsEmpty());
-  CHECK_EQ(5, static_cast<int>(vector.Size()));
+  CHECK(!vector.empty());
+  CHECK_EQ(5, static_cast<int>(vector.size()));
   CHECK(obj3.IsEmpty());
-  CHECK(obj1->Equals(env.local(), vector.Get(0)).FromJust());
-  CHECK(obj1->Equals(env.local(), vector.Get(2)).FromJust());
-  CHECK(obj1->Equals(env.local(), vector.Get(4)).FromJust());
-  CHECK(obj2->Equals(env.local(), vector.Get(1)).FromJust());
+  CHECK(obj1->Equals(env.local(), vector[0].Get(isolate)).FromJust());
+  CHECK(obj1->Equals(env.local(), vector[2].Get(isolate)).FromJust());
+  CHECK(obj1->Equals(env.local(), vector[4].Get(isolate)).FromJust());
+  CHECK(obj2->Equals(env.local(), vector[1].Get(isolate)).FromJust());
 
   CHECK_EQ(5 + handle_count, global_handles->handles_count());
 
-  vector.Clear();
-  CHECK(vector.IsEmpty());
-  CHECK_EQ(0, static_cast<int>(vector.Size()));
+  vector.clear();
+  CHECK(vector.empty());
+  CHECK_EQ(0, static_cast<int>(vector.size()));
   CHECK_EQ(handle_count, global_handles->handles_count());
 }
 
-
 THREADED_TEST(GlobalHandleUpcast) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::HandleScope scope(isolate);
diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc
index 5e2b5f024f..7c2c599d8c 100644
--- a/test/cctest/test-debug.cc
+++ b/test/cctest/test-debug.cc
@@ -4519,11 +4519,11 @@ TEST(DebugGetPossibleBreakpointsReturnLocations) {
       "  return x > 2 ? fib(x - 1) + fib(x - 2) : fib(1) + fib(0);\n"
       "}");
   CompileRun(source);
-  v8::PersistentValueVector<v8::debug::Script> scripts(isolate);
+  std::vector<v8::Global<v8::debug::Script>> scripts;
   v8::debug::GetLoadedScripts(isolate, scripts);
-  CHECK_EQ(scripts.Size(), 1);
+  CHECK_EQ(scripts.size(), 1);
   std::vector<v8::debug::BreakLocation> locations;
-  CHECK(scripts.Get(0)->GetPossibleBreakpoints(
+  CHECK(scripts[0].Get(isolate)->GetPossibleBreakpoints(
       v8::debug::Location(0, 17), v8::debug::Location(), true, &locations));
   int returns_count = 0;
   for (size_t i = 0; i < locations.size(); ++i) {