Make ThreadChecker::CalledOnValidThread() return true when called from the same task.

Make ThreadChecker::CalledOnValidThread() return true when called from the same task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().

ThreadCheckerImpl uses TaskToken to make CalledOnValidThread() return
true when called multiple times from the same task. This allows usage
of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

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

[fdoray, 2016-08-04 22:28:51]

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

[fdoray, 2016-08-04 22:28:51]

PTAL

[fdoray, 2016-08-05 18:45:15]

Forgot to add some files...

[gab, 2016-08-08 16:34:23]

lg, mostly nits (and request about maybe not renaming everything?)

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
File base/sequence_and_task_token.h (right):

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token.h:5: #ifndef BASE_SEQUENCE_AND_TASK_TOKEN_H_
Since TaskToken is more-or-less an implementation detail (i.e. not created by
users of the API and only used in ThreadCheckerImpl), is it really worth
renaming everything to expose its name in the public API?

It also makes things like ScopedSetSequenceAndTaskTokenForCurrentThread a
mouthful for no added readability IMO.

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token.h:48: // A token that identifies a task.
Expand comment to explain why this is necessary.

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token.h:74: TaskToken(int token) : token_(token) {}
Comment explaining why this is private (and also justifying friend decl)

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token.h:98: friend class TaskToken;
Cyclic-friending is weird, why is this needed?

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
File base/sequence_and_task_token_unittest.cc (right):

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token_unittest.cc:80: // valid and equal with each other,
but not equal to an invalid TaskToken.
"but not equal to an invalid TaskToken" -- isn't this overkill since invalid
tokens aren't equal?

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token_unittest.cc:94: EXPECT_NE(token_b, TaskToken());
rm last two conditions IMO per above comment

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token_unittest.cc:122: EXPECT_NE(token_b, TaskToken());
ditto

https://codereview.chromium.org/2213263002/diff/40001/base/threading/thread_c...
File base/threading/thread_checker_impl.h (right):

https://codereview.chromium.org/2213263002/diff/40001/base/threading/thread_c...
base/threading/thread_checker_impl.h:47: // same task. CalledOnValidThread() may
return true even if the current
Prefix the last sentence with "Note: "

https://codereview.chromium.org/2213263002/diff/40001/base/threading/thread_c...
base/threading/thread_checker_impl.h:47: // same task. CalledOnValidThread() may
return true even if the current
s/same task/same task, even if it's not running in a single-threaded context
itself (allowing usage of ThreadChecker/NonThreadSafe objects on the stack in
the scope of one-off tasks)/

[fdoray, 2016-08-08 17:45:18]

gab@: PTAnL

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
File base/sequence_and_task_token.h (right):

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token.h:5: #ifndef BASE_SEQUENCE_AND_TASK_TOKEN_H_
On 2016/08/08 16:34:22, gab wrote:
> Since TaskToken is more-or-less an implementation detail (i.e. not created by
> users of the API and only used in ThreadCheckerImpl), is it really worth
> renaming everything to expose its name in the public API?
> 
> It also makes things like ScopedSetSequenceAndTaskTokenForCurrentThread a
> mouthful for no added readability IMO.

Done.

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token.h:48: // A token that identifies a task.
On 2016/08/08 16:34:22, gab wrote:
> Expand comment to explain why this is necessary.

Done.

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token.h:74: TaskToken(int token) : token_(token) {}
On 2016/08/08 16:34:22, gab wrote:
> Comment explaining why this is private (and also justifying friend decl)

Done.

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token.h:98: friend class TaskToken;
On 2016/08/08 16:34:22, gab wrote:
> Cyclic-friending is weird, why is this needed?

ScopedSetSequenceTokenForCurrentThread needs to be able to call
TaskToken::Create(). I don't want to allow other callers.

TaskToken::GetForCurrentThread() has to get the current
ScopedSetSequenceTokenForCurrentThread and access it (private) task_token_
member to return it. I agree that this friend declaration wouldn't be needed if
I stored a pointer to the TaskToken and SequenceToken in TLS instead of a
pointer to the ScopedSetSequenceTokenForCurrentThread. I changed the code so
that these friend declarations are no longer needed.

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
File base/sequence_and_task_token_unittest.cc (right):

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token_unittest.cc:80: // valid and equal with each other,
but not equal to an invalid TaskToken.
On 2016/08/08 16:34:22, gab wrote:
> "but not equal to an invalid TaskToken" -- isn't this overkill since invalid
> tokens aren't equal?

Done.

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token_unittest.cc:94: EXPECT_NE(token_b, TaskToken());
On 2016/08/08 16:34:22, gab wrote:
> rm last two conditions IMO per above comment 

Done.

https://codereview.chromium.org/2213263002/diff/40001/base/sequence_and_task_...
base/sequence_and_task_token_unittest.cc:122: EXPECT_NE(token_b, TaskToken());
On 2016/08/08 16:34:22, gab wrote:
> ditto

Done.

https://codereview.chromium.org/2213263002/diff/40001/base/threading/thread_c...
File base/threading/thread_checker_impl.h (right):

https://codereview.chromium.org/2213263002/diff/40001/base/threading/thread_c...
base/threading/thread_checker_impl.h:47: // same task. CalledOnValidThread() may
return true even if the current
On 2016/08/08 16:34:22, gab wrote:
> Prefix the last sentence with "Note: "

Done.

https://codereview.chromium.org/2213263002/diff/40001/base/threading/thread_c...
base/threading/thread_checker_impl.h:47: // same task. CalledOnValidThread() may
return true even if the current
On 2016/08/08 16:34:22, gab wrote:
> s/same task/same task, even if it's not running in a single-threaded context
> itself (allowing usage of ThreadChecker/NonThreadSafe objects on the stack in
> the scope of one-off tasks)/

Done.

[gab, 2016-08-08 17:56:36]

Add to CL description that this allows usage of ThreadChecker/NonThreadSafe
objects from the stack on tasks not otherwise running in a single-threaded
context.

lgtm :-)

https://codereview.chromium.org/2213263002/diff/100001/base/sequence_token.h
File base/sequence_token.h (right):

https://codereview.chromium.org/2213263002/diff/100001/base/sequence_token.h#...
base/sequence_token.h:84: // a ScopedSetSequenceTokenForCurrentThread is
instantiated.
How about "This is private as it only meant to be instantiated by
ScopedSetSequenceTokenForCurrentThread."

[fdoray, 2016-08-08 18:09:17]

Description was changed from

==========
Make ThreadChecker::CalledOnValidThread() return true when called from the same
task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceAndTaskTokenForCurrentThread, a unique TaskToken
is set in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().

This CL also modifies ThreadCheckerImpl so that CalledOnValidThread()
returns true when called from the task from which it was assigned to a
thread.

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

to

==========
Make ThreadChecker::CalledOnValidThread() return true when called from the same
task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().


ThreadCheckerImpl uses TaskToken to allow CalledOnValidThread() to
return true when called multiple times from the same task.  This allows
usage of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

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

[fdoray, 2016-08-08 18:10:51]

Description was changed from

==========
Make ThreadChecker::CalledOnValidThread() return true when called from the same
task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().


ThreadCheckerImpl uses TaskToken to allow CalledOnValidThread() to
return true when called multiple times from the same task.  This allows
usage of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

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

to

==========
Make ThreadChecker::CalledOnValidThread() return true when called from the same
task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().

ThreadCheckerImpl uses TaskToken to make CalledOnValidThread() return
true when called multiple times from the same task. This allows usage
of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

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

[fdoray, 2016-08-08 18:11:34]

fdoray@chromium.org changed reviewers:
+ thestig@chromium.org

[fdoray, 2016-08-08 18:11:35]

thestig@: PTAL

https://codereview.chromium.org/2213263002/diff/100001/base/sequence_token.h
File base/sequence_token.h (right):

https://codereview.chromium.org/2213263002/diff/100001/base/sequence_token.h#...
base/sequence_token.h:84: // a ScopedSetSequenceTokenForCurrentThread is
instantiated.
On 2016/08/08 17:56:35, gab wrote:
> How about "This is private as it only meant to be instantiated by
> ScopedSetSequenceTokenForCurrentThread."

Done.

[Lei Zhang, 2016-08-08 22:36:38]

Looking... Strangely, sequence_token* are marked as 'D' rather than 'M'.

[fdoray, 2016-08-08 22:39:17]

On 2016/08/08 22:36:38, Lei Zhang wrote:
> Looking... Strangely, sequence_token* are marked as 'D' rather than 'M'.

I think it's because I deleted (renamed) these files in the first patch set. For
some reason, the letter in front of the file names hasn't been updated even
though the files are no longer deleted (renamed) in the latest patch set.

[Lei Zhang, 2016-08-08 23:21:26]

lgtm

https://codereview.chromium.org/2213263002/diff/120001/base/sequence_token.cc
File base/sequence_token.cc (right):

https://codereview.chromium.org/2213263002/diff/120001/base/sequence_token.cc...
base/sequence_token.cc:51: return token_ == other.token_ && token_ !=
kInvalidTaskToken;
Just call IsValid() in the latter half?

https://codereview.chromium.org/2213263002/diff/120001/base/sequence_token.h
File base/sequence_token.h (right):

https://codereview.chromium.org/2213263002/diff/120001/base/sequence_token.h#...
base/sequence_token.h:80: TaskToken(int token) : token_(token) {}
explicit

[gab, 2016-08-09 15:50:15]

Thanks Lei, nits addressed in https://codereview.chromium.org/2228993003/ so I
can land this as-is under fdoray's authorship (I'm taking this over as fdoray is
OOO as of today).

https://codereview.chromium.org/2213263002/diff/120001/base/sequence_token.cc
File base/sequence_token.cc (right):

https://codereview.chromium.org/2213263002/diff/120001/base/sequence_token.cc...
base/sequence_token.cc:51: return token_ == other.token_ && token_ !=
kInvalidTaskToken;
On 2016/08/08 23:21:26, Lei Zhang wrote:
> Just call IsValid() in the latter half?

Done.

https://codereview.chromium.org/2213263002/diff/120001/base/sequence_token.h
File base/sequence_token.h (right):

https://codereview.chromium.org/2213263002/diff/120001/base/sequence_token.h#...
base/sequence_token.h:80: TaskToken(int token) : token_(token) {}
On 2016/08/08 23:21:26, Lei Zhang wrote:
> explicit

Done.

[gab, 2016-08-09 15:50:19]

The CQ bit was checked by gab@chromium.org

[gab, 2016-08-09 15:50:20]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2213263002/#ps120001
(title: "CR gab #6 (fix comment)")

[commit-bot: I haz the power, 2016-08-09 15:50:52]

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-08-09 17:52:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 17:52:03]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-08-09 17:56:28]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-08-09 17:57:14]

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-08-09 20:20:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 20:20:01]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-08-09 20:28:34]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-08-09 20:29:17]

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-08-09 21:50:08]

Description was changed from

==========
Make ThreadChecker::CalledOnValidThread() return true when called from the same
task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().

ThreadCheckerImpl uses TaskToken to make CalledOnValidThread() return
true when called multiple times from the same task. This allows usage
of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

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

to

==========
Make ThreadChecker::CalledOnValidThread() return true when called from the same
task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().

ThreadCheckerImpl uses TaskToken to make CalledOnValidThread() return
true when called multiple times from the same task. This allows usage
of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

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

[commit-bot: I haz the power, 2016-08-09 21:50:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-09 21:52:08]

Description was changed from

==========
Make ThreadChecker::CalledOnValidThread() return true when called from the same
task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().

ThreadCheckerImpl uses TaskToken to make CalledOnValidThread() return
true when called multiple times from the same task. This allows usage
of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

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

to

==========
Make ThreadChecker::CalledOnValidThread() return true when called from the same
task.

This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().

ThreadCheckerImpl uses TaskToken to make CalledOnValidThread() return
true when called multiple times from the same task. This allows usage
of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

BUG=553459

Committed: https://crrev.com/b339954beda5b00c4a482d74594cbe2e7cd39516
Cr-Commit-Position: refs/heads/master@{#410839}
==========

[commit-bot: I haz the power, 2016-08-09 21:52:09]

Patchset 7 (id:??) landed as
https://crrev.com/b339954beda5b00c4a482d74594cbe2e7cd39516
Cr-Commit-Position: refs/heads/master@{#410839}