ProfileSyncService: Handle null SupervisedUserService

ProfileSyncService: Handle null SupervisedUserService

SupervisedUserService might be null as profilesyncservice does not depend on it.
So handle null service gracefully instead of npe. Since services are created one per profile, this can happen only when browser is exiting.

BUG=705545
Review-Url: https://codereview.chromium.org/2847803008
Cr-Commit-Position: refs/heads/master@{#470356}
Committed: https://chromium.googlesource.com/chromium/src/+/1c532233baf652b7af9189fb2bce159ae172e739

[rishiag, 2017-04-28 22:36:55]

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

[commit-bot: I haz the power, 2017-04-28 22:37:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rishiag, 2017-04-28 22:43:47]

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

[commit-bot: I haz the power, 2017-04-28 22:44:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rishiag, 2017-04-28 23:04:00]

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

[commit-bot: I haz the power, 2017-04-28 23:04:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rishiag1, 2017-04-28 23:24:24]

Description was changed from

==========
Supervised user service might be null as profilesyncservice does not depend on
it.
So handle null service gracefully instead of npe. Since these service object are
singleton, this can happen only when browser is exiting.

BUG=705545
==========

to

==========
SupervisedUserService might be null as profilesyncservice does not depend on it.
So handle null service gracefully instead of npe. Since these service objects
are
singleton, this can happen only when browser is exiting.

BUG=705545
==========

[rishiag1, 2017-04-28 23:24:41]

rishiag@google.com changed reviewers:
+ rishiag@google.com, sky@chromium.org

[commit-bot: I haz the power, 2017-04-29 00:40:34]

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

[commit-bot: I haz the power, 2017-04-29 00:40:34]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-01 17:07:55]

sky@chromium.org changed reviewers:
+ stanisc@chromium.org
- rishiag@google.com

[sky, 2017-05-01 17:07:56]

I'm not familiar with this code. Could you get a local owner to do the review?
I'm randomly adding stanisc.

[rishiag, 2017-05-01 17:12:22]

rishiag@chromium.org changed reviewers:
+ skym@chromium.org
- sky@chromium.org

[Patrick Noland, 2017-05-01 17:17:42]

On 2017/05/01 17:07:56, sky wrote:
> I'm not familiar with this code. Could you get a local owner to do the review?
> I'm randomly adding stanisc.

I'm guessing rishiag@ meant to add skym@ as a reviewer, not sky@

[skym, 2017-05-01 17:24:03]

skym@chromium.org changed reviewers:
+ treib@chromium.org
- stanisc@chromium.org

[skym, 2017-05-01 17:24:05]

lgtm

Removing sky@/stanic@, adding treib@ for SUS change, PTAL.

https://codereview.chromium.org/2847803008/diff/60001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2847803008/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:451: // SupervisedUserServiceFactory
depends on ProfileSyncServiceFactory
I'd rephrase this to talk about the dep in the other direction.

// Unlike other types here, ProfileSyncServiceFactory does not declare a
DependsOn the SupervisedUserServiceFactory (in order to avoid circular
dependency), which means we cannot assume it is still alive.

[Marc Treib, 2017-05-02 08:27:36]

https://codereview.chromium.org/2847803008/diff/60001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2847803008/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:451: // SupervisedUserServiceFactory
depends on ProfileSyncServiceFactory
On 2017/05/01 17:24:03, skym wrote:
> I'd rephrase this to talk about the dep in the other direction.
> 
> // Unlike other types here, ProfileSyncServiceFactory does not declare a
> DependsOn the SupervisedUserServiceFactory (in order to avoid circular
> dependency), which means we cannot assume it is still alive.

+1

https://codereview.chromium.org/2847803008/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:454:
SupervisedUserServiceFactory::GetForProfile(profile_);
SupervisedUserServiceFactory::GetForProfile will create the service if it
doesn't exist. I don't know what will happen if this gets called after the SUS
has already been destroyed, but I assume nothing good?
We should probably add a GetForProfileIfExists method on the SUSFactory.

[Marc Treib, 2017-05-02 08:30:01]

Ah, and a description nit: Please put a short "title" into the first line of the
CL description. (That's what'll end up as the commit title!)
Something like: "ProfileSyncService: Handle null SupervisedUserService"

[rishiag, 2017-05-05 17:16:11]

Description was changed from

==========
SupervisedUserService might be null as profilesyncservice does not depend on it.
So handle null service gracefully instead of npe. Since these service objects
are
singleton, this can happen only when browser is exiting.

BUG=705545
==========

to

==========
ProfileSyncService: Handle null SupervisedUserService

SupervisedUserService might be null as profilesyncservice does not depend on it.
So handle null service gracefully instead of npe. Since these service objects
are
singleton, this can happen only when browser is exiting.

