[cleanup] Replace List with std::vector in api.

The members of HandleScopeImplementer are copied with memcpy when
the isolate is transferred to another thread. List contained some
primitives which allowed us to manually free the backing store, which
was needed in order to ensure that threads would not hold on to
old pointers and use them later. With std::vector, we can't do that.

Here we change the HandleScopeImplementer to instead use a custom
structure DetachableVector, which contains a std::vector but allows
manual detaching and freeing of the backing store. This allows us to
maintain the old behavior.

Bug: v8:6333
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I6361d161cdb19878ba19ed51d6ba2fae99e8cdc0
Reviewed-on: https://chromium-review.googlesource.com/660125Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48197}

BUILD.gn

@@ -1547,6 +1547,7 @@ v8_source_set("v8_base") {
     "src/deoptimize-reason.h",
     "src/deoptimizer.cc",
     "src/deoptimizer.h",
+    "src/detachable-vector.h",
     "src/disasm.h",
     "src/disassembler.cc",
     "src/disassembler.h",

src/api.cc

@@ -38,6 +38,7 @@
 #include "src/debug/debug-type-profile.h"
 #include "src/debug/debug.h"
 #include "src/deoptimizer.h"
+#include "src/detachable-vector.h"
 #include "src/execution.h"
 #include "src/frames-inl.h"
 #include "src/gdb-jit.h"
@@ -684,7 +685,8 @@ StartupData SnapshotCreator::CreateBlob(
 
   i::DisallowHeapAllocation no_gc_from_here_on;
 
-  i::List<i::Object*> contexts(num_additional_contexts);
+  std::vector<i::Object*> contexts;
+  contexts.reserve(num_additional_contexts);
   i::Object* default_context;
   {
     i::HandleScope scope(isolate);
@@ -694,7 +696,7 @@ StartupData SnapshotCreator::CreateBlob(
     for (int i = 0; i < num_additional_contexts; i++) {
       i::Handle<i::Context> context =
           v8::Utils::OpenHandle(*data->contexts_.Get(i));
-      contexts.Add(*context);
+      contexts.push_back(*context);
     }
     data->contexts_.Clear();
   }
@@ -10763,7 +10765,6 @@ void Testing::DeoptimizeAll(Isolate* isolate) {
 
 namespace internal {
 
-
 void HandleScopeImplementer::FreeThreadResources() {
   Free();
 }
@@ -10797,7 +10798,7 @@ void HandleScopeImplementer::IterateThis(RootVisitor* v) {
   bool found_block_before_deferred = false;
 #endif
   // Iterate over all handles in the blocks except for the last.
-  for (int i = blocks()->length() - 2; i >= 0; --i) {
+  for (int i = static_cast<int>(blocks()->size()) - 2; i >= 0; --i) {
     Object** block = blocks()->at(i);
     if (last_handle_before_deferred_block_ != NULL &&
         (last_handle_before_deferred_block_ <= &block[kHandleBlockSize]) &&
@@ -10817,17 +10818,18 @@ void HandleScopeImplementer::IterateThis(RootVisitor* v) {
          found_block_before_deferred);
 
   // Iterate over live handles in the last block (if any).
-  if (!blocks()->is_empty()) {
-    v->VisitRootPointers(Root::kHandleScope, blocks()->last(),
+  if (!blocks()->empty()) {
+    v->VisitRootPointers(Root::kHandleScope, blocks()->back(),
                          handle_scope_data_.next);
   }
 
-  List<Context*>* context_lists[2] = { &saved_contexts_, &entered_contexts_};
+  DetachableVector<Context*>* context_lists[2] = {&saved_contexts_,
+                                                  &entered_contexts_};
   for (unsigned i = 0; i < arraysize(context_lists); i++) {
-    if (context_lists[i]->is_empty()) continue;
-    Object** start = reinterpret_cast<Object**>(&context_lists[i]->first());
+    if (context_lists[i]->empty()) continue;
+    Object** start = reinterpret_cast<Object**>(&context_lists[i]->front());
     v->VisitRootPointers(Root::kHandleScope, start,
-                         start + context_lists[i]->length());
+                         start + context_lists[i]->size());
   }
   if (microtask_context_) {
     v->VisitRootPointer(Root::kHandleScope,
@@ -10853,24 +10855,24 @@ DeferredHandles* HandleScopeImplementer::Detach(Object** prev_limit) {
   DeferredHandles* deferred =
       new DeferredHandles(isolate()->handle_scope_data()->next, isolate());
 
-  while (!blocks_.is_empty()) {
-    Object** block_start = blocks_.last();
+  while (!blocks_.empty()) {
+    Object** block_start = blocks_.back();
     Object** block_limit = &block_start[kHandleBlockSize];
     // We should not need to check for SealHandleScope here. Assert this.
     DCHECK(prev_limit == block_limit ||
            !(block_start <= prev_limit && prev_limit <= block_limit));
     if (prev_limit == block_limit) break;
-    deferred->blocks_.Add(blocks_.last());
-    blocks_.RemoveLast();
+    deferred->blocks_.push_back(blocks_.back());
+    blocks_.pop_back();
   }
 
   // deferred->blocks_ now contains the blocks installed on the
   // HandleScope stack since BeginDeferredScope was called, but in
   // reverse order.
 
-  DCHECK(prev_limit == NULL || !blocks_.is_empty());
+  DCHECK(prev_limit == NULL || !blocks_.empty());
 
-  DCHECK(!blocks_.is_empty() && prev_limit != NULL);
+  DCHECK(!blocks_.empty() && prev_limit != NULL);
   DCHECK(last_handle_before_deferred_block_ != NULL);
   last_handle_before_deferred_block_ = NULL;
   return deferred;
@@ -10886,7 +10888,7 @@ void HandleScopeImplementer::BeginDeferredScope() {
 DeferredHandles::~DeferredHandles() {
   isolate_->UnlinkDeferredHandles(this);
 
-  for (int i = 0; i < blocks_.length(); i++) {
+  for (size_t i = 0; i < blocks_.size(); i++) {
 #ifdef ENABLE_HANDLE_ZAPPING
     HandleScope::ZapRange(blocks_[i], &blocks_[i][kHandleBlockSize]);
 #endif
@@ -10895,14 +10897,14 @@ DeferredHandles::~DeferredHandles() {
 }
 
 void DeferredHandles::Iterate(RootVisitor* v) {
-  DCHECK(!blocks_.is_empty());
+  DCHECK(!blocks_.empty());
 
-  DCHECK((first_block_limit_ >= blocks_.first()) &&
-         (first_block_limit_ <= &(blocks_.first())[kHandleBlockSize]));
+  DCHECK((first_block_limit_ >= blocks_.front()) &&
+         (first_block_limit_ <= &(blocks_.front())[kHandleBlockSize]));
 
-  v->VisitRootPointers(Root::kHandleScope, blocks_.first(), first_block_limit_);
+  v->VisitRootPointers(Root::kHandleScope, blocks_.front(), first_block_limit_);
 
-  for (int i = 1; i < blocks_.length(); i++) {
+  for (size_t i = 1; i < blocks_.size(); i++) {
     v->VisitRootPointers(Root::kHandleScope, blocks_[i],
                          &blocks_[i][kHandleBlockSize]);
   }

src/api.h

@@ -8,9 +8,9 @@
 #include "include/v8-testing.h"
 #include "src/contexts.h"
 #include "src/debug/debug-interface.h"
+#include "src/detachable-vector.h"
 #include "src/factory.h"
 #include "src/isolate.h"
-#include "src/list.h"
 
 namespace v8 {
 
@@ -379,7 +379,7 @@ class V8_EXPORT_PRIVATE DeferredHandles {
 
   void Iterate(RootVisitor* v);
 
-  List<Object**> blocks_;
+  std::vector<Object**> blocks_;
   DeferredHandles* next_;
   DeferredHandles* previous_;
   Object** first_block_limit_;
@@ -403,9 +403,6 @@ class HandleScopeImplementer {
  public:
   explicit HandleScopeImplementer(Isolate* isolate)
       : isolate_(isolate),
-        blocks_(0),
-        entered_contexts_(0),
-        saved_contexts_(0),
         microtask_context_(nullptr),
         spare_(NULL),
         call_depth_(0),
@@ -416,7 +413,8 @@ class HandleScopeImplementer {
         debug_microtasks_depth_(0),
 #endif
         microtasks_policy_(v8::MicrotasksPolicy::kAuto),
-        last_handle_before_deferred_block_(NULL) { }
+        last_handle_before_deferred_block_(NULL) {
+  }
 
   ~HandleScopeImplementer() {
     DeleteArray(spare_);
@@ -478,15 +476,14 @@ class HandleScopeImplementer {
   inline Handle<Context> MicrotaskContext();
   inline bool MicrotaskContextIsLastEnteredContext() const {
     return microtask_context_ &&
-           entered_context_count_during_microtasks_ ==
-               entered_contexts_.length();
+           entered_context_count_during_microtasks_ == entered_contexts_.size();
   }
 
   inline void SaveContext(Context* context);
   inline Context* RestoreContext();
   inline bool HasSavedContexts();
 
-  inline List<internal::Object**>* blocks() { return &blocks_; }
+  inline DetachableVector<Object**>* blocks() { return &blocks_; }
   Isolate* isolate() const { return isolate_; }
 
   void ReturnBlock(Object** block) {
@@ -497,9 +494,9 @@ class HandleScopeImplementer {
 
  private:
   void ResetAfterArchive() {
-    blocks_.Initialize(0);
-    entered_contexts_.Initialize(0);
-    saved_contexts_.Initialize(0);
+    blocks_.detach();
+    entered_contexts_.detach();
+    saved_contexts_.detach();
     microtask_context_ = nullptr;
     entered_context_count_during_microtasks_ = 0;
     spare_ = NULL;
@@ -508,13 +505,14 @@ class HandleScopeImplementer {
   }
 
   void Free() {
-    DCHECK(blocks_.length() == 0);
-    DCHECK(entered_contexts_.length() == 0);
-    DCHECK(saved_contexts_.length() == 0);
+    DCHECK(blocks_.empty());
+    DCHECK(entered_contexts_.empty());
+    DCHECK(saved_contexts_.empty());
     DCHECK(!microtask_context_);
-    blocks_.Free();
-    entered_contexts_.Free();
-    saved_contexts_.Free();
+
+    blocks_.free();
+    entered_contexts_.free();
+    saved_contexts_.free();
     if (spare_ != NULL) {
       DeleteArray(spare_);
       spare_ = NULL;
@@ -526,17 +524,17 @@ class HandleScopeImplementer {
   DeferredHandles* Detach(Object** prev_limit);
 
   Isolate* isolate_;
-  List<internal::Object**> blocks_;
+  DetachableVector<Object**> blocks_;
   // Used as a stack to keep track of entered contexts.
-  List<Context*> entered_contexts_;
+  DetachableVector<Context*> entered_contexts_;
   // Used as a stack to keep track of saved contexts.
-  List<Context*> saved_contexts_;
+  DetachableVector<Context*> saved_contexts_;
   Context* microtask_context_;
   Object** spare_;
   int call_depth_;
   int microtasks_depth_;
   int microtasks_suppressions_;
-  int entered_context_count_during_microtasks_;
+  size_t entered_context_count_during_microtasks_;
 #ifdef DEBUG
   int debug_microtasks_depth_;
 #endif
@@ -571,44 +569,42 @@ v8::MicrotasksPolicy HandleScopeImplementer::microtasks_policy() const {
 
 
 void HandleScopeImplementer::SaveContext(Context* context) {
-  saved_contexts_.Add(context);
+  saved_contexts_.push_back(context);
 }
 
 
 Context* HandleScopeImplementer::RestoreContext() {
-  return saved_contexts_.RemoveLast();
+  Context* last_context = saved_contexts_.back();
+  saved_contexts_.pop_back();
+  return last_context;
 }
 
 
 bool HandleScopeImplementer::HasSavedContexts() {
-  return !saved_contexts_.is_empty();
+  return !saved_contexts_.empty();
 }
 
 
 void HandleScopeImplementer::EnterContext(Handle<Context> context) {
-  entered_contexts_.Add(*context);
-}
-
-
-void HandleScopeImplementer::LeaveContext() {
-  entered_contexts_.RemoveLast();
+  entered_contexts_.push_back(*context);
 }
 
+void HandleScopeImplementer::LeaveContext() { entered_contexts_.pop_back(); }
 
 bool HandleScopeImplementer::LastEnteredContextWas(Handle<Context> context) {
-  return !entered_contexts_.is_empty() && entered_contexts_.last() == *context;
+  return !entered_contexts_.empty() && entered_contexts_.back() == *context;
 }
 
 
 Handle<Context> HandleScopeImplementer::LastEnteredContext() {
-  if (entered_contexts_.is_empty()) return Handle<Context>::null();
-  return Handle<Context>(entered_contexts_.last());
+  if (entered_contexts_.empty()) return Handle<Context>::null();
+  return Handle<Context>(entered_contexts_.back());
 }
 
 void HandleScopeImplementer::EnterMicrotaskContext(Handle<Context> context) {
   DCHECK(!microtask_context_);
   microtask_context_ = *context;
-  entered_context_count_during_microtasks_ = entered_contexts_.length();
+  entered_context_count_during_microtasks_ = entered_contexts_.size();
 }
 
 void HandleScopeImplementer::LeaveMicrotaskContext() {
@@ -633,8 +629,8 @@ internal::Object** HandleScopeImplementer::GetSpareOrNewBlock() {
 
 
 void HandleScopeImplementer::DeleteExtensions(internal::Object** prev_limit) {
-  while (!blocks_.is_empty()) {
-    internal::Object** block_start = blocks_.last();
+  while (!blocks_.empty()) {
+    internal::Object** block_start = blocks_.back();
     internal::Object** block_limit = block_start + kHandleBlockSize;
 
     // SealHandleScope may make the prev_limit to point inside the block.
@@ -645,7 +641,7 @@ void HandleScopeImplementer::DeleteExtensions(internal::Object** prev_limit) {
       break;
     }
 
-    blocks_.RemoveLast();
+    blocks_.pop_back();
 #ifdef ENABLE_HANDLE_ZAPPING
     internal::HandleScope::ZapRange(block_start, block_limit);
 #endif
@@ -654,8 +650,8 @@ void HandleScopeImplementer::DeleteExtensions(internal::Object** prev_limit) {
     }
     spare_ = block_start;
   }
-  DCHECK((blocks_.is_empty() && prev_limit == NULL) ||
-         (!blocks_.is_empty() && prev_limit != NULL));
+  DCHECK((blocks_.empty() && prev_limit == NULL) ||
+         (!blocks_.empty() && prev_limit != NULL));
 }
 
 // Interceptor functions called from generated inline caches to notify

src/detachable-vector.h

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_DETACHABLE_VECTOR_H_
+#define V8_DETACHABLE_VECTOR_H_
+
+#include <vector>
+
+namespace v8 {
+namespace internal {
+
+// This class wraps a std::vector and provides a few of the common member
+// functions for accessing the data. It acts as a lazy wrapper of the vector,
+// not initiliazing the backing store until push_back() is first called. Two
+// extra methods are also provided: free() and detach(), which allow for manual
+// control of the backing store. This is currently required for use in the
+// HandleScopeImplementer. Any other class should just use a std::vector
+// directly.
+template <typename T>
+class DetachableVector {
+ public:
+  DetachableVector() : vector_(nullptr) {}
+
+  ~DetachableVector() { delete vector_; }
+
+  void push_back(const T& value) {
+    ensureAttached();
+    vector_->push_back(value);
+  }
+
+  // Free the backing store and clear our reference to it.
+  void free() {
+    delete vector_;
+    vector_ = nullptr;
+  }
+
+  // Clear our reference to the backing store. Does not delete it!
+  void detach() { vector_ = nullptr; }
+
+  T& at(typename std::vector<T>::size_type i) const { return vector_->at(i); }
+
+  T& back() const { return vector_->back(); }
+
+  T& front() const { return vector_->front(); }
+
+  void pop_back() { vector_->pop_back(); }
+
+  typename std::vector<T>::size_type size() const {
+    if (vector_) return vector_->size();
+    return 0;
+  }
+
+  bool empty() const {
+    if (vector_) return vector_->empty();
+    return true;
+  }
+
+ private:
+  std::vector<T>* vector_;
+
+  // Attach a vector backing store if not present.
+  void ensureAttached() {
+    if (vector_ == nullptr) {
+      vector_ = new std::vector<T>();
+    }
+  }
+};
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_DETACHABLE_VECTOR_H_

src/handles.cc

@@ -53,10 +53,11 @@ bool HandleBase::IsDereferenceAllowed(DereferenceCheckMode mode) const {
 
 int HandleScope::NumberOfHandles(Isolate* isolate) {
   HandleScopeImplementer* impl = isolate->handle_scope_implementer();
-  int n = impl->blocks()->length();
+  int n = static_cast<int>(impl->blocks()->size());
   if (n == 0) return 0;
-  return ((n - 1) * kHandleBlockSize) + static_cast<int>(
-      (isolate->handle_scope_data()->next - impl->blocks()->last()));
+  return ((n - 1) * kHandleBlockSize) +
+         static_cast<int>(
+             (isolate->handle_scope_data()->next - impl->blocks()->back()));
 }
 
 
@@ -76,8 +77,8 @@ Object** HandleScope::Extend(Isolate* isolate) {
   HandleScopeImplementer* impl = isolate->handle_scope_implementer();
   // If there's more room in the last block, we use that. This is used
   // for fast creation of scopes after scope barriers.
-  if (!impl->blocks()->is_empty()) {
-    Object** limit = &impl->blocks()->last()[kHandleBlockSize];
+  if (!impl->blocks()->empty()) {
+    Object** limit = &impl->blocks()->back()[kHandleBlockSize];
     if (current->limit != limit) {
       current->limit = limit;
       DCHECK(limit - current->next < kHandleBlockSize);
@@ -91,7 +92,7 @@ Object** HandleScope::Extend(Isolate* isolate) {
     result = impl->GetSpareOrNewBlock();
     // Add the extension to the global list of blocks, but count the
     // extension as part of the current scope.
-    impl->blocks()->Add(result);
+    impl->blocks()->push_back(result);
     current->limit = &result[kHandleBlockSize];
   }
 
@@ -179,10 +180,10 @@ DeferredHandleScope::DeferredHandleScope(Isolate* isolate)
   Object** new_next = impl_->GetSpareOrNewBlock();
   Object** new_limit = &new_next[kHandleBlockSize];
   // Check that at least one HandleScope exists, see the class description.
-  DCHECK(!impl_->blocks()->is_empty());
+  DCHECK(!impl_->blocks()->empty());
   // Check that we are not in a SealedHandleScope.
-  DCHECK(data->limit == &impl_->blocks()->last()[kHandleBlockSize]);
-  impl_->blocks()->Add(new_next);
+  DCHECK(data->limit == &impl_->blocks()->back()[kHandleBlockSize]);
+  impl_->blocks()->push_back(new_next);
 
 #ifdef DEBUG
   prev_level_ = data->level;

src/isolate.cc

@@ -262,8 +262,8 @@ bool Isolate::IsDeferredHandle(Object** handle) {
   for (DeferredHandles* deferred = deferred_handles_head_;
        deferred != NULL;
        deferred = deferred->next_) {
-    List<Object**>* blocks = &deferred->blocks_;
-    for (int i = 0; i < blocks->length(); i++) {
+    std::vector<Object**>* blocks = &deferred->blocks_;
+    for (size_t i = 0; i < blocks->size(); i++) {
       Object** block_limit = (i == 0) ? deferred->first_block_limit_
                                       : blocks->at(i) + kHandleBlockSize;
       if (blocks->at(i) <= handle && handle < block_limit) return true;

src/snapshot/startup-deserializer.cc

@@ -25,7 +25,7 @@ void StartupDeserializer::DeserializeInto(Isolate* isolate) {
   // No active threads.
   DCHECK_NULL(isolate->thread_manager()->FirstThreadStateInUse());
   // No active handles.
-  DCHECK(isolate->handle_scope_implementer()->blocks()->is_empty());
+  DCHECK(isolate->handle_scope_implementer()->blocks()->empty());
   // Partial snapshot cache is not yet populated.
   DCHECK(isolate->partial_snapshot_cache()->empty());
   // Builtins are not yet created.

src/snapshot/startup-serializer.cc

@@ -118,7 +118,7 @@ void StartupSerializer::SerializeStrongReferences() {
   // No active threads.
   CHECK_NULL(isolate->thread_manager()->FirstThreadStateInUse());
   // No active or weak handles.
-  CHECK(isolate->handle_scope_implementer()->blocks()->is_empty());
+  CHECK(isolate->handle_scope_implementer()->blocks()->empty());
   CHECK_EQ(0, isolate->global_handles()->global_handles_count());
   CHECK_EQ(0, isolate->eternal_handles()->NumberOfHandles());
   // First visit immortal immovables to make sure they end up in the first page.

src/v8.gyp

@@ -929,6 +929,7 @@
         'deoptimize-reason.h',
         'deoptimizer.cc',
         'deoptimizer.h',
+        'detachable-vector.h',
         'disasm.h',
         'disassembler.cc',
         'disassembler.h',

test/unittests/BUILD.gn

@@ -104,6 +104,7 @@ v8_executable("unittests") {
     "compiler/value-numbering-reducer-unittest.cc",
     "compiler/zone-stats-unittest.cc",
     "counters-unittest.cc",
+    "detachable-vector-unittest.cc",
     "eh-frame-iterator-unittest.cc",
     "eh-frame-writer-unittest.cc",
     "heap/barrier-unittest.cc",

test/unittests/detachable-vector-unittest.cc

+// Copyright 2017 the V8 project 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 "src/detachable-vector.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace v8 {
+namespace internal {
+
+TEST(DetachableVector, ConstructIsEmpty) {
+  DetachableVector<int> v;
+
+  size_t empty_size = 0;
+  EXPECT_EQ(empty_size, v.size());
+  EXPECT_TRUE(v.empty());
+}
+
+TEST(DetachableVector, PushAddsElement) {
+  DetachableVector<int> v;
+
+  v.push_back(1);
+
+  EXPECT_EQ(1, v.front());
+  EXPECT_EQ(1, v.back());
+  EXPECT_EQ(1, v.at(0));
+  size_t one_size = 1;
+  EXPECT_EQ(one_size, v.size());
+  EXPECT_FALSE(v.empty());
+}
+
+TEST(DetachableVector, AfterFreeIsEmpty) {
+  DetachableVector<int> v;
+
+  v.push_back(1);
+  v.free();
+
+  size_t empty_size = 0;
+  EXPECT_EQ(empty_size, v.size());
+  EXPECT_TRUE(v.empty());
+}
+
+// This test relies on ASAN to detect leaks and double-frees.
+TEST(DetachableVector, DetachLeaksBackingStore) {
+  DetachableVector<int> v;
+  DetachableVector<int> v2;
+
+  size_t one_size = 1;
+  EXPECT_TRUE(v2.empty());
+
+  // Force allocation of the backing store.
+  v.push_back(1);
+  // Bit-copy the data structure.
+  memcpy(&v2, &v, sizeof(DetachableVector<int>));
+  // The backing store should be leaked here - free was not called.
+  v.detach();
+
+  // We have transferred the backing store to the second vector.
+  EXPECT_EQ(one_size, v2.size());
+  EXPECT_TRUE(v.empty());
+
+  // The destructor of v2 will release the backing store.
+}
+
+}  // namespace internal
+}  // namespace v8

test/unittests/unittests.gyp

@@ -101,6 +101,7 @@
       'compiler-dispatcher/optimizing-compile-dispatcher-unittest.cc',
       'compiler-dispatcher/unoptimized-compile-job-unittest.cc',
       'counters-unittest.cc',
+      'detachable-vector-unittest.cc',
       'eh-frame-iterator-unittest.cc',
       'eh-frame-writer-unittest.cc',
       'heap/barrier-unittest.cc',