base: Change WeakPtr to use SequenceChecker instead of ThreadChecker.

base: Change WeakPtr to use SequenceChecker instead of ThreadChecker.

This will enable WeakPtr to be used in SequencedWorkerPool, et al. with a sequence token. 

This is a continuation of issue: https://chromiumcodereview.appspot.com/18231002/

The original issue got messed up by a rietveld bug, so refer there for history and comments.

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

[tommycli, 2013-07-03 18:34:33]

Original issue is screwed and can't be fixed without deleting comment history,
so this is a continuation of that issue. Put any new comments / approvals on
this one please.

Thanks.

[akalin, 2013-07-03 20:07:11]

Mind pulling out the SWP changes to a separate CL? This change is starting to
become unwieldy.

Also, probably want to address jar's comments in my original CL:
https://codereview.chromium.org/11564003/

[tommycli, 2013-07-03 21:08:43]

Thanks. SequencedWorkerPool pulled out into separate CL here:

https://codereview.chromium.org/18650006/

I addressed jar's comments also. Most didn't apply.

[tommycli, 2013-07-08 18:45:02]

akalin: I've adapted this CL to the lg'ed globally unique SequenceToken CL. May
I have a review?

[tommycli, 2013-07-10 18:06:07]

akalin: ping

[akalin, 2013-07-11 18:55:41]

On 2013/07/10 18:06:07, tommycli wrote:
> akalin: ping

Sorry for the delay, I'll look today

[akalin, 2013-07-11 19:31:22]

few nits, some comments on tests

may want to get owners approval now, too, either from darin or jar

https://codereview.chromium.org/18501008/diff/33001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/18501008/diff/33001/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:21: << "WeakPtrs must be checked and invalidated on the
same sequence.";
sequence -> sequenced thread?

https://codereview.chromium.org/18501008/diff/33001/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:27: << "WeakPtrs must be checked and invalidated on the
same sequence.";
here too

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

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:7: #include "base/logging.h"
still need logging.h?

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_imp...
base/sequence_checker_impl.h:41: // Used if |pool_| is NULL.
fix comment (what pool_?)

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:32: // Simple class to exercise the basics of
Sequencesequence_checker_class->
typo in this line?

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:35: class SequenceCheckerClass : public
SequenceChecker {
compose instead of inherit?

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:38: ~SequenceCheckerClass() {}
shouldn't the destructor also assert? or change comment?

[Wez, 2013-07-11 21:00:11]

https://chromiumcodereview.appspot.com/18501008/diff/33001/base/memory/weak_p...
File base/memory/weak_ptr.cc (right):

https://chromiumcodereview.appspot.com/18501008/diff/33001/base/memory/weak_p...
base/memory/weak_ptr.cc:13: // the same Flag take place on the same thread.
nit: thread -> sequence

[tommycli, 2013-07-11 21:39:13]

akalin / wez : addressed your comments.

https://codereview.chromium.org/18501008/diff/33001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/18501008/diff/33001/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:13: // the same Flag take place on the same thread.
On 2013/07/11 21:00:11, Wez wrote:
> nit: thread -> sequence

Done.

https://codereview.chromium.org/18501008/diff/33001/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:21: << "WeakPtrs must be checked and invalidated on the
same sequence.";
On 2013/07/11 19:31:23, akalin wrote:
> sequence -> sequenced thread?

Done.

https://codereview.chromium.org/18501008/diff/33001/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:27: << "WeakPtrs must be checked and invalidated on the
same sequence.";
On 2013/07/11 19:31:23, akalin wrote:
> here too

Done.

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

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:7: #include "base/logging.h"
On 2013/07/11 19:31:23, akalin wrote:
> still need logging.h?

Done.

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_imp...
base/sequence_checker_impl.h:41: // Used if |pool_| is NULL.
On 2013/07/11 19:31:23, akalin wrote:
> fix comment (what pool_?)

Done.

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:32: // Simple class to exercise the basics of
Sequencesequence_checker_class->
On 2013/07/11 19:31:23, akalin wrote:
> typo in this line?

Done.

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:35: class SequenceCheckerClass : public
SequenceChecker {
On 2013/07/11 19:31:23, akalin wrote:
> compose instead of inherit?

I could. I would have to add a DetatchFromSequence delegating method. Still want
me to do it?

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:38: ~SequenceCheckerClass() {}
On 2013/07/11 19:31:23, akalin wrote:
> shouldn't the destructor also assert? or change comment?

Done.

[tommycli, 2013-07-12 00:21:24]

darin: would like an OWNER review. thanks.

[akalin, 2013-07-12 00:26:38]

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

https://codereview.chromium.org/18501008/diff/51001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:17: EnsureSequenceTokenAssigned();
i think this is racy. It's entirely possible that when
EnsureSequenceTokenAssigned() finishes, another thread comes in and calls
DetachFromSequence(), and then when the lock is entered the token isn't assigned
anymore.

It looks like ThreadChecker suffers from the same problem.

Maybe assert that the lock is held in EnsureSequenceTokenAssigned() instead of
holding the lock? Maybe fix ThreadChecker, too?

[tommycli, 2013-07-12 00:48:06]

akalin: Fixed. good catch.

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

https://codereview.chromium.org/18501008/diff/51001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:17: EnsureSequenceTokenAssigned();
On 2013/07/12 00:26:38, akalin wrote:
> i think this is racy. It's entirely possible that when
> EnsureSequenceTokenAssigned() finishes, another thread comes in and calls
> DetachFromSequence(), and then when the lock is entered the token isn't
assigned
> anymore.
> 
> It looks like ThreadChecker suffers from the same problem.
> 
> Maybe assert that the lock is held in EnsureSequenceTokenAssigned() instead of
> holding the lock? Maybe fix ThreadChecker, too?

Done. Wow good eye.

[akalin, 2013-07-12 01:09:20]

I still got a few more comments on tests (sorry, I'm a stickler for good tests)

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:35: class SequenceCheckerClass : public
SequenceChecker {
On 2013/07/11 21:39:13, tommycli wrote:
> On 2013/07/11 19:31:23, akalin wrote:
> > compose instead of inherit?
> 
> I could. I would have to add a DetatchFromSequence delegating method. Still
want
> me to do it?

yeah, I think so.

https://codereview.chromium.org/18501008/diff/51002/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/18501008/diff/51002/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:21: << "WeakPtrs must be checked and invalidated on the
same sequenced "
may as well do "checked and invalidated" -> "invalidated"...

https://codereview.chromium.org/18501008/diff/51002/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:28: << "WeakPtrs must be checked and invalidated on the
same sequenced "
and "checked and invalidated" -> "checked" here

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:33: // DoStuff should verify that it's called
on a valid sequenced thread.
add comment like "SequenceCheckedObject can be destroyed on any thread (like
WeakPtr)".

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:34: class SequenceCheckerClass : public
SequenceChecker {
in addition to having the SequenceChecker be a member, how 'bout naming this
class SequenceCheckedObject? Having 'Class' in a class name is a bit of a code
smell.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:48: class CallDoStuffThread : public
base::SimpleThread {
rather than create a different thread for each thing you want to do, why not
create a base::Thread object (which comes with a message loop) then post tasks
to it? I feel like that would be simpler.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:52:
sequence_checker_class_(sequence_checker_class) {
then rename sequence_checker_class etc. to sequence_checked_object

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:68: SequenceCheckerClass*
sequence_checker_class)
have this take a scoped_ptr<SequenceCheckerClass> and use ptr.Pass() when
passing to this class

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:86: ResetPool();
may as well put this in SetUp() (to mirror the shutdown call)

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:122: MessageLoop message_loop_;
comment as to what needs the message loop (I guess the SWP at least)

[tommycli, 2013-07-12 19:16:24]

akalin: I overhauled the unit test. Hope it addresses your concerns.

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/33001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:35: class SequenceCheckerClass : public
SequenceChecker {
On 2013/07/12 01:09:21, akalin wrote:
> On 2013/07/11 21:39:13, tommycli wrote:
> > On 2013/07/11 19:31:23, akalin wrote:
> > > compose instead of inherit?
> > 
> > I could. I would have to add a DetatchFromSequence delegating method. Still
> want
> > me to do it?
> 
> yeah, I think so.

Done.

https://codereview.chromium.org/18501008/diff/51002/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/18501008/diff/51002/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:21: << "WeakPtrs must be checked and invalidated on the
same sequenced "
On 2013/07/12 01:09:21, akalin wrote:
> may as well do "checked and invalidated" -> "invalidated"...

Done.

https://codereview.chromium.org/18501008/diff/51002/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:28: << "WeakPtrs must be checked and invalidated on the
same sequenced "
On 2013/07/12 01:09:21, akalin wrote:
> and "checked and invalidated" -> "checked" here

Done.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:33: // DoStuff should verify that it's called
on a valid sequenced thread.
On 2013/07/12 01:09:21, akalin wrote:
> add comment like "SequenceCheckedObject can be destroyed on any thread (like
> WeakPtr)".

Done.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:34: class SequenceCheckerClass : public
SequenceChecker {
On 2013/07/12 01:09:21, akalin wrote:
> in addition to having the SequenceChecker be a member, how 'bout naming this
> class SequenceCheckedObject? Having 'Class' in a class name is a bit of a code
> smell.

Done.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:48: class CallDoStuffThread : public
base::SimpleThread {
On 2013/07/12 01:09:21, akalin wrote:
> rather than create a different thread for each thing you want to do, why not
> create a base::Thread object (which comes with a message loop) then post tasks
> to it? I feel like that would be simpler.

Done.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:52:
sequence_checker_class_(sequence_checker_class) {
On 2013/07/12 01:09:21, akalin wrote:
> then rename sequence_checker_class etc. to sequence_checked_object

Done.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:68: SequenceCheckerClass*
sequence_checker_class)
On 2013/07/12 01:09:21, akalin wrote:
> have this take a scoped_ptr<SequenceCheckerClass> and use ptr.Pass() when
> passing to this class

Done.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:86: ResetPool();
On 2013/07/12 01:09:21, akalin wrote:
> may as well put this in SetUp() (to mirror the shutdown call)

Done.

https://codereview.chromium.org/18501008/diff/51002/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:122: MessageLoop message_loop_;
On 2013/07/12 01:09:21, akalin wrote:
> comment as to what needs the message loop (I guess the SWP at least)

Done.

[akalin, 2013-07-12 19:41:49]

looks much better! few more comments

https://codereview.chromium.org/18501008/diff/73001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/18501008/diff/73001/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:13: // the same Flag take place on the same sequence.
sequence -> sequenced thread

https://codereview.chromium.org/18501008/diff/73001/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/73001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:71: pool()->Shutdown();
perhaps stop other_thread_ manually here

https://codereview.chromium.org/18501008/diff/73001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:98: void PostDeleteToOtherThread(
you can remove DeleteTask and instead use MessageLoop::DeleteSoon

https://codereview.chromium.org/18501008/diff/73001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:210: TEST_F(SequenceCheckerTest,
SameSequenceTokenValid) {
can you lump all the death tests at the bottom and the non-death tests at the
top? I think it'll make it easier to figure out which are the 'easy' tests

[tommycli, 2013-07-12 22:00:06]

akalin: thx, i made more changes.

https://codereview.chromium.org/18501008/diff/73001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/18501008/diff/73001/base/memory/weak_ptr.cc#n...
base/memory/weak_ptr.cc:13: // the same Flag take place on the same sequence.
On 2013/07/12 19:41:50, akalin wrote:
> sequence -> sequenced thread

Done.

https://codereview.chromium.org/18501008/diff/73001/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/73001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:71: pool()->Shutdown();
On 2013/07/12 19:41:50, akalin wrote:
> perhaps stop other_thread_ manually here

Done.

https://codereview.chromium.org/18501008/diff/73001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:98: void PostDeleteToOtherThread(
On 2013/07/12 19:41:50, akalin wrote:
> you can remove DeleteTask and instead use MessageLoop::DeleteSoon

Done.

https://codereview.chromium.org/18501008/diff/73001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:210: TEST_F(SequenceCheckerTest,
SameSequenceTokenValid) {
On 2013/07/12 19:41:50, akalin wrote:
> can you lump all the death tests at the bottom and the non-death tests at the
> top? I think it'll make it easier to figure out which are the 'easy' tests

Done.

[akalin, 2013-07-12 22:05:13]

lgtm!

https://codereview.chromium.org/18501008/diff/82001/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/82001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:122: TEST_F(SequenceCheckerTest,
CallsAllowedOnSameThread) {
can you move the tests inside the anon namespace? that should be possible unless
you're FRIEND_TESTing them (doesn't look like you are)

[tommycli, 2013-07-12 22:09:34]

akalin: thank you sir!

https://codereview.chromium.org/18501008/diff/82001/base/sequence_checker_uni...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18501008/diff/82001/base/sequence_checker_uni...
base/sequence_checker_unittest.cc:122: TEST_F(SequenceCheckerTest,
CallsAllowedOnSameThread) {
On 2013/07/12 22:05:13, akalin wrote:
> can you move the tests inside the anon namespace? that should be possible
unless
> you're FRIEND_TESTing them (doesn't look like you are)

Done.

[tommycli, 2013-07-15 20:44:27]

darin: ping for final OWNER approval.

[darin (slow to review), 2013-07-16 16:18:33]

LGTM

[commit-bot: I haz the power, 2013-07-16 16:20:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18501008/90001

[commit-bot: I haz the power, 2013-07-16 20:49:06]

Failed to trigger a try job on android_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-07-16 20:50:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18501008/111001

[commit-bot: I haz the power, 2013-07-17 05:35:17]

Change committed as 211956