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 aeb6bc551b60 from chromium #28089

Merged
merged 3 commits into from Mar 10, 2021
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
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -129,3 +129,4 @@ merge_to_m88_xproto_switch_event_queue_from_a_std_list_to_a.patch
websocket_don_t_clear_event_queue_on_destruction.patch
cherry-pick-7e0e52df283c.patch
cherry-pick-6ed1c0c425e0.patch
cherry-pick-aeb6bc551b60.patch
114 changes: 114 additions & 0 deletions patches/chromium/cherry-pick-aeb6bc551b60.patch
@@ -0,0 +1,114 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Hongchan Choi <hongchan@chromium.org>
Date: Tue, 23 Feb 2021 23:27:31 +0000
Subject: Prevent accessing shared buffers from audio rendering thread

The shared buffer in ScriptProcessorNode can be accessed by the
audio rendering thread when it is held by the main thread.

The solution suggested here is simply to expand the scope of
the mutex to minimize the code change. This is a deprecated
feature in Web Audio, so making significant changes is not
sensible. By locking the entire scope of Process() call, this
area would be immune to the similar problems in the future.

(cherry picked from commit 60987aa224f369fc0ea38c56e498389440921356)

Bug: 1174582
Test: The repro case doesn't crash on ASAN.
Change-Id: I2b292f94be65e6ec26c6eb0e0ed32b3fb2d88466
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2681193
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: Raymond Toy <rtoy@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#852240}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2715585
Commit-Queue: Krishna Govind <govind@chromium.org>
Reviewed-by: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/4324@{#2238}
Cr-Branched-From: c73b5a651d37a6c4d0b8e3262cc4015a5579c6c8-refs/heads/master@{#827102}

diff --git a/third_party/blink/renderer/modules/webaudio/script_processor_node.cc b/third_party/blink/renderer/modules/webaudio/script_processor_node.cc
index b1ca691b07b53b927a92753906f7f25edebac919..6e80b23a32dd1895a0d51d08ee16c8cb2d44fc55 100644
--- a/third_party/blink/renderer/modules/webaudio/script_processor_node.cc
+++ b/third_party/blink/renderer/modules/webaudio/script_processor_node.cc
@@ -110,6 +110,14 @@ void ScriptProcessorHandler::Initialize() {
}

void ScriptProcessorHandler::Process(uint32_t frames_to_process) {
+ // The main thread might be accessing the shared buffers. If so, silience
+ // the output and return.
+ MutexTryLocker try_locker(process_event_lock_);
+ if (!try_locker.Locked()) {
+ Output(0).Bus()->Zero();
+ return;
+ }
+
// Discussion about inputs and outputs:
// As in other AudioNodes, ScriptProcessorNode uses an AudioBus for its input
// and output (see inputBus and outputBus below). Additionally, there is a
@@ -181,47 +189,26 @@ void ScriptProcessorHandler::Process(uint32_t frames_to_process) {
buffer_read_write_index_ =
(buffer_read_write_index_ + frames_to_process) % BufferSize();

- // m_bufferReadWriteIndex will wrap back around to 0 when the current input
- // and output buffers are full.
- // When this happens, fire an event and swap buffers.
+ // Fire an event and swap buffers when |buffer_read_write_index_| wraps back
+ // around to 0. It means the current input and output buffers are full.
if (!buffer_read_write_index_) {
- // Avoid building up requests on the main thread to fire process events when
- // they're not being handled. This could be a problem if the main thread is
- // very busy doing other things and is being held up handling previous
- // requests. The audio thread can't block on this lock, so we call
- // tryLock() instead.
- MutexTryLocker try_locker(process_event_lock_);
- if (!try_locker.Locked()) {
- // We're late in handling the previous request. The main thread must be
- // very busy. The best we can do is clear out the buffer ourself here.
- shared_output_buffer->Zero();
+ if (Context()->HasRealtimeConstraint()) {
+ // For a realtime context, fire an event and do not wait.
+ PostCrossThreadTask(
+ *task_runner_, FROM_HERE,
+ CrossThreadBindOnce(&ScriptProcessorHandler::FireProcessEvent,
+ AsWeakPtr(), double_buffer_index_));
} else {
- // With the realtime context, execute the script code asynchronously
- // and do not wait.
- if (Context()->HasRealtimeConstraint()) {
- // Fire the event on the main thread with the appropriate buffer
- // index.
- PostCrossThreadTask(
- *task_runner_, FROM_HERE,
- CrossThreadBindOnce(&ScriptProcessorHandler::FireProcessEvent,
- AsWeakPtr(), double_buffer_index_));
- } else {
- // If this node is in the offline audio context, use the
- // waitable event to synchronize to the offline rendering thread.
- std::unique_ptr<base::WaitableEvent> waitable_event =
- std::make_unique<base::WaitableEvent>();
-
- PostCrossThreadTask(
- *task_runner_, FROM_HERE,
- CrossThreadBindOnce(
- &ScriptProcessorHandler::FireProcessEventForOfflineAudioContext,
- AsWeakPtr(), double_buffer_index_,
- CrossThreadUnretained(waitable_event.get())));
-
- // Okay to block the offline audio rendering thread since it is
- // not the actual audio device thread.
- waitable_event->Wait();
- }
+ // For an offline context, wait until the script execution is finished.
+ std::unique_ptr<base::WaitableEvent> waitable_event =
+ std::make_unique<base::WaitableEvent>();
+ PostCrossThreadTask(
+ *task_runner_, FROM_HERE,
+ CrossThreadBindOnce(
+ &ScriptProcessorHandler::FireProcessEventForOfflineAudioContext,
+ AsWeakPtr(), double_buffer_index_,
+ CrossThreadUnretained(waitable_event.get())));
+ waitable_event->Wait();
}

SwapBuffers();