Commit 214678c7 authored by Jeremy Roman's avatar Jeremy Roman Committed by Commit Bot

Revert "[Reland] webaudio: Always delete handlers in a non-nestable task on the main thread."

This reverts commit 02090026.

Reason for revert: Causing crashes in canary.

Original change's description:
> [Reland] webaudio: Always delete handlers in a non-nestable task on the main thread.
>
> Deleting handlers does considerable non-trivial work that requires the
> graph lock. Allowing this to happen during finalization introduces
> considerable complexity because it can happen during any GC heap allocation,
> including ones that occur while the graph lock is already held.
>
> Instead, when the last reference is dropped, the handler is not deleted
> immediately, but a non-nestable task to delete it is enqueued. Since the
> task is non-nestable, it will not run while the graph lock is held by the
> main thread (and if it is held by another thread, it will block on it).
>
> This slightly extends the lifetime of audio handlers, during which time
> it is illegal to take a new reference (and RefCounted checks this in
> debug builds). So a flag is set in the destructor (though it could be
> sensibly set as early as the ref count reaches zero) that prevents the
> handler from adding itself to the tail processing queue if it is already
> being destroyed.
>
> The cycle collection layout test is updated to handle the fact that the
> handler count does not go to zero immediately on GC, but shortly thereafter.
> This has been done by merging with another recently added unit test, which
> tests something very similar (a cycle of one element).
>
> The leak detector has been augmented to wait for audio handlers that are
> awaiting deletion, if there are any.
>
> Bug: 884059
> Change-Id: I818bcdbf87dc3e83359ad7a62aa56547dea2d7a5
> Reviewed-on: https://chromium-review.googlesource.com/c/1392109
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Hongchan Choi <hongchan@chromium.org>
> Commit-Queue: Jeremy Roman <jbroman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#621840}

TBR=jbroman@chromium.org,haraken@chromium.org,hongchan@chromium.org

