Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick ac4785387fff from chromium
- Loading branch information
Showing
2 changed files
with
287 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,286 @@ | ||
From ac4785387fff2efedef9fe784b5cb98bc1fecd04 Mon Sep 17 00:00:00 2001 | ||
From: Dave Tapuska <dtapuska@chromium.org> | ||
Date: Thu, 03 Nov 2022 23:16:16 +0000 | ||
Subject: [PATCH] [m106] 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 c54d0a9..dbe40e69 100644 | ||
--- a/third_party/blink/renderer/core/workers/threaded_worklet_test.cc | ||
+++ b/third_party/blink/renderer/core/workers/threaded_worklet_test.cc | ||
@@ -234,6 +234,7 @@ | ||
|
||
private: | ||
friend class ThreadedWorkletTest; | ||
+ FRIEND_TEST_ALL_PREFIXES(ThreadedWorkletTest, NestedRunLoopTermination); | ||
|
||
std::unique_ptr<WorkerThread> CreateWorkerThread() final { | ||
return std::make_unique<ThreadedWorkletThreadForTest>(WorkletObjectProxy()); | ||
@@ -280,6 +281,16 @@ | ||
} | ||
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_; | ||
@@ -403,4 +414,40 @@ | ||
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 20d6741..156d8d8 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/lock.h" | ||
#include "base/synchronization/waitable_event.h" | ||
@@ -597,6 +596,7 @@ | ||
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"); | ||
@@ -738,11 +738,13 @@ | ||
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(); | ||
|
||
if (WorkerThreadDebugger* debugger = WorkerThreadDebugger::From(GetIsolate())) | ||
debugger->WorkerThreadDestroyed(this); | ||
@@ -887,8 +889,7 @@ | ||
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 | ||
@@ -896,13 +897,20 @@ | ||
// 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 7254460e..298c36e 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/lock.h" | ||
#include "base/synchronization/waitable_event.h" | ||
#include "base/task/single_thread_task_runner.h" | ||
@@ -82,7 +83,7 @@ | ||
// 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 | ||
@@ -105,7 +106,7 @@ | ||
~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 | ||
@@ -117,14 +118,14 @@ | ||
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> | ||
@@ -136,7 +137,7 @@ | ||
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> | ||
@@ -158,7 +159,7 @@ | ||
// 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. | ||
@@ -185,7 +186,7 @@ | ||
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_; | ||
} | ||
@@ -330,7 +331,7 @@ | ||
// 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(lock_); | ||
|
||
// These are called in this order during worker thread startup. | ||
@@ -417,7 +418,7 @@ | ||
// 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(lock_) = false; | ||
|
||
ThreadState thread_state_ GUARDED_BY(lock_) = ThreadState::kNotStarted; | ||
@@ -451,7 +452,7 @@ | ||
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. | ||
base::Lock lock_; | ||
@@ -460,6 +461,10 @@ | ||
// 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; | ||
@@ -485,6 +490,12 @@ | ||
// a pointer to a member in this list. | ||
HashSet<std::unique_ptr<InterruptData>> pending_interrupts_ GUARDED_BY(lock_); | ||
|
||
+ // 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_); | ||
}; | ||
|