commit 7567b3b14e64f5fc8f34b61a3049d333ff7f3f81
author Michael Lippautz <mlippautz@chromium.org> Wed Feb 20 15:40:47 2019
committer Commit Bot <commit-bot@chromium.org> Wed Feb 20 15:40:47 2019
tree fe05537d9f4426530b9f93d202efb335b8564a6e
parent 4fd14d81afa31b6f29d98ed739dc4f3e10e13731

AbortSignal: Model dependent signals as Member instead of root

The root in AbortSignal was creating the following cycle with unified
heap:
  Request -> AbortSignal -> AbortSignal (root) -> ExecutionContext -> Request

Bug: 928781,843903
Change-Id: Iefad41f37fe3ccb3960a7c3d9e1b963730aab7ec
Reviewed-on: https://chromium-review.googlesource.com/c/1477241
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#633714}

third_party/blink/renderer/core/dom/abort_signal.cc

diff --git a/third_party/blink/renderer/core/dom/abort_signal.cc b/third_party/blink/renderer/core/dom/abort_signal.cc
index b1774cba..64cafa08 100644
--- a/third_party/blink/renderer/core/dom/abort_signal.cc
+++ b/third_party/blink/renderer/core/dom/abort_signal.cc
@@ -36,6 +36,24 @@
   abort_algorithms_.push_back(std::move(algorithm));
 }
 
+void AbortSignal::AddSignalAbortAlgorithm(AbortSignal* dependent_signal) {
+  if (aborted_flag_)
+    return;
+
+  // The signal should be kept alive as long as parentSignal is allow chained
+  // requests like the following:
+  // controller -owns-> signal1 -owns-> signal2 -owns-> signal3 <-owns- request
+  //
+  // Due to lack to traced closures we pass a weak persistent but also add
+  // |dependent_signal| as a dependency that is traced. We do not use
+  // WrapPersistent here as this would create a root for Oilpan and unified heap
+  // that leaks the |execution_context_| as there is no explicit event removing
+  // the root anymore.
+  abort_algorithms_.emplace_back(WTF::Bind(
+      &AbortSignal::SignalAbort, WrapWeakPersistent(dependent_signal)));
+  dependent_signals_.push_back(dependent_signal);
+}
+
 void AbortSignal::SignalAbort() {
   if (aborted_flag_)
     return;
@@ -44,6 +62,7 @@
     std::move(closure).Run();
   }
   abort_algorithms_.clear();
+  dependent_signals_.clear();
   DispatchEvent(*Event::Create(event_type_names::kAbort));
 }
 
@@ -53,24 +72,12 @@
   if (parentSignal->aborted_flag_)
     SignalAbort();
 
-  // Unlike the usual practice for AddAlgorithm(), we don't use a weak pointer
-  // here. To see why, consider the following object graph:
-  //
-  // controller --owns--> signal1 -?-> signal2 -?-> signal3 <--owns-- request
-  //
-  // It's easy to create chained signals like this using the Request
-  // constructor. If the -?-> pointers were weak, then |signal2| could be
-  // collected even though |controller| and |request| were still
-  // referenced. This would prevent controller.abort() from working. So the
-  // pointers need to be strong. This won't artificially extend the lifetime of
-  // |request|, because the pointer to it in the closure held by |signal3| is
-  // still weak.
-  parentSignal->AddAlgorithm(
-      WTF::Bind(&AbortSignal::SignalAbort, WrapPersistent(this)));
+  parentSignal->AddSignalAbortAlgorithm(this);
 }
 
 void AbortSignal::Trace(Visitor* visitor) {
   visitor->Trace(execution_context_);
+  visitor->Trace(dependent_signals_);
   EventTargetWithInlineData::Trace(visitor);
 }
 

third_party/blink/renderer/core/dom/abort_signal.h

diff --git a/third_party/blink/renderer/core/dom/abort_signal.h b/third_party/blink/renderer/core/dom/abort_signal.h
index b6abfc3e..96dcea1 100644
--- a/third_party/blink/renderer/core/dom/abort_signal.h
+++ b/third_party/blink/renderer/core/dom/abort_signal.h
@@ -62,8 +62,11 @@
   void Trace(Visitor*) override;
 
  private:
+  void AddSignalAbortAlgorithm(AbortSignal*);
+
   bool aborted_flag_ = false;
   Vector<base::OnceClosure> abort_algorithms_;
+  HeapVector<Member<AbortSignal>> dependent_signals_;
   Member<ExecutionContext> execution_context_;
 };