Skip to content

Commit d5b7539

Browse files
shaseleyChromium LUCI CQ
shaseley
authored and
Chromium LUCI CQ
committed
Reland: "[dom] Implement AbortSignal.any() memory management"
The reland fixes a leak in one of the tests caused by the test ending before an AbortSignal.timeout() fired. This is the second part of the AbortSignal.any() prototype; it implements the memory management described in [1]. The idea is as follows: - A signal is "settled" if it can no longer emit events associated with the composition type (abort or priority). For abort, this means either the signal is aborted or it never will be; for priority, this means the priority can no longer change. A signal becomes settled if it aborts (for abort composition), if the signal's associated controller has been GCed (implemented with a prefinalizer), or if it's a composite signal and all of its parents have been settled. - Unsettled composite signals are kept alive as long as their effects (active event listeners, abort/priority algorithms) can be observed. This is implemented by making AbortSignal inherit from LazyActiveScriptWrappable (only enabled for composite signals) and determining activity based on settled state + active listeners and algorithms. This allows AbortSignalCompositionManager to hold weak references to all signals. This also adds a number of wpt_internal/ tests using a finalization registry to test signal lifetimes. The tests are mostly parameterized with an interface type so we can run these for TaskSignal as well (follow-up). [1] https://docs.google.com/document/d/1LvmsBLV85p-PhSGvTH-YwgD6onuhh1VXLg8jPlH32H4/edit?usp=sharing Bug: 1323391 Change-Id: Ie5e3b283912693a2757ba188cef97b3a1182595e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4201001 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Scott Haseley <[email protected]> Cr-Commit-Position: refs/heads/main@{#1100760}
1 parent d4e7794 commit d5b7539

11 files changed

Lines changed: 561 additions & 34 deletions

‎third_party/blink/renderer/core/dom/abort_controller.cc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "third_party/blink/renderer/core/dom/abort_signal.h"
99
#include "third_party/blink/renderer/platform/bindings/exception_code.h"
1010
#include "third_party/blink/renderer/platform/heap/visitor.h"
11+
#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
1112