BUG=705545
==========

[rishiag, 2017-05-05 17:22:02]

https://codereview.chromium.org/2847803008/diff/60001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2847803008/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:451: // SupervisedUserServiceFactory
depends on ProfileSyncServiceFactory
On 2017/05/01 17:24:03, skym wrote:
> I'd rephrase this to talk about the dep in the other direction.
> 
> // Unlike other types here, ProfileSyncServiceFactory does not declare a
> DependsOn the SupervisedUserServiceFactory (in order to avoid circular
> dependency), which means we cannot assume it is still alive.

Done.

https://codereview.chromium.org/2847803008/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:454:
SupervisedUserServiceFactory::GetForProfile(profile_);
On 2017/05/02 08:27:36, Marc Treib wrote:
> SupervisedUserServiceFactory::GetForProfile will create the service if it
> doesn't exist. I don't know what will happen if this gets called after the SUS
> has already been destroyed, but I assume nothing good?
> We should probably add a GetForProfileIfExists method on the SUSFactory.

Done.

[rishiag, 2017-05-05 17:22:31]

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

[commit-bot: I haz the power, 2017-05-05 17:23:07]

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-05-05 18:21:10]

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

[commit-bot: I haz the power, 2017-05-05 18:21:11]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-05-08 08:14:14]

lgtm

Description nit: These services are not singletons; they exist per-profile.

https://codereview.chromium.org/2847803008/diff/100001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service_factory.cc (right):

https://codereview.chromium.org/2847803008/diff/100001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service_factory.cc:31:
GetInstance()->GetServiceForBrowserContext(profile, false /* create */));
nit: Prefer
/*create=*/true
because some tools such as clang-tidy understand that style (and verify that the
comment actually matches the declared parameter name).

[rishiag, 2017-05-08 17:53:48]

Description was changed from

==========
ProfileSyncService: Handle null SupervisedUserService

SupervisedUserService might be null as profilesyncservice does not depend on it.
So handle null service gracefully instead of npe. Since these service objects
are
singleton, this can happen only when browser is exiting.

BUG=705545
==========

to

==========
ProfileSyncService: Handle null SupervisedUserService

SupervisedUserService might be null as profilesyncservice does not depend on it.
So handle null service gracefully instead of npe. Since services are created one
per profile, this can happen only when browser is exiting.

BUG=705545
==========

[rishiag, 2017-05-08 17:55:15]

https://codereview.chromium.org/2847803008/diff/100001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service_factory.cc (right):

https://codereview.chromium.org/2847803008/diff/100001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service_factory.cc:31:
GetInstance()->GetServiceForBrowserContext(profile, false /* create */));
On 2017/05/08 08:14:13, Marc Treib wrote:
> nit: Prefer
> /*create=*/true
> because some tools such as clang-tidy understand that style (and verify that
the
> comment actually matches the declared parameter name).

Done.

[rishiag, 2017-05-08 18:21:26]

The CQ bit was checked by rishiag@chromium.org

[rishiag, 2017-05-08 18:21:26]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2847803008/#ps140001
(title: "Merge remote-tracking branch 'refs/remotes/origin/master' into dep")

[commit-bot: I haz the power, 2017-05-08 18:22:22]

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-05-08 18:31:26]

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

[commit-bot: I haz the power, 2017-05-08 18:31:27]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[rishiag, 2017-05-09 16:47:55]

The CQ bit was checked by rishiag@chromium.org

[commit-bot: I haz the power, 2017-05-09 16:48:57]

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-05-09 16:58:46]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494348475830350,
"parent_rev": "e442dc593b6d8efae7feba37958e637ae48a55d4", "commit_rev":
"1c532233baf652b7af9189fb2bce159ae172e739"}

[commit-bot: I haz the power, 2017-05-09 16:58:57]

Description was changed from

==========
ProfileSyncService: Handle null SupervisedUserService

SupervisedUserService might be null as profilesyncservice does not depend on it.
So handle null service gracefully instead of npe. Since services are created one
per profile, this can happen only when browser is exiting.

BUG=705545
==========

to

==========
ProfileSyncService: Handle null SupervisedUserService

SupervisedUserService might be null as profilesyncservice does not depend on it.
So handle null service gracefully instead of npe. Since services are created one
per profile, this can happen only when browser is exiting.

BUG=705545

Review-Url: https://codereview.chromium.org/2847803008
Cr-Commit-Position: refs/heads/master@{#470356}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c532233baf652b7af9189fb2bce...
==========

[commit-bot: I haz the power, 2017-05-09 16:58:59]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/1c532233baf652b7af9189fb2bce...