Skip to content

Commit

Permalink
Revert "[dom] Implement AbortSignal.any() memory management"
Browse files Browse the repository at this point in the history 
This reverts commit c2b6a48.

Reason for revert:
  New test wpt_internal/dom/abort/abort-signal-memory-tests.https.any.html
  likely causes a leak: https://ci.chromium.org/ui/p/chromium/builders/ci/WebKit%20Linux%20Leak/47389/overview

Original change's description:
> [dom] Implement AbortSignal.any() memory management
>
> 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}

Bug: 1323391
Change-Id: Icc50f231f660afeb7d1d7f77211f3c7ffbf41725
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4200932
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Jonathan Lee <jonathanjlee@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Jonathan Lee <jonathanjlee@google.com>
Cr-Commit-Position: refs/heads/main@{#1098105}
  • Loading branch information
@jonathan-j-lee
jonathan-j-lee authored and Chromium LUCI CQ committed Jan 27, 2023
1 parent 5918a20 commit 4efe206
Show file tree
Hide file tree
Showing 11 changed files with 34 additions and 473 deletions.
7 changes: 0 additions & 7 deletions third_party/blink/renderer/core/dom/abort_controller.cc
View file
Expand Up @@ -8,7 +8,6 @@
#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 @@ -22,12 +21,6 @@ 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: 0 additions & 4 deletions third_party/blink/renderer/core/dom/abort_controller.h
View file
Expand Up @@ -8,7 +8,6 @@
#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 @@ -21,7 +20,6 @@ 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 @@ -38,8 +36,6 @@ 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: 5 additions & 56 deletions third_party/blink/renderer/core/dom/abort_signal.cc
View file
Expand Up @@ -98,8 +98,6 @@ 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 @@ -138,8 +136,6 @@ 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 @@ -205,13 +201,6 @@ 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 @@ -232,9 +221,6 @@ 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 @@ -305,29 +291,24 @@ ExecutionContext* AbortSignal::GetExecutionContext() const {
}

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

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

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

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

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

Expand Down Expand Up @@ -412,34 +389,6 @@ 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: 1 addition & 12 deletions third_party/blink/renderer/core/dom/abort_signal.h
View file
Expand Up @@ -6,7 +6,6 @@
#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 @@ -22,8 +21,7 @@ class ExecutionContext;
class ScriptState;

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

public:
Expand Down Expand Up @@ -88,7 +86,6 @@ 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 @@ -118,7 +115,6 @@ 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 @@ -167,13 +163,6 @@ 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: 1 addition & 2 deletions third_party/blink/renderer/core/dom/abort_signal.idl
View file
Expand Up @@ -5,8 +5,7 @@
// https://dom.spec.whatwg.org/#interface-AbortSignal

[
Exposed=(Window,Worker),
ActiveScriptWrappable
Exposed=(Window,Worker)
] interface AbortSignal : EventTarget {
[
CallWith=ScriptState,
Expand Down
64 changes: 4 additions & 60 deletions third_party/blink/renderer/core/dom/abort_signal_composition_manager.cc
View file
Expand Up @@ -26,13 +26,6 @@ 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 @@ -52,10 +45,6 @@ DependentSignalCompositionManager::DependentSignalCompositionManager(
AddSourceSignal(*source.Get());
}
}

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

DependentSignalCompositionManager::~DependentSignalCompositionManager() =
Expand All @@ -67,15 +56,6 @@ 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 @@ -90,27 +70,11 @@ void DependentSignalCompositionManager::AddSourceSignal(AbortSignal& source) {
return;
}
source_signals_.insert(&source);
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();
}
auto* source_manager = To<SourceSignalCompositionManager>(
source.GetCompositionManager(GetCompositionType()));
DCHECK(source_manager);
source_manager->AddDependentSignal(*this);
}

SourceSignalCompositionManager::SourceSignalCompositionManager(
Expand All @@ -127,8 +91,6 @@ 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 @@ -137,22 +99,4 @@ 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 4efe206

Please sign in to comment.