1213
namespace blink {
1314

@@ -21,6 +22,12 @@ AbortController::AbortController(AbortSignal* signal) : signal_(signal) {}
2122

2223
AbortController::~AbortController() = default;
2324

25+
void AbortController::Dispose() {
26+
if (RuntimeEnabledFeatures::AbortSignalAnyEnabled()) {
27+
signal_->DetachFromController();
28+
}
29+
}
30+
2431
void AbortController::abort(ScriptState* script_state) {
2532
v8::Local<v8::Value> dom_exception = V8ThrowDOMException::CreateOrEmpty(
2633
script_state->GetIsolate(), DOMExceptionCode::kAbortError,

‎third_party/blink/renderer/core/dom/abort_controller.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "third_party/blink/renderer/core/core_export.h"
99
#include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
1010
#include "third_party/blink/renderer/platform/heap/member.h"
11+
#include "third_party/blink/renderer/platform/heap/prefinalizer.h"
1112

1213
namespace blink {
1314

@@ -20,6 +21,7 @@ class ScriptValue;
2021
// https://docs.google.com/document/d/1OuoCG2uiijbAwbCw9jaS7tHEO0LBO_4gMNio1ox0qlY/edit
2122
class CORE_EXPORT AbortController : public ScriptWrappable {
2223
DEFINE_WRAPPERTYPEINFO();
24+
USING_PRE_FINALIZER(AbortController, Dispose);
2325

2426
public:
2527
static AbortController* Create(ExecutionContext*);
@@ -36,6 +38,8 @@ class CORE_EXPORT AbortController : public ScriptWrappable {
3638
void abort(ScriptState*);
3739
void abort(ScriptState*, ScriptValue reason);
3840

41+
void Dispose();
42+
3943
void Trace(Visitor*) const override;
4044

4145
private:

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

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,8 @@ class RemovableAbortAlgorithmCollection final
9898

9999
void Clear() override { abort_algorithms_.clear(); }
100100

101+
bool Empty() const override { return abort_algorithms_.empty(); }
102+
101103
void Run() override {
102104
for (AbortSignal::AlgorithmHandle* handle : abort_algorithms_) {
103105
handle->GetAlgorithm()->Run();
@@ -136,6 +138,8 @@ class UnremovableAbortAlgorithmCollection final
136138

137139
void Clear() override { abort_algorithms_.clear(); }
138140

141+
bool Empty() const override { return abort_algorithms_.empty(); }
142+
139143
void Run() override {
140144
for (AbortSignal::Algorithm* algorithm : abort_algorithms_) {
141145
algorithm->Run();
@@ -201,6 +205,13 @@ void AbortSignal::InitializeCommon(ExecutionContext* execution_context,
201205
abort_algorithms_ =
202206
MakeGarbageCollected<UnremovableAbortAlgorithmCollection>();
203207
}
208+
209+
if (RuntimeEnabledFeatures::AbortSignalAnyEnabled() &&
210+
signal_type_ == AbortSignal::SignalType::kComposite) {
211+
// Composite signals need to be kept alive when they have relevant event
212+
// listeners or pending algorithms.
213+
RegisterActiveScriptWrappable();
214+
}
204215
}
205216

206217
AbortSignal::~AbortSignal() = default;
@@ -221,6 +232,9 @@ AbortSignal* AbortSignal::abort(ScriptState* script_state, ScriptValue reason) {
221232
AbortSignal* signal = MakeGarbageCollected<AbortSignal>(
222233
ExecutionContext::From(script_state), SignalType::kAborted);
223234
signal->abort_reason_ = reason;
235+
if (RuntimeEnabledFeatures::AbortSignalAnyEnabled()) {
236+
signal->composition_manager_->Settle();
237+
}
224238
return signal;
225239
}
226240

@@ -291,24 +305,29 @@ ExecutionContext* AbortSignal::GetExecutionContext() const {
291305
}
292306

293307
AbortSignal::AlgorithmHandle* AbortSignal::AddAlgorithm(Algorithm* algorithm) {
294-
if (aborted())
308+
if (aborted() || (RuntimeEnabledFeatures::AbortSignalAnyEnabled() &&
309+
composition_manager_->IsSettled())) {
295310
return nullptr;
296-
311+
}
297312
auto* handle = MakeGarbageCollected<AlgorithmHandle>(algorithm);
298313
abort_algorithms_->AddAlgorithm(handle);
299314
return handle;
300315
}
301316

302317
void AbortSignal::RemoveAlgorithm(AlgorithmHandle* handle) {
303-
if (aborted())
318+
if (aborted() || (RuntimeEnabledFeatures::AbortSignalAnyEnabled() &&
319+
composition_manager_->IsSettled())) {
304320
return;
321+
}
305322
abort_algorithms_->RemoveAlgorithm(handle);
306323
}
307324

308325
AbortSignal::AlgorithmHandle* AbortSignal::AddAlgorithm(
309326
base::OnceClosure algorithm) {
310-
if (aborted())
327+
if (aborted() || (RuntimeEnabledFeatures::AbortSignalAnyEnabled() &&
328+
composition_manager_->IsSettled())) {
311329
return nullptr;
330+
}
312331
auto* callback_algorithm =
313332
MakeGarbageCollected<OnceCallbackAlgorithm>(std::move(algorithm));
314333
auto* handle = MakeGarbageCollected<AlgorithmHandle>(callback_algorithm);
@@ -339,7 +358,10 @@ void AbortSignal::SignalAbort(ScriptState* script_state, ScriptValue reason) {
339358
abort_reason_ = reason;
340359
}
341360
abort_algorithms_->Run();
342-
abort_algorithms_->Clear();
361+
if (!RuntimeEnabledFeatures::AbortSignalAnyEnabled()) {
362+
// This is cleared when the signal is settled when the feature is enabled.
363+
abort_algorithms_->Clear();
364+
}
343365
dependent_signal_algorithms_.clear();
344366
DispatchEvent(*Event::Create(event_type_names::kAbort));
345367

@@ -355,6 +377,7 @@ void AbortSignal::SignalAbort(ScriptState* script_state, ScriptValue reason) {
355377
signal->SignalAbort(script_state, abort_reason_);
356378
}
357379
}
380+
composition_manager_->Settle();
358381
}
359382
}
360383

@@ -389,6 +412,34 @@ AbortSignalCompositionManager* AbortSignal::GetCompositionManager(
389412
return nullptr;
390413
}
391414

415+
void AbortSignal::DetachFromController() {
416+
DCHECK(RuntimeEnabledFeatures::AbortSignalAnyEnabled());
417+
if (aborted()) {
418+
return;
419+
}
420+
composition_manager_->Settle();
421+
}
422+
423+
void AbortSignal::OnSignalSettled(AbortSignalCompositionType type) {
424+
DCHECK(RuntimeEnabledFeatures::AbortSignalAnyEnabled());
425+
DCHECK_EQ(type, AbortSignalCompositionType::kAbort);
426+
abort_algorithms_->Clear();
427+
}
428+
429+
bool AbortSignal::HasPendingActivity() const {
430+
if (signal_type_ != SignalType::kComposite) {
431+
return false;
432+
}
433+
DCHECK(RuntimeEnabledFeatures::AbortSignalAnyEnabled());
434+
// Settled signals cannot signal abort, so they can be GCed.
435+
if (composition_manager_->IsSettled()) {
436+
return false;
437+
}
438+
// Otherwise the signal needs to be kept alive if aborting can be observed.
439+
return HasEventListeners(event_type_names::kAbort) ||
440+
!abort_algorithms_->Empty();
441+
}
442+
392443
AbortSignal::AlgorithmHandle::AlgorithmHandle(AbortSignal::Algorithm* algorithm)
393444
: algorithm_(algorithm) {}
394445

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#define THIRD_PARTY_BLINK_RENDERER_CORE_DOM_ABORT_SIGNAL_H_
77

88
#include "base/functional/callback_forward.h"
9+
#include "third_party/blink/renderer/bindings/core/v8/active_script_wrappable.h"
910
#include "third_party/blink/renderer/bindings/core/v8/script_value.h"
1011
#include "third_party/blink/renderer/core/core_export.h"
1112
#include "third_party/blink/renderer/core/dom/abort_signal_composition_type.h"
@@ -21,7 +22,8 @@ class ExecutionContext;
2122
class ScriptState;
2223

2324
// Implementation of https://dom.spec.whatwg.org/#interface-AbortSignal
24-
class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
25+
class CORE_EXPORT AbortSignal : public EventTargetWithInlineData,
26+
public LazyActiveScriptWrappable<AbortSignal> {
2527
DEFINE_WRAPPERTYPEINFO();
2628

2729
public:
@@ -86,6 +88,7 @@ class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
8688
virtual void RemoveAlgorithm(AlgorithmHandle*) = 0;
8789
virtual void Clear() = 0;
8890
virtual void Run() = 0;
91+
virtual bool Empty() const = 0;
8992
virtual void Trace(Visitor*) const {}
9093
};
9194

@@ -115,6 +118,7 @@ class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
115118

116119
const AtomicString& InterfaceName() const override;
117120
ExecutionContext* GetExecutionContext() const override;
121+
bool HasPendingActivity() const override;
118122

119123
// Internal API
120124

@@ -163,6 +167,13 @@ class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
163167
virtual AbortSignalCompositionManager* GetCompositionManager(
164168
AbortSignalCompositionType);
165169

170+
// Called by the composition manager when the signal is settled.
171+
virtual void OnSignalSettled(AbortSignalCompositionType);
172+
173+
// Callback from `AbortController` during prefinalization, when the controller
174+
// can no longer emit events.
175+
virtual void DetachFromController();
176+
166177
private:
167178
// Common constructor initialization separated out to make mutually exclusive
168179
// constructors more readable.

‎third_party/blink/renderer/core/dom/abort_signal.idl‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
// https://dom.spec.whatwg.org/#interface-AbortSignal
66

77
[
8-
Exposed=(Window,Worker)
8+
Exposed=(Window,Worker),
9+
ActiveScriptWrappable
910
] interface AbortSignal : EventTarget {
1011
[
1112
CallWith=ScriptState,

‎third_party/blink/renderer/core/dom/abort_signal_composition_manager.cc‎

Lines changed: 60 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ void AbortSignalCompositionManager::Trace(Visitor* visitor) const {
2626
visitor->Trace(signal_);
2727
}
2828

29+
void AbortSignalCompositionManager::Settle() {
30+
DCHECK(!is_settled_);
31+
is_settled_ = true;
32+
33+
signal_->OnSignalSettled(composition_type_);
34+
}
35+
2936
DependentSignalCompositionManager::DependentSignalCompositionManager(
3037
AbortSignal& managed_signal,
3138
AbortSignalCompositionType type,
@@ -45,6 +52,10 @@ DependentSignalCompositionManager::DependentSignalCompositionManager(
4552
AddSourceSignal(*source.Get());
4653
}
4754
}
55+
56+
if (source_signals_.empty()) {
57+
Settle();
58+
}
4859
}
4960

5061
DependentSignalCompositionManager::~DependentSignalCompositionManager() =
@@ -56,6 +67,15 @@ void DependentSignalCompositionManager::Trace(Visitor* visitor) const {
5667
}
5768

5869
void DependentSignalCompositionManager::AddSourceSignal(AbortSignal& source) {
70+
auto* source_manager = To<SourceSignalCompositionManager>(
71+
source.GetCompositionManager(GetCompositionType()));
72+
DCHECK(source_manager);
73+
// `source` won't emit `composition_type_` any longer, so there's no need to
74+
// follow. This can happen if `source` is associated with a GCed controller.
75+
if (source_manager->IsSettled()) {
76+
return;
77+
}
78+
5979
DCHECK(!source.IsCompositeSignal());
6080
// Internal signals can add dependent signals after construction via
6181
// AbortSignal::Follow, which would violate our assumptions for
@@ -70,13 +90,29 @@ void DependentSignalCompositionManager::AddSourceSignal(AbortSignal& source) {
7090
return;
7191
}
7292
source_signals_.insert(&source);
73-
74-
auto* source_manager = To<SourceSignalCompositionManager>(
75-
source.GetCompositionManager(GetCompositionType()));
76-
DCHECK(source_manager);
7793
source_manager->AddDependentSignal(*this);
7894
}
7995

96+
void DependentSignalCompositionManager::Settle() {
97+
AbortSignalCompositionManager::Settle();
98+
source_signals_.clear();
99+
}
100+
101+
void DependentSignalCompositionManager::OnSourceSettled(
102+
SourceSignalCompositionManager& source_manager) {
103+
DCHECK(GetSignal().IsCompositeSignal());
104+
DCHECK(!IsSettled());
105+
106+
// Note: `source_signals_` might not contain the source, and it might already
107+
// be empty if this source was removed during prefinalization. That's okay --
108+
// we only need to detect that the collection is empty on this path (if the
109+
// signal is being kept alive by the registry).
110+
source_signals_.erase(&source_manager.GetSignal());
111+
if (source_signals_.empty()) {
112+
Settle();
113+
}
114+
}
115+
80116
SourceSignalCompositionManager::SourceSignalCompositionManager(
81117
AbortSignal& signal,
82118
AbortSignalCompositionType composition_type)
@@ -91,6 +127,8 @@ void SourceSignalCompositionManager::Trace(Visitor* visitor) const {
91127

92128
void SourceSignalCompositionManager::AddDependentSignal(
93129
DependentSignalCompositionManager& dependent_manager) {
130+
DCHECK(!IsSettled());
131+
DCHECK(!dependent_manager.IsSettled());
94132
DCHECK(dependent_manager.GetSignal().IsCompositeSignal());
95133
// New dependents should not be added to aborted signals.
96134
DCHECK(GetCompositionType() != AbortSignalCompositionType::kAbort ||
@@ -99,4 +137,22 @@ void SourceSignalCompositionManager::AddDependentSignal(
99137
dependent_signals_.insert(&dependent_manager.GetSignal());
100138
}
101139

140+
void SourceSignalCompositionManager::Settle() {
141+
AbortSignalCompositionManager::Settle();
142+
143+
for (auto& signal : dependent_signals_) {
144+
auto* manager = To<DependentSignalCompositionManager>(
145+
signal->GetCompositionManager(GetCompositionType()));
146+
DCHECK(manager);
147+
// The signal might have been settled if its `source_signals_` were cleared
148+
// during prefinalization and another source already notified it, or if the
149+
// signal was aborted.
150+
if (manager->IsSettled()) {
151+
continue;
152+
}
153+
manager->OnSourceSettled(*this);
154+
}
155+
dependent_signals_.clear();
156+
}
157+
102158
} // namespace blink

0 commit comments

Comments
 (0)