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

[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
Committed: https://crrev.com/8446b33c7ea20bd1596f876ab225034b7c5d1162
Cr-Commit-Position: refs/heads/master@{#309396}

[hiroshige, 2014-12-18 07:47:33]

hiroshige@chromium.org changed reviewers:
+ nasko@chromium.org

[hiroshige, 2014-12-18 09:00:45]

PTAL.

[nasko, 2014-12-18 17:53:24]

nasko@chromium.org changed reviewers:
+ dcheng@chromium.org

[nasko, 2014-12-18 17:53:25]

dcheng@ is probably a better reviewer of this CL than myself. I've noted couple
of style nits.

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl.cc
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:64: // Take ownership of blink::WebThread::Task
and delete it on first |run|
nit: Comment should be before the method starts.

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:66: {
Brace goes on the same line as the method params ending.

[dcheng, 2014-12-18 19:31:38]

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl.cc
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:63: static void
RunWebThreadTask(scoped_ptr<blink::WebThread::Task> task)
This code is really subtle. Can we expand this comment to describe why this is
necessary instead of just referencing a bug?

Also, if this task never runs at all, won't it potentially still be destroyed on
the wrong thread?

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:73: base::Bind(RunWebThreadTask,
base::Passed(scoped_ptr<Task>(task))));
It's slightly shorter to use make_scoped_ptr.

[hiroshige, 2014-12-19 09:16:12]

Thank you for reviewing!

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl.cc
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:63: static void
RunWebThreadTask(scoped_ptr<blink::WebThread::Task> task)
Added comments.

> won't it potentially still be destroyed on the wrong thread?
Yes, |WebThread::Task| can be destoyed on any thread, but in that case there is
no race condition, as described in the comment I added.
I think deleting |WebThread::Task| (and thus its parameters including
|StringImpl|) on a different thread from the thread where the task is intended
to be executed is not a problem. (if there is any specific problem please let me
know...)

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:64: // Take ownership of blink::WebThread::Task
and delete it on first |run|
On 2014/12/18 17:53:25, nasko wrote:
> nit: Comment should be before the method starts.

Done.

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:66: {
On 2014/12/18 17:53:25, nasko wrote:
> Brace goes on the same line as the method params ending.

Done.

https://codereview.chromium.org/807423002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:73: base::Bind(RunWebThreadTask,
base::Passed(scoped_ptr<Task>(task))));
On 2014/12/18 19:31:38, dcheng wrote:
> It's slightly shorter to use make_scoped_ptr.

Done.

[dcheng, 2014-12-19 09:20:37]

Thanks for the updated explanations. This looks reasonable to me but you'll need
approval from nasko@.

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

Based on dcheng@'s review, LGTM. I do have a few nits to improve the comment
though.

https://codereview.chromium.org/807423002/diff/20001/content/child/webthread_...
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/807423002/diff/20001/content/child/webthread_...
content/child/webthread_impl.cc:63: // |RunWebThreadTask| takes the ownership of
|task| from |base::Closure| and
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.

https://codereview.chromium.org/807423002/diff/20001/content/child/webthread_...
content/child/webthread_impl.cc:66: // should be at most only once.
"should be at most only once" < What should be at most only once? Copied?

https://codereview.chromium.org/807423002/diff/20001/content/child/webthread_...
content/child/webthread_impl.cc:85: // http://crbug.com/390851
nit: "See ... for more details." and let's use https instead of http.

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

Thanks!

https://codereview.chromium.org/807423002/diff/20001/content/child/webthread_...
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/807423002/diff/20001/content/child/webthread_...
content/child/webthread_impl.cc:63: // |RunWebThreadTask| takes the ownership of
|task| from |base::Closure| and
Done. Also updated the CL description.

https://codereview.chromium.org/807423002/diff/20001/content/child/webthread_...
content/child/webthread_impl.cc:66: // should be at most only once.
Called at most once. Done.

https://codereview.chromium.org/807423002/diff/20001/content/child/webthread_...
content/child/webthread_impl.cc:85: // http://crbug.com/390851
On 2014/12/19 17:22:44, nasko wrote:
> nit: "See ... for more details." and let's use https instead of http.

Done.

[hiroshige, 2014-12-22 04:22:31]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2014-12-22 04:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/807423002/40001

[commit-bot: I haz the power, 2014-12-22 05:45:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-22 05:46:20]

Patchset 3 (id:??) landed as
https://crrev.com/8446b33c7ea20bd1596f876ab225034b7c5d1162
Cr-Commit-Position: refs/heads/master@{#309396}