[api] Accept Extensions via unique_ptr

This way we can remove them correctly and avoid leaks.

R=mstarzinger@chromium.org, ulan@chromium.org

Bug: v8:8725
Change-Id: I52cbbf34a94171aaeb581b55aecb25311465544d
Reviewed-on: https://chromium-review.googlesource.com/c/1446453Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59266}

include/v8.h

@@ -6489,8 +6489,8 @@ class V8_EXPORT Extension {  // NOLINT
   const String::ExternalOneByteStringResource* source() const {
     return source_;
   }
-  int dependency_count() { return dep_count_; }
-  const char** dependencies() { return deps_; }
+  int dependency_count() const { return dep_count_; }
+  const char** dependencies() const { return deps_; }
   void set_auto_enable(bool value) { auto_enable_ = value; }
   bool auto_enable() { return auto_enable_; }
 
@@ -6507,9 +6507,11 @@ class V8_EXPORT Extension {  // NOLINT
   bool auto_enable_;
 };
 
+V8_DEPRECATE_SOON(
+    "Use unique_ptr version or stop using extension (http://crbug.com/334679).",
+    void V8_EXPORT RegisterExtension(Extension* extension));
 
-void V8_EXPORT RegisterExtension(Extension* extension);
-
+void V8_EXPORT RegisterExtension(std::unique_ptr<Extension>);
 
 // --- Statics ---
 

src/api.cc

@@ -895,27 +895,31 @@ void V8::SetFlagsFromCommandLine(int* argc, char** argv, bool remove_flags) {
 RegisteredExtension* RegisteredExtension::first_extension_ = nullptr;
 
 RegisteredExtension::RegisteredExtension(Extension* extension)
-    : extension_(extension) { }
+    : legacy_unowned_extension_(extension) {}
 
+RegisteredExtension::RegisteredExtension(std::unique_ptr<Extension> extension)
+    : extension_(std::move(extension)) {}
 
-void RegisteredExtension::Register(RegisteredExtension* that) {
-  that->next_ = first_extension_;
-  first_extension_ = that;
+// static
+void RegisteredExtension::Register(Extension* extension) {
+  RegisteredExtension* new_extension = new RegisteredExtension(extension);
+  new_extension->next_ = first_extension_;
+  first_extension_ = new_extension;
 }
 
+// static
+void RegisteredExtension::Register(std::unique_ptr<Extension> extension) {
+  RegisteredExtension* new_extension =
+      new RegisteredExtension(std::move(extension));
+  new_extension->next_ = first_extension_;
+  first_extension_ = new_extension;
+}
 
+// static
 void RegisteredExtension::UnregisterAll() {
-  // Keep a list of all leaked Extension objects, to suppress ASan leak reports.
-  // We cannot suppress via lsan suppressions, since the objects are allocated
-  // at different call sites.
-  // TODO(clemensh): Fix this (https://crbug.com/v8/8725).
-  static std::vector<Extension*>* leaked_extensions =
-      new std::vector<Extension*>;
-
   RegisteredExtension* re = first_extension_;
   while (re != nullptr) {
     RegisteredExtension* next = re->next();
-    leaked_extensions->push_back(re->extension_);
     delete re;
     re = next;
   }
@@ -938,11 +942,11 @@ class ExtensionResource : public String::ExternalOneByteStringResource {
 };
 }  // anonymous namespace
 
-void RegisterExtension(Extension* that) {
-  RegisteredExtension* extension = new RegisteredExtension(that);
-  RegisteredExtension::Register(extension);
-}
+void RegisterExtension(Extension* that) { RegisteredExtension::Register(that); }
 
+void RegisterExtension(std::unique_ptr<Extension> extension) {
+  RegisteredExtension::Register(std::move(extension));
+}
 
 Extension::Extension(const char* name,
                      const char* source,

src/api.h

@@ -66,15 +66,22 @@ class ApiFunction {
 
 class RegisteredExtension {
  public:
-  explicit RegisteredExtension(Extension* extension);
-  static void Register(RegisteredExtension* that);
+  static void Register(Extension*);
+  static void Register(std::unique_ptr<Extension>);
   static void UnregisterAll();
-  Extension* extension() { return extension_; }
-  RegisteredExtension* next() { return next_; }
+  Extension* extension() const {
+    return legacy_unowned_extension_ ? legacy_unowned_extension_
+                                     : extension_.get();
+  }
+  RegisteredExtension* next() const { return next_; }
   static RegisteredExtension* first_extension() { return first_extension_; }
  private:
-  Extension* extension_;
-  RegisteredExtension* next_;
+  explicit RegisteredExtension(Extension*);
+  explicit RegisteredExtension(std::unique_ptr<Extension>);
+  // TODO(clemensh): Remove this after the 7.4 branch.
+  Extension* legacy_unowned_extension_ = nullptr;
+  std::unique_ptr<Extension> extension_;
+  RegisteredExtension* next_ = nullptr;
   static RegisteredExtension* first_extension_;
 };
 

src/bootstrapper.cc

@@ -121,42 +121,13 @@ static const char* GCFunctionName() {
   return flag_given ? FLAG_expose_gc_as : "gc";
 }
 
-v8::Extension* Bootstrapper::free_buffer_extension_ = nullptr;
-v8::Extension* Bootstrapper::gc_extension_ = nullptr;
-v8::Extension* Bootstrapper::externalize_string_extension_ = nullptr;
-v8::Extension* Bootstrapper::statistics_extension_ = nullptr;
-v8::Extension* Bootstrapper::trigger_failure_extension_ = nullptr;
-v8::Extension* Bootstrapper::ignition_statistics_extension_ = nullptr;
-
 void Bootstrapper::InitializeOncePerProcess() {
-  free_buffer_extension_ = new FreeBufferExtension;
-  v8::RegisterExtension(free_buffer_extension_);
-  gc_extension_ = new GCExtension(GCFunctionName());
-  v8::RegisterExtension(gc_extension_);
-  externalize_string_extension_ = new ExternalizeStringExtension;
-  v8::RegisterExtension(externalize_string_extension_);
-  statistics_extension_ = new StatisticsExtension;
-  v8::RegisterExtension(statistics_extension_);
-  trigger_failure_extension_ = new TriggerFailureExtension;
-  v8::RegisterExtension(trigger_failure_extension_);
-  ignition_statistics_extension_ = new IgnitionStatisticsExtension;
-  v8::RegisterExtension(ignition_statistics_extension_);
-}
-
-
-void Bootstrapper::TearDownExtensions() {
-  delete free_buffer_extension_;
-  free_buffer_extension_ = nullptr;
-  delete gc_extension_;
-  gc_extension_ = nullptr;
-  delete externalize_string_extension_;
-  externalize_string_extension_ = nullptr;
-  delete statistics_extension_;
-  statistics_extension_ = nullptr;
-  delete trigger_failure_extension_;
-  trigger_failure_extension_ = nullptr;
-  delete ignition_statistics_extension_;
-  ignition_statistics_extension_ = nullptr;
+  v8::RegisterExtension(v8::base::make_unique<FreeBufferExtension>());
+  v8::RegisterExtension(v8::base::make_unique<GCExtension>(GCFunctionName()));
+  v8::RegisterExtension(v8::base::make_unique<ExternalizeStringExtension>());
+  v8::RegisterExtension(v8::base::make_unique<StatisticsExtension>());
+  v8::RegisterExtension(v8::base::make_unique<TriggerFailureExtension>());
+  v8::RegisterExtension(v8::base::make_unique<IgnitionStatisticsExtension>());
 }
 
 void Bootstrapper::TearDown() {

src/bootstrapper.h

@@ -46,7 +46,6 @@ class SourceCodeCache final {
 class Bootstrapper final {
  public:
   static void InitializeOncePerProcess();
-  static void TearDownExtensions();
 
   // Requires: Heap::SetUp has been called.
   void Initialize(bool create_heap_objects);
@@ -109,13 +108,6 @@ class Bootstrapper final {
 
   explicit Bootstrapper(Isolate* isolate);
 
-  static v8::Extension* free_buffer_extension_;
-  static v8::Extension* gc_extension_;
-  static v8::Extension* externalize_string_extension_;
-  static v8::Extension* statistics_extension_;
-  static v8::Extension* trigger_failure_extension_;
-  static v8::Extension* ignition_statistics_extension_;
-
   DISALLOW_COPY_AND_ASSIGN(Bootstrapper);
 };
 

src/v8.cc

@@ -53,7 +53,6 @@ void V8::TearDown() {
   Simulator::GlobalTearDown();
 #endif
   CallDescriptors::TearDown();
-  Bootstrapper::TearDownExtensions();
   ElementsAccessor::TearDown();
   RegisteredExtension::UnregisterAll();
   FlagList::ResetAllFlags();  // Frees memory held by string arguments.

test/cctest/cctest.cc

@@ -315,12 +315,9 @@ int main(int argc, char* argv[]) {
   CcTest::set_array_buffer_allocator(
       v8::ArrayBuffer::Allocator::NewDefaultAllocator());
 
-  i::PrintExtension print_extension;
-  v8::RegisterExtension(&print_extension);
-  i::ProfilerExtension profiler_extension;
-  v8::RegisterExtension(&profiler_extension);
-  i::TraceExtension trace_extension;
-  v8::RegisterExtension(&trace_extension);
+  v8::RegisterExtension(v8::base::make_unique<i::PrintExtension>());
+  v8::RegisterExtension(v8::base::make_unique<i::ProfilerExtension>());
+  v8::RegisterExtension(v8::base::make_unique<i::TraceExtension>());
 
   int tests_run = 0;
   bool print_run_count = true;

test/cctest/test-debug.cc

@@ -2949,8 +2949,8 @@ TEST(NoBreakWhenBootstrapping) {
   {
     // Create a context with an extension to make sure that some JavaScript
     // code is executed during bootstrapping.
-    v8::RegisterExtension(new v8::Extension("simpletest",
-                                            kSimpleExtensionSource));
+    v8::RegisterExtension(v8::base::make_unique<v8::Extension>(
+        "simpletest", kSimpleExtensionSource));
     const char* extension_names[] = { "simpletest" };
     v8::ExtensionConfiguration extensions(1, extension_names);
     v8::HandleScope handle_scope(isolate);

test/cctest/test-lockers.cc

@@ -943,25 +943,12 @@ TEST(ExtensionsRegistration) {
 #else
   const int kNThreads = 40;
 #endif
-  v8::RegisterExtension(new v8::Extension("test0",
-                                          kSimpleExtensionSource));
-  v8::RegisterExtension(new v8::Extension("test1",
-                                          kSimpleExtensionSource));
-  v8::RegisterExtension(new v8::Extension("test2",
-                                          kSimpleExtensionSource));
-  v8::RegisterExtension(new v8::Extension("test3",
-                                          kSimpleExtensionSource));
-  v8::RegisterExtension(new v8::Extension("test4",
-                                          kSimpleExtensionSource));
-  v8::RegisterExtension(new v8::Extension("test5",
-                                          kSimpleExtensionSource));
-  v8::RegisterExtension(new v8::Extension("test6",
-                                          kSimpleExtensionSource));
-  v8::RegisterExtension(new v8::Extension("test7",
-                                          kSimpleExtensionSource));
-  const char* extension_names[] = { "test0", "test1",
-                                    "test2", "test3", "test4",
-                                    "test5", "test6", "test7" };
+  const char* extension_names[] = {"test0", "test1", "test2", "test3",
+                                   "test4", "test5", "test6", "test7"};
+  for (const char* name : extension_names) {
+    v8::RegisterExtension(
+        v8::base::make_unique<v8::Extension>(name, kSimpleExtensionSource));
+  }
   std::vector<JoinableThread*> threads;
   threads.reserve(kNThreads);
   for (int i = 0; i < kNThreads; i++) {

test/cctest/test-serialize.cc

@@ -3554,14 +3554,15 @@ UNINITIALIZED_TEST(SnapshotCreatorIncludeGlobalProxy) {
       v8::Isolate::Scope isolate_scope(isolate);
       // We can introduce new extensions, which could override functions already
       // in the snapshot.
-      v8::Extension* extension = new v8::Extension("new extension",
-                                                   "function i() { return 24; }"
-                                                   "function j() { return 25; }"
-                                                   "try {"
-                                                   "  if (o.p == 7) o.p++;"
-                                                   "} catch {}");
+      auto extension =
+          base::make_unique<v8::Extension>("new extension",
+                                           "function i() { return 24; }"
+                                           "function j() { return 25; }"
+                                           "try {"
+                                           "  if (o.p == 7) o.p++;"
+                                           "} catch {}");
       extension->set_auto_enable(true);
-      v8::RegisterExtension(extension);
+      v8::RegisterExtension(std::move(extension));
       {
         // Create a new context from default context snapshot. This will
         // create a new global object from a new global object template