Add BlinkSchedulerSingleThreadTaskRunner and use in blink

DeleteSoon/ReleaseSoon leak memory if the underlying task queue is
shut down at the time of posting or gets shut down before running the
deleter task. This is problematic in Blink because frame- and
worker-associated task queues might be shut down well before the process
is shut down.

This CL adds BlinkSchedulerSingleThreadTaskRunner to fix some of these
memory leaks by specializing the behavior of DeleteSoon/ReleaseSoon.

There are two main cases to consider:
 1. The deleter task scheduled by DeleteSoon/ReleaseSoon gets posted. In
    this case, the new task runner deletes the object on task
    destruction. This should be thread-safe because we shut down task
    queues on the associated thread and sequence-safe since the targeted
    task runner won't run any other tasks.
 2. The deleter task doesn't get posted. In this case we try a backup
    task runner -- a thread scheduler-associated queue associated with
    the same thread which has a potentially longer (but not shorter)
    lifetime. This is sequence-safe since and thread-safe for the same
    reasons as (1). If this fails, the thread scheduler has already been
    shut down or is shutting down. In this case, we delete the
    object synchronously if it's the same-thread, but leak it
    in the cross-thread case since it might trigger thread checks or
    violate assumptions (e.g. TLS).

    Note: this case was not triggered on the bots, but might happen in
    practice. We can add a DumpWithoutCrashing as a follow-up to see if
    this happens in practice and try to fix those places.

Design doc: https://docs.google.com/document/d/17RyF69EwmDVPCxdQYBWCv_A4aPBdj08BBvG8UsQeKwQ/edit?usp=sharing

Bug: 1376851
Change-Id: Ie2964bcec9dbf8c240312973d0c3c5b8f5e9e908
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4049162
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Francois Pierre Doray <fdoray@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085171}

base/task/sequenced_task_runner.h

@@ -221,6 +221,10 @@ class BASE_EXPORT SequencedTaskRunner : public TaskRunner {
   // Submits a non-nestable task to delete the given object.  Returns
   // true if the object may be deleted at some point in the future,
   // and false if the object definitely will not be deleted.
+  //
+  // By default, this leaks `object` if the deleter task doesn't run, e.g. if
+  // the underlying task queue is shut down first. Subclasses can override this
+  // behavior by specializing `DeleteOrReleaseSoonInternal()`.
   template <class T>
   bool DeleteSoon(const Location& from_here, const T* object) {
     return DeleteOrReleaseSoonInternal(from_here, &DeleteHelper<T>::DoDelete,
@@ -235,6 +239,10 @@ class BASE_EXPORT SequencedTaskRunner : public TaskRunner {
 
   // Submits a non-nestable task to release the given object.
   //
+  // By default, this leaks `object` if the releaser task doesn't run, e.g. if
+  // the underlying task queue is shut down first. Subclasses can override this
+  // behavior by specializing `DeleteOrReleaseSoonInternal()`.
+  //
   // ReleaseSoon makes sure that the object it the scoped_refptr points to gets
   // properly released on the correct thread.
   // We apply ReleaseSoon to the rvalue as the side-effects can be unclear to
@@ -320,10 +328,9 @@ class BASE_EXPORT SequencedTaskRunner : public TaskRunner {
     current_default.task_runner_ = task_runner;
   }
 
- private:
-  bool DeleteOrReleaseSoonInternal(const Location& from_here,
-                                   void (*deleter)(const void*),
-                                   const void* object);
+  virtual bool DeleteOrReleaseSoonInternal(const Location& from_here,
+                                           void (*deleter)(const void*),
+                                           const void* object);
 };
 
 // Sample usage with std::unique_ptr :

third_party/blink/common/features.cc

@@ -1670,5 +1670,10 @@ BASE_FEATURE(kSpeculationRulesPrefetchFuture,
 BASE_FEATURE(kAllowPageWithIDBConnectionInBFCache,
              "AllowPageWithIDBConnectionInBFCache",
              base::FEATURE_DISABLED_BY_DEFAULT);
+
+BASE_FEATURE(kUseBlinkSchedulerTaskRunnerWithCustomDeleter,
+             "UseBlinkSchedulerTaskRunnerWithCustomDeleter",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 }  // namespace features
 }  // namespace blink

third_party/blink/public/common/features.h

@@ -962,6 +962,12 @@ BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kSpeculationRulesPrefetchFuture);
 // Feature for allowing page with open IDB connection to be stored in
 // back/forward cache.
 BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kAllowPageWithIDBConnectionInBFCache);
+
+// Kill switch for using a custom task runner in the blink scheduler that makes
+// DeleteSoon/ReleaseSoon less prone to memory leaks.
+BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(
+    kUseBlinkSchedulerTaskRunnerWithCustomDeleter);
+
 }  // namespace features
 }  // namespace blink
 

