[test] Create one Isolate per unit test (not test suite)

Change the unittests Isolate mixin to create one Isolate per test,
rather than one per test suite. We usually run these tests independently
in separate processes anyway, so this shouldn't affect normal test
execution, but it will avoid Isolate state leaking across tests when
running the unittests binary directly.

Take this opportunity to also clean up the mixins, changing counter
initialization and forcing pointer compression into template traits.

Bug: v8:10142
Change-Id: If92046f9c6f2056252d099faed04d97844ef7319
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143818Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67110}

test/unittests/execution/microtask-queue-unittest.cc

@@ -65,7 +65,7 @@ using TestWithNativeContextAndFinalizationRegistry =  //
         WithContextMixin<                             //
             WithFinalizationRegistryMixin<            //
                 WithIsolateScopeMixin<                //
-                    WithSharedIsolateMixin<           //
+                    WithIsolateMixin<                 //
                         ::testing::Test>>>>>;
 
 namespace {

test/unittests/heap/spaces-unittest.cc

@@ -15,15 +15,7 @@
 namespace v8 {
 namespace internal {
 
-class SpacesTest : public TestWithIsolate {
- protected:
-  void TearDown() final {
-    // Clean up the heap between tests.
-    i_isolate()->heap()->CollectAllAvailableGarbage(
-        GarbageCollectionReason::kTesting);
-    TestWithIsolate::TearDown();
-  }
-};
+using SpacesTest = TestWithIsolate;
 
 TEST_F(SpacesTest, CompactionSpaceMerge) {
   Heap* heap = i_isolate()->heap();

test/unittests/test-utils.cc

@@ -15,50 +15,61 @@
 
 namespace v8 {
 
-IsolateWrapper::IsolateWrapper(CounterLookupCallback counter_lookup_callback,
-                               bool enforce_pointer_compression)
+namespace {
+// counter_lookup_callback doesn't pass through any state information about
+// the current Isolate, so we have to store the current counter map somewhere.
+// Fortunately tests run serially, so we can just store it in a static global.
+CounterMap* kCurrentCounterMap = nullptr;
+}  // namespace
+
+IsolateWrapper::IsolateWrapper(CountersMode counters_mode,
+                               PointerCompressionMode pointer_compression_mode)
     : array_buffer_allocator_(
           v8::ArrayBuffer::Allocator::NewDefaultAllocator()) {
+  CHECK_NULL(kCurrentCounterMap);
+
   v8::Isolate::CreateParams create_params;
-  create_params.array_buffer_allocator = array_buffer_allocator_;
-  create_params.counter_lookup_callback = counter_lookup_callback;
-  if (enforce_pointer_compression) {
+  create_params.array_buffer_allocator = array_buffer_allocator_.get();
+
+  if (counters_mode == kEnableCounters) {
+    counter_map_ = std::make_unique<CounterMap>();
+    kCurrentCounterMap = counter_map_.get();
+
+    create_params.counter_lookup_callback = [](const char* name) {
+      CHECK_NOT_NULL(kCurrentCounterMap);
+      // If the name doesn't exist in the counter map, operator[] will default
+      // initialize it to zero.
+      return &(*kCurrentCounterMap)[name];
+    };
+  } else {
+    create_params.counter_lookup_callback = [](const char* name) -> int* {
+      return nullptr;
+    };
+  }
+
+  if (pointer_compression_mode == kEnforcePointerCompression) {
     isolate_ = reinterpret_cast<v8::Isolate*>(
         i::Isolate::New(i::IsolateAllocationMode::kInV8Heap));
-    v8::Isolate::Initialize(isolate_, create_params);
+    v8::Isolate::Initialize(isolate(), create_params);
   } else {
     isolate_ = v8::Isolate::New(create_params);
   }
-  CHECK_NOT_NULL(isolate_);
+  CHECK_NOT_NULL(isolate());
 }
 
 IsolateWrapper::~IsolateWrapper() {
   v8::Platform* platform = internal::V8::GetCurrentPlatform();
   CHECK_NOT_NULL(platform);
-  while (platform::PumpMessageLoop(platform, isolate_)) continue;
+  while (platform::PumpMessageLoop(platform, isolate())) continue;
   isolate_->Dispose();
-  delete array_buffer_allocator_;
-}
-
-// static
-v8::IsolateWrapper* SharedIsolateHolder::isolate_wrapper_ = nullptr;
-
-// static
-int* SharedIsolateAndCountersHolder::LookupCounter(const char* name) {
-  DCHECK_NOT_NULL(counter_map_);
-  auto map_entry = counter_map_->find(name);
-  if (map_entry == counter_map_->end()) {
-    counter_map_->emplace(name, 0);
+  if (counter_map_) {
+    CHECK_EQ(kCurrentCounterMap, counter_map_.get());
+    kCurrentCounterMap = nullptr;
+  } else {
+    CHECK_NULL(kCurrentCounterMap);
   }
-  return &counter_map_->at(name);
 }
 
-// static
-v8::IsolateWrapper* SharedIsolateAndCountersHolder::isolate_wrapper_ = nullptr;
-
-// static
-CounterMap* SharedIsolateAndCountersHolder::counter_map_ = nullptr;
-
 namespace internal {
 
 SaveFlags::SaveFlags() {

test/unittests/test-utils.h

@@ -5,6 +5,7 @@
 #ifndef V8_UNITTESTS_TEST_UTILS_H_
 #define V8_UNITTESTS_TEST_UTILS_H_
 
+#include <memory>
 #include <vector>
 
 #include "include/v8.h"
@@ -24,144 +25,67 @@ class ArrayBufferAllocator;
 
 using CounterMap = std::map<std::string, int>;
 
+enum CountersMode { kNoCounters, kEnableCounters };
+
+// When PointerCompressionMode is kEnforcePointerCompression, the Isolate is
+// created with pointer compression force enabled. When it's
+// kDefaultPointerCompression then the Isolate is created with the default
+// pointer compression state for the current build.
+enum PointerCompressionMode {
+  kDefaultPointerCompression,
+  kEnforcePointerCompression
+};
+
 // RAII-like Isolate instance wrapper.
 class IsolateWrapper final {
  public:
-  // When enforce_pointer_compression is true the Isolate is created with
-  // enabled pointer compression. When it's false then the Isolate is created
-  // with the default pointer compression state for current build.
-  explicit IsolateWrapper(CounterLookupCallback counter_lookup_callback,
-                          bool enforce_pointer_compression = false);
+  explicit IsolateWrapper(CountersMode counters_mode,
+                          PointerCompressionMode pointer_compression_mode);
   ~IsolateWrapper();
 
   v8::Isolate* isolate() const { return isolate_; }
 
  private:
-  v8::ArrayBuffer::Allocator* array_buffer_allocator_;
+  std::unique_ptr<v8::ArrayBuffer::Allocator> array_buffer_allocator_;
+  std::unique_ptr<CounterMap> counter_map_;
   v8::Isolate* isolate_;
 
   DISALLOW_COPY_AND_ASSIGN(IsolateWrapper);
 };
 
-class SharedIsolateHolder final {
- public:
-  static v8::Isolate* isolate() { return isolate_wrapper_->isolate(); }
-
-  static void CreateIsolate() {
-    CHECK_NULL(isolate_wrapper_);
-    isolate_wrapper_ =
-        new IsolateWrapper([](const char* name) -> int* { return nullptr; });
-  }
-
-  static void DeleteIsolate() {
-    CHECK_NOT_NULL(isolate_wrapper_);
-    delete isolate_wrapper_;
-    isolate_wrapper_ = nullptr;
-  }
-
- private:
-  static v8::IsolateWrapper* isolate_wrapper_;
-
-  DISALLOW_IMPLICIT_CONSTRUCTORS(SharedIsolateHolder);
-};
-
-class SharedIsolateAndCountersHolder final {
- public:
-  static v8::Isolate* isolate() { return isolate_wrapper_->isolate(); }
-
-  static void CreateIsolate() {
-    CHECK_NULL(counter_map_);
-    CHECK_NULL(isolate_wrapper_);
-    counter_map_ = new CounterMap();
-    isolate_wrapper_ = new IsolateWrapper(LookupCounter);
-  }
-
-  static void DeleteIsolate() {
-    CHECK_NOT_NULL(counter_map_);
-    CHECK_NOT_NULL(isolate_wrapper_);
-    delete isolate_wrapper_;
-    isolate_wrapper_ = nullptr;
-    delete counter_map_;
-    counter_map_ = nullptr;
-  }
-
- private:
-  static int* LookupCounter(const char* name);
-  static CounterMap* counter_map_;
-  static v8::IsolateWrapper* isolate_wrapper_;
-
-  DISALLOW_IMPLICIT_CONSTRUCTORS(SharedIsolateAndCountersHolder);
-};
-
 //
 // A set of mixins from which the test fixtures will be constructed.
 //
-template <typename TMixin>
-class WithPrivateIsolateMixin : public TMixin {
+template <typename TMixin, CountersMode kCountersMode = kNoCounters,
+          PointerCompressionMode kPointerCompressionMode =
+              kDefaultPointerCompression>
+class WithIsolateMixin : public TMixin {
  public:
-  explicit WithPrivateIsolateMixin(bool enforce_pointer_compression = false)
-      : isolate_wrapper_([](const char* name) -> int* { return nullptr; },
-                         enforce_pointer_compression) {}
+  WithIsolateMixin()
+      : isolate_wrapper_(kCountersMode, kPointerCompressionMode) {}
 
   v8::Isolate* v8_isolate() const { return isolate_wrapper_.isolate(); }
 
-  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
-  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
-
  private:
   v8::IsolateWrapper isolate_wrapper_;
-
-  DISALLOW_COPY_AND_ASSIGN(WithPrivateIsolateMixin);
 };
 
-template <typename TMixin, typename TSharedIsolateHolder = SharedIsolateHolder>
-class WithSharedIsolateMixin : public TMixin {
- public:
-  WithSharedIsolateMixin() = default;
-
-  v8::Isolate* v8_isolate() const { return TSharedIsolateHolder::isolate(); }
-
-  static void SetUpTestCase() {
-    TMixin::SetUpTestCase();
-    TSharedIsolateHolder::CreateIsolate();
-  }
-
-  static void TearDownTestCase() {
-    TSharedIsolateHolder::DeleteIsolate();
-    TMixin::TearDownTestCase();
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(WithSharedIsolateMixin);
-};
-
-template <typename TMixin>
-class WithPointerCompressionIsolateMixin
-    : public WithPrivateIsolateMixin<TMixin> {
- public:
-  WithPointerCompressionIsolateMixin()
-      : WithPrivateIsolateMixin<TMixin>(true) {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(WithPointerCompressionIsolateMixin);
-};
+template <typename TMixin, CountersMode kCountersMode = kNoCounters>
+using WithPointerCompressionIsolateMixin =
+    WithIsolateMixin<TMixin, kCountersMode, kEnforcePointerCompression>;
 
 template <typename TMixin>
 class WithIsolateScopeMixin : public TMixin {
  public:
   WithIsolateScopeMixin()
-      : isolate_scope_(v8_isolate()), handle_scope_(v8_isolate()) {}
+      : isolate_scope_(this->v8_isolate()), handle_scope_(this->v8_isolate()) {}
 
-  v8::Isolate* isolate() const { return v8_isolate(); }
-  v8::Isolate* v8_isolate() const { return TMixin::v8_isolate(); }
+  v8::Isolate* isolate() const { return this->v8_isolate(); }
 
   v8::internal::Isolate* i_isolate() const {
-    return reinterpret_cast<v8::internal::Isolate*>(v8_isolate());
+    return reinterpret_cast<v8::internal::Isolate*>(this->v8_isolate());
   }
 
-  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
-  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
-
  private:
   v8::Isolate::Scope isolate_scope_;
   v8::HandleScope handle_scope_;
@@ -173,25 +97,23 @@ template <typename TMixin>
 class WithContextMixin : public TMixin {
  public:
   WithContextMixin()
-      : context_(Context::New(v8_isolate())), context_scope_(context_) {}
-
-  v8::Isolate* v8_isolate() const { return TMixin::v8_isolate(); }
+      : context_(Context::New(this->v8_isolate())), context_scope_(context_) {}
 
   const Local<Context>& context() const { return v8_context(); }
   const Local<Context>& v8_context() const { return context_; }
 
   Local<Value> RunJS(const char* source) {
     return RunJS(
-        v8::String::NewFromUtf8(v8_isolate(), source).ToLocalChecked());
+        v8::String::NewFromUtf8(this->v8_isolate(), source).ToLocalChecked());
   }
 
   Local<Value> RunJS(v8::String::ExternalOneByteStringResource* source) {
-    return RunJS(
-        v8::String::NewExternalOneByte(v8_isolate(), source).ToLocalChecked());
+    return RunJS(v8::String::NewExternalOneByte(this->v8_isolate(), source)
+                     .ToLocalChecked());
   }
 
   v8::Local<v8::String> NewString(const char* string) {
-    return v8::String::NewFromUtf8(v8_isolate(), string).ToLocalChecked();
+    return v8::String::NewFromUtf8(this->v8_isolate(), string).ToLocalChecked();
   }
 
   void SetGlobalProperty(const char* name, v8::Local<v8::Value> value) {
@@ -201,12 +123,9 @@ class WithContextMixin : public TMixin {
               .FromJust());
   }
 
-  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
-  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
-
  private:
   Local<Value> RunJS(Local<String> source) {
-    auto context = v8_isolate()->GetCurrentContext();
+    auto context = this->v8_isolate()->GetCurrentContext();
     Local<Script> script =
         v8::Script::Compile(context, source).ToLocalChecked();
     return script->Run(context).ToLocalChecked();
@@ -220,17 +139,17 @@ class WithContextMixin : public TMixin {
 
 // Use v8::internal::TestWithIsolate if you are testing internals,
 // aka. directly work with Handles.
-using TestWithIsolate =          //
-    WithIsolateScopeMixin<       //
-        WithSharedIsolateMixin<  //
+using TestWithIsolate =     //
+    WithIsolateScopeMixin<  //
+        WithIsolateMixin<   //
             ::testing::Test>>;
 
 // Use v8::internal::TestWithNativeContext if you are testing internals,
 // aka. directly work with Handles.
-using TestWithContext =              //
-    WithContextMixin<                //
-        WithIsolateScopeMixin<       //
-            WithSharedIsolateMixin<  //
+using TestWithContext =         //
+    WithContextMixin<           //
+        WithIsolateScopeMixin<  //
+            WithIsolateMixin<   //
                 ::testing::Test>>>;
 
 using TestWithIsolateAndPointerCompression =     //
@@ -279,9 +198,6 @@ class WithInternalIsolateMixin : public TMixin {
     return isolate()->random_number_generator();
   }
 
-  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
-  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
-
  private:
   DISALLOW_COPY_AND_ASSIGN(WithInternalIsolateMixin);
 };
@@ -293,9 +209,6 @@ class WithZoneMixin : public TMixin {
 
   Zone* zone() { return &zone_; }
 
-  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
-  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
-
  private:
   v8::internal::AccountingAllocator allocator_;
   Zone zone_;
@@ -303,42 +216,41 @@ class WithZoneMixin : public TMixin {
   DISALLOW_COPY_AND_ASSIGN(WithZoneMixin);
 };
 
-using TestWithIsolate =              //
-    WithInternalIsolateMixin<        //
-        WithIsolateScopeMixin<       //
-            WithSharedIsolateMixin<  //
+using TestWithIsolate =         //
+    WithInternalIsolateMixin<   //
+        WithIsolateScopeMixin<  //
+            WithIsolateMixin<   //
                 ::testing::Test>>>;
 
 using TestWithZone = WithZoneMixin<::testing::Test>;
 
-using TestWithIsolateAndZone =       //
-    WithInternalIsolateMixin<        //
-        WithIsolateScopeMixin<       //
-            WithSharedIsolateMixin<  //
-                WithZoneMixin<       //
+using TestWithIsolateAndZone =  //
+    WithInternalIsolateMixin<   //
+        WithIsolateScopeMixin<  //
+            WithIsolateMixin<   //
+                WithZoneMixin<  //
                     ::testing::Test>>>>;
 
-using TestWithNativeContext =            //
-    WithInternalIsolateMixin<            //
-        WithContextMixin<                //
-            WithIsolateScopeMixin<       //
-                WithSharedIsolateMixin<  //
+using TestWithNativeContext =       //
+    WithInternalIsolateMixin<       //
+        WithContextMixin<           //
+            WithIsolateScopeMixin<  //
+                WithIsolateMixin<   //
                     ::testing::Test>>>>;
 
 using TestWithNativeContextAndCounters =  //
     WithInternalIsolateMixin<             //
         WithContextMixin<                 //
             WithIsolateScopeMixin<        //
-                WithSharedIsolateMixin<   //
-                    ::testing::Test,      //
-                    SharedIsolateAndCountersHolder>>>>;
-
-using TestWithNativeContextAndZone =         //
-    WithZoneMixin<                           //
-        WithInternalIsolateMixin<            //
-            WithContextMixin<                //
-                WithIsolateScopeMixin<       //
-                    WithSharedIsolateMixin<  //
+                WithIsolateMixin<         //
+                    ::testing::Test, kEnableCounters>>>>;
+
+using TestWithNativeContextAndZone =    //
+    WithZoneMixin<                      //
+        WithInternalIsolateMixin<       //
+            WithContextMixin<           //
+                WithIsolateScopeMixin<  //
+                    WithIsolateMixin<   //
                         ::testing::Test>>>>>;
 
 class SaveFlags {