TaskScheduler: Don't delete Tasks in the scope of a Sequence's lock.

TaskScheduler: Don't delete Tasks in the scope of a Sequence's lock.

Currently, Sequence::PopTask() deletes a Task within the scope of
the Sequence's lock. This is bad because:
- A deadlock occurs if the Task's destructor tries to post a
  Task to the same Sequence.
- Multiple locks are held at the same time if the Task's destructor
  tries to post a task to another Sequence.
- The Sequence's lock can't be acquired by another thread while
  the Task's destructor runs (unnecessary contention).

With this CL, Sequence::PopTask() releases its lock before deleting
the popped Task.

BUG=553459
Committed: https://crrev.com/0503a773fdb9a6b7c446560f6f02c70655017cc8
Cr-Commit-Position: refs/heads/master@{#406948}

[fdoray, 2016-07-20 19:21:51]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org, robliao@chromium.org

[fdoray, 2016-07-20 19:21:52]

PTAL

[gab, 2016-07-20 20:34:51]

lgtm w/ english nit :-)

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.cc:39: // Delete the popped task outside the scope
of |lock_|. That prevents a double
s/That/This/

[robliao, 2016-07-20 22:39:00]

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.cc:44: AutoSchedulerLock auto_lock(lock_);
This would be more robust and clearer if AutoSchedulerLock and associated
statements were in its own block. You'd need to store the value of
queue_.empty() in a temporary.

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence_unittest.cc (right):

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence_unittest.cc:36: PushTaskInDestructor&
operator=(PushTaskInDestructor&&) = default;
Group this assignment operator with the constructor above.
https://google.github.io/styleguide/cppguide.html#Declaration_Order

[fdoray, 2016-07-21 13:36:03]

robliao@: PTAnL

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.cc:39: // Delete the popped task outside the scope
of |lock_|. That prevents a double
On 2016/07/20 20:34:51, gab wrote:
> s/That/This/

Done.

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.cc:44: AutoSchedulerLock auto_lock(lock_);
On 2016/07/20 22:39:00, robliao wrote:
> This would be more robust and clearer if AutoSchedulerLock and associated
> statements were in its own block. You'd need to store the value of
> queue_.empty() in a temporary. 

Done.

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence_unittest.cc (right):

https://codereview.chromium.org/2164103002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence_unittest.cc:36: PushTaskInDestructor&
operator=(PushTaskInDestructor&&) = default;
On 2016/07/20 22:39:00, robliao wrote:
> Group this assignment operator with the constructor above.
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

[robliao, 2016-07-21 17:53:18]

lgtm + comment.

https://codereview.chromium.org/2164103002/diff/20001/base/task_scheduler/seq...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/2164103002/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.cc:43: bool sequence_empty_after_pop;
Variables have to be initialized at declaration, so go ahead and set it to
false.
https://google.github.io/styleguide/cppguide.html#Local_Variables

[fdoray, 2016-07-21 17:57:41]

https://codereview.chromium.org/2164103002/diff/20001/base/task_scheduler/seq...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/2164103002/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.cc:43: bool sequence_empty_after_pop;
On 2016/07/21 17:53:18, robliao wrote:
> Variables have to be initialized at declaration, so go ahead and set it to
> false.
> https://google.github.io/styleguide/cppguide.html#Local_Variables

Done.

[fdoray, 2016-07-21 17:57:45]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-07-21 17:57:45]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
robliao@chromium.org
Link to the patchset: https://codereview.chromium.org/2164103002/#ps40001
(title: "CR robliao #6 (initialize variable at declaration)")

[commit-bot: I haz the power, 2016-07-21 17:58:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 21:23:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-21 21:25:50]

Description was changed from

==========
TaskScheduler: Don't delete Tasks in the scope of a Sequence's lock.

Currently, Sequence::PopTask() deletes a Task within the scope of
the Sequence's lock. This is bad because:
- A deadlock occurs if the Task's destructor tries to post a
  Task to the same Sequence.
- Multiple locks are held at the same time if the Task's destructor
  tries to post a task to another Sequence.
- The Sequence's lock can't be acquired by another thread while
  the Task's destructor runs (unnecessary contention).

With this CL, Sequence::PopTask() releases its lock before deleting
the popped Task.

BUG=553459
==========

to

==========
TaskScheduler: Don't delete Tasks in the scope of a Sequence's lock.

Currently, Sequence::PopTask() deletes a Task within the scope of
the Sequence's lock. This is bad because:
- A deadlock occurs if the Task's destructor tries to post a
  Task to the same Sequence.
- Multiple locks are held at the same time if the Task's destructor
  tries to post a task to another Sequence.
- The Sequence's lock can't be acquired by another thread while
  the Task's destructor runs (unnecessary contention).

With this CL, Sequence::PopTask() releases its lock before deleting
the popped Task.

BUG=553459

Committed: https://crrev.com/0503a773fdb9a6b7c446560f6f02c70655017cc8
Cr-Commit-Position: refs/heads/master@{#406948}
==========

[commit-bot: I haz the power, 2016-07-21 21:25:51]

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