Skip to content

Commit

Permalink
[dom] Implement AbortSignal.any() memory management
Browse files Browse the repository at this point in the history
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: I989c937e5359fffad72ff2c26068cc49b28e1a10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3704585
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098063}
  • Loading branch information
shaseley authored and Chromium LUCI CQ committed Jan 27, 2023
1 parent 20acb3a commit c2b6a48
Show file tree
Hide file tree
Showing 11 changed files with 473 additions and 34 deletions.
7 changes: 7 additions & 0 deletions third_party/blink/renderer/core/dom/abort_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "third_party/blink/renderer/core/dom/abort_signal.h"
#include "third_party/blink/renderer/platform/bindings/exception_code.h"
#include "third_party/blink/renderer/platform/heap/visitor.h"
#include "third_party/blink/renderer/platform/runtime_enabled_features.h"

namespace blink {

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

AbortController::~AbortController() = default;

void AbortController::Dispose() {
if (RuntimeEnabledFeatures::AbortSignalAnyEnabled()) {
signal_->DetachFromController();
}
}

void AbortController::abort(ScriptState* script_state) {
v8::Local<v8::Value> dom_exception = V8ThrowDOMException::CreateOrEmpty(
script_state->GetIsolate(), DOMExceptionCode::kAbortError,
Expand Down
4 changes: 4 additions & 0 deletions third_party/blink/renderer/core/dom/abort_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
#include "third_party/blink/renderer/platform/heap/member.h"
#include "third_party/blink/renderer/platform/heap/prefinalizer.h"

namespace blink {

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

public:
static AbortController* Create(ExecutionContext*);
Expand All @@ -36,6 +38,8 @@ class CORE_EXPORT AbortController : public ScriptWrappable {
void abort(ScriptState*);
void abort(ScriptState*, ScriptValue reason);

void Dispose();

void Trace(Visitor*) const override;

private:
Expand Down
61 changes: 56 additions & 5 deletions third_party/blink/renderer/core/dom/abort_signal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ class RemovableAbortAlgorithmCollection final

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

bool Empty() const override { return abort_algorithms_.empty(); }

void Run() override {
for (AbortSignal::AlgorithmHandle* handle : abort_algorithms_) {
handle->GetAlgorithm()->Run();
Expand Down Expand Up @@ -136,6 +138,8 @@ class UnremovableAbortAlgorithmCollection final

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

bool Empty() const override { return abort_algorithms_.empty(); }

void Run() override {
for (AbortSignal::Algorithm* algorithm : abort_algorithms_) {
algorithm->Run();
Expand Down Expand Up @@ -201,6 +205,13 @@ void AbortSignal::InitializeCommon(ExecutionContext* execution_context,
abort_algorithms_ =
MakeGarbageCollected<UnremovableAbortAlgorithmCollection>();
}

if (RuntimeEnabledFeatures::AbortSignalAnyEnabled() &&
signal_type_ == AbortSignal::SignalType::kComposite) {
// Composite signals need to be kept alive when they have relevant event
// listeners or pending algorithms.
RegisterActiveScriptWrappable();
}
}

AbortSignal::~AbortSignal() = default;
Expand All @@ -221,6 +232,9 @@ AbortSignal* AbortSignal::abort(ScriptState* script_state, ScriptValue reason) {
AbortSignal* signal = MakeGarbageCollected<AbortSignal>(
ExecutionContext::From(script_state), SignalType::kAborted);
signal->abort_reason_ = reason;
if (RuntimeEnabledFeatures::AbortSignalAnyEnabled()) {
signal->composition_manager_->Settle();
}
return signal;
}

Expand Down Expand Up @@ -291,24 +305,29 @@ ExecutionContext* AbortSignal::GetExecutionContext() const {
}

AbortSignal::AlgorithmHandle* AbortSignal::AddAlgorithm(Algorithm* algorithm) {
if (aborted())
if (aborted() || (RuntimeEnabledFeatures::AbortSignalAnyEnabled() &&
composition_manager_->IsSettled())) {
return nullptr;

}
auto* handle = MakeGarbageCollected<AlgorithmHandle>(algorithm);
abort_algorithms_->AddAlgorithm(handle);
return handle;
}

void AbortSignal::RemoveAlgorithm(AlgorithmHandle* handle) {
if (aborted())
if (aborted() || (RuntimeEnabledFeatures::AbortSignalAnyEnabled() &&
composition_manager_->IsSettled())) {
return;
}
abort_algorithms_->RemoveAlgorithm(handle);
}

AbortSignal::AlgorithmHandle* AbortSignal::AddAlgorithm(
base::OnceClosure algorithm) {
if (aborted())
if (aborted() || (RuntimeEnabledFeatures::AbortSignalAnyEnabled() &&
composition_manager_->IsSettled())) {
return nullptr;
}
auto* callback_algorithm =
MakeGarbageCollected<OnceCallbackAlgorithm>(std::move(algorithm));
auto* handle = MakeGarbageCollected<AlgorithmHandle>(callback_algorithm);
Expand Down Expand Up @@ -339,7 +358,10 @@ void AbortSignal::SignalAbort(ScriptState* script_state, ScriptValue reason) {
abort_reason_ = reason;
}
abort_algorithms_->Run();
abort_algorithms_->Clear();
if (!RuntimeEnabledFeatures::AbortSignalAnyEnabled()) {
// This is cleared when the signal is settled when the feature is enabled.
abort_algorithms_->Clear();
}
dependent_signal_algorithms_.clear();
DispatchEvent(*Event::Create(event_type_names::kAbort));

Expand All @@ -355,6 +377,7 @@ void AbortSignal::SignalAbort(ScriptState* script_state, ScriptValue reason) {
signal->SignalAbort(script_state, abort_reason_);
}
}
composition_manager_->Settle();
}
}

Expand Down Expand Up @@ -389,6 +412,34 @@ AbortSignalCompositionManager* AbortSignal::GetCompositionManager(
return nullptr;
}

void AbortSignal::DetachFromController() {
DCHECK(RuntimeEnabledFeatures::AbortSignalAnyEnabled());
if (aborted()) {
return;
}
composition_manager_->Settle();
}

void AbortSignal::OnSignalSettled(AbortSignalCompositionType type) {
DCHECK(RuntimeEnabledFeatures::AbortSignalAnyEnabled());
DCHECK_EQ(type, AbortSignalCompositionType::kAbort);
abort_algorithms_->Clear();
}

bool AbortSignal::HasPendingActivity() const {
if (signal_type_ != SignalType::kComposite) {
return false;
}
DCHECK(RuntimeEnabledFeatures::AbortSignalAnyEnabled());
// Settled signals cannot signal abort, so they can be GCed.
if (composition_manager_->IsSettled()) {
return false;
}
// Otherwise the signal needs to be kept alive if aborting can be observed.
return HasEventListeners(event_type_names::kAbort) ||
!abort_algorithms_->Empty();
}

AbortSignal::AlgorithmHandle::AlgorithmHandle(AbortSignal::Algorithm* algorithm)
: algorithm_(algorithm) {}

Expand Down
13 changes: 12 additions & 1 deletion third_party/blink/renderer/core/dom/abort_signal.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define THIRD_PARTY_BLINK_RENDERER_CORE_DOM_ABORT_SIGNAL_H_

#include "base/functional/callback_forward.h"
#include "third_party/blink/renderer/bindings/core/v8/active_script_wrappable.h"
#include "third_party/blink/renderer/bindings/core/v8/script_value.h"
#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/dom/abort_signal_composition_type.h"
Expand All @@ -21,7 +22,8 @@ class ExecutionContext;
class ScriptState;

// Implementation of https://dom.spec.whatwg.org/#interface-AbortSignal
class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
class CORE_EXPORT AbortSignal : public EventTargetWithInlineData,
public LazyActiveScriptWrappable<AbortSignal> {
DEFINE_WRAPPERTYPEINFO();

public:
Expand Down Expand Up @@ -86,6 +88,7 @@ class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
virtual void RemoveAlgorithm(AlgorithmHandle*) = 0;
virtual void Clear() = 0;
virtual void Run() = 0;
virtual bool Empty() const = 0;
virtual void Trace(Visitor*) const {}
};

Expand Down Expand Up @@ -115,6 +118,7 @@ class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {

const AtomicString& InterfaceName() const override;
ExecutionContext* GetExecutionContext() const override;
bool HasPendingActivity() const override;

// Internal API

Expand Down Expand Up @@ -163,6 +167,13 @@ class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
virtual AbortSignalCompositionManager* GetCompositionManager(
AbortSignalCompositionType);

// Called by the composition manager when the signal is settled.
virtual void OnSignalSettled(AbortSignalCompositionType);

// Callback from `AbortController` during prefinalization, when the controller
// can no longer emit events.
virtual void DetachFromController();

private:
// Common constructor initialization separated out to make mutually exclusive
// constructors more readable.
Expand Down
3 changes: 2 additions & 1 deletion third_party/blink/renderer/core/dom/abort_signal.idl
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
// https://dom.spec.whatwg.org/#interface-AbortSignal

[
Exposed=(Window,Worker)
Exposed=(Window,Worker),
ActiveScriptWrappable
] interface AbortSignal : EventTarget {
[
CallWith=ScriptState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ void AbortSignalCompositionManager::Trace(Visitor* visitor) const {
visitor->Trace(signal_);
}

void AbortSignalCompositionManager::Settle() {
DCHECK(!is_settled_);
is_settled_ = true;

signal_->OnSignalSettled(composition_type_);
}

DependentSignalCompositionManager::DependentSignalCompositionManager(
AbortSignal& managed_signal,
AbortSignalCompositionType type,
Expand All @@ -45,6 +52,10 @@ DependentSignalCompositionManager::DependentSignalCompositionManager(
AddSourceSignal(*source.Get());
}
}

if (source_signals_.empty()) {
Settle();
}
}

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

void DependentSignalCompositionManager::AddSourceSignal(AbortSignal& source) {
auto* source_manager = To<SourceSignalCompositionManager>(
source.GetCompositionManager(GetCompositionType()));
DCHECK(source_manager);
// `source` won't emit `composition_type_` any longer, so there's no need to
// follow. This can happen if `source` is associated with a GCed controller.
if (source_manager->IsSettled()) {
return;
}

DCHECK(!source.IsCompositeSignal());
// Internal signals can add dependent signals after construction via
// AbortSignal::Follow, which would violate our assumptions for
Expand All @@ -70,13 +90,29 @@ void DependentSignalCompositionManager::AddSourceSignal(AbortSignal& source) {
return;
}
source_signals_.insert(&source);

auto* source_manager = To<SourceSignalCompositionManager>(
source.GetCompositionManager(GetCompositionType()));
DCHECK(source_manager);
source_manager->AddDependentSignal(*this);
}

void DependentSignalCompositionManager::Settle() {
AbortSignalCompositionManager::Settle();
source_signals_.clear();
}

void DependentSignalCompositionManager::OnSourceSettled(
SourceSignalCompositionManager& source_manager) {
DCHECK(GetSignal().IsCompositeSignal());
DCHECK(!IsSettled());

// Note: `source_signals_` might not contain the source, and it might already
// be empty if this source was removed during prefinalization. That's okay --
// we only need to detect that the collection is empty on this path (if the
// signal is being kept alive by the registry).
source_signals_.erase(&source_manager.GetSignal());
if (source_signals_.empty()) {
Settle();
}
}

SourceSignalCompositionManager::SourceSignalCompositionManager(
AbortSignal& signal,
AbortSignalCompositionType composition_type)
Expand All @@ -91,6 +127,8 @@ void SourceSignalCompositionManager::Trace(Visitor* visitor) const {

void SourceSignalCompositionManager::AddDependentSignal(
DependentSignalCompositionManager& dependent_manager) {
DCHECK(!IsSettled());
DCHECK(!dependent_manager.IsSettled());
DCHECK(dependent_manager.GetSignal().IsCompositeSignal());
// New dependents should not be added to aborted signals.
DCHECK(GetCompositionType() != AbortSignalCompositionType::kAbort ||
Expand All @@ -99,4 +137,22 @@ void SourceSignalCompositionManager::AddDependentSignal(
dependent_signals_.insert(&dependent_manager.GetSignal());
}

void SourceSignalCompositionManager::Settle() {
AbortSignalCompositionManager::Settle();

for (auto& signal : dependent_signals_) {
auto* manager = To<DependentSignalCompositionManager>(
signal->GetCompositionManager(GetCompositionType()));
DCHECK(manager);
// The signal might have been settled if its `source_signals_` were cleared
// during prefinalization and another source already notified it, or if the
// signal was aborted.
if (manager->IsSettled()) {
continue;
}
manager->OnSourceSettled(*this);
}
dependent_signals_.clear();
}

} // namespace blink

0 comments on commit c2b6a48

Please sign in to comment.