[Thread-safe] Apply base::Passed to WebThread::Task

WebThread::Task can contain RefPtr to a thread-unsafe-reference-counted object
(e.g. WorkerThreadTask can contain RefPtr to WebKit's StringImpl).
This caused a race condition:

[A] When WebThread::Task::run is executed, more RefPtr's to the refcounted
    object can be created and the reference counter of the object can be
    modified via these RefPtr's (as intended) on the thread where the task
    is executed.
[B] However, base::Closure still retains the ownership of WebThread::Task
    even after WebThread::Task::run is called.
    When base::Closure is deleted, WebThread::Task is deleted and the
    reference counter of the object is decreased by one, possibly from a
    different thread from [A], which is a race condition.

This CL removes the ownership of WebThread::Task from base::Closure after
WebThread::Task::run is executed by using scoped_ptr and base::Passed.
This removes the reference counter modification of [B] and hence removes the
race condition.

BUG=390851

Review URL: https://codereview.chromium.org/807423002

Cr-Commit-Position: refs/heads/master@{#309396}

content/child/webthread_impl.cc

@@ -60,15 +60,43 @@ WebThreadImpl::WebThreadImpl(const char* name)
   thread_->Start();
 }
 
+// RunWebThreadTask takes the ownership of |task| from base::Closure and
+// deletes it on the first invocation of the closure for thread-safety.
+// base::Closure made from RunWebThreadTask is copyable but Closure::Run
+// should be called at most only once.
+// This is because WebThread::Task can contain RefPtr to a
+// thread-unsafe-reference-counted object (e.g. WorkerThreadTask can contain
+// RefPtr to WebKit's StringImpl), and if we don't delete |task| here,
+// it causes a race condition as follows:
+// [A] In task->run(), more RefPtr's to the refcounted object can be created,
+//     and the reference counter of the object can be modified via these
+//     RefPtr's (as intended) on the thread where the task is executed.
+// [B] However, base::Closure still retains the ownership of WebThread::Task
+//     even after RunWebThreadTask is called.
+//     When base::Closure is deleted, WebThread::Task is deleted and the
+//     reference counter of the object is decreased by one, possibly from a
+//     different thread from [A], which is a race condition.
+// Taking the ownership of |task| here by using scoped_ptr and base::Passed
+// removes the reference counter modification of [B] and the race condition.
+// When the closure never runs at all, the corresponding WebThread::Task is
+// destructed when base::Closure is deleted (like [B]). In this case, there
+// are no reference counter modification like [A] (because task->run() is not
+// executed), so there are no race conditions.
+// See https://crbug.com/390851 for more details.
+static void RunWebThreadTask(scoped_ptr<blink::WebThread::Task> task) {
+    task->run();
+}
+
 void WebThreadImpl::postTask(Task* task) {
   thread_->message_loop()->PostTask(
-      FROM_HERE, base::Bind(&blink::WebThread::Task::run, base::Owned(task)));
+      FROM_HERE,
+      base::Bind(RunWebThreadTask, base::Passed(scoped_ptr<Task>(task))));
 }
 
 void WebThreadImpl::postDelayedTask(Task* task, long long delay_ms) {
   thread_->message_loop()->PostDelayedTask(
       FROM_HERE,
-      base::Bind(&blink::WebThread::Task::run, base::Owned(task)),
+      base::Bind(RunWebThreadTask, base::Passed(scoped_ptr<Task>(task))),
       base::TimeDelta::FromMilliseconds(delay_ms));
 }
 
@@ -103,14 +131,15 @@ WebThreadImplForMessageLoop::WebThreadImplForMessageLoop(
 
 void WebThreadImplForMessageLoop::postTask(Task* task) {
   main_thread_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&blink::WebThread::Task::run, base::Owned(task)));
+      FROM_HERE,
+      base::Bind(RunWebThreadTask, base::Passed(make_scoped_ptr(task))));
 }
 
 void WebThreadImplForMessageLoop::postDelayedTask(Task* task,
                                                   long long delay_ms) {
   main_thread_task_runner_->PostDelayedTask(
       FROM_HERE,
-      base::Bind(&blink::WebThread::Task::run, base::Owned(task)),
+      base::Bind(RunWebThreadTask, base::Passed(make_scoped_ptr(task))),
       base::TimeDelta::FromMilliseconds(delay_ms));
 }