Make WebTaskRunner::postTask thread safe

third_party/WebKit/Source/platform/WebTaskRunner.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "platform/WebTaskRunner.h"
 
 #include "base/bind_helpers.h"
 #include "base/single_thread_task_runner.h"
 
 namespace base {
@@ skipping 11 matching lines @@
                           const WTF::WeakPtr<blink::TaskHandle::Runner>&,
                           const blink::TaskHandle& handle) {
     return !handle.isActive();
   }
 };
 
 }  // namespace base
 
 namespace blink {
 
+namespace {
+
+void runCrossThreadClosure(std::unique_ptr<CrossThreadClosure> task) {
+  (*task)();
+}
+
+}  // namespace
+
 class TaskHandle::Runner : public WTF::ThreadSafeRefCounted<Runner> {
  public:
   explicit Runner(std::unique_ptr<WTF::Closure> task)
       : m_task(std::move(task)), m_weakPtrFactory(this) {}
 
   WTF::WeakPtr<Runner> asWeakPtr() { return m_weakPtrFactory.createWeakPtr(); }
 
   bool isActive() const { return m_task && !m_task->isCancelled(); }
 
   void cancel() {
@@ skipping 54 matching lines @@
 TaskHandle& TaskHandle::operator=(TaskHandle&& other) {
   TaskHandle tmp(std::move(other));
   m_runner.swap(tmp.m_runner);
   return *this;
 }
 
 TaskHandle::TaskHandle(RefPtr<Runner> runner) : m_runner(std::move(runner)) {
   DCHECK(m_runner);
 }
 
+// Use a custom function for base::Bind instead of convertToBaseCallback to
+// avoid copying the closure later in the call chain. Copying the bound state
+// can lead to data races with ref counted objects like StringImpl. See
+// crbug.com/679915 for more details.

[Wez, 2017/01/11 21:21:49]

Wouldn't it be better to "fix" convertToBaseCallback() to do this wrapping of
the |task| into a Callback?

This issue is caused by moving from a Task with move semantics to a Callback
with copy semantics, so it will also affect other sites where
convertToBaseCallback() is used, e.g. the WTF::Closure postTask() overload
below, right?

 void WebTaskRunner::postTask(const WebTraceLocation& location,
                              std::unique_ptr<CrossThreadClosure> task) {
-  toSingleThreadTaskRunner()->PostTask(location,
-                                       convertToBaseCallback(std::move(task)));
+  toSingleThreadTaskRunner()->PostTask(
+      location, base::Bind(&runCrossThreadClosure, base::Passed(&task)));
 }
 
 void WebTaskRunner::postDelayedTask(const WebTraceLocation& location,
                                     std::unique_ptr<CrossThreadClosure> task,
                                     long long delayMs) {
   toSingleThreadTaskRunner()->PostDelayedTask(
-      location, convertToBaseCallback(std::move(task)),
+      location, base::Bind(&runCrossThreadClosure, base::Passed(&task)),
       base::TimeDelta::FromMilliseconds(delayMs));
 }
 
 void WebTaskRunner::postTask(const WebTraceLocation& location,
                              std::unique_ptr<WTF::Closure> task) {
   toSingleThreadTaskRunner()->PostTask(location,
                                        convertToBaseCallback(std::move(task)));
 }
 
 void WebTaskRunner::postDelayedTask(const WebTraceLocation& location,
@@ skipping 24 matching lines @@
       adoptRef(new TaskHandle::Runner(std::move(task)));
   postDelayedTask(location, WTF::bind(&TaskHandle::Runner::run,
                                       runner->asWeakPtr(), TaskHandle(runner)),
                   delayMs);
   return TaskHandle(runner);
 }
 
 WebTaskRunner::~WebTaskRunner() = default;
 
 }  // namespace blink