Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick 85f708fa7ab8 from chromium #23047

Merged
merged 2 commits into from Apr 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions patches/chromium/.patches
Expand Up @@ -91,3 +91,5 @@ feat_add_support_for_overriding_the_base_spellchecker_download_url.patch
os_metrics_mac.patch
feat_allow_embedders_to_add_observers_on_created_hunspell.patch
feat_enable_offscreen_rendering_with_viz_compositor.patch
when_suspending_context_don_t_clear_handlers.patch
use_keepselfalive_on_audiocontext_to_keep_it_alive_until_rendering.patch
@@ -0,0 +1,132 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Hongchan Choi <hongchan@chromium.org>
Date: Tue, 18 Feb 2020 22:39:05 +0000
Subject: Use KeepSelfAlive on AudioContext to keep it alive until rendering
stops

When an ExecutionContext is abruptly/unexpectedly destroyed (e.g.
shutting down of document or iframe), an AudioContext can also
go away. This type of shutdown can be problematic because the render
thread still might be touching resources in the AudioContext allocated
by the main thread.

This CL introduces a self-referencing pointer to the AudioContext,
and it is cleared after the underlying render thread is stopped. In
that way, the destruction of AudioContext can be done safely.

Test: Locally confirmed the repro case doesn't crash (UAP) after 1hr.
Bug: 1043446
Change-Id: I2e40b7d58ca9d647eed8a5971fc69dc87ee3d1fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2049912
Reviewed-by: Raymond Toy <rtoy@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742338}

diff --git a/third_party/blink/renderer/modules/webaudio/audio_context.cc b/third_party/blink/renderer/modules/webaudio/audio_context.cc
index f544a4658f31632b85cea8f3f2939e3760b0dfb5..6618c8d74bdeaa8f470085956c30f5992497013a 100644
--- a/third_party/blink/renderer/modules/webaudio/audio_context.cc
+++ b/third_party/blink/renderer/modules/webaudio/audio_context.cc
@@ -132,7 +132,8 @@ AudioContext::AudioContext(Document& document,
const WebAudioLatencyHint& latency_hint,
base::Optional<float> sample_rate)
: BaseAudioContext(&document, kRealtimeContext),
- context_id_(g_context_id++) {
+ context_id_(g_context_id++),
+ keep_alive_(PERSISTENT_FROM_HERE, this) {
destination_node_ =
RealtimeAudioDestinationNode::Create(this, latency_hint, sample_rate);

@@ -169,13 +170,14 @@ AudioContext::AudioContext(Document& document,
destination()->GetAudioDestinationHandler());
base_latency_ = destination_handler.GetFramesPerBuffer() /
static_cast<double>(sampleRate());
+
}

void AudioContext::Uninitialize() {
DCHECK(IsMainThread());
DCHECK_NE(g_hardware_context_count, 0u);
--g_hardware_context_count;
-
+ StopRendering();
DidClose();
RecordAutoplayMetrics();
BaseAudioContext::Uninitialize();
@@ -358,14 +360,26 @@ bool AudioContext::IsContextClosed() const {
return close_resolver_ || BaseAudioContext::IsContextClosed();
}

+void AudioContext::StartRendering() {
+ DCHECK(IsMainThread());
+
+ if (!keep_alive_)
+ keep_alive_ = this;
+ BaseAudioContext::StartRendering();
+}
+
void AudioContext::StopRendering() {
DCHECK(IsMainThread());
DCHECK(destination());

- if (ContextState() == kRunning) {
+ // It is okay to perform the following on a suspended AudioContext because
+ // this method gets called from ExecutionContext::ContextDestroyed() meaning
+ // the AudioContext is already unreachable from the user code.
+ if (ContextState() != kClosed) {
destination()->GetAudioDestinationHandler().StopRendering();
SetContextState(kClosed);
GetDeferredTaskHandler().ClearHandlersToBeDeleted();
+ keep_alive_.Clear();
}
}

diff --git a/third_party/blink/renderer/modules/webaudio/audio_context.h b/third_party/blink/renderer/modules/webaudio/audio_context.h
index 6e3455921f5a1b81fe8a43d44beecfbd9aa93dc1..d3e521f1291a2f90963199cadba02ae38400858e 100644
--- a/third_party/blink/renderer/modules/webaudio/audio_context.h
+++ b/third_party/blink/renderer/modules/webaudio/audio_context.h
@@ -13,6 +13,7 @@
#include "third_party/blink/renderer/modules/webaudio/audio_context_options.h"
#include "third_party/blink/renderer/modules/webaudio/base_audio_context.h"
#include "third_party/blink/renderer/platform/heap/handle.h"
+#include "third_party/blink/renderer/platform/heap/self_keep_alive.h"

namespace blink {

@@ -133,8 +134,13 @@ class MODULES_EXPORT AudioContext : public BaseAudioContext {
// Record the current autoplay metrics.
void RecordAutoplayMetrics();

+ // Starts rendering via AudioDestinationNode. This sets the self-referencing
+ // pointer to this object.
+ void StartRendering() override;
+
// Called when the context is being closed to stop rendering audio and clean
- // up handlers.
+ // up handlers. This clears the self-referencing pointer, making this object
+ // available for the potential GC.
void StopRendering();

// Called when suspending the context to stop reundering audio, but don't
@@ -196,6 +202,8 @@ class MODULES_EXPORT AudioContext : public BaseAudioContext {
// determine audibility on render quantum boundaries, so counting quanta is
// all that's needed.
size_t total_audible_renders_ = 0;
+
+ SelfKeepAlive<AudioContext> keep_alive_;
};

} // namespace blink
diff --git a/third_party/blink/renderer/modules/webaudio/base_audio_context.h b/third_party/blink/renderer/modules/webaudio/base_audio_context.h
index a99b2dddad44416d8761335f1111c441be79c486..051890e2742344d3ed2c8b6ae3b0c384eef252a0 100644
--- a/third_party/blink/renderer/modules/webaudio/base_audio_context.h
+++ b/third_party/blink/renderer/modules/webaudio/base_audio_context.h
@@ -285,7 +285,7 @@ class MODULES_EXPORT BaseAudioContext

DEFINE_ATTRIBUTE_EVENT_LISTENER(statechange, kStatechange)

- void StartRendering();
+ virtual void StartRendering();

void NotifyStateChange();

@@ -0,0 +1,80 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Raymond Toy <rtoy@chromium.org>
Date: Wed, 11 Dec 2019 00:33:00 +0000
Subject: When suspending context, don't clear handlers

AudioContext.suspend() would call StopRendering(). This stops the audio
thread from pulling the graph (eventually) but it also clears out any
handlers, including those associated with automatic pull nodes for any
AnalyserNode that isn't connected to the destination. When the context
is resumed, the AnalyserNode isn't pulled anymore, so the output never
changes.

Add a SuspendRendering() method to handle AudioContext.suspend() which
doesn't clear the handlers. Then when the context is resumed,
AnalyserNodes will get pulled again. Then StopRendering() is used only
for AudioContext.close() where it is ok to clear out the handlers since
we can't resume a closed context.

Bug: 1018499
Change-Id: I4b4ccf688b37e6b81d310d2596cfff9603048876
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1903894
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Raymond Toy <rtoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#723609}

diff --git a/third_party/blink/renderer/modules/webaudio/audio_context.cc b/third_party/blink/renderer/modules/webaudio/audio_context.cc
index 063475274c1686ddef524fc48f7d73e6987e12ac..f544a4658f31632b85cea8f3f2939e3760b0dfb5 100644
--- a/third_party/blink/renderer/modules/webaudio/audio_context.cc
+++ b/third_party/blink/renderer/modules/webaudio/audio_context.cc
@@ -214,7 +214,7 @@ ScriptPromise AudioContext::suspendContext(ScriptState* script_state) {

// Stop rendering now.
if (destination())
- StopRendering();
+ SuspendRendering();

// Since we don't have any way of knowing when the hardware actually stops,
// we'll just resolve the promise now.
@@ -364,11 +364,21 @@ void AudioContext::StopRendering() {

if (ContextState() == kRunning) {
destination()->GetAudioDestinationHandler().StopRendering();
- SetContextState(kSuspended);
+ SetContextState(kClosed);
GetDeferredTaskHandler().ClearHandlersToBeDeleted();
}
}

+void AudioContext::SuspendRendering() {
+ DCHECK(IsMainThread());
+ DCHECK(destination());
+
+ if (ContextState() == kRunning) {
+ destination()->GetAudioDestinationHandler().StopRendering();
+ SetContextState(kSuspended);
+ }
+}
+
double AudioContext::baseLatency() const {
DCHECK(IsMainThread());
DCHECK(destination());
diff --git a/third_party/blink/renderer/modules/webaudio/audio_context.h b/third_party/blink/renderer/modules/webaudio/audio_context.h
index 013eee567252753863de1ecaa1664e8051941f8c..6e3455921f5a1b81fe8a43d44beecfbd9aa93dc1 100644
--- a/third_party/blink/renderer/modules/webaudio/audio_context.h
+++ b/third_party/blink/renderer/modules/webaudio/audio_context.h
@@ -133,8 +133,14 @@ class MODULES_EXPORT AudioContext : public BaseAudioContext {
// Record the current autoplay metrics.
void RecordAutoplayMetrics();

+ // Called when the context is being closed to stop rendering audio and clean
+ // up handlers.
void StopRendering();

+ // Called when suspending the context to stop reundering audio, but don't
+ // clean up handlers because we expect to be resuming where we left off.
+ void SuspendRendering();
+
void DidClose();

// Called by the audio thread to handle Promises for resume() and suspend(),