third_party/blink/renderer/platform/scheduler/BUILD.gn

@@ -16,6 +16,8 @@ blink_platform_sources("scheduler") {
     "common/auto_advancing_virtual_time_domain.h",
     "common/back_forward_cache_disabling_feature_tracker.cc",
     "common/back_forward_cache_disabling_feature_tracker.h",
+    "common/blink_scheduler_single_thread_task_runner.cc",
+    "common/blink_scheduler_single_thread_task_runner.h",
     "common/cancelable_closure_holder.cc",
     "common/cancelable_closure_holder.h",
     "common/cooperative_scheduling_manager.cc",
@@ -233,6 +235,7 @@ source_set("unit_tests") {
   sources = [
     "common/auto_advancing_virtual_time_domain_unittest.cc",
     "common/back_forward_cache_disabling_feature_tracker_unittest.cc",
+    "common/blink_scheduler_single_thread_task_runner_unittest.cc",
     "common/cooperative_scheduling_manager_unittest.cc",
     "common/idle_helper_unittest.cc",
     "common/metrics_helper_unittest.cc",

third_party/blink/renderer/platform/scheduler/common/DEPS

@@ -5,6 +5,12 @@ specific_include_rules = {
   "back_forward_cache_disabling_feature_tracker_unittest.cc": [
     "+third_party/blink/renderer/platform/bindings/source_location.h",
   ],
+  "blink_scheduler_single_thread_task_runner.cc": [
+    "+base/functional/callback_helpers.h",
+  ],
+  "blink_scheduler_single_thread_task_runner.h": [
+    "+base/functional/callback_forward.h",
+  ],
   "frame_or_worker_scheduler.cc": [
     "+third_party/blink/renderer/platform/bindings/source_location.h",
   ],

third_party/blink/renderer/platform/scheduler/common/blink_scheduler_single_thread_task_runner.cc

@@ -0,0 +1,122 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/platform/scheduler/common/blink_scheduler_single_thread_task_runner.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/location.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/task/single_thread_task_runner.h"
+
+namespace blink::scheduler {
+
+namespace {
+
+void DeleteOrReleaseSoonImpl(
+    const base::Location& from_here,
+    void (*deleter)(const void*),
+    const void* object,
+    scoped_refptr<base::SingleThreadTaskRunner> preferred_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> fallback_task_runner);
+
+class DeleteHelper {
+ public:
+  DeleteHelper(
+      const base::Location& from_here,
+      void (*deleter)(const void*),
+      const void* object,
+      scoped_refptr<base::SingleThreadTaskRunner> preferred_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> fallback_task_runner)
+      : from_here_(from_here),
+        deleter_(deleter),
+        object_(object),
+        preferred_task_runner_(std::move(preferred_task_runner)),
+        fallback_task_runner_(std::move(fallback_task_runner)) {}
+
+  void Delete() {
+    deleter_(object_);
+    object_ = nullptr;
+  }
+
+  ~DeleteHelper() {
+    if (!object_) {
+      return;
+    }
+
+    // The deleter task is being destroyed without running, which happens if the
+    // task queue is shut down after queuing the task queued or if posting it
+    // failed. It's safe to run the deleter in the former case, but since these
+    // cases can't be differentiated without synchronization or API changes, use
+    // the `fallback_task_runner_` if present and delete synchronously if not.
+    if (fallback_task_runner_) {
+      DeleteOrReleaseSoonImpl(from_here_, deleter_, object_,
+                              fallback_task_runner_, nullptr);
+    } else if (preferred_task_runner_->BelongsToCurrentThread()) {
+      // Note: `deleter_` will run synchronously in [Delete|Release]Soon() if
+      // the deleter task failed to post to the original preferred and fallback
+      // task runners. This happens when the APIs are called during thread
+      // shutdown, and should only occur if invoking those APIs in object
+      // destructors (on task destruction), where it should be safe to
+      // synchronously delete.
+      Delete();
+    } else {
+      // The deleter task couldn't be posted to the intended thread, so the only
+      // safe thing to do is leak the object.
+      // TODO(crbug.com/1376851): Add a CHECK, DumpWithoutCrashing, or trace
+      // event to determine if leaks still occur.
+    }
+  }
+
+ private:
+  base::Location from_here_;
+  void (*deleter_)(const void*) = nullptr;
+  const void* object_ = nullptr;
+  scoped_refptr<base::SingleThreadTaskRunner> preferred_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> fallback_task_runner_;
+};
+
+void DeleteOrReleaseSoonImpl(
+    const base::Location& from_here,
+    void (*deleter)(const void*),
+    const void* object,
+    scoped_refptr<base::SingleThreadTaskRunner> preferred_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> fallback_task_runner) {
+  auto delete_helper = std::make_unique<DeleteHelper>(
+      from_here, deleter, object, preferred_task_runner, fallback_task_runner);
+  preferred_task_runner->PostNonNestableTask(
+      from_here,
+      base::BindOnce(&DeleteHelper::Delete, std::move(delete_helper)));
+}
+
+}  // namespace
+
+BlinkSchedulerSingleThreadTaskRunner::BlinkSchedulerSingleThreadTaskRunner(
+    scoped_refptr<base::SingleThreadTaskRunner> wrapped_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> thread_task_runner)
+    : outer_(std::move(wrapped_task_runner)),
+      thread_task_runner_(std::move(thread_task_runner)) {
+  DCHECK(outer_);
+}
+
+BlinkSchedulerSingleThreadTaskRunner::~BlinkSchedulerSingleThreadTaskRunner() =
+    default;
+
+bool BlinkSchedulerSingleThreadTaskRunner::DeleteOrReleaseSoonInternal(
+    const base::Location& from_here,
+    void (*deleter)(const void*),
+    const void* object) {
+  DCHECK(deleter);
+  // `object` might be null, in which case there's nothing to do.
+  if (!object) {
+    return true;
+  }
+
+  DeleteOrReleaseSoonImpl(from_here, deleter, object, outer_,
+                          thread_task_runner_);
+  return true;
+}
+
+}  // namespace blink::scheduler

third_party/blink/renderer/platform/scheduler/common/blink_scheduler_single_thread_task_runner.h

@@ -0,0 +1,112 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_SCHEDULER_COMMON_BLINK_SCHEDULER_SINGLE_THREAD_TASK_RUNNER_H_
+#define THIRD_PARTY_BLINK_RENDERER_PLATFORM_SCHEDULER_COMMON_BLINK_SCHEDULER_SINGLE_THREAD_TASK_RUNNER_H_
+
+#include "base/functional/callback_forward.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/task/delayed_task_handle.h"
+#include "base/task/single_thread_task_runner.h"
+#include "base/time/time.h"
+#include "third_party/blink/renderer/platform/platform_export.h"
+
+namespace base {
+class Location;
+}  // namespace base
+
+namespace blink::scheduler {
+
+// TaskRunner used in the scheduler.
+//
+// This class specializes the DeleteSoon/ReleaseSoon implementation to prevent
+// the object from leaking when possible (see DeleteOrReleaseSoonInternal). This
+// is needed in Blink since frame and worker schedulers can get torn down long
+// before the process shuts down.
+//
+// All other task-posting functionality is forwarded to to an underlying task
+// runner.
+class PLATFORM_EXPORT BlinkSchedulerSingleThreadTaskRunner
+    : public base::SingleThreadTaskRunner {
+ public:
+  // `wrapped_task_runner` is the task runner used for scheduling tasks.
+  // `thread_task_runner` is used to schedule deleter tasks if
+  // `wrapped_task_runner`'s task queue is already shut down.
+  BlinkSchedulerSingleThreadTaskRunner(
+      scoped_refptr<base::SingleThreadTaskRunner> wrapped_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> thread_task_runner);
+  ~BlinkSchedulerSingleThreadTaskRunner() override;
+
+  BlinkSchedulerSingleThreadTaskRunner(BlinkSchedulerSingleThreadTaskRunner&&) =
+      delete;
+  BlinkSchedulerSingleThreadTaskRunner& operator=(
+      BlinkSchedulerSingleThreadTaskRunner&&) = delete;
+
+  // base::TaskRunner overrides:
+  bool PostDelayedTask(const base::Location& from_here,
+                       base::OnceClosure task,
+                       base::TimeDelta delay) override {
+    return outer_->PostDelayedTask(from_here, std::move(task), delay);
+  }
+
+  // base::SequencedTaskRunner overrides:
+  bool PostNonNestableDelayedTask(const base::Location& from_here,
+                                  base::OnceClosure task,
+                                  base::TimeDelta delay) override {
+    return outer_->PostNonNestableDelayedTask(from_here, std::move(task),
+                                              delay);
+  }
+
+  base::DelayedTaskHandle PostCancelableDelayedTask(
+      base::subtle::PostDelayedTaskPassKey pass_key,
+      const base::Location& from_here,
+      base::OnceClosure task,
+      base::TimeDelta delay) override {
+    return outer_->PostCancelableDelayedTask(pass_key, from_here,
+                                             std::move(task), delay);
+  }
+
+  [[nodiscard]] base::DelayedTaskHandle PostCancelableDelayedTaskAt(
+      base::subtle::PostDelayedTaskPassKey pass_key,
+      const base::Location& from_here,
+      base::OnceClosure task,
+      base::TimeTicks delayed_run_time,
+      base::subtle::DelayPolicy delay_policy) override {
+    return outer_->PostCancelableDelayedTaskAt(
+        pass_key, from_here, std::move(task), delayed_run_time, delay_policy);
+  }
+
+  bool PostDelayedTaskAt(base::subtle::PostDelayedTaskPassKey pass_key,
+                         const base::Location& from_here,
+                         base::OnceClosure task,
+                         base::TimeTicks delayed_run_time,
+                         base::subtle::DelayPolicy delay_policy) override {
+    return outer_->PostDelayedTaskAt(pass_key, from_here, std::move(task),
+                                     delayed_run_time, delay_policy);
+  }
+
+  bool RunsTasksInCurrentSequence() const override {
+    return outer_->RunsTasksInCurrentSequence();
+  }
+
+ protected:
+  // This always returns true, even if `object` gets leaked because the deleter
+  // task was not posted.
+  // TODO(crbug.com/1376851): Determine if leaking still occurs and whether to
+  // CHECK or handle at callsites.
+  bool DeleteOrReleaseSoonInternal(const base::Location& from_here,
+                                   void (*deleter)(const void*),
+                                   const void* object) override;
+
+ private:
+  // The task runner this object forwards all non-delete/release tasks to.
+  scoped_refptr<base::SingleThreadTaskRunner> outer_;
+  // Backup task runner used for delete/release tasks if `outer_`'s task queue
+  // is already shut down when `DeleteOrReleaseSoonInternal()` is called.
+  scoped_refptr<base::SingleThreadTaskRunner> thread_task_runner_;
+};
+
+}  // namespace blink::scheduler
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_SCHEDULER_COMMON_BLINK_SCHEDULER_SINGLE_THREAD_TASK_RUNNER_H_