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 #23048

Merged
merged 3 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 @@ -111,4 +111,6 @@ mojovideoencodeacceleratorservice_handle_potential_later.patch
speculative_fix_for_crashes_in_filechooserimpl.patch
reland_sequentialise_access_to_callbacks_in.patch
handle_err_cache_race_in_dodoneheadersaddtoentrycomplete.patch
when_suspending_context_don_t_clear_handlers.patch
use_keepselfalive_on_audiocontext_to_keep_it_alive_until_rendering.patch
worker_stop_passing_creator_s_origin_for_starting_a_dedicated_worker.patch
@@ -0,0 +1,125 @@
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 83a1ad5d891ebb5566b41068cc0dc18de3d40059..99dbfb89005270b1a13e988bbdea9b5bfedfe89a 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);

@@ -161,7 +162,7 @@ void AudioContext::Uninitialize() {
DCHECK(IsMainThread());
DCHECK_NE(g_hardware_context_count, 0u);
--g_hardware_context_count;
-
+ StopRendering();
DidClose();
RecordAutoplayMetrics();
BaseAudioContext::Uninitialize();
@@ -344,14 +345,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 146e9e56086642edec99030a8a008accf0d408d9..31730c152a3b3f0e743cf7231a92a51a4d7eed69 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
@@ -193,6 +199,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 d5aadf7e58ea8a67df7d0a83e5ed5b9b1d7772bb..bb0b2335d848c6b5f1b1cb9ce24ad5adb8b331c5 100644
--- a/third_party/blink/renderer/modules/webaudio/base_audio_context.h
+++ b/third_party/blink/renderer/modules/webaudio/base_audio_context.h
@@ -282,7 +282,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 990fe75f1ddd4ebf9a526a9e416d2ee865afca86..83a1ad5d891ebb5566b41068cc0dc18de3d40059 100644
--- a/third_party/blink/renderer/modules/webaudio/audio_context.cc
+++ b/third_party/blink/renderer/modules/webaudio/audio_context.cc
@@ -200,7 +200,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.
@@ -350,11 +350,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 e0d4cf1aef21f06146b309ecd77b7e580aa8ae11..146e9e56086642edec99030a8a008accf0d408d9 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(),