Change-Id: Ib13b613f6fa696935f64362f65eb94cba50acdfd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 884059,921018
Reviewed-on: https://chromium-review.googlesource.com/c/1407501
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: default avatarJeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622109}
parent dcc0fd9f
include_rules = [
"+base/atomic_ref_count.h",
"+base/auto_reset.h",
"+base/bit_cast.h",
"+base/callback.h",
......
......@@ -17,12 +17,10 @@
#include "third_party/blink/renderer/core/page/page.h"
#include "third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.h"
#include "third_party/blink/renderer/core/workers/worker_thread.h"
#include "third_party/blink/renderer/modules/webaudio/delete_soon_with_graph_lock.h"
#include "third_party/blink/renderer/platform/bindings/v8_per_isolate_data.h"
#include "third_party/blink/renderer/platform/instance_counters.h"
#include "third_party/blink/renderer/platform/loader/fetch/memory_cache.h"
#include "third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h"
#include "third_party/blink/renderer/platform/wtf/functional.h"
namespace blink {
......@@ -118,11 +116,7 @@ void BlinkLeakDetector::TimerFiredGC(TimerBase*) {
// (crbug.com/616714)
delayed_gc_timer_.StartOneShot(TimeDelta(), FROM_HERE);
} else {
// WebAudio has some objects which have deferred deletion since they are
// managed in part but not wholly by the GC heap. Wait for any of those that
// have been freed by the GC cycles to be cleaned up.
WaitForAudioHandlerDeletion(WTF::Bind(&BlinkLeakDetector::ReportResult,
weak_ptr_factory_.GetWeakPtr()));
ReportResult();
}
}
......
......@@ -5,7 +5,6 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_CONTROLLER_BLINK_LEAK_DETECTOR_H_
#define THIRD_PARTY_BLINK_RENDERER_CONTROLLER_BLINK_LEAK_DETECTOR_H_
#include "base/memory/weak_ptr.h"
#include "mojo/public/cpp/bindings/binding.h"
#include "third_party/blink/public/mojom/leak_detector/leak_detector.mojom-blink.h"
#include "third_party/blink/renderer/controller/controller_export.h"
......@@ -32,7 +31,6 @@ class CONTROLLER_EXPORT BlinkLeakDetector : public mojom::blink::LeakDetector {
TaskRunnerTimer<BlinkLeakDetector> delayed_gc_timer_;
int number_of_gc_needed_ = 0;
PerformLeakDetectionCallback callback_;
base::WeakPtrFactory<BlinkLeakDetector> weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(BlinkLeakDetector);
};
......
......@@ -88,8 +88,6 @@ blink_modules_sources("webaudio") {
"delay_node.h",
"delay_processor.cc",
"delay_processor.h",
"delete_soon_with_graph_lock.cc",
"delete_soon_with_graph_lock.h",
"dynamics_compressor_node.cc",
"dynamics_compressor_node.h",
"gain_node.cc",
......
......@@ -24,7 +24,6 @@
*/
#include "third_party/blink/renderer/modules/webaudio/audio_node.h"
#include "third_party/blink/renderer/modules/webaudio/audio_node_input.h"
#include "third_party/blink/renderer/modules/webaudio/audio_node_options.h"
#include "third_party/blink/renderer/modules/webaudio/audio_node_output.h"
......@@ -78,15 +77,6 @@ AudioHandler::AudioHandler(NodeType node_type,
AudioHandler::~AudioHandler() {
DCHECK(IsMainThread());
DCHECK(!GetNode())
<< "The AudioNode should have been cleared by weak processing";
deferred_task_handler_->AssertGraphOwner();
deferred_task_handler_->RemoveChangedChannelCountMode(this);
deferred_task_handler_->RemoveChangedChannelInterpretation(this);
deferred_task_handler_->RemoveAutomaticPullNode(this);
for (auto& output : outputs_)
output->Dispose();
InstanceCounters::DecrementCounter(InstanceCounters::kAudioHandlerCounter);
#if DEBUG_AUDIONODE_REFERENCES
--node_count_[GetNodeType()];
......@@ -112,7 +102,14 @@ void AudioHandler::Uninitialize() {
}
void AudioHandler::Dispose() {
// TODO(jbroman): Remove this when derived classes don't need it.
DCHECK(IsMainThread());
deferred_task_handler_->AssertGraphOwner();
deferred_task_handler_->RemoveChangedChannelCountMode(this);
deferred_task_handler_->RemoveChangedChannelInterpretation(this);
deferred_task_handler_->RemoveAutomaticPullNode(this);
for (auto& output : outputs_)
output->Dispose();
}
AudioNode* AudioHandler::GetNode() const {
......@@ -124,10 +121,6 @@ BaseAudioContext* AudioHandler::Context() const {
return context_;
}
void AudioHandler::ClearContext() {
context_ = nullptr;
}
String AudioHandler::NodeTypeName() const {
switch (node_type_) {
case kNodeTypeDestination:
......@@ -443,7 +436,7 @@ void AudioHandler::DisableOutputsIfNecessary() {
// The case of 1 is from AudioNodeInput::disable() where we want to disable
// outputs when there's only one connection left because we're ready to go
// away, but can't quite yet.
if (connection_ref_count_ <= 1 && !is_disabled_ && !IsDoomed()) {
if (connection_ref_count_ <= 1 && !is_disabled_) {
// Still may have JavaScript references, but no more "active" connection
// references, so put all of our outputs in a "dormant" disabled state.
// Garbage collection may take a very long time after this time, so the
......@@ -593,7 +586,6 @@ unsigned AudioHandler::NumberOfOutputChannels() const {
<< GetNodeType();
return 1;
}
// ----------------------------------------------------------------
AudioNode::AudioNode(BaseAudioContext& context)
......
......@@ -28,10 +28,8 @@
#include <memory>
#include "base/memory/scoped_refptr.h"
#include "base/synchronization/atomic_flag.h"
#include "third_party/blink/renderer/modules/event_target_modules.h"
#include "third_party/blink/renderer/modules/modules_export.h"
#include "third_party/blink/renderer/modules/webaudio/delete_soon_with_graph_lock.h"
#include "third_party/blink/renderer/platform/audio/audio_bus.h"
#include "third_party/blink/renderer/platform/audio/audio_utilities.h"
#include "third_party/blink/renderer/platform/heap/persistent.h"
......@@ -53,8 +51,6 @@ class AudioParam;
class DeferredTaskHandler;
class ExceptionState;
struct AudioHandlerTraits;
// An AudioNode is the basic building block for handling audio within an
// BaseAudioContext. It may be an audio source, an intermediate processing
// module, or an audio destination. Each AudioNode can have inputs and/or
......@@ -77,13 +73,7 @@ struct AudioHandlerTraits;
// Be careful to avoid reference cycles. If an AudioHandler has a reference
// cycle including the owner AudioNode, objects in the cycle are never
// collected.
//
// Due to AudioHandlerTraits, AudioHandler is not deleted immediately upon
// losing its last reference. This is required since its last reference may be
// dropped during Oilpan finalization on the main thread, but the graph lock is
// required to perform handler teardown. See also delete_soon_with_graph_lock.h.
class MODULES_EXPORT AudioHandler
: public ThreadSafeRefCounted<AudioHandler, AudioHandlerTraits> {
class MODULES_EXPORT AudioHandler : public ThreadSafeRefCounted<AudioHandler> {
public:
enum NodeType {
kNodeTypeUnknown = 0,
......@@ -129,9 +119,7 @@ class MODULES_EXPORT AudioHandler
// rendering thread, and inside dispose(). We must not call context() in the
// destructor.
virtual BaseAudioContext* Context() const;
// Explicitly drop the pointer to the BaseAudioContext (during finalization).
void ClearContext();
void ClearContext() { context_ = nullptr; }
DeferredTaskHandler& GetDeferredTaskHandler() const {
return *deferred_task_handler_;
......@@ -279,19 +267,17 @@ class MODULES_EXPORT AudioHandler
private:
void SetNodeType(NodeType);
// Returns true if the object has no remaining references, and is now
// scheduled for deletion. At this point it is no longer safe to take any new
// references to the object to extend its lifetime.
bool IsDoomed() { return !HasAtLeastOneRef(); }
bool is_initialized_;
NodeType node_type_;
// The owner AudioNode. Accessed only on the main thread.
const WeakPersistent<AudioNode> node_;
// Accessed on multiple threads.
CrossThreadWeakPersistent<BaseAudioContext> context_;
// This untraced member is safe because this is cleared for all of live
// AudioHandlers when the BaseAudioContext dies. Do not access m_context
// directly, use context() instead.
// See http://crbug.com/404527 for the detail.
UntracedMember<BaseAudioContext> context_;
// Legal to access even when |context_| may be gone, such as during the
// destructor.
......@@ -331,12 +317,6 @@ class MODULES_EXPORT AudioHandler
AudioBus::ChannelInterpretation new_channel_interpretation_;
};
struct AudioHandlerTraits {
static void Destruct(const AudioHandler* handler) {
DeleteSoonWithGraphLock(handler);
}
};
class MODULES_EXPORT AudioNode : public EventTargetWithInlineData {
DEFINE_WRAPPERTYPEINFO();
USING_PRE_FINALIZER(AudioNode, Dispose);
......
......@@ -37,7 +37,6 @@
#include "third_party/blink/renderer/modules/webaudio/audio_param_timeline.h"
#include "third_party/blink/renderer/modules/webaudio/audio_summing_junction.h"
#include "third_party/blink/renderer/modules/webaudio/base_audio_context.h"
#include "third_party/blink/renderer/modules/webaudio/delete_soon_with_graph_lock.h"
#include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
#include "third_party/blink/renderer/platform/wtf/thread_safe_ref_counted.h"
......@@ -45,7 +44,6 @@
namespace blink {
class AudioNodeOutput;
struct AudioParamHandlerTraits;
// Each AudioParam gets an identifier here. This is mostly for instrospection
// if warnings or other messages need to be printed. It's useful to know what
......@@ -96,15 +94,8 @@ enum AudioParamType {
// dies.
//
// Connected to AudioNodeOutput using AudioNodeWiring.
//
// Due to AudioParamHandlerTraits, AudioParamHandler is not deleted immediately
// upon losing its last reference. This is required since its last reference may
// be dropped during Oilpan finalization on the main thread, but the graph lock
// is required to perform handler teardown. See also
// delete_soon_with_graph_lock.h.
class AudioParamHandler final
: public ThreadSafeRefCounted<AudioParamHandler, AudioParamHandlerTraits>,
public AudioSummingJunction {
class AudioParamHandler final : public ThreadSafeRefCounted<AudioParamHandler>,
public AudioSummingJunction {
public:
// Automation rate of the AudioParam
enum AutomationRate {
......@@ -265,12 +256,6 @@ class AudioParamHandler final
friend class AudioNodeWiring;
};
struct AudioParamHandlerTraits {
static void Destruct(const AudioParamHandler* handler) {
DeleteSoonWithGraphLock(handler);
}
};
// AudioParam class represents web-exposed AudioParam interface.
class AudioParam final : public ScriptWrappable {
DEFINE_WRAPPERTYPEINFO();
......
......@@ -105,6 +105,13 @@ BaseAudioContext::BaseAudioContext(Document* document,
task_runner_(document->GetTaskRunner(TaskType::kInternalMedia)) {}
BaseAudioContext::~BaseAudioContext() {
{
// We may need to destroy summing junctions, which must happen while this
// object is still valid and with the graph lock held.
GraphAutoLocker locker(this);
destination_handler_ = nullptr;
}
GetDeferredTaskHandler().ContextWillBeDestroyed();
}
......
// Copyright 2018 The Chromium Authors. All rights reserved.
// 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/modules/webaudio/delete_soon_with_graph_lock.h"
#include <memory>
#include <utility>
#include "base/atomic_ref_count.h"
#include "base/callback.h"
#include "base/memory/scoped_refptr.h"
#include "base/sequenced_task_runner.h"
#include "third_party/blink/renderer/modules/webaudio/audio_node.h"
#include "third_party/blink/renderer/modules/webaudio/audio_param.h"
#include "third_party/blink/renderer/modules/webaudio/deferred_task_handler.h"
#include "third_party/blink/renderer/platform/scheduler/public/thread.h"
#include "third_party/blink/renderer/platform/wtf/functional.h"
#include "third_party/blink/renderer/platform/wtf/std_lib_extras.h"
#include "third_party/blink/renderer/platform/wtf/vector.h"
#include "third_party/blink/renderer/platform/wtf/wtf.h"
namespace blink {
namespace {
base::AtomicRefCount g_pending_deletions;
Vector<base::OnceClosure>& PendingDeletionClosures() {
DEFINE_STATIC_LOCAL(Vector<base::OnceClosure>, closures, ());
return closures;
}
void RunPendingDeletionClosures() {
Vector<base::OnceClosure> closures;
closures.swap(PendingDeletionClosures());
for (base::OnceClosure& closure : closures)
std::move(closure).Run();
}
template <typename T>
void DeleteSoonWithGraphLockImpl(const T* object) {
auto deleter = [](const T* object) {
// We need to keep the DeferredTaskHandler alive in this scope, as the
// handler could otherwise hold the last reference to the graph lock.
DCHECK(IsMainThread());
{
scoped_refptr<DeferredTaskHandler> deferred_task_handler =
&object->GetDeferredTaskHandler();
DeferredTaskHandler::GraphAutoLocker locker(*deferred_task_handler);
delete object;
}
// Possibly notify anything waiting on us.
if (!g_pending_deletions.Decrement())
RunPendingDeletionClosures();
};
// Add one to the number of pending deletions, if we're waiting.
g_pending_deletions.Increment();
// Transfer ownership of the doomed object to a unique owner.
using OwningPtr = std::unique_ptr<const T, decltype(deleter)>;
OwningPtr owning_ptr(object, deleter);
// Post a task which will, when run or destroyed, delete the object.
scoped_refptr<base::SequencedTaskRunner> task_runner =
Thread::MainThread()->GetTaskRunner();
task_runner->PostNonNestableTask(
FROM_HERE, WTF::Bind([](OwningPtr) {}, std::move(owning_ptr)));
}
} // namespace
void DeleteSoonWithGraphLock(const AudioHandler* handler) {
DeleteSoonWithGraphLockImpl(handler);
}
void DeleteSoonWithGraphLock(const AudioParamHandler* handler) {
DeleteSoonWithGraphLockImpl(handler);
}
void WaitForAudioHandlerDeletion(base::OnceClosure closure) {
if (g_pending_deletions.IsZero()) {
std::move(closure).Run();
} else {
PendingDeletionClosures().push_back(std::move(closure));
}
}
} // namespace blink
// Copyright 2018 The Chromium Authors. All rights reserved.
// 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_MODULES_WEBAUDIO_DELETE_SOON_WITH_GRAPH_LOCK_H_
#define THIRD_PARTY_BLINK_RENDERER_MODULES_WEBAUDIO_DELETE_SOON_WITH_GRAPH_LOCK_H_
#include "base/callback_forward.h"
#include "third_party/blink/renderer/modules/modules_export.h"
namespace blink {
class AudioHandler;
class AudioParamHandler;
// Performs deferred (non-nestable) deletion of the relevant object, acquiring
// the graph lock for the duration of the deletion.
//
// This is similar to base::RefCountedDeleteOnSequence, but assures non-nesting
// to avoid bad interactions with garbage collection. In particular, it assures
// that the destructor will not run within another task due to lazy sweeping.
MODULES_EXPORT void DeleteSoonWithGraphLock(const AudioHandler*);
MODULES_EXPORT void DeleteSoonWithGraphLock(const AudioParamHandler*);
// Waits for any audio handlers that are scheduled for deletion, and invoke the
// callback once no more remain. Will run immediately if there currently are
// none. This is intended for use by the leak detector and similar tests.
MODULES_EXPORT void WaitForAudioHandlerDeletion(base::OnceClosure);
} // namespace blink
#endif // THIRD_PARTY_BLINK_RENDERER_MODULES_WEBAUDIO_DELETE_SOON_WITH_GRAPH_LOCK_H_
......@@ -138,7 +138,6 @@ _CONFIG = [
# Base atomic utilities
'base::AtomicFlag',
'base::AtomicRefCount',
'base::AtomicSequenceNumber',
# Task traits
......
Cycles of AudioNode connections should be collected.
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
A cycle was created:
PASS internals.audioHandlerCount() > initialCount is true
GC happened:
PASS internals.audioHandlerCount() is initialCount
PASS successfullyParsed is true
TEST COMPLETE
<!DOCTYPE html>
<script src="../../resources/gc.js"></script>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script>
function soon() { return new Promise(resolve => setTimeout(resolve, 0)); }
async function becomesTrue(predicate) {
while (!predicate()) await soon();
}
function createCycle(context) {
let source = context.createBufferSource();
let delay1 = context.createDelay(1 / context.sampleRate);
let delay2 = context.createDelay(1 / context.sampleRate);
source.connect(delay1);
delay1.connect(delay2);
delay2.connect(delay1);
delay1.connect(context.destination);
}
promise_test(async () => {
await asyncGC();
await becomesTrue(() => internals.audioHandlerCount() == 0);
let context = new OfflineAudioContext(2, 1024, 44100);
let initialCount = internals.audioHandlerCount();
createCycle(context);
assert_greater_than(internals.audioHandlerCount(), 0);
// Need to render to clean up a cycle on an offline context.
await context.startRendering();
// Wait for the cycle and its associated handlers to be destroyed.
await asyncGC();
await becomesTrue(() => internals.audioHandlerCount() <= initialCount);
}, "Cycles of AudioNode connections should be collected.");
function createSelfConnectedNode(context) {
const gain = context.createGain();
gain.connect(gain);
}
promise_test(async () => {
await asyncGC();
await becomesTrue(() => internals.audioHandlerCount() == 0);
let context = new OfflineAudioContext(2, 1024, 44100);
let initialCount = internals.audioHandlerCount();
createSelfConnectedNode(context);
assert_greater_than(internals.audioHandlerCount(), 0);
// Need to render to clean up a cycle on an offline context.
await context.startRendering();
// Wait until the gain node's handler has been destroyed to declare victory.
await asyncGC();
await becomesTrue(() => internals.audioHandlerCount() <= initialCount);
}, "No crash should occur when a node connected to itself is collected.");
</script>
<html>
<head>
<title>
cycle-connection-gc.html
</title>
<script src="../../resources/gc.js"></script>
<script src="../../resources/js-test.js"></script>
</head>
<body>
<script id="layout-test-code">
description('Cycles of AudioNode connections should be collected.');
window.jsTestIsAsync = true;
let context;
let initialCount;
(async () => {
await asyncGC();
context = new OfflineAudioContext(2, 1024, 44100);
initialCount = internals.audioHandlerCount();
createCycle();
debug('A cycle was created:');
shouldBeTrue('internals.audioHandlerCount() > initialCount');
// Need to render to cleanup the cycle on an offline context
await context.startRendering();
await asyncGC();
debug('GC happened:');
shouldBe('internals.audioHandlerCount()', 'initialCount');
finishJSTest();
})();
function createCycle() {
let source = context.createBufferSource();
let delay1 = context.createDelay(1 / context.sampleRate);
let delay2 = context.createDelay(1 / context.sampleRate);
source.connect(delay1);
delay1.connect(delay2);
delay2.connect(delay1);
delay1.connect(context.destination);
}
</script>
</body>
</html>
<!DOCTYPE html>
<script src="../../resources/gc.js"></script>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script>
function soon() { return new Promise(resolve => setTimeout(resolve, 0)); }
async function becomesTrue(predicate) {
while (!predicate()) await soon();
}
function createSelfConnectedNode(context) {
const gain = context.createGain();
gain.connect(gain);
}
promise_test(async () => {
await asyncGC();
await becomesTrue(() => internals.audioHandlerCount() == 0);
let context = new OfflineAudioContext(2, 1024, 44100);
let initialCount = internals.audioHandlerCount();
createSelfConnectedNode(context);
assert_greater_than(internals.audioHandlerCount(), 0);
// Need to render to clean up a cycle on an offline context.
await context.startRendering();
// Wait until the gain node's handler has been destroyed to declare victory.
await asyncGC();
await becomesTrue(() => internals.audioHandlerCount() <= initialCount);
}, "No crash should occur when a node connected to itself is collected.");
</script>
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment