Skip to content

Commit

Permalink
Merge branch '19-x-y' into cherry-pick/19-x-y/v8/2ac0620a5bbb
Browse files Browse the repository at this point in the history
  • Loading branch information
jkleinsc committed Nov 10, 2022
2 parents 8569055 + d4a9ee7 commit f01f15f
Show file tree
Hide file tree
Showing 5 changed files with 501 additions and 0 deletions.
2 changes: 2 additions & 0 deletions patches/chromium/.patches
Expand Up @@ -154,3 +154,5 @@ cherry-pick-d5ffb4dd4112.patch
cherry-pick-06c87f9f42ff.patch
refresh_cached_attributes_before_name_computation_traversal.patch
review_add_clear_children_checks_during_accname_traversal.patch
cherry-pick-a1cbf05b4163.patch
cherry-pick-ac4785387fff.patch
102 changes: 102 additions & 0 deletions patches/chromium/cherry-pick-a1cbf05b4163.patch
@@ -0,0 +1,102 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri, 4 Nov 2022 22:06:47 +0000
Subject: webcodecs: Fix race in VE.isConfigSupported() promise resolution

If the context is destroyed before VideoEncoder calls `done_callback`, bad
things can happen. That's why we extract a callback runner before doing
anything asynchronous. Since we hold a ref-counted pointer to the
runner it should be safe now.

(cherry picked from commit 2acf28478008315f302fd52b571623e784be707b)

Bug: 1375059
Change-Id: I984ab27e03e50bd5ae4bf0eb13431834b14f89b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3965544
Commit-Queue: Eugene Zemtsov <eugene@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1061737}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4005574
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/branch-heads/5249@{#911}
Cr-Branched-From: 4f7bea5de862aaa52e6bde5920755a9ef9db120b-refs/heads/main@{#1036826}

diff --git a/third_party/blink/renderer/modules/webcodecs/video_encoder.cc b/third_party/blink/renderer/modules/webcodecs/video_encoder.cc
index 291c18fc9b8f4fbb869e580843403818b44f2d43..ed7b3551fc9ddb5768fec46833363806129bd505 100644
--- a/third_party/blink/renderer/modules/webcodecs/video_encoder.cc
+++ b/third_party/blink/renderer/modules/webcodecs/video_encoder.cc
@@ -68,6 +68,7 @@
#include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
#include "third_party/blink/renderer/platform/scheduler/public/post_cross_thread_task.h"
#include "third_party/blink/renderer/platform/scheduler/public/thread.h"
+#include "third_party/blink/renderer/platform/wtf/cross_thread_copier_base.h"
#include "third_party/blink/renderer/platform/wtf/cross_thread_copier_std.h"
#include "third_party/blink/renderer/platform/wtf/cross_thread_functional.h"

@@ -100,16 +101,6 @@ namespace {
constexpr const char kCategory[] = "media";
constexpr int kMaxActiveEncodes = 5;

-// Use this function in cases when we can't immediately delete |ptr| because
-// there might be its methods on the call stack.
-template <typename T>
-void DeleteLater(ScriptState* state, std::unique_ptr<T> ptr) {
- DCHECK(state->ContextIsValid());
- auto* context = ExecutionContext::From(state);
- auto runner = context->GetTaskRunner(TaskType::kInternalDefault);
- runner->DeleteSoon(FROM_HERE, std::move(ptr));
-}
-
bool IsAcceleratedConfigurationSupported(
media::VideoCodecProfile profile,
const media::VideoEncoder::Options& options,
@@ -988,6 +979,7 @@ void VideoEncoder::ResetInternal() {
}

static void isConfigSupportedWithSoftwareOnly(
+ ScriptState* script_state,
ScriptPromiseResolver* resolver,
VideoEncoderSupport* support,
VideoEncoderTraits::ParsedConfig* config) {
@@ -1012,22 +1004,25 @@ static void isConfigSupportedWithSoftwareOnly(
return;
}

- auto done_callback = [](std::unique_ptr<media::VideoEncoder> sw_encoder,
+ auto done_callback = [](std::unique_ptr<media::VideoEncoder> encoder,
ScriptPromiseResolver* resolver,
+ scoped_refptr<base::SingleThreadTaskRunner> runner,
VideoEncoderSupport* support,
media::EncoderStatus status) {
support->setSupported(status.is_ok());
resolver->Resolve(support);
- DeleteLater(resolver->GetScriptState(), std::move(sw_encoder));
+ runner->DeleteSoon(FROM_HERE, std::move(encoder));
};

+ auto* context = ExecutionContext::From(script_state);
+ auto runner = context->GetTaskRunner(TaskType::kInternalDefault);
auto* software_encoder_raw = software_encoder.get();
software_encoder_raw->Initialize(
config->profile, config->options, base::DoNothing(),
- ConvertToBaseOnceCallback(
- CrossThreadBindOnce(done_callback, std::move(software_encoder),
- WrapCrossThreadPersistent(resolver),
- WrapCrossThreadPersistent(support))));
+ ConvertToBaseOnceCallback(CrossThreadBindOnce(
+ done_callback, std::move(software_encoder),
+ WrapCrossThreadPersistent(resolver), std::move(runner),
+ WrapCrossThreadPersistent(support))));
}

static void isConfigSupportedWithHardwareOnly(
@@ -1114,7 +1109,8 @@ ScriptPromise VideoEncoder::isConfigSupported(ScriptState* script_state,
promises.push_back(resolver->Promise());
auto* support = VideoEncoderSupport::Create();
support->setConfig(config_copy);
- isConfigSupportedWithSoftwareOnly(resolver, support, parsed_config);
+ isConfigSupportedWithSoftwareOnly(script_state, resolver, support,
+ parsed_config);
}

// Wait for all |promises| to resolve and check if any of them have
285 changes: 285 additions & 0 deletions patches/chromium/cherry-pick-ac4785387fff.patch
@@ -0,0 +1,285 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Dave Tapuska <dtapuska@chromium.org>
Date: Thu, 3 Nov 2022 23:16:16 +0000
Subject: Fix PauseOrFreezeOnWorkerThread with nested Worklets.

Worklets can use the same backing thread which means we can have
nested WorkerThreads paused. If a parent WorkerThread gets deallocated
make sure we don't access anything after it is deallocated once the
nested event queue is released.

BUG=1372695

(cherry picked from commit ff5696ba4bc0f8782e3de40e04685507d9f17fd2)

Change-Id: I176b8f750da5a41d19d1b3a623944d9a2ed4a441
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3953152
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1059485}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4004562
Cr-Commit-Position: refs/branch-heads/5249@{#906}
Cr-Branched-From: 4f7bea5de862aaa52e6bde5920755a9ef9db120b-refs/heads/main@{#1036826}

diff --git a/third_party/blink/renderer/core/workers/threaded_worklet_test.cc b/third_party/blink/renderer/core/workers/threaded_worklet_test.cc
index 80fc6e758afbfebedfbb0303854366d2c13d6bc0..5cc0138609f1944f03500b75d40634d1a7e946fa 100644
--- a/third_party/blink/renderer/core/workers/threaded_worklet_test.cc
+++ b/third_party/blink/renderer/core/workers/threaded_worklet_test.cc
@@ -235,6 +235,7 @@ class ThreadedWorkletMessagingProxyForTest

private:
friend class ThreadedWorkletTest;
+ FRIEND_TEST_ALL_PREFIXES(ThreadedWorkletTest, NestedRunLoopTermination);

std::unique_ptr<WorkerThread> CreateWorkerThread() final {
return std::make_unique<ThreadedWorkletThreadForTest>(WorkletObjectProxy());
@@ -281,6 +282,16 @@ class ThreadedWorkletTest : public testing::Test {
}
Document& GetDocument() { return page_->GetDocument(); }

+ void WaitForReady(WorkerThread* worker_thread) {
+ base::WaitableEvent child_waitable;
+ PostCrossThreadTask(
+ *worker_thread->GetTaskRunner(TaskType::kInternalTest), FROM_HERE,
+ CrossThreadBindOnce(&base::WaitableEvent::Signal,
+ CrossThreadUnretained(&child_waitable)));
+
+ child_waitable.Wait();
+ }
+
private:
std::unique_ptr<DummyPageHolder> page_;
Persistent<ThreadedWorkletMessagingProxyForTest> messaging_proxy_;
@@ -404,4 +415,40 @@ TEST_F(ThreadedWorkletTest, TaskRunner) {
test::EnterRunLoop();
}

+TEST_F(ThreadedWorkletTest, NestedRunLoopTermination) {
+ MessagingProxy()->Start();
+
+ ThreadedWorkletMessagingProxyForTest* second_messaging_proxy =
+ MakeGarbageCollected<ThreadedWorkletMessagingProxyForTest>(
+ GetExecutionContext());
+
+ // Get a nested event loop where the first one is on the stack
+ // and the second is still alive.
+ second_messaging_proxy->Start();
+
+ // Wait until the workers are setup and ready to accept work before we
+ // pause them.
+ WaitForReady(GetWorkerThread());
+ WaitForReady(second_messaging_proxy->GetWorkerThread());
+
+ // Pause the second worker, then the first.
+ second_messaging_proxy->GetWorkerThread()->Pause();
+ GetWorkerThread()->Pause();
+
+ // Resume then terminate the second worker.
+ second_messaging_proxy->GetWorkerThread()->Resume();
+ second_messaging_proxy->GetWorkerThread()->Terminate();
+ second_messaging_proxy = nullptr;
+
+ // Now resume the first worker.
+ GetWorkerThread()->Resume();
+
+ // Make sure execution still works without crashing.
+ PostCrossThreadTask(
+ *GetWorkerThread()->GetTaskRunner(TaskType::kInternalTest), FROM_HERE,
+ CrossThreadBindOnce(&ThreadedWorkletThreadForTest::TestTaskRunner,
+ CrossThreadUnretained(GetWorkerThread())));
+ test::EnterRunLoop();
+}
+
} // namespace blink
diff --git a/third_party/blink/renderer/core/workers/worker_thread.cc b/third_party/blink/renderer/core/workers/worker_thread.cc
index 892b3e58443f1a82a6a41c6d52df7d2d701b377d..3a98c2d192e1e47d1586d1dab1fc9e9dc9daf83b 100644
--- a/third_party/blink/renderer/core/workers/worker_thread.cc
+++ b/third_party/blink/renderer/core/workers/worker_thread.cc
@@ -30,7 +30,6 @@
#include <memory>
#include <utility>

-#include "base/auto_reset.h"
#include "base/metrics/histogram_functions.h"
#include "base/synchronization/waitable_event.h"
#include "base/threading/thread_restrictions.h"
@@ -593,6 +592,7 @@ void WorkerThread::InitializeOnWorkerThread(
const absl::optional<WorkerBackingThreadStartupData>& thread_startup_data,
std::unique_ptr<WorkerDevToolsParams> devtools_params) {
DCHECK(IsCurrentThread());
+ backing_thread_weak_factory_.emplace(this);
worker_reporting_proxy_.WillInitializeWorkerContext();
{
TRACE_EVENT0("blink.worker", "WorkerThread::InitializeWorkerContext");
@@ -728,11 +728,13 @@ void WorkerThread::PrepareForShutdownOnWorkerThread() {
SetThreadState(ThreadState::kReadyToShutdown);
}

+ backing_thread_weak_factory_ = absl::nullopt;
if (pause_or_freeze_count_ > 0) {
DCHECK(nested_runner_);
pause_or_freeze_count_ = 0;
nested_runner_->QuitNow();
}
+ pause_handle_.reset();

{
v8::HandleScope handle_scope(GetIsolate());
@@ -883,8 +885,7 @@ void WorkerThread::PauseOrFreezeOnWorkerThread(
if (pause_or_freeze_count_ > 1)
return;

- std::unique_ptr<scheduler::WorkerScheduler::PauseHandle> pause_handle =
- GetScheduler()->Pause();
+ pause_handle_ = GetScheduler()->Pause();
{
// Since the nested message loop runner needs to be created and destroyed on
// the same thread we allocate and destroy a new message loop runner each
@@ -892,13 +893,20 @@ void WorkerThread::PauseOrFreezeOnWorkerThread(
// the worker thread such that the resume/terminate can quit this runner.
std::unique_ptr<Platform::NestedMessageLoopRunner> nested_runner =
Platform::Current()->CreateNestedMessageLoopRunner();
- base::AutoReset<Platform::NestedMessageLoopRunner*> nested_runner_autoreset(
- &nested_runner_, nested_runner.get());
+ auto weak_this = backing_thread_weak_factory_->GetWeakPtr();
+ nested_runner_ = nested_runner.get();
nested_runner->Run();
+
+ // Careful `this` may be destroyed.
+ if (!weak_this) {
+ return;
+ }
+ nested_runner_ = nullptr;
}
GlobalScope()->SetDefersLoadingForResourceFetchers(LoaderFreezeMode::kNone);
GlobalScope()->SetIsInBackForwardCache(false);
GlobalScope()->SetLifecycleState(mojom::blink::FrameLifecycleState::kRunning);
+ pause_handle_.reset();
}

void WorkerThread::ResumeOnWorkerThread() {
diff --git a/third_party/blink/renderer/core/workers/worker_thread.h b/third_party/blink/renderer/core/workers/worker_thread.h
index 89f75cbfba55c2760a018e99c4746c2e09b83c1b..b8b93f827d1892886fde662bf660d43f7bd646e3 100644
--- a/third_party/blink/renderer/core/workers/worker_thread.h
+++ b/third_party/blink/renderer/core/workers/worker_thread.h
@@ -31,6 +31,7 @@

#include "base/gtest_prod_util.h"
#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
#include "base/synchronization/waitable_event.h"
#include "base/task/single_thread_task_runner.h"
#include "base/thread_annotations.h"
@@ -78,7 +79,7 @@ struct WorkerMainScriptLoadParameters;
// abstract class. Multiple WorkerThreads may share one WorkerBackingThread for
// worklets.
//
-// WorkerThread start and termination must be initiated on the main thread and
+// WorkerThread start and termination must be initiated on the parent thread and
// an actual task is executed on the worker thread.
//
// When termination starts, (debugger) tasks on WorkerThread are handled as
@@ -101,7 +102,7 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
~WorkerThread() override;

// Starts the underlying thread and creates the global scope. Called on the
- // main thread.
+ // parent thread.
// Startup data for WorkerBackingThread is absl::nullopt if |this| doesn't own
// the underlying WorkerBackingThread.
// TODO(nhiroki): We could separate WorkerBackingThread initialization from
@@ -113,14 +114,14 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
std::unique_ptr<WorkerDevToolsParams>);

// Posts a task to evaluate a top-level classic script on the worker thread.
- // Called on the main thread after Start().
+ // Called on the parent thread after Start().
void EvaluateClassicScript(const KURL& script_url,
const String& source_code,
std::unique_ptr<Vector<uint8_t>> cached_meta_data,
const v8_inspector::V8StackTraceId& stack_id);

// Posts a task to fetch and run a top-level classic script on the worker
- // thread. Called on the main thread after Start().
+ // thread. Called on the parent thread after Start().
void FetchAndRunClassicScript(
const KURL& script_url,
std::unique_ptr<WorkerMainScriptLoadParameters>
@@ -131,7 +132,7 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
const v8_inspector::V8StackTraceId& stack_id);

// Posts a task to fetch and run a top-level module script on the worker
- // thread. Called on the main thread after Start().
+ // thread. Called on the parent thread after Start().
void FetchAndRunModuleScript(
const KURL& script_url,
std::unique_ptr<WorkerMainScriptLoadParameters>
@@ -152,7 +153,7 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
// Terminates the worker thread. Subclasses of WorkerThread can override this
// to do cleanup. The default behavior is to call Terminate() and
// synchronously call EnsureScriptExecutionTerminates() to ensure the thread
- // is quickly terminated. Called on the main thread.
+ // is quickly terminated. Called on the parent thread.
virtual void TerminateForTesting();

// Thread::TaskObserver.
@@ -179,7 +180,7 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
void DebuggerTaskStarted();
void DebuggerTaskFinished();

- // Callable on both the main thread and the worker thread.
+ // Callable on both the parent thread and the worker thread.
const base::UnguessableToken& GetDevToolsWorkerToken() const {
return devtools_worker_token_;
}
@@ -323,7 +324,7 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
// already shutting down. Does not terminate if a debugger task is running,
// because the debugger task is guaranteed to finish and it heavily uses V8
// API calls which would crash after forcible script termination. Called on
- // the main thread.
+ // the parent thread.
void EnsureScriptExecutionTerminates(ExitCode) LOCKS_EXCLUDED(mutex_);

// These are called in this order during worker thread startup.
@@ -408,7 +409,7 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
// A unique identifier among all WorkerThreads.
const int worker_thread_id_;

- // Set on the main thread.
+ // Set on the parent thread.
bool requested_to_terminate_ GUARDED_BY(mutex_) = false;

ThreadState thread_state_ GUARDED_BY(mutex_) = ThreadState::kNotStarted;
@@ -442,7 +443,7 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
TaskTypeTraits>;
TaskRunnerHashMap worker_task_runners_;

- // This lock protects shared states between the main thread and the worker
+ // This lock protects shared states between the parent thread and the worker
// thread. See thread-safety annotations (e.g., GUARDED_BY) in this header
// file.
Mutex mutex_;
@@ -451,6 +452,10 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
// only on the worker thread.
int pause_or_freeze_count_ = 0;

+ // The `PauseHandle` needs to be destroyed before the scheduler is destroyed
+ // otherwise we will hit a DCHECK.
+ std::unique_ptr<scheduler::WorkerScheduler::PauseHandle> pause_handle_;
+
// A nested message loop for handling pausing. Pointer is not owned. Used only
// on the worker thread.
Platform::NestedMessageLoopRunner* nested_runner_ = nullptr;
@@ -477,6 +482,12 @@ class CORE_EXPORT WorkerThread : public Thread::TaskObserver {
HashSet<std::unique_ptr<InterruptData>> pending_interrupts_
GUARDED_BY(mutex_);

+ // Since the WorkerThread is allocated and deallocated on the parent thread,
+ // we need a WeakPtrFactory that is allocated and cleared on the backing
+ // thread.
+ absl::optional<base::WeakPtrFactory<WorkerThread>>
+ backing_thread_weak_factory_;
+
THREAD_CHECKER(parent_thread_checker_);
};

1 change: 1 addition & 0 deletions patches/v8/.patches
Expand Up @@ -13,3 +13,4 @@ cherry-pick-8b040cb69e96.patch
cherry-pick-194bcc127f21.patch
cherry-pick-ec236fef54b8.patch
cherry-pick-2ac0620a5bbb.patch
cherry-pick-80ed4b917477.patch

0 comments on commit f01f15f

Please sign in to comment.