[inspector][test] Avoid leaks via tasks

Keep tasks in unique_ptrs, such that they are freed independent of
whether they have been executed or not.

R=szuend@chromium.org

Bug: chromium:1142437, v8:11107, v8:11074
Change-Id: Ia265df3187c724b63e0f576d33235c1bfa522c4f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2517694
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71005}

src/utils/locked-queue-inl.h

@@ -38,10 +38,10 @@ inline LockedQueue<Record>::~LockedQueue() {
 }
 
 template <typename Record>
-inline void LockedQueue<Record>::Enqueue(const Record& record) {
+inline void LockedQueue<Record>::Enqueue(Record record) {
   Node* n = new Node();
   CHECK_NOT_NULL(n);
-  n->value = record;
+  n->value = std::move(record);
   {
     base::MutexGuard guard(&tail_mutex_);
     tail_->next.SetValue(n);
@@ -57,7 +57,7 @@ inline bool LockedQueue<Record>::Dequeue(Record* record) {
     old_head = head_;
     Node* const next_node = head_->next.Value();
     if (next_node == nullptr) return false;
-    *record = next_node->value;
+    *record = std::move(next_node->value);
     head_ = next_node;
   }
   delete old_head;

src/utils/locked-queue.h

@@ -21,7 +21,7 @@ class LockedQueue final {
  public:
   inline LockedQueue();
   inline ~LockedQueue();
-  inline void Enqueue(const Record& record);
+  inline void Enqueue(Record record);
   inline bool Dequeue(Record* record);
   inline bool IsEmpty() const;
   inline bool Peek(Record* record) const;

test/fuzzer/inspector-fuzzer.cc

@@ -98,7 +98,7 @@ class UtilsExtension : public IsolateData::SetupGlobalTask {
       return;
     }
 
-    backend_runner_->Append(new ExecuteStringTask(
+    backend_runner_->Append(std::make_unique<ExecuteStringTask>(
         args.GetIsolate(), args[0].As<v8::Int32>()->Value(),
         ToVector(args.GetIsolate(), args[1].As<v8::String>()),
         args[2].As<v8::String>(), args[3].As<v8::Int32>(),
@@ -206,7 +206,7 @@ class UtilsExtension : public IsolateData::SetupGlobalTask {
     if (args.Length() != 2 || !args[0]->IsInt32() || !args[1]->IsString()) {
       return;
     }
-    backend_runner_->Append(new SendMessageToBackendTask(
+    backend_runner_->Append(std::make_unique<SendMessageToBackendTask>(
         args[0].As<v8::Int32>()->Value(),
         ToVector(args.GetIsolate(), args[1].As<v8::String>())));
   }
@@ -518,9 +518,10 @@ class InspectorExtension : public IsolateData::SetupGlobalTask {
         ToVector(isolate, args[1].As<v8::String>());
     v8_inspector::StringView task_name_view(task_name.data(), task_name.size());
 
-    RunAsyncTask(data->task_runner(), task_name_view,
-                 new SetTimeoutTask(context_group_id, isolate,
-                                    v8::Local<v8::Function>::Cast(args[0])));
+    RunAsyncTask(
+        data->task_runner(), task_name_view,
+        std::make_unique<SetTimeoutTask>(
+            context_group_id, isolate, v8::Local<v8::Function>::Cast(args[0])));
     if (with_empty_stack) context->Enter();
   }
 
@@ -593,7 +594,7 @@ void FuzzInspector(const uint8_t* data, size_t size) {
 
   Watchdog watchdog(&ready_semaphore);
 
-  frontend_runner.Append(new ExecuteStringTask(
+  frontend_runner.Append(std::make_unique<ExecuteStringTask>(
       std::string{reinterpret_cast<const char*>(data), size},
       frontend_context_group_id));
 

test/inspector/frontend-channel.h

@@ -31,12 +31,12 @@ class FrontendChannelImpl : public v8_inspector::V8Inspector::Channel {
       int callId,
       std::unique_ptr<v8_inspector::StringBuffer> message) override {
     task_runner_->Append(
-        new SendMessageTask(this, ToVector(message->string())));
+        std::make_unique<SendMessageTask>(this, ToVector(message->string())));
   }
   void sendNotification(
       std::unique_ptr<v8_inspector::StringBuffer> message) override {
     task_runner_->Append(
-        new SendMessageTask(this, ToVector(message->string())));
+        std::make_unique<SendMessageTask>(this, ToVector(message->string())));
   }
   void flushProtocolNotifications() override {}
 

test/inspector/inspector-test.cc

@@ -214,7 +214,7 @@ class UtilsExtension : public IsolateData::SetupGlobalTask {
           "name, line, column, is_module).");
     }
 
-    backend_runner_->Append(new ExecuteStringTask(
+    backend_runner_->Append(std::make_unique<ExecuteStringTask>(
         args.GetIsolate(), args[0].As<v8::Int32>()->Value(),
         ToVector(args.GetIsolate(), args[1].As<v8::String>()),
         args[2].As<v8::String>(), args[3].As<v8::Int32>(),
@@ -388,7 +388,7 @@ class UtilsExtension : public IsolateData::SetupGlobalTask {
     if (args.Length() != 2 || !args[0]->IsInt32() || !args[1]->IsString()) {
       FATAL("Internal error: sendMessageToBackend(session_id, message).");
     }
-    backend_runner_->Append(new SendMessageToBackendTask(
+    backend_runner_->Append(std::make_unique<SendMessageToBackendTask>(
         args[0].As<v8::Int32>()->Value(),
         ToVector(args.GetIsolate(), args[1].As<v8::String>())));
   }
@@ -704,9 +704,10 @@ class InspectorExtension : public IsolateData::SetupGlobalTask {
         ToVector(isolate, args[1].As<v8::String>());
     v8_inspector::StringView task_name_view(task_name.data(), task_name.size());
 
-    RunAsyncTask(data->task_runner(), task_name_view,
-                 new SetTimeoutTask(context_group_id, isolate,
-                                    v8::Local<v8::Function>::Cast(args[0])));
+    RunAsyncTask(
+        data->task_runner(), task_name_view,
+        std::make_unique<SetTimeoutTask>(
+            context_group_id, isolate, v8::Local<v8::Function>::Cast(args[0])));
     if (with_empty_stack) context->Enter();
   }
 
@@ -788,8 +789,8 @@ int InspectorTestMain(int argc, char* argv[]) {
       if (!exists) {
         FATAL("Internal error: script file doesn't exists: %s\n", argv[i]);
       }
-      frontend_runner.Append(
-          new ExecuteStringTask(chars, frontend_context_group_id));
+      frontend_runner.Append(std::make_unique<ExecuteStringTask>(
+          chars, frontend_context_group_id));
     }
 
     frontend_runner.Join();

test/inspector/task-runner.cc

@@ -64,13 +64,12 @@ void TaskRunner::Run() {
 void TaskRunner::RunMessageLoop(bool only_protocol) {
   int loop_number = ++nested_loop_count_;
   while (nested_loop_count_ == loop_number && !is_terminated_) {
-    TaskRunner::Task* task = GetNext(only_protocol);
+    std::unique_ptr<TaskRunner::Task> task = GetNext(only_protocol);
     if (!task) return;
     v8::Isolate::Scope isolate_scope(isolate());
     if (catch_exceptions_) {
       v8::TryCatch try_catch(isolate());
       task->Run(data_.get());
-      delete task;
       if (try_catch.HasCaught()) {
         ReportUncaughtException(isolate(), try_catch);
         fflush(stdout);
@@ -79,8 +78,8 @@ void TaskRunner::RunMessageLoop(bool only_protocol) {
       }
     } else {
       task->Run(data_.get());
-      delete task;
     }
+    task.reset();
     // Also pump isolate's foreground task queue to ensure progress.
     // This can be removed once https://crbug.com/v8/10747 is fixed.
     // TODO(10748): Enable --stress-incremental-marking after the existing
@@ -98,8 +97,8 @@ void TaskRunner::QuitMessageLoop() {
   --nested_loop_count_;
 }
 
-void TaskRunner::Append(Task* task) {
-  queue_.Enqueue(task);
+void TaskRunner::Append(std::unique_ptr<Task> task) {
+  queue_.Enqueue(std::move(task));
   process_queue_semaphore_.Signal();
 }
 
@@ -108,17 +107,17 @@ void TaskRunner::Terminate() {
   process_queue_semaphore_.Signal();
 }
 
-TaskRunner::Task* TaskRunner::GetNext(bool only_protocol) {
+std::unique_ptr<TaskRunner::Task> TaskRunner::GetNext(bool only_protocol) {
   for (;;) {
     if (is_terminated_) return nullptr;
     if (only_protocol) {
-      Task* task = nullptr;
+      std::unique_ptr<Task> task;
       if (queue_.Dequeue(&task)) {
         if (task->is_priority_task()) return task;
-        deffered_queue_.Enqueue(task);
+        deffered_queue_.Enqueue(std::move(task));
       }
     } else {
-      Task* task = nullptr;
+      std::unique_ptr<Task> task;
       if (deffered_queue_.Dequeue(&task)) return task;
       if (queue_.Dequeue(&task)) return task;
     }

test/inspector/task-runner.h

@@ -42,13 +42,12 @@ class TaskRunner : public v8::base::Thread {
   void RunMessageLoop(bool only_protocol);
   void QuitMessageLoop();
 
-  // TaskRunner takes ownership.
-  void Append(Task* task);
+  void Append(std::unique_ptr<Task>);
 
   void Terminate();
 
  private:
-  Task* GetNext(bool only_protocol);
+  std::unique_ptr<Task> GetNext(bool only_protocol);
   v8::Isolate* isolate() const { return data_->isolate(); }
 
   IsolateData::SetupGlobalTasks setup_global_tasks_;
@@ -61,8 +60,8 @@ class TaskRunner : public v8::base::Thread {
   // deferred_queue_ combined with queue_ (in this order) have all tasks in the
   // correct order. Sometimes we skip non-protocol tasks by moving them from
   // queue_ to deferred_queue_.
-  v8::internal::LockedQueue<Task*> queue_;
-  v8::internal::LockedQueue<Task*> deffered_queue_;
+  v8::internal::LockedQueue<std::unique_ptr<Task>> queue_;
+  v8::internal::LockedQueue<std::unique_ptr<Task>> deffered_queue_;
   v8::base::Semaphore process_queue_semaphore_;
 
   int nested_loop_count_;

test/inspector/tasks.h

@@ -37,7 +37,7 @@ void RunSyncTask(TaskRunner* task_runner, T callback) {
   };
 
   v8::base::Semaphore ready_semaphore(0);
-  task_runner->Append(new SyncTask(&ready_semaphore, callback));
+  task_runner->Append(std::make_unique<SyncTask>(&ready_semaphore, callback));
   ready_semaphore.Wait();
 }
 
@@ -59,10 +59,11 @@ class SendMessageToBackendTask : public TaskRunner::Task {
 
 inline void RunAsyncTask(TaskRunner* task_runner,
                          const v8_inspector::StringView& task_name,
-                         TaskRunner::Task* task) {
+                         std::unique_ptr<TaskRunner::Task> task) {
   class AsyncTask : public TaskRunner::Task {
    public:
-    explicit AsyncTask(TaskRunner::Task* inner) : inner_(inner) {}
+    explicit AsyncTask(std::unique_ptr<TaskRunner::Task> inner)
+        : inner_(std::move(inner)) {}
     ~AsyncTask() override = default;
     bool is_priority_task() override { return inner_->is_priority_task(); }
     void Run(IsolateData* data) override {
@@ -76,8 +77,8 @@ inline void RunAsyncTask(TaskRunner* task_runner,
     DISALLOW_COPY_AND_ASSIGN(AsyncTask);
   };
 
-  task_runner->data()->AsyncTaskScheduled(task_name, task, false);
-  task_runner->Append(new AsyncTask(task));
+  task_runner->data()->AsyncTaskScheduled(task_name, task.get(), false);
+  task_runner->Append(std::make_unique<AsyncTask>(std::move(task)));
 }
 
 class ExecuteStringTask : public TaskRunner::Task {
@@ -164,12 +165,13 @@ class SetTimeoutExtension : public IsolateData::SetupGlobalTask {
         reinterpret_cast<const uint8_t*>(task_name), strlen(task_name));
     if (args[0]->IsFunction()) {
       RunAsyncTask(data->task_runner(), task_name_view,
-                   new SetTimeoutTask(context_group_id, isolate,
-                                      v8::Local<v8::Function>::Cast(args[0])));
+                   std::make_unique<SetTimeoutTask>(
+                       context_group_id, isolate,
+                       v8::Local<v8::Function>::Cast(args[0])));
     } else {
       RunAsyncTask(
           data->task_runner(), task_name_view,
-          new ExecuteStringTask(
+          std::make_unique<ExecuteStringTask>(
               isolate, context_group_id,
               ToVector(isolate, args[0].As<v8::String>()),
               v8::String::Empty(isolate), v8::Integer::New(isolate, 0),