Fix occasional crash in remote commands code

Fix occasional crash in remote commands code

The RemoteCommandsService depends on a reference to CloudPolicyClient, but
it doesn't get deleted before CloudPolicyClient is gone. This leads to dangling
pointer and will probably crash the browser.

This CL also remove code attempts to workaround this issue in crrev.com/330082

BUG=480982
Committed: https://crrev.com/939825aae3cf24f32074f084d90647cb9bb90733
Cr-Commit-Position: refs/heads/master@{#330497}

[binjin, 2015-05-18 12:21:47]

binjin@chromium.org changed reviewers:
+ bartfab@chromium.org

[binjin, 2015-05-18 12:21:47]

Bartosz, PTAL. I think we need to merge this back into 44, the previous
workaround I added last friday is not working.

[bartfab (slow), 2015-05-18 12:24:49]

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_core.cc (right):

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_core.cc:52:
remote_commands_service_.reset();
If another Connect() call is made later, will StartRemoteCommandsService() run
and recreate the |remote_commands_service_|?

[binjin, 2015-05-18 12:28:56]

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_core.cc (right):

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_core.cc:52:
remote_commands_service_.reset();
On 2015/05/18 12:24:49, bartfab wrote:
> If another Connect() call is made later, will StartRemoteCommandsService() run
> and recreate the |remote_commands_service_|?

No. Unless another StartRemoteCommandsService call is made explicitly.

I don't see this as an issue, since the first Connect() call doesn't call
StartRemoteCommandsService() itself either.

[bartfab (slow), 2015-05-18 12:30:45]

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_core.cc (right):

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_core.cc:52:
remote_commands_service_.reset();
On 2015/05/18 12:28:56, binjin wrote:
> On 2015/05/18 12:24:49, bartfab wrote:
> > If another Connect() call is made later, will StartRemoteCommandsService()
run
> > and recreate the |remote_commands_service_|?
> 
> No. Unless another StartRemoteCommandsService call is made explicitly.
> 
> I don't see this as an issue, since the first Connect() call doesn't call
> StartRemoteCommandsService() itself either.

Sure, it does not need to be automatic and tied to each other. What I want to
understand is whether in the flows we have in the code right now, Connect() and
StartRemoteCommandsService() are called alongside each other so that we do not
end up with a remote commands service in some cases and without one in others.

[binjin, 2015-05-18 12:37:12]

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_core.cc (right):

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_core.cc:52:
remote_commands_service_.reset();
On 2015/05/18 12:30:45, bartfab wrote:
> On 2015/05/18 12:28:56, binjin wrote:
> > On 2015/05/18 12:24:49, bartfab wrote:
> > > If another Connect() call is made later, will StartRemoteCommandsService()
> run
> > > and recreate the |remote_commands_service_|?
> > 
> > No. Unless another StartRemoteCommandsService call is made explicitly.
> > 
> > I don't see this as an issue, since the first Connect() call doesn't call
> > StartRemoteCommandsService() itself either.
> 
> Sure, it does not need to be automatic and tied to each other. What I want to
> understand is whether in the flows we have in the code right now, Connect()
and
> StartRemoteCommandsService() are called alongside each other so that we do not
> end up with a remote commands service in some cases and without one in others.

I see. I did a grep and the only place where StartRemoteCommandsService is
called is at device_cloud_policy_manager_chromeos.cc, following the Connect()
call.      

There are multiple places where Connect() is called, but for device remote
commands, it's also the only place. So I think we will have
RemoteCommandsService running in that case.

[bartfab (slow), 2015-05-19 09:31:08]

lgtm

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_core.cc (right):

https://codereview.chromium.org/1134133005/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_core.cc:52:
remote_commands_service_.reset();
On 2015/05/18 12:37:12, binjin wrote:
> On 2015/05/18 12:30:45, bartfab wrote:
> > On 2015/05/18 12:28:56, binjin wrote:
> > > On 2015/05/18 12:24:49, bartfab wrote:
> > > > If another Connect() call is made later, will
StartRemoteCommandsService()
> > run
> > > > and recreate the |remote_commands_service_|?
> > > 
> > > No. Unless another StartRemoteCommandsService call is made explicitly.
> > > 
> > > I don't see this as an issue, since the first Connect() call doesn't call
> > > StartRemoteCommandsService() itself either.
> > 
> > Sure, it does not need to be automatic and tied to each other. What I want
to
> > understand is whether in the flows we have in the code right now, Connect()
> and
> > StartRemoteCommandsService() are called alongside each other so that we do
not
> > end up with a remote commands service in some cases and without one in
others.
> 
> I see. I did a grep and the only place where StartRemoteCommandsService is
> called is at device_cloud_policy_manager_chromeos.cc, following the Connect()
> call.      
> 
> There are multiple places where Connect() is called, but for device remote
> commands, it's also the only place. So I think we will have
> RemoteCommandsService running in that case.

I checked: All the other Connect() calls deal with user policy and we do not
have per-user commands yet. So we have confirmation that yes,
StartRemoteCommandsService() will always be called together with Connect() for
the Core that handles device policy (and device-wide commands).

[binjin, 2015-05-19 09:37:52]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2015-05-19 09:37:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134133005/1

[commit-bot: I haz the power, 2015-05-19 09:41:00]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-19 09:41:50]

Patchset 1 (id:??) landed as
https://crrev.com/939825aae3cf24f32074f084d90647cb9bb90733
Cr-Commit-Position: refs/heads/master@{#330497}