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

content/child/webthread_impl.cc

Index: content/child/webthread_impl.cc
diff --git a/content/child/webthread_impl.cc b/content/child/webthread_impl.cc
index d29b36cb9878c3a15f4265e9c52182da17778eb8..45058b15badcc207abf9d916ddb75781db81ca84 100644
--- a/content/child/webthread_impl.cc
+++ b/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

[nasko, 2014/12/19 17:22:44]

In general, we use | to surround parameters, not their types. Feel free to use
the type names (such as base::Closure) without using | around them.

[hiroshige, 2014/12/22 04:18:47]

Done. Also updated the CL description.

+// deletes it on the first invocation of the closure for thread-safety.
+// |base::Closure| made from |RunWebThreadTask| is copyable but |Closure::Run|
+// should be at most only once.

[nasko, 2014/12/19 17:22:44]

"should be at most only once" < What should be at most only once? Copied?

[hiroshige, 2014/12/22 04:18:47]

Called at most once. Done.

+// 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.
+// http://crbug.com/390851

[nasko, 2014/12/19 17:22:44]

nit: "See ... for more details." and let's use https instead of http.

[hiroshige, 2014/12/22 04:18:47]

Done.

+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));
 }