Commit 02090026 authored by Jeremy Roman's avatar Jeremy Roman Committed by Commit Bot

[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/1392109Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Reviewed-by: default avatarHongchan Choi <hongchan@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621840}
parent 3ae9799d
include_rules = [
"+base/atomic_ref_count.h",
"+base/auto_reset.h",
"+base/bit_cast.h",
"+base/callback.h",
......
......@@ -17,10 +17,12 @@
#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 {
......@@ -116,7 +118,11 @@ void BlinkLeakDetector::TimerFiredGC(TimerBase*) {
// (crbug.com/616714)
delayed_gc_timer_.StartOneShot(TimeDelta(), FROM_HERE);
} else {
ReportResult();
// 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()));
}
}
......
......@@ -5,6 +5,7 @@
#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"
......@@ -31,6 +32,7 @@ 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,6 +88,8 @@ 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,6 +24,7 @@
*/
#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"
......@@ -77,6 +78,15 @@ 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()];
......@@ -102,14 +112,7 @@ void AudioHandler::Uninitialize() {
}
void AudioHandler::Dispose() {
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();
// TODO(jbroman): Remove this when derived classes don't need it.
}
AudioNode* AudioHandler::GetNode() const {
......@@ -121,6 +124,10 @@ BaseAudioContext* AudioHandler::Context() const {
return context_;
}
void AudioHandler::ClearContext() {
context_ = nullptr;
}
String AudioHandler::NodeTypeName() const {
switch (node_type_) {
case kNodeTypeDestination:
......@@ -436,7 +443,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_) {
if (connection_ref_count_ <= 1 && !is_disabled_ && !IsDoomed()) {
// 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
......@@ -586,6 +593,7 @@ unsigned AudioHandler::NumberOfOutputChannels() const {
<< GetNodeType();
return 1;
}
// ----------------------------------------------------------------
AudioNode::AudioNode(BaseAudioContext& context)
......
......@@ -28,8 +28,10 @@
#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"
......@@ -51,6 +53,8 @@ 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
......@@ -73,7 +77,13 @@ class ExceptionState;
// Be careful to avoid reference cycles. If an AudioHandler has a reference
// cycle including the owner AudioNode, objects in the cycle are never
// collected.
class MODULES_EXPORT AudioHandler : public ThreadSafeRefCounted<AudioHandler> {
//
// 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> {
public:
enum NodeType {
kNodeTypeUnknown = 0,
......@@ -119,7 +129,9 @@ class MODULES_EXPORT AudioHandler : public ThreadSafeRefCounted<AudioHandler> {
// rendering thread, and inside dispose(). We must not call context() in the
// destructor.
virtual BaseAudioContext* Context() const;
void ClearContext() { context_ = nullptr; }
// Explicitly drop the pointer to the BaseAudioContext (during finalization).
void ClearContext();
DeferredTaskHandler& GetDeferredTaskHandler() const {
return *deferred_task_handler_;
......@@ -267,17 +279,19 @@ class MODULES_EXPORT AudioHandler : public ThreadSafeRefCounted<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_;
// 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_;
// Accessed on multiple threads.
CrossThreadWeakPersistent<BaseAudioContext> context_;
// Legal to access even when |context_| may be gone, such as during the
// destructor.
......@@ -317,6 +331,12 @@ class MODULES_EXPORT AudioHandler : public ThreadSafeRefCounted<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,6 +37,7 @@
#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"
......@@ -44,6 +45,7 @@
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
......@@ -94,8 +96,15 @@ enum AudioParamType {
// dies.
//
// Connected to AudioNodeOutput using AudioNodeWiring.
class AudioParamHandler final : public ThreadSafeRefCounted<AudioParamHandler>,
public AudioSummingJunction {
//
// 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 {
public:
// Automation rate of the AudioParam
enum AutomationRate {
......@@ -256,6 +265,12 @@ class AudioParamHandler final : public ThreadSafeRefCounted<AudioParamHandler>,
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,13 +105,6 @@ 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,6 +138,7 @@ _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>
<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>
<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>
<!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