Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209173002:
Migrate ArcUserDataService into ArcAuthService. (Closed)
|
Created:
4 years, 4 months ago by hidehiko Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrate ArcUserDataService into ArcAuthService.
There are race problems around data removing.
One of the cause is that state management is done in
ArcAuthService and ArcUserDataService separately.
This CL migrates ArcUserDataService into ArcAuthService
so that we can remove the data when necessary.
BUG=633258
TEST=Ran manually.
- "Enable -> ARC boot wait -> Disable" triggers data removal after
ARC stop.
- "Enable -> Disable before ARC boot" triggers immediate data
removal.
- "Enable -> ARC boot wait -> Kill ARC instance" does not trigger
data removal.
Committed: https://crrev.com/6d95ad318e992261c45f5a88a0a599663a6d66ae
Cr-Commit-Position: refs/heads/master@{#411923}
Patch Set 1 #Patch Set 2 : Fix unittest #
Total comments: 9
Patch Set 3 : Added more comments. #
Total comments: 12
Patch Set 4 : Address comment. #Patch Set 5 : Address comments. #
Total comments: 2
Patch Set 6 : Address comments. #Patch Set 7 : Fix unittest. #Patch Set 8 : Rebase #Messages
Total messages: 44 (26 generated)
The CQ bit was checked by hidehiko@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hidehiko@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.
hidehiko@chromium.org changed reviewers: + dspaid@chromium.org, khmel@chromium.org
This is not a perfect fix, but should cover practically more often cases. PTAL.
https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:242: OnArcDataRemoved(true); nit: this used quite confusion for me. May we rename it or split into 2 functions: OnArcRemoved() MayReenableArc()? https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:383: RemoveArcData(); nit: {} https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_enterprise_reporting_service.cc (right): https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_enterprise_reporting_service.cc:41: ArcAuthService::Get()->RemoveArcData(); nit. May we have one function instead? They are both public and as I understand should be used always together in right order. For example OnManagementLost()
On 2016/08/04 17:28:36, hidehiko wrote: > This is not a perfect fix, but should cover practically more often cases. > > PTAL. What about the idea of moving the wiping functionality to ArcBridgeService? We would have stronger guarantees of the state being correct over there. Plus, the number of flags/states in this class is starting to get out of hand.
PTAL. Luis, > What about the idea of moving the wiping functionality to ArcBridgeService? We would have stronger guarantees of the state being correct over there. Plus, the number of flags/states in this class is starting to get out of hand. I believe you and I are almost on the same page. Moving ArcBridgeService is an option, but it needs huge change. So, I'd like to prioritize the serious bug fix. I agree that making the responsibility of ArcAuthService bigger is concerned because of the complexity, but it already handles data removal in some case, and actually the fix needs what ArcAuthService knows, so this looks a simpler solution to me, unfortunately... WDYT? For refactoring, let me send another email to you for the discussion, because it is out of this CL's focus. https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:242: OnArcDataRemoved(true); On 2016/08/04 23:23:29, khmel wrote: > nit: this used quite confusion for me. May we rename it or split into 2 > functions: > OnArcRemoved() > MayReenableArc()? I agree that this may be confusing, but TBH, your splitting suggestion also confusing to me, because calling MayReenableArc() in both here and OnArcDataRemoved() does not have any good reasoning. So, instead I added comments. Does it work for you? Note that I'd like to drastically restructure the code, but the considering the size and its risk, I'll do it in later release. https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:383: RemoveArcData(); On 2016/08/04 23:23:29, khmel wrote: > nit: {} Let me keep this as is for consistency. No {} for oneline-if-body. https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_enterprise_reporting_service.cc (right): https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_enterprise_reporting_service.cc:41: ArcAuthService::Get()->RemoveArcData(); On 2016/08/04 23:23:29, khmel wrote: > nit. May we have one function instead? They are both public and as I understand > should be used always together in right order. > > For example OnManagementLost() I'd like to keep the concept as is for now, so let me keep the "device lost" concept here. I'm open for better approaches/code restructuring, but let's discuss it separately.
lgtm https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:242: OnArcDataRemoved(true); On 2016/08/09 13:23:56, hidehiko wrote: > On 2016/08/04 23:23:29, khmel wrote: > > nit: this used quite confusion for me. May we rename it or split into 2 > > functions: > > OnArcRemoved() > > MayReenableArc()? > > I agree that this may be confusing, but TBH, your splitting suggestion also > confusing to me, because calling MayReenableArc() in both here and > OnArcDataRemoved() does not have any good reasoning. > > So, instead I added comments. Does it work for you? > Note that I'd like to drastically restructure the code, but the considering the > size and its risk, I'll do it in later release. > > sure, it was only nit :) https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:383: RemoveArcData(); On 2016/08/09 13:23:56, hidehiko wrote: > On 2016/08/04 23:23:29, khmel wrote: > > nit: {} > > Let me keep this as is for consistency. > No {} for oneline-if-body. We have multi-line conditions. Based on my previous CLs I was always recommended by reviewers to use {} in such cases. https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_enterprise_reporting_service.cc (right): https://codereview.chromium.org/2209173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_enterprise_reporting_service.cc:41: ArcAuthService::Get()->RemoveArcData(); On 2016/08/09 13:23:56, hidehiko wrote: > On 2016/08/04 23:23:29, khmel wrote: > > nit. May we have one function instead? They are both public and as I > understand > > should be used always together in right order. > > > > For example OnManagementLost() > > I'd like to keep the concept as is for now, so let me keep the "device lost" > concept here. > > I'm open for better approaches/code restructuring, but let's discuss it > separately. Acknowledged. https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:191: void OnArcDataRemoved(bool success); it seems, it can be private
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:239: clear_required_ = false; This is not needed, since it's the first thing RemoveArcData() will do. If you're just worried that we can get into a bad state, I'd rather to a DCHECK(arc_bridge_service()->stopped()) to bail out if we are. If you end up doing the ScheduleArcDataWipe() thing, inline the data wiping logic here. https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:186: void ReenableArc(); How about calling this "RestartArc()"? Also, the documentation seems a bit inaccurate, since you assert if ARC is not running. How about "Calls StopArc() followed by StartArc(). When ARC is stopped, data wiping will occur if it was scheduled. This must be called only when ARC is running". https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:190: void RemoveArcData(); How about to make this less confusing, call this "ScheduleArcDataWipe()", and require it to be called when ARC is running. IIUC you only call it while ARC is stopped just in one place. https://codereview.chromium.org/2209173002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2209173002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:103: arc_user_data_service_.reset(new ArcUserDataService(arc_bridge_service(), Wouldn't this break https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_enterpri... ?
The CQ bit was checked by hidehiko@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...
Thank you for reivew. PTAL. https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:239: clear_required_ = false; On 2016/08/09 16:40:02, Luis Héctor Chávez wrote: > This is not needed, since it's the first thing RemoveArcData() will do. If > you're just worried that we can get into a bad state, I'd rather to a > DCHECK(arc_bridge_service()->stopped()) to bail out if we are. Done. > > If you end up doing the ScheduleArcDataWipe() thing, inline the data wiping > logic here. It's an option, but as RemoveArcData can be called while ARC is not running, so I prefer not to inline the removal here. WDYT? https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:186: void ReenableArc(); On 2016/08/09 16:40:02, Luis Héctor Chávez wrote: > How about calling this "RestartArc()"? Also, the documentation seems a bit > inaccurate, since you assert if ARC is not running. How about "Calls StopArc() > followed by StartArc(). When ARC is stopped, data wiping will occur if it was > scheduled. This must be called only when ARC is running". Can we keep the name? The comment was wrong and fixed in the latest PS. Sorry for confusion. https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:190: void RemoveArcData(); On 2016/08/09 16:40:02, Luis Héctor Chávez wrote: > How about to make this less confusing, call this "ScheduleArcDataWipe()", and > require it to be called when ARC is running. IIUC you only call it while ARC is > stopped just in one place. This does not actually "schedule" but remove the data. If ARC is running, it is just postponed, I think. And actually it can be called while ARC is stopped. WDYT? https://codereview.chromium.org/2209173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:191: void OnArcDataRemoved(bool success); On 2016/08/09 15:23:11, khmel wrote: > it seems, it can be private Done. https://codereview.chromium.org/2209173002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2209173002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:103: arc_user_data_service_.reset(new ArcUserDataService(arc_bridge_service(), On 2016/08/09 16:40:02, Luis Héctor Chávez wrote: > Wouldn't this break > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_enterpri... > ? In new code, it doesn't use ArcUserDataService, so should be fine.
https://codereview.chromium.org/2209173002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2209173002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:103: arc_user_data_service_.reset(new ArcUserDataService(arc_bridge_service(), On 2016/08/10 05:41:19, hidehiko wrote: > On 2016/08/09 16:40:02, Luis Héctor Chávez wrote: > > Wouldn't this break > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_enterpri... > > ? > > In new code, it doesn't use ArcUserDataService, so should be fine. can you remove the getter? it will now be useless for anybody that tries to call it. https://codereview.chromium.org/2209173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2209173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:187: void ReenableArc(); i'm still not happy with this name. reenabling omplies it is currently not enabled, but it is actually not the case since it can only be callef while it is enable. Restart is a better name overall. The |reenable_arc_| member is actually correct since it is checked while it is stopped.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In the ArcUserDataService we delete user data on startup if the setting is disabled. If the user somehow gets in to a state where there is user data but the setting is disabled (such as the case where the user powers down/crashes after disabling but before the bridge has stopped) they won't be able to clear their data without re-enabling ARC, and trying to enable ARC will fail because the sign in flow expects a signal that android only sends on first boot. Another potential edge case: If enterprise policy changes while the device is logged out, is the setting synced before or after the bridge is started? If its before then the OnBridgeStopped will never be called so data will never be cleared.
PTAL. Luis, as I told you, I'll re-run manual testing. Let me send out to shorten the iteration. Dan, thank you for pointing out edge cases. I fixed the case, but let me double-check removing data during "stopped" state is actually safe enough. I'm sending another email to discuss, and you're CC'ed. Thank you all for your review! - hidehiko https://codereview.chromium.org/2209173002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2209173002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:103: arc_user_data_service_.reset(new ArcUserDataService(arc_bridge_service(), On 2016/08/10 06:25:48, Luis Héctor Chávez wrote: > On 2016/08/10 05:41:19, hidehiko wrote: > > On 2016/08/09 16:40:02, Luis Héctor Chávez wrote: > > > Wouldn't this break > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_enterpri... > > > ? > > > > In new code, it doesn't use ArcUserDataService, so should be fine. > > can you remove the getter? it will now be useless for anybody that tries to call > it. Ok. Done. Father clean up will be done in a follow-up CL. https://codereview.chromium.org/2209173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2209173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:187: void ReenableArc(); On 2016/08/10 06:25:48, Luis Héctor Chávez wrote: > i'm still not happy with this name. reenabling omplies it is currently not > enabled, but it is actually not the case since it can only be callef while it is > enable. Restart is a better name overall. > > The |reenable_arc_| member is actually correct since it is checked while it is > stopped. Per offline discussion, renamed to StopAndEnableArc(). Later, let's clean them up more.
The CQ bit was checked by hidehiko@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...
lgtm defer to dspaid@ to shorten iteration time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
lgtm
The CQ bit was checked by hidehiko@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.
hidehiko@chromium.org changed reviewers: + stevenjb@chromium.org
Thank you for review.
stevenjb@, could you take a look at
chrome/browser/ui/app_list/arc/arc_app_test.{cc,h} as an OWNER?
lgtm
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org, dspaid@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2209173002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
Migrate ArcUserDataService into ArcAuthService.
There are race problems around data removing.
One of the cause is that state management is done in
ArcAuthService and ArcUserDataService separately.
This CL migrates ArcUserDataService into ArcAuthService
so that we can remove the data when necessary.
BUG=633258
TEST=Ran manually.
- "Enable -> ARC boot wait -> Disable" triggers data removal after
ARC stop.
- "Enable -> Disable before ARC boot" triggers immediate data
removal.
- "Enable -> ARC boot wait -> Kill ARC instance" does not trigger
data removal.
==========
to
==========
Migrate ArcUserDataService into ArcAuthService.
There are race problems around data removing.
One of the cause is that state management is done in
ArcAuthService and ArcUserDataService separately.
This CL migrates ArcUserDataService into ArcAuthService
so that we can remove the data when necessary.
BUG=633258
TEST=Ran manually.
- "Enable -> ARC boot wait -> Disable" triggers data removal after
ARC stop.
- "Enable -> Disable before ARC boot" triggers immediate data
removal.
- "Enable -> ARC boot wait -> Kill ARC instance" does not trigger
data removal.
Committed: https://crrev.com/6d95ad318e992261c45f5a88a0a599663a6d66ae
Cr-Commit-Position: refs/heads/master@{#411923}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6d95ad318e992261c45f5a88a0a599663a6d66ae Cr-Commit-Position: refs/heads/master@{#411923} |
