Use base::RefCountedThreadSafe on BindStateBase

Use base::RefCountedThreadSafe on BindStateBase

BindStateBase had its own reference count management. However, what it
needs is just custom destruction, rather than custom ref count.

This CL replaces that with RefCountedThreadSafe for dedupe the ref count
implementation.

Review-Url: https://codereview.chromium.org/2747673002
Cr-Commit-Position: refs/heads/master@{#460683}
Committed: https://chromium.googlesource.com/chromium/src/+/65adef848f642a861ff781e24f8aaee11607698e

[tzik, 2017-03-21 09:40:24]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 09:40:39]

Dry run: 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, 2017-03-21 10:15:26]

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

[commit-bot: I haz the power, 2017-03-21 10:15:27]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tzik, 2017-03-21 10:50:03]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 10:50:12]

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

[tzik, 2017-03-21 10:57:10]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 10:57:29]

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

[tzik, 2017-03-21 12:26:30]

Description was changed from

==========
Use base::RefCountedThreadSafe on BindStateBase
==========

to

==========
Use base::RefCountedThreadSafe on BindStateBase

BindStateBase had its own reference count management. However, what it
needs is just custom destruction, rather than custom ref count.

This CL replaces that with RefCountedThreadSafe for dedupe the ref count
implementation.
==========

[commit-bot: I haz the power, 2017-03-21 12:59:31]

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

[commit-bot: I haz the power, 2017-03-21 12:59:32]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-03-21 14:05:43]

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

[tzik, 2017-03-21 14:05:44]

PTAL

[dcheng, 2017-03-22 00:26:17]

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal....
base/callback_internal.h:65: // Whitelist subclasses that access the destructor
of BindStateBase.
Hmm, wouldn't it be easier just to keep the destructor protected? What is the
disadvantage of doing that?

[tzik, 2017-03-22 01:50:05]

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal....
base/callback_internal.h:65: // Whitelist subclasses that access the destructor
of BindStateBase.
On 2017/03/22 00:26:17, dcheng wrote:
> Hmm, wouldn't it be easier just to keep the destructor protected? What is the
> disadvantage of doing that?

That hits our clang plugin check. It requires the protected destructor of
RefCounted type should be virtual.

[dcheng, 2017-03-27 07:15:26]

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal....
base/callback_internal.h:65: // Whitelist subclasses that access the destructor
of BindStateBase.
On 2017/03/22 01:50:05, tzik wrote:
> On 2017/03/22 00:26:17, dcheng wrote:
> > Hmm, wouldn't it be easier just to keep the destructor protected? What is
the
> > disadvantage of doing that?
> 
> That hits our clang plugin check. It requires the protected destructor of
> RefCounted type should be virtual.

Hmm... so this seems pretty dangerous (though it's safe at the moment). Is there
any way to make this safer? Or note that subclasses must not add any additional
fields?

[tzik, 2017-03-29 08:29:47]

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal....
base/callback_internal.h:65: // Whitelist subclasses that access the destructor
of BindStateBase.
On 2017/03/27 07:15:26, dcheng wrote:
> On 2017/03/22 01:50:05, tzik wrote:
> > On 2017/03/22 00:26:17, dcheng wrote:
> > > Hmm, wouldn't it be easier just to keep the destructor protected? What is
> the
> > > disadvantage of doing that?
> > 
> > That hits our clang plugin check. It requires the protected destructor of
> > RefCounted type should be virtual.
> 
> Hmm... so this seems pretty dangerous (though it's safe at the moment). Is
there
> any way to make this safer? Or note that subclasses must not add any
additional
> fields?

Do we have a pitfall where we may easily fall into?
It's broken if a subclass destroys BindStateBase through a BindStateBase pointer
rather than the pointer of the subclass, but I see no other error prone part.
Also, it looks safe to me to add a field to the subclass.

[dcheng, 2017-03-29 17:38:00]

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal....
base/callback_internal.h:65: // Whitelist subclasses that access the destructor
of BindStateBase.
On 2017/03/29 08:29:47, tzik wrote:
> On 2017/03/27 07:15:26, dcheng wrote:
> > On 2017/03/22 01:50:05, tzik wrote:
> > > On 2017/03/22 00:26:17, dcheng wrote:
> > > > Hmm, wouldn't it be easier just to keep the destructor protected? What
is
> > the
> > > > disadvantage of doing that?
> > > 
> > > That hits our clang plugin check. It requires the protected destructor of
> > > RefCounted type should be virtual.
> > 
> > Hmm... so this seems pretty dangerous (though it's safe at the moment). Is
> there
> > any way to make this safer? Or note that subclasses must not add any
> additional
> > fields?
> 
> Do we have a pitfall where we may easily fall into?
> It's broken if a subclass destroys BindStateBase through a BindStateBase
pointer
> rather than the pointer of the subclass, but I see no other error prone part.

Right, this is the thing I'm concerned about. I don't think it's likely to
happen, but it also seems like it's not quite what typical C++ best practices
ought to be.

> Also, it looks safe to me to add a field to the subclass.

Unless I'm missing something, it's always unsafe to add a field to a subclass:
CallbackBase holds on to the bind state as a BindStateBase.

[tzik, 2017-03-29 18:25:27]

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal....
base/callback_internal.h:65: // Whitelist subclasses that access the destructor
of BindStateBase.
On 2017/03/29 17:38:00, dcheng wrote:
> On 2017/03/29 08:29:47, tzik wrote:
> > On 2017/03/27 07:15:26, dcheng wrote:
> > > On 2017/03/22 01:50:05, tzik wrote:
> > > > On 2017/03/22 00:26:17, dcheng wrote:
> > > > > Hmm, wouldn't it be easier just to keep the destructor protected? What
> is
> > > the
> > > > > disadvantage of doing that?
> > > > 
> > > > That hits our clang plugin check. It requires the protected destructor
of
> > > > RefCounted type should be virtual.
> > > 
> > > Hmm... so this seems pretty dangerous (though it's safe at the moment). Is
> > there
> > > any way to make this safer? Or note that subclasses must not add any
> > additional
> > > fields?
> > 
> > Do we have a pitfall where we may easily fall into?
> > It's broken if a subclass destroys BindStateBase through a BindStateBase
> pointer
> > rather than the pointer of the subclass, but I see no other error prone
part.
> 
> Right, this is the thing I'm concerned about. I don't think it's likely to
> happen, but it also seems like it's not quite what typical C++ best practices
> ought to be.
> 
> > Also, it looks safe to me to add a field to the subclass.
> 
> Unless I'm missing something, it's always unsafe to add a field to a subclass:
> CallbackBase holds on to the bind state as a BindStateBase.

CallbackBase holds it as a scoped_refptr of BindStateBase, but it is deleted as
the subclass rather than BindStateBase, since it has the custom deleter that
deletes BindStateBase after casting it back to the right subclass.

[dcheng, 2017-03-30 04:11:02]

LGTM

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2747673002/diff/60001/base/callback_internal....
base/callback_internal.h:65: // Whitelist subclasses that access the destructor
of BindStateBase.
On 2017/03/29 18:25:26, tzik wrote:
> On 2017/03/29 17:38:00, dcheng wrote:
> > On 2017/03/29 08:29:47, tzik wrote:
> > > On 2017/03/27 07:15:26, dcheng wrote:
> > > > On 2017/03/22 01:50:05, tzik wrote:
> > > > > On 2017/03/22 00:26:17, dcheng wrote:
> > > > > > Hmm, wouldn't it be easier just to keep the destructor protected?
What
> > is
> > > > the
> > > > > > disadvantage of doing that?
> > > > > 
> > > > > That hits our clang plugin check. It requires the protected destructor
> of
> > > > > RefCounted type should be virtual.
> > > > 
> > > > Hmm... so this seems pretty dangerous (though it's safe at the moment).
Is
> > > there
> > > > any way to make this safer? Or note that subclasses must not add any
> > > additional
> > > > fields?
> > > 
> > > Do we have a pitfall where we may easily fall into?
> > > It's broken if a subclass destroys BindStateBase through a BindStateBase
> > pointer
> > > rather than the pointer of the subclass, but I see no other error prone
> part.
> > 
> > Right, this is the thing I'm concerned about. I don't think it's likely to
> > happen, but it also seems like it's not quite what typical C++ best
practices
> > ought to be.
> > 
> > > Also, it looks safe to me to add a field to the subclass.
> > 
> > Unless I'm missing something, it's always unsafe to add a field to a
subclass:
> > CallbackBase holds on to the bind state as a BindStateBase.
> 
> CallbackBase holds it as a scoped_refptr of BindStateBase, but it is deleted
as
> the subclass rather than BindStateBase, since it has the custom deleter that
> deletes BindStateBase after casting it back to the right subclass.

Ah... right. This is a bit subtle, but I guess it's my fault for not reading the
comments more carefully =)

[tzik, 2017-03-30 04:26:54]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-03-30 04:27:13]

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, 2017-03-30 06:45:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490848014806530,
"parent_rev": "17e71cc2eb3ab44853501b26b751c5d3c057ffad", "commit_rev":
"65adef848f642a861ff781e24f8aaee11607698e"}

[commit-bot: I haz the power, 2017-03-30 06:46:38]

Description was changed from

==========
Use base::RefCountedThreadSafe on BindStateBase

BindStateBase had its own reference count management. However, what it
needs is just custom destruction, rather than custom ref count.

This CL replaces that with RefCountedThreadSafe for dedupe the ref count
implementation.
==========

to

==========
Use base::RefCountedThreadSafe on BindStateBase

BindStateBase had its own reference count management. However, what it
needs is just custom destruction, rather than custom ref count.

This CL replaces that with RefCountedThreadSafe for dedupe the ref count
implementation.

Review-Url: https://codereview.chromium.org/2747673002
Cr-Commit-Position: refs/heads/master@{#460683}
Committed:
https://chromium.googlesource.com/chromium/src/+/65adef848f642a861ff781e24f8a...
==========

[commit-bot: I haz the power, 2017-03-30 06:46:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/65adef848f642a861ff781e24f8a...