Track active references in ShillClientHelper

Track active references in ShillClientHelper
To prevent Shill Service DBus ObjectProxy instances from accumulating,
remove them when the service becomes inactive.

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

[stevenjb, 2013-09-13 23:04:07]

OK, after trying a couple of different approaches, I think this is the simplest
reliable way to track and remove stale Service object overhead.

[hashimoto, 2013-09-17 03:28:53]

https://codereview.chromium.org/23658053/diff/4001/chromeos/dbus/shill_client...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/23658053/diff/4001/chromeos/dbus/shill_client...
chromeos/dbus/shill_client_helper.cc:21: class HelperRelease {
nit: "Helper" here sounds ambiguous, does it mean "this class is a helper to
release some class", or "this class is to release ShillClientHelper"? (maybe
both)
How about renaming this to ReferenceKeeper or ReleaseCaller or something?

https://codereview.chromium.org/23658053/diff/4001/chromeos/dbus/shill_client...
chromeos/dbus/shill_client_helper.cc:39: HelperRelease release(helper);
I'm a bit worried about manually adding HelperRelease and AddRef for each
methods because it's quite easy to forget to add one or both of this pair in
future.

How about doing something like:
 1. Call AddRef from the ctor of HelperRelease.
 2. Create an object which deletes HelperRelease in its dtor. (you can use
WeakPtr<HelperRelease> to avoid deleting the same HelperRelease twice)
 3. Change the argument of OnBooleanMethodWithErrorCallback (and its family
members) from base::WeakPtr<ShillClientHelper> to scoped_ptr<The new class
mentioned in #2>. (or use base::Owned instead of scoped_ptr)

This way, what you should do for each method is just adding one scoped_ptr
argument for the callback function.

https://codereview.chromium.org/23658053/diff/4001/chromeos/dbus/shill_client...
File chromeos/dbus/shill_client_helper.h (right):

https://codereview.chromium.org/23658053/diff/4001/chromeos/dbus/shill_client...
chromeos/dbus/shill_client_helper.h:87: void SetOwner(Owner* owner);
nit: Since the Owner interface has only one method, it can be easily replaced
with a callback base::Bind(&ShillServiceClient::NotifyReleased, ...).
I think callbacks are better than interfaces because the cause and the result
(which method is called as which method's callback) is understandable without
looking at header files.

https://codereview.chromium.org/23658053/diff/4001/chromeos/dbus/shill_client...
chromeos/dbus/shill_client_helper.h:150: void Release();
nit: How about making these methods private by making HelperRelease an inner
class of ShillClientHelper?

[stevenjb, 2013-09-17 21:03:16]

OK, I addressed the ownership issue by passing base::Owned(new RefHolder) to the
success callbacks. I tried a couple of other options, but it's complicated by
the fact that sometimes there is both a success and an error callback. I'm not
100% convinced that this is better than explicitly calling AddRef/Release, but I
don't think it is any worse, and it is better than any of the alternatives I
came up with.

PTAL

https://chromiumcodereview.appspot.com/23658053/diff/4001/chromeos/dbus/shill...
File chromeos/dbus/shill_client_helper.cc (right):

https://chromiumcodereview.appspot.com/23658053/diff/4001/chromeos/dbus/shill...
chromeos/dbus/shill_client_helper.cc:21: class HelperRelease {
On 2013/09/17 03:28:53, hashimoto wrote:
> nit: "Helper" here sounds ambiguous, does it mean "this class is a helper to
> release some class", or "this class is to release ShillClientHelper"? (maybe
> both)
> How about renaming this to ReferenceKeeper or ReleaseCaller or something?

-> ShillClientHelperAutoRelease

https://chromiumcodereview.appspot.com/23658053/diff/4001/chromeos/dbus/shill...
chromeos/dbus/shill_client_helper.cc:39: HelperRelease release(helper);
On 2013/09/17 03:28:53, hashimoto wrote:
> I'm a bit worried about manually adding HelperRelease and AddRef for each
> methods because it's quite easy to forget to add one or both of this pair in
> future.
> 
> How about doing something like:
>  1. Call AddRef from the ctor of HelperRelease.
>  2. Create an object which deletes HelperRelease in its dtor. (you can use
> WeakPtr<HelperRelease> to avoid deleting the same HelperRelease twice)
>  3. Change the argument of OnBooleanMethodWithErrorCallback (and its family
> members) from base::WeakPtr<ShillClientHelper> to scoped_ptr<The new class
> mentioned in #2>. (or use base::Owned instead of scoped_ptr)
> 
> This way, what you should do for each method is just adding one scoped_ptr
> argument for the callback function.

I initially considered that approach but decided against it because I thought it
would add more complexity then it would be worth. That said, I made the decision
before adding HelperRelease, but maybe if I create a ShillClientHelperWrapper
object to these instead, that can handle the AddRef and Release calls.

https://chromiumcodereview.appspot.com/23658053/diff/4001/chromeos/dbus/shill...
File chromeos/dbus/shill_client_helper.h (right):

https://chromiumcodereview.appspot.com/23658053/diff/4001/chromeos/dbus/shill...
chromeos/dbus/shill_client_helper.h:87: void SetOwner(Owner* owner);
On 2013/09/17 03:28:53, hashimoto wrote:
> nit: Since the Owner interface has only one method, it can be easily replaced
> with a callback base::Bind(&ShillServiceClient::NotifyReleased, ...).
> I think callbacks are better than interfaces because the cause and the result
> (which method is called as which method's callback) is understandable without
> looking at header files.

Sure, I'll change this to SetReleasedCallback.

https://chromiumcodereview.appspot.com/23658053/diff/4001/chromeos/dbus/shill...
chromeos/dbus/shill_client_helper.h:150: void Release();
On 2013/09/17 03:28:53, hashimoto wrote:
> nit: How about making these methods private by making HelperRelease an inner
> class of ShillClientHelper?

Done.

[hashimoto, 2013-09-18 04:32:30]

lgtm

https://codereview.chromium.org/23658053/diff/14001/chromeos/dbus/shill_clien...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/23658053/diff/14001/chromeos/dbus/shill_clien...
chromeos/dbus/shill_client_helper.cc:238: Release();
question: Can we be sure that the users of this class do not remove an observer
which is already removed (or not yet added) from the list?
If not, I think we should call observer_list_.HasObserver() to avoid wrong
counting.
The same goes for AddPropertyChangedObserver.

https://codereview.chromium.org/23658053/diff/14001/chromeos/dbus/shill_clien...
chromeos/dbus/shill_client_helper.cc:480: released_callback_.Run(this);  // May
delete this
nit: base::ResetAndReturn(&released_callback_).Run() should be safer.
"delete helper" in the user code results in destroying |released_callback_| and
thus destroying arguments bound to the destroyed callback.
It seems this does not cause a serious problem now because "delete helper" is
executed at last and |helper| is not referenced after the deletion, but new
argumetns can be added in future.

[stevenjb, 2013-09-18 16:58:28]

PTAL

https://codereview.chromium.org/23658053/diff/14001/chromeos/dbus/shill_clien...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/23658053/diff/14001/chromeos/dbus/shill_clien...
chromeos/dbus/shill_client_helper.cc:238: Release();
On 2013/09/18 04:32:30, hashimoto wrote:
> question: Can we be sure that the users of this class do not remove an
observer
> which is already removed (or not yet added) from the list?
> If not, I think we should call observer_list_.HasObserver() to avoid wrong
> counting.
> The same goes for AddPropertyChangedObserver.
Good suggestion. Done. (Note: ObserverList::AddObserver does have a NOTREACHED()
on adding an observer twice, but not for removing. I'll go ahead and early exit
both though).

https://codereview.chromium.org/23658053/diff/14001/chromeos/dbus/shill_clien...
chromeos/dbus/shill_client_helper.cc:480: released_callback_.Run(this);  // May
delete this
On 2013/09/18 04:32:30, hashimoto wrote:
> nit: base::ResetAndReturn(&released_callback_).Run() should be safer.
> "delete helper" in the user code results in destroying |released_callback_|
and
> thus destroying arguments bound to the destroyed callback.
> It seems this does not cause a serious problem now because "delete helper" is
> executed at last and |helper| is not referenced after the deletion, but new
> argumetns can be added in future.
Hmm, good suggestion. Hopefully that wouldn't be added given the comment, but
that's a handy wrapper. Done.

[hashimoto, 2013-09-19 02:52:12]

lgtm

[commit-bot: I haz the power, 2013-09-19 16:56:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23658053/20001

[commit-bot: I haz the power, 2013-09-19 19:12:20]

Change committed as 224179