|
|
DescriptionProfileSyncService: 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
Patch Set 1 #Patch Set 2 : Fix lint #Patch Set 3 : style #Patch Set 4 : break #
Total comments: 5
Patch Set 5 : add ifexists method; #Patch Set 6 : add .h file #
Total comments: 2
Patch Set 7 : fix lint #Patch Set 8 : Merge remote-tracking branch 'refs/remotes/origin/master' into dep #
Messages
Total messages: 40 (25 generated)
The CQ bit was checked by rishiag@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rishiag@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rishiag@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
rishiag@google.com changed reviewers: + rishiag@google.com, sky@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@chromium.org changed reviewers: + stanisc@chromium.org - rishiag@google.com
I'm not familiar with this code. Could you get a local owner to do the review? I'm randomly adding stanisc.
rishiag@chromium.org changed reviewers: + skym@chromium.org - sky@chromium.org
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@chromium.org changed reviewers: + treib@chromium.org - stanisc@chromium.org
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.
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.
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"
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 ==========
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.
The CQ bit was checked by rishiag@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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).
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 ==========
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.
The CQ bit was checked by rishiag@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by rishiag@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494348475830350, "parent_rev": "e442dc593b6d8efae7feba37958e637ae48a55d4", "commit_rev": "1c532233baf652b7af9189fb2bce159ae172e739"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/1c532233baf652b7af9189fb2bce... |