Skip to content

Commit

Permalink
chore: cherry-pick 72ee7c437c88 from chromium (#25242)
Browse files Browse the repository at this point in the history
  • Loading branch information
nornagon committed Sep 2, 2020
1 parent 8a69889 commit 88898ac
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 0 deletions.
1 change: 1 addition & 0 deletions patches/chromium/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ cherry-pick-9d100199c92b.patch
cherry-pick-bee371eeaf66.patch
cherry-pick-9746a4cde14a.patch
avoid_loading_dri_via_gbm_when_gpumemorybuffers_are_disabled.patch
cherry-pick-72ee7c437c88.patch
cherry-pick-9ad8c9610d0a.patch
indexeddb_fix_crash_in_webidbgetdbnamescallbacksimpl.patch
indexeddb_reset_async_tasks_in_webidbgetdbnamescallbacksimpl.patch
Expand Down
83 changes: 83 additions & 0 deletions patches/chromium/cherry-pick-72ee7c437c88.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri, 7 Aug 2020 15:27:06 +0000
Subject: Worker: Fix a race condition on task runner handling

WebSharedWorkerImpl accesses WorkerScheduler from the main thread to
take a task runner, and then dispatches a connect event to
SharedWorkerGlobalScope using the task runner.

This causes a race condition if close() is called on the global scope
on the worker thread while the task runner is being taken on the main
thread: close() call disposes of WorkerScheduler, and accessing the
scheduler after that is not allowed. See the issue for details.

To fix this, this CL makes WebSharedWorkerImpl capture the task runner
between starting a worker thread (initializing WorkerScheduler) and
posting a task to evaluate worker scripts that may call close(). This
ensures that WebSharedWorkerImpl accesses WorkerScheduler before the
scheduler is disposed of.

(cherry picked from commit c7bbec3e595c4359e36e5472b7265c4b6d047f2c)

Bug: 1104046
Change-Id: I145cd39f706019c33220fcb01ed81f76963ffff0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2308550
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#790284}
Tbr: bashi@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2342337
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/branch-heads/4147@{#1050}
Cr-Branched-From: 16307825352720ae04d898f37efa5449ad68b606-refs/heads/master@{#768962}

diff --git a/third_party/blink/renderer/core/exported/web_shared_worker_impl.cc b/third_party/blink/renderer/core/exported/web_shared_worker_impl.cc
index 7582cdbd3a1ee5e8a58686715020f304319860d8..9942e60d41f4add60bf28388902e08bc1d071dc7 100644
--- a/third_party/blink/renderer/core/exported/web_shared_worker_impl.cc
+++ b/third_party/blink/renderer/core/exported/web_shared_worker_impl.cc
@@ -145,11 +145,8 @@ void WebSharedWorkerImpl::Connect(MessagePortChannel web_channel) {
DCHECK(IsMainThread());
if (asked_to_terminate_)
return;
- // The HTML spec requires to queue a connect event using the DOM manipulation
- // task source.
- // https://html.spec.whatwg.org/C/#shared-workers-and-the-sharedworker-interface
PostCrossThreadTask(
- *GetWorkerThread()->GetTaskRunner(TaskType::kDOMManipulation), FROM_HERE,
+ *task_runner_for_connect_event_, FROM_HERE,
CrossThreadBindOnce(&WebSharedWorkerImpl::ConnectTaskOnWorkerThread,
WTF::CrossThreadUnretained(this),
WTF::Passed(std::move(web_channel))));
@@ -269,6 +266,18 @@ void WebSharedWorkerImpl::StartWorkerContext(

GetWorkerThread()->Start(std::move(creation_params), thread_startup_data,
std::move(devtools_params));
+
+ // Capture the task runner for dispatching connect events. This is necessary
+ // for avoiding race condition with WorkerScheduler termination induced by
+ // close() call on SharedWorkerGlobalScope. See https://crbug.com/1104046 for
+ // details.
+ //
+ // The HTML spec requires to queue a connect event using the DOM manipulation
+ // task source.
+ // https://html.spec.whatwg.org/C/#shared-workers-and-the-sharedworker-interface
+ task_runner_for_connect_event_ =
+ GetWorkerThread()->GetTaskRunner(TaskType::kDOMManipulation);
+
switch (script_type) {
case mojom::ScriptType::kClassic:
GetWorkerThread()->FetchAndRunClassicScript(
diff --git a/third_party/blink/renderer/core/exported/web_shared_worker_impl.h b/third_party/blink/renderer/core/exported/web_shared_worker_impl.h
index 13006f67bd3e10b8d6c38e504d4bf235aa2f9022..98374fb9031e200a225e6e2982ff2d364fa007f5 100644
--- a/third_party/blink/renderer/core/exported/web_shared_worker_impl.h
+++ b/third_party/blink/renderer/core/exported/web_shared_worker_impl.h
@@ -114,6 +114,8 @@ class CORE_EXPORT WebSharedWorkerImpl final : public WebSharedWorker {
// |client_| owns |this|.
WebSharedWorkerClient* client_;

+ scoped_refptr<base::SingleThreadTaskRunner> task_runner_for_connect_event_;
+
bool asked_to_terminate_ = false;

base::WeakPtrFactory<WebSharedWorkerImpl> weak_ptr_factory_{this};

0 comments on commit 88898ac

Please sign in to comment.