Implement SequenceChecker, which is a generalization of ThreadChecker

Implement SequenceChecker, which is a generalization of ThreadChecker

SequenceChecker will be used in WeakPtr instead of ThreadChecker, since
WeakPtr needs only the guarantees of a SequencedTaskRunner.

Add NullTaskRunner implementation and make test_browser_context.cc
use it.

BUG=165590
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174693

[akalin, 2012-12-13 00:56:40]

+rsleevi for review
+darin for review and OWNERS

[Ryan Sleevi, 2012-12-13 22:49:15]

Implementation seems correct, but a question about the unit tests and how GTest
handles them.

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl.cc
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl.cc...
base/sequence_checker_impl.cc:20: sequenced_task_runner_.get() ?
indent pedantry nit:
return sequenced_task_runner_.get() ?
    x :
    y;

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl.h#...
base/sequence_checker_impl.h:36: bool CalledInSequence() const;
nit: I find the naming of this a little confusing. Mostly, the distinction of
"in" vs "on". CalledInSequence feels like it suggests that this is doing some
sort of sequence tasking (eg: 1, 2, 3), rather than being called on the right
STR.

CalledOnValidTaskRunner() ?

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl_un...
File base/sequence_checker_impl_unittest.cc (right):

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl_un...
base/sequence_checker_impl_unittest.cc:100: false));
I didn't think it was safe for GTest to use ADD_FAILURE()-and-friends from a
spawned thread. That is, ISTR it using thread-local errors when checking results
at the end of the test.

PostTaskAndReplyWithResult perhaps? ¯\(°_o)/¯

Or could you just avoid spawning the threads and make the
FakeSequencedTaskRunner just lie?

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl_un...
base/sequence_checker_impl_unittest.cc:136: // Binds to current thread.
Move this comment up a line?

https://codereview.chromium.org/11550031/diff/1/base/test/null_task_runner.h
File base/test/null_task_runner.h (right):

https://codereview.chromium.org/11550031/diff/1/base/test/null_task_runner.h#...
base/test/null_task_runner.h:3: // found in the LICENSE file.
My initial question is, given that this is only used in two places, does it
really need to be in base? Is this just a general dummy mock that could be 'as
easily' implemented in test harnesses?

Just two places using this pattern doesn't really seem to be necessary to put it
in base/...

[akalin, 2012-12-18 22:12:19]

PTAL

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl.cc
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl.cc...
base/sequence_checker_impl.cc:20: sequenced_task_runner_.get() ?
On 2012/12/13 22:49:15, Ryan Sleevi wrote:
> indent pedantry nit:
> return sequenced_task_runner_.get() ?
>     x :
>     y;

Done.

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl.h#...
base/sequence_checker_impl.h:36: bool CalledInSequence() const;
On 2012/12/13 22:49:15, Ryan Sleevi wrote:
> nit: I find the naming of this a little confusing. Mostly, the distinction of
> "in" vs "on". CalledInSequence feels like it suggests that this is doing some
> sort of sequence tasking (eg: 1, 2, 3), rather than being called on the right
> STR.
> 
> CalledOnValidTaskRunner() ?

How about CalledOnValidSequence()?

I'd like to avoid naming the task runner since it can be NULL.

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl_un...
File base/sequence_checker_impl_unittest.cc (right):

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl_un...
base/sequence_checker_impl_unittest.cc:100: false));
On 2012/12/13 22:49:15, Ryan Sleevi wrote:
> I didn't think it was safe for GTest to use ADD_FAILURE()-and-friends from a
> spawned thread. That is, ISTR it using thread-local errors when checking
results
> at the end of the test.
> 
> PostTaskAndReplyWithResult perhaps? ¯\(°_o)/¯
> 
> Or could you just avoid spawning the threads and make the
> FakeSequencedTaskRunner just lie?

ADD_FAILURE() seems to work from other threads.

I know that GTest is technically not thread-safe, but I don't know if it's worth
the trouble to work around it if it works okay in practice.  What do you think?

https://codereview.chromium.org/11550031/diff/1/base/sequence_checker_impl_un...
base/sequence_checker_impl_unittest.cc:136: // Binds to current thread.
On 2012/12/13 22:49:15, Ryan Sleevi wrote:
> Move this comment up a line?

Actually, the comment is in the right place -- the bind happens only when
CalledOnValidSequence() is next called (Like DetachFromThread)

[Ryan Sleevi, 2012-12-21 20:26:47]

lgtm

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
File base/sequence_checker_impl_unittest.cc (right):

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:102: }
I would add one more check here, before exiting, to ensure it's still TRUE for
the current thread.

This is to guarantee that CalledOnValidSequence() from thread 1 does not at all
mutate the state of the STR.

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:106: // it to another one. 
CalledOnValidSequence() should return what its
comment nit: In what will probably prove to be my most pedantic (and obscure)
nit ever, both the Chicago Manual of Style and the AP style book have been
updated to no longer recommend two spaces after a period. Even the Reuters
editorial guidelines (which are typically the slowest to adopt) have updated.

So... one space between sentences.

And don't judge me for this pedantry :)

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:123:
EXPECT_TRUE(sequence_checker_impl.CalledOnValidSequence());
For complete-ness sake in terms of testing, I would add to this test:

sequence_checker_impl.ChangeSequence(fake_sequenced_task_runner1);
EXPECT_FALSE(sequence_checker_impl.CalledOnValidSequence());


That is mostly as a test guard against ChangeSequence being bugged and
automatically flipping things to true whenever it changes.

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:148: 
I'd add an EXPECT_TRUE() here (for the same reasons as line 102), so that you
can ensure that ChangeSequence(NULL) is the one 'truly' doing the unbinding, and
not some side-effect of, say, thread 1 going out of scope.

Pedantry, yes.

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:164: // and switch it to other ones. 
CalledOnValidSequence() should return true
comment nit: "ones" here is a bit weird. You mean "task runners", right?

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:205: }
nit: Add an EXPECT_TRUE here as well (same as line 102)

[akalin, 2012-12-21 20:41:40]

All comments addressed.

+jar for base/ OWNERS
+sky for content/public/test OWNERS

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
File base/sequence_checker_impl_unittest.cc (right):

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:102: }
On 2012/12/21 20:26:47, Ryan Sleevi wrote:
> I would add one more check here, before exiting, to ensure it's still TRUE for
> the current thread.
> 
> This is to guarantee that CalledOnValidSequence() from thread 1 does not at
all
> mutate the state of the STR.

Done.

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:106: // it to another one. 
CalledOnValidSequence() should return what its
On 2012/12/21 20:26:47, Ryan Sleevi wrote:
> comment nit: In what will probably prove to be my most pedantic (and obscure)
> nit ever, both the Chicago Manual of Style and the AP style book have been
> updated to no longer recommend two spaces after a period. Even the Reuters
> editorial guidelines (which are typically the slowest to adopt) have updated.
> 
> So... one space between sentences.
> 
> And don't judge me for this pedantry :)

Done.  Not judged.

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:123:
EXPECT_TRUE(sequence_checker_impl.CalledOnValidSequence());
On 2012/12/21 20:26:47, Ryan Sleevi wrote:
> For complete-ness sake in terms of testing, I would add to this test:
> 
> sequence_checker_impl.ChangeSequence(fake_sequenced_task_runner1);
> EXPECT_FALSE(sequence_checker_impl.CalledOnValidSequence());
> 
> 
> That is mostly as a test guard against ChangeSequence being bugged and
> automatically flipping things to true whenever it changes.

Done.

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:148: 
On 2012/12/21 20:26:47, Ryan Sleevi wrote:
> I'd add an EXPECT_TRUE() here (for the same reasons as line 102), so that you
> can ensure that ChangeSequence(NULL) is the one 'truly' doing the unbinding,
and
> not some side-effect of, say, thread 1 going out of scope.
> 
> Pedantry, yes.

Done.

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:164: // and switch it to other ones. 
CalledOnValidSequence() should return true
On 2012/12/21 20:26:47, Ryan Sleevi wrote:
> comment nit: "ones" here is a bit weird. You mean "task runners", right?

Done.

https://codereview.chromium.org/11550031/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl_unittest.cc:205: }
On 2012/12/21 20:26:47, Ryan Sleevi wrote:
> nit: Add an EXPECT_TRUE here as well (same as line 102)

Done.

[sky, 2012-12-21 21:41:21]

LGTM

[michaeln, 2012-12-21 22:07:31]

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:20:
sequenced_task_runner_->RunsTasksOnCurrentThread() :
Hmmm... as coded right now, the base::Bind closures and parameters are deleted
outside of the scope of IsRunningSequenceOnCurrentThread.

          this_worker->set_running_task_info(
              SequenceToken(task.sequence_token_id), task.shutdown_behavior);

          task.task.Run();

          this_worker->set_running_task_info(
              SequenceToken(), CONTINUE_ON_SHUTDOWN);

          task.task = Closure();  // <---- Closures killed here.

I haven't looked much at your CLs for this, but I think this might be wrinkle in
your intended use of of SequenceChecker? I don't recall exactly why closures are
cleared outside of the scope.

[akalin, 2012-12-21 23:11:51]

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:20:
sequenced_task_runner_->RunsTasksOnCurrentThread() :
On 2012/12/21 22:07:31, michaeln wrote:
> Hmmm... as coded right now, the base::Bind closures and parameters are deleted
> outside of the scope of IsRunningSequenceOnCurrentThread.
> 
>           this_worker->set_running_task_info(
>               SequenceToken(task.sequence_token_id), task.shutdown_behavior);
> 
>           task.task.Run();
> 
>           this_worker->set_running_task_info(
>               SequenceToken(), CONTINUE_ON_SHUTDOWN);
> 
>           task.task = Closure();  // <---- Closures killed here.
> 
> I haven't looked much at your CLs for this, but I think this might be wrinkle
in
> your intended use of of SequenceChecker? I don't recall exactly why closures
are
> cleared outside of the scope.

Hmm that may indeed be a problem.  I'll look into it and fix it in another CL.

[jar (doing other things), 2012-12-21 23:50:41]

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:20:
sequenced_task_runner_->RunsTasksOnCurrentThread() :
It is always good hygiene to do the least possible work within a locked region. 
Especially notable is to avoid calling arbitrary functions from within a locked
region.  A destructor is generally one such example.  Often the pattern is to
take local ownership of a an instance that you are about to delete, release the
lock, and then instigate the destruction. 

On 2012/12/21 22:07:31, michaeln wrote:
> Hmmm... as coded right now, the base::Bind closures and parameters are deleted
> outside of the scope of IsRunningSequenceOnCurrentThread.
> 
>           this_worker->set_running_task_info(
>               SequenceToken(task.sequence_token_id), task.shutdown_behavior);
> 
>           task.task.Run();
> 
>           this_worker->set_running_task_info(
>               SequenceToken(), CONTINUE_ON_SHUTDOWN);
> 
>           task.task = Closure();  // <---- Closures killed here.
> 
> I haven't looked much at your CLs for this, but I think this might be wrinkle
in
> your intended use of of SequenceChecker? I don't recall exactly why closures
are
> cleared outside of the scope.

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.h:49: scoped_refptr<SequencedTaskRunner>
sequenced_task_runner_;
This has me scared that we may impact the lifetime of the task runner.  We need
to look at shutdown and be sure we understand the implications.

I would have expected that you would save a token the moral equivalent of a
thread ID), and verify that the same token is used, and you would not bind to
the task runner itself.

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl_unittest.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl_unittest.cc:97: thread.message_loop()->PostTask(
When do you expect this task to run?  Why do you think it will complete before
this test exits at line 105?

Do you think it will run before the thread is torn down, at line 102?

What did I miss?

[Ryan Sleevi, 2012-12-21 23:55:40]

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.h:49: scoped_refptr<SequencedTaskRunner>
sequenced_task_runner_;
On 2012/12/21 23:50:41, jar wrote:
> This has me scared that we may impact the lifetime of the task runner.  We
need
> to look at shutdown and be sure we understand the implications.
> 
> I would have expected that you would save a token the moral equivalent of a
> thread ID), and verify that the same token is used, and you would not bind to
> the task runner itself.

Jim: This is the safe and recommended approach for dealing with TaskRunners. It
does not affect the lifetime of the task-running portion of the code. Yes, it
will keep "an object" alive, but that object is the moral equivalent to a
MessageLoopProxy, in which the object-kept-alive knows where the task running
part lives.

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl_unittest.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl_unittest.cc:97: thread.message_loop()->PostTask(
On 2012/12/21 23:50:41, jar wrote:
> When do you expect this task to run?  Why do you think it will complete before
> this test exits at line 105?
> 
> Do you think it will run before the thread is torn down, at line 102?
> 
> What did I miss?

base::Thread's destructor (from the 'owning' thread) posts a QuitTask to the
base::Thread and then joins the thread.

By virtue of the PostTask, you know that all tasks posted before the destructor
runs will be executed.

By virtue of joining the thread, you know the thread will be fully torn down
before it reaches line 104.

[akalin, 2012-12-22 00:00:04]

Ryan addressed your other comments pretty much as I would have.

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:20:
sequenced_task_runner_->RunsTasksOnCurrentThread() :
But that entire block of code (that michaeln@ mentioned) is within an AutoUnlock
block.  So I think moving the closure-killing before the set_running_task_info
is pretty safe.

On 2012/12/21 23:50:41, jar wrote:
> It is always good hygiene to do the least possible work within a locked
region. 
> Especially notable is to avoid calling arbitrary functions from within a
locked
> region.  A destructor is generally one such example.  Often the pattern is to
> take local ownership of a an instance that you are about to delete, release
the
> lock, and then instigate the destruction. 
> 
> On 2012/12/21 22:07:31, michaeln wrote:
> > Hmmm... as coded right now, the base::Bind closures and parameters are
deleted
> > outside of the scope of IsRunningSequenceOnCurrentThread.
> > 
> >           this_worker->set_running_task_info(
> >               SequenceToken(task.sequence_token_id),
task.shutdown_behavior);
> > 
> >           task.task.Run();
> > 
> >           this_worker->set_running_task_info(
> >               SequenceToken(), CONTINUE_ON_SHUTDOWN);
> > 
> >           task.task = Closure();  // <---- Closures killed here.
> > 
> > I haven't looked much at your CLs for this, but I think this might be
wrinkle
> in
> > your intended use of of SequenceChecker? I don't recall exactly why closures
> are
> > cleared outside of the scope.
>

[michaeln, 2012-12-22 00:21:53]

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:20:
sequenced_task_runner_->RunsTasksOnCurrentThread() :
This IsRunningSequenceOnCurrentThread went in with this old CL...
  https://chromiumcodereview.appspot.com/9845037

The rationale jim mentioned is probably what I had in mind (even though this
isn't a lock exactly). Also I vaguely remember thinking about refcounted object
writers making assertions about dtor thread and thinking they really shouldn't
be depending on base::Bind closures to drop that ref on the right thread.

I think closure dtor'ing could be moved inside the scope safely if it helps with
WeakPtr instrumentation. Lets ask the trybots.

[jar (doing other things), 2012-12-26 22:39:08]

lgtm

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:20:
sequenced_task_runner_->RunsTasksOnCurrentThread() :
My mistake.  I conflated the comment about *distant* code, with the code above. 
My comments are completely off... the code is releasing the lock before allowing
all these calls to go wild.

On 2012/12/22 00:21:53, michaeln wrote:
> This IsRunningSequenceOnCurrentThread went in with this old CL...
>   https://chromiumcodereview.appspot.com/9845037
> 
> The rationale jim mentioned is probably what I had in mind (even though this
> isn't a lock exactly). Also I vaguely remember thinking about refcounted
object
> writers making assertions about dtor thread and thinking they really shouldn't
> be depending on base::Bind closures to drop that ref on the right thread.
> 
> I think closure dtor'ing could be moved inside the scope safely if it helps
with
> WeakPtr instrumentation. Lets ask the trybots.

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl.h:49: scoped_refptr<SequencedTaskRunner>
sequenced_task_runner_;
On 2012/12/21 23:55:40, Ryan Sleevi wrote:
> On 2012/12/21 23:50:41, jar wrote:
> > This has me scared that we may impact the lifetime of the task runner.  We
> need
> > to look at shutdown and be sure we understand the implications.
> > 
> > I would have expected that you would save a token the moral equivalent of a
> > thread ID), and verify that the same token is used, and you would not bind
to
> > the task runner itself.
> 
> Jim: This is the safe and recommended approach for dealing with TaskRunners.
It
> does not affect the lifetime of the task-running portion of the code. Yes, it
> will keep "an object" alive, but that object is the moral equivalent to a
> MessageLoopProxy, in which the object-kept-alive knows where the task running
> part lives.

Thanks for the explanation.  I misremembered what a SequencedTaskRunner was.  It
is indeed 1:1 with the token I was expecting.

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
File base/sequence_checker_impl_unittest.cc (right):

https://codereview.chromium.org/11550031/diff/21001/base/sequence_checker_imp...
base/sequence_checker_impl_unittest.cc:97: thread.message_loop()->PostTask(
I'm convinced.

On 2012/12/21 23:55:40, Ryan Sleevi wrote:
> On 2012/12/21 23:50:41, jar wrote:
> > When do you expect this task to run?  Why do you think it will complete
before
> > this test exits at line 105?
> > 
> > Do you think it will run before the thread is torn down, at line 102?
> > 
> > What did I miss?
> 
> base::Thread's destructor (from the 'owning' thread) posts a QuitTask to the
> base::Thread and then joins the thread.
> 
> By virtue of the PostTask, you know that all tasks posted before the
destructor
> runs will be executed.
> 
> By virtue of joining the thread, you know the thread will be fully torn down
> before it reaches line 104.

[commit-bot: I haz the power, 2012-12-27 07:11:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/11550031/21001