Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541173002:
arc: Make request to remove Android's data folder persistent. (Closed)
|
Created:
4 years ago by khmel Modified:
4 years 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Make request to remove Android's data folder persistent.
Before request to remove Android's data folder lived runtime
only during the current user session. This caused problem when
remove data folder request did not complete due interruption on
user sign out. As result on next sign up data folder was not
removed and this might cause problems with sign in. This CL
makes this request persistent by adding pref flag which is
reset by callback that data is actually removed or by signalling
that sing in completed successfully. In case user sings up,
Arc enabled and request to remove data is active then data folder
is removed first and Arc is restarted after this operation.
BUG=b/32500143
TEST=Manually on device. Simulate provisioning fail and sign up when
error window is active. Sign-in again and based on logs, data
folder was removed and Arc started after this.
Login normally, sign-out and sign in again. No data removal
occurred.
Committed: https://crrev.com/04471f8085185bbdeb796b5539844f8dcf257ba8
Cr-Commit-Position: refs/heads/master@{#436688}
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 12
Patch Set 3 : rename pref flag / update comments #
Total comments: 12
Patch Set 4 : browser tests extended/updated #
Total comments: 4
Patch Set 5 : comments addressed #
Total comments: 25
Patch Set 6 : use states / rebased #
Total comments: 7
Patch Set 7 : comments addressed #Patch Set 8 : fix browser tests #
Total comments: 1
Messages
Total messages: 36 (17 generated)
khmel@chromium.org changed reviewers: + hidehiko@chromium.org, lhchavez@lhchavez.com
Hi, PTAL
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
lgtm, but please s/sing/sign/, s/Arc/ARC/, s/clean/remove/ (when it refers to data removal) on the commit description. https://codereview.chromium.org/2541173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:209: // OnArcDataRemoved resets this flag. Ah, I like this better :) https://codereview.chromium.org/2541173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:269: // Make sure request to clean data folder is off. nit: s/clean/remove/ https://codereview.chromium.org/2541173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:577: // Don't start Arc until data is marked as invalid. Restart Arc once data is nit: Don't start ARC if there is a pending request to remove the data. Restart ARC once data removal finishes. https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:25: // A preference to indicate that Android data is invalid and must be cleared. nit: A preference to indicate that Android's /data directory should be removed. https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:26: const char kArcDataInvalid[] = "arc.data.invalid"; nit: "arc.data.remove_requested" https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.h:24: extern const char kArcDataInvalid[]; nit: kArcDataRemoveRequested
Description was changed from
==========
arc: Make request to clean Android data persistent.
Before request to clean Android data lived runtime only during
current user session. This caused problem when clean up request
did not complete due interruption on user sign out. As result on
next sign up data folder was not cleaned up and this might cause
problems with sign in. This CL makes request persistent by adding
pref flag which is reset by callback that data is actually removed
or by signalling that sing in completed successfully. In case user
sings up, Arc enabled and request to clean data is active then
data is cleaned first and Arc is restarted after this operation.
BUG=b/32500143
TEST=Manually on device. Simulate provisioning fail and sign up when
error window is active. Sign-in again and based on logs, data
was cleaned up and Arc started after this.
Login normally, sign-out and sign in again. No data clean up
occured.
==========
to
==========
arc: Make request to remove Android's data folder persistent.
Before request to remove Android's data folder lived runtime
only during the current user session. This caused problem when
remove data folder request did not complete due interruption on
user sign out. As result on next sign up data folder was not
removed and this might cause problems with sign in. This CL
makes this request persistent by adding pref flag which is
reset by callback that data is actually removed or by signalling
that sing in completed successfully. In case user sings up,
Arc enabled and request to remove data is active then data folder
is removed first and Arc is restarted after this operation.
BUG=b/32500143
TEST=Manually on device. Simulate provisioning fail and sign up when
error window is active. Sign-in again and based on logs, data
folder was removed and Arc started after this.
Login normally, sign-out and sign in again. No data removal
occurred.
==========
khmel@chromium.org changed reviewers: - lhchavez@lhchavez.com
Thank you Luis for comments! https://codereview.chromium.org/2541173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:209: // OnArcDataRemoved resets this flag. On 2016/11/30 23:30:14, Luis Héctor Chávez wrote: > Ah, I like this better :) Acknowledged. https://codereview.chromium.org/2541173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:269: // Make sure request to clean data folder is off. On 2016/11/30 23:30:14, Luis Héctor Chávez wrote: > nit: s/clean/remove/ Done. https://codereview.chromium.org/2541173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:577: // Don't start Arc until data is marked as invalid. Restart Arc once data is On 2016/11/30 23:30:14, Luis Héctor Chávez wrote: > nit: Don't start ARC if there is a pending request to remove the data. Restart > ARC once data removal finishes. Done. https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:25: // A preference to indicate that Android data is invalid and must be cleared. On 2016/11/30 23:30:14, Luis Héctor Chávez wrote: > nit: A preference to indicate that Android's /data directory should be removed. Done. https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:26: const char kArcDataInvalid[] = "arc.data.invalid"; On 2016/11/30 23:30:14, Luis Héctor Chávez wrote: > nit: "arc.data.remove_requested" Done. That is nicer, thanks! https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2541173002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.h:24: extern const char kArcDataInvalid[]; On 2016/11/30 23:30:15, Luis Héctor Chávez wrote: > nit: kArcDataRemoveRequested Done.
The CQ bit was checked by khmel@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Could you add unittest to cover the case? https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:269: // Make sure request to remove data folder is off. Better to comment an example case that this actually needs to be. https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:579: if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) { Should this be in OnPrimaryUserProfilePrepared() ? https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:580: reenable_arc_ = true; This has an edge case: Remove request is set in perf. System rebooted. StartArc() triggers. RemoveArcData() triggers to remove the data. * While removing, ARC is disabled. Then, OnRemoveArcData() overrides the state?
Also, linux_chromium_chromeos_rel_ng failure looks a real one. Could you take a look into it, as well?
Thank you Hidehiko for your comments. I also added additional browser_tests. PTAL https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:269: // Make sure request to remove data folder is off. On 2016/12/01 00:58:02, hidehiko wrote: > Better to comment an example case that this actually needs to be. I don't see any use case. Probably we can change it to DHCECK(!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)); https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:579: if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) { On 2016/12/01 00:58:02, hidehiko wrote: > Should this be in OnPrimaryUserProfilePrepared() ? I think this is more reliable point to handle this case. Assume following test case: Profile is inited and arc.enabled is off In this case we force removing data. While data is been removing arc.enabled is set to true. As result StartArc may be called while data removal is in progress. Having it here guaranties that Arc won't be started until invalidated data is removed. https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:580: reenable_arc_ = true; On 2016/12/01 00:58:02, hidehiko wrote: > This has an edge case: > > Remove request is set in perf. > System rebooted. > StartArc() triggers. > RemoveArcData() triggers to remove the data. > * While removing, ARC is disabled. > Then, OnRemoveArcData() overrides the state? Good catch. I think in this case we can reset reenable_arc_ to make sure that Arc won't be re-enabled. WDYT?
https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:269: // Make sure request to remove data folder is off. On 2016/12/01 03:12:20, khmel wrote: > On 2016/12/01 00:58:02, hidehiko wrote: > > Better to comment an example case that this actually needs to be. > > I don't see any use case. Probably we can change it to > DHCECK(!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)); After some more investigation I found a case that may hit this: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/enterprise/a... IIUC, there is no guarantee about the invocation timing of the mojo call. If no guarantee, please remove this. Otherwise the flag will be unintentionally removed. If there is, could you replace by DCHECK and comment the reason why this should be always false? https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:579: if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) { On 2016/12/01 03:12:21, khmel wrote: > On 2016/12/01 00:58:02, hidehiko wrote: > > Should this be in OnPrimaryUserProfilePrepared() ? > > I think this is more reliable point to handle this case. Assume following test > case: > Profile is inited and arc.enabled is off > In this case we force removing data. > While data is been removing arc.enabled is set to true. As result StartArc may > be called while data removal is in progress. Good example scenario. I think, in that case, we should not call StartArc(). It is because; - It actually calls RemoveArcData() twice, which is not what we want here. - Additionally, Not-starting the ARC by StartArc() is confusing. Also, could you add the unittest scenario for that, too? > Having it here guaranties that Arc won't be started until invalidated data is > removed. https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:580: reenable_arc_ = true; On 2016/12/01 03:12:20, khmel wrote: > On 2016/12/01 00:58:02, hidehiko wrote: > > This has an edge case: > > > > Remove request is set in perf. > > System rebooted. > > StartArc() triggers. > > RemoveArcData() triggers to remove the data. > > * While removing, ARC is disabled. > > Then, OnRemoveArcData() overrides the state? > > Good catch. I think in this case we can reset reenable_arc_ to make sure that > Arc won't be re-enabled. > WDYT? IsAllowed() check looks not the one we need here? We actually need to check if the ARC is still enabled there. Also, L230 (in new PS) looks not the right place to check, because, e.g., anyway the flag needs to be reset. https://codereview.chromium.org/2541173002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc (right): https://codereview.chromium.org/2541173002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:238: IN_PROC_BROWSER_TEST_F(ArcSessionManagerTest, RemoveDataFolder) { Could you move this into unittest, instead of browser_test? It should be much simpler and quickly run. https://codereview.chromium.org/2541173002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:264: // After remove Arc is started. nit: s/Arc/ARC/ for consistency.
PTAL https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:269: // Make sure request to remove data folder is off. On 2016/12/01 16:15:03, hidehiko wrote: > On 2016/12/01 03:12:20, khmel wrote: > > On 2016/12/01 00:58:02, hidehiko wrote: > > > Better to comment an example case that this actually needs to be. > > > > I don't see any use case. Probably we can change it to > > DHCECK(!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)); > > After some more investigation I found a case that may hit this: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/enterprise/a... > > IIUC, there is no guarantee about the invocation timing of the mojo call. > If no guarantee, please remove this. Otherwise the flag will be unintentionally > removed. > If there is, could you replace by DCHECK and comment the reason why this should > be always false? Replaced by DCHECK https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:579: if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) { On 2016/12/01 16:15:03, hidehiko wrote: > On 2016/12/01 03:12:21, khmel wrote: > > On 2016/12/01 00:58:02, hidehiko wrote: > > > Should this be in OnPrimaryUserProfilePrepared() ? > > > > I think this is more reliable point to handle this case. Assume following test > > case: > > Profile is inited and arc.enabled is off > > In this case we force removing data. > > While data is been removing arc.enabled is set to true. As result StartArc may > > be called while data removal is in progress. > > Good example scenario. I think, in that case, we should not call StartArc(). It > is because; > - It actually calls RemoveArcData() twice, which is not what we want here. > - Additionally, Not-starting the ARC by StartArc() is confusing. > > Also, could you add the unittest scenario for that, too? > > > Having it here guaranties that Arc won't be started until invalidated data is > > removed. > I moved this check to upper level OnOptInPreferenceChanged. Also added runtime flag to prevent double removal request https://codereview.chromium.org/2541173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:580: reenable_arc_ = true; On 2016/12/01 16:15:03, hidehiko wrote: > On 2016/12/01 03:12:20, khmel wrote: > > On 2016/12/01 00:58:02, hidehiko wrote: > > > This has an edge case: > > > > > > Remove request is set in perf. > > > System rebooted. > > > StartArc() triggers. > > > RemoveArcData() triggers to remove the data. > > > * While removing, ARC is disabled. > > > Then, OnRemoveArcData() overrides the state? > > > > Good catch. I think in this case we can reset reenable_arc_ to make sure that > > Arc won't be re-enabled. > > WDYT? > > IsAllowed() check looks not the one we need here? > We actually need to check if the ARC is still enabled there. > > Also, L230 (in new PS) looks not the right place to check, because, e.g., anyway > the flag needs to be reset. IsAllowed is required to safely reset request. It checks for profile_ attached (Shutdown resets profile). Also added check IsArcEnabled below. https://codereview.chromium.org/2541173002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc (right): https://codereview.chromium.org/2541173002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:238: IN_PROC_BROWSER_TEST_F(ArcSessionManagerTest, RemoveDataFolder) { On 2016/12/01 16:15:03, hidehiko wrote: > Could you move this into unittest, instead of browser_test? > It should be much simpler and quickly run. Done. https://codereview.chromium.org/2541173002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:264: // After remove Arc is started. On 2016/12/01 16:15:03, hidehiko wrote: > nit: s/Arc/ARC/ for consistency. Done.
https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:210: if (data_removal_in_progress_) So this is state management. Could you add a new State instead of independent bool flag? https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:237: if (IsAllowed()) Hmm, ok. So this check is only for browser_test, right? Indeed, in some test, Shutdown() is directly called, but in real usage, it is only from dtor. (Note: it is also in OnPrimaryUserProfilePrepared(), but it looks something redundant). Then, could you return immediately at the top of this function with commenting it is only for browser test and the code should be cleaned later? https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:279: // No data removal request is expected on Arc boot. It is still unclear to me why this is guaranteed. # And, actually, consecutive ArcEnterpriseReportingService::ReportManagementState(MANAGED_DO_LOST) and ArcAuthService::OnSignInComplete() invocation looks to hit the case, at least. Could you comment what guarantees the condition? https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:516: // Don't start ARC if there is a pending request to remove the data. Restart I still think this is a part of OnPrimaryUserProfilePrepared, because this should happen only when user logs in at ChromeOS. Is there any other cases we need to take a look? Then, could you comment? https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:411: ArcSessionManager::Get()->RemoveArcData(); s/ArcSessionManager::Get()/arc_session_manager()/ https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:415: ArcSessionManager::Get()->Shutdown(); Ditto. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:421: // Emulate next sign-in. Data should be removed first and ARC started after. To emulate re-sign-in, shouldn't we re-instantiate the arc_session_manager()? https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:433: ArcSessionManager::Get()->Shutdown(); Ditto.
Thank you for your comments. PTAL updated version https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:210: if (data_removal_in_progress_) On 2016/12/02 08:54:55, hidehiko wrote: > So this is state management. Could you add a new State instead of independent > bool flag? Good idea, done. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:237: if (IsAllowed()) On 2016/12/02 08:54:55, hidehiko wrote: > Hmm, ok. So this check is only for browser_test, right? > Indeed, in some test, Shutdown() is directly called, but in real usage, it is > only from dtor. (Note: it is also in OnPrimaryUserProfilePrepared(), but it > looks something redundant). > > Then, could you return immediately at the top of this function with commenting > it is only for browser test and the code should be cleaned later? Done. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:279: // No data removal request is expected on Arc boot. On 2016/12/02 08:54:55, hidehiko wrote: > It is still unclear to me why this is guaranteed. > > # And, actually, consecutive > ArcEnterpriseReportingService::ReportManagementState(MANAGED_DO_LOST) and > ArcAuthService::OnSignInComplete() invocation looks to hit the case, at least. > > Could you comment what guarantees the condition? ReportManagementState(MANAGED_DO_LOST) calls 2 functions: RemoveArcData and StopAndEnableArc Theoretically OnProvisioningFinished(ProvisioningResult::SUCCESS) may happen due async nature of bridge stopping. However we already have check DCHECK_EQ(state_, State::ACTIVE) at the beginning of this function; That means this check is also incorrect because StopAndEnableArc sets state to STOP, right? From this they are both incorrect or correct. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:516: // Don't start ARC if there is a pending request to remove the data. Restart On 2016/12/02 08:54:55, hidehiko wrote: > I still think this is a part of OnPrimaryUserProfilePrepared, because this > should happen only when user logs in at ChromeOS. > Is there any other cases we need to take a look? Then, could you comment? I refactored this code based on state we introduced and moving analyzing this flag to OnPrimaryUserProfilePrepared https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:411: ArcSessionManager::Get()->RemoveArcData(); On 2016/12/02 08:54:55, hidehiko wrote: > s/ArcSessionManager::Get()/arc_session_manager()/ Done. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:415: ArcSessionManager::Get()->Shutdown(); On 2016/12/02 08:54:55, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:421: // Emulate next sign-in. Data should be removed first and ARC started after. On 2016/12/02 08:54:55, hidehiko wrote: > To emulate re-sign-in, shouldn't we re-instantiate the arc_session_manager()? Hmm, probably I did not get you. Second and next OnPrimaryUserProfilePrepared(profile()) is very common pattern in this file and used in many tests here. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:433: ArcSessionManager::Get()->Shutdown(); On 2016/12/02 08:54:55, hidehiko wrote: > Ditto. Done.
Almost looks good! Several minor comments. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:279: // No data removal request is expected on Arc boot. On 2016/12/06 03:23:57, khmel wrote: > On 2016/12/02 08:54:55, hidehiko wrote: > > It is still unclear to me why this is guaranteed. > > > > # And, actually, consecutive > > ArcEnterpriseReportingService::ReportManagementState(MANAGED_DO_LOST) and > > ArcAuthService::OnSignInComplete() invocation looks to hit the case, at least. > > > > Could you comment what guarantees the condition? > > ReportManagementState(MANAGED_DO_LOST) calls 2 functions: > RemoveArcData and > StopAndEnableArc > > Theoretically OnProvisioningFinished(ProvisioningResult::SUCCESS) may happen > due async nature of bridge stopping. However we already have check > DCHECK_EQ(state_, State::ACTIVE) at the beginning of this function; That means > this check is also incorrect because StopAndEnableArc sets state to STOP, right? > From this they are both incorrect or correct. Oh, good catch! So, the DCHECK above is wrong... Could you remove both DCHECK with comments? https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:516: // Don't start ARC if there is a pending request to remove the data. Restart On 2016/12/06 03:23:57, khmel wrote: > On 2016/12/02 08:54:55, hidehiko wrote: > > I still think this is a part of OnPrimaryUserProfilePrepared, because this > > should happen only when user logs in at ChromeOS. > > Is there any other cases we need to take a look? Then, could you comment? > > I refactored this code based on state we introduced and moving analyzing this > flag to OnPrimaryUserProfilePrepared Nice! https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:58: base::WrapUnique(new chromeos::FakeSessionManagerClient)); nit: base::MakeUnique<chromeos::FakeSessionManagerClient>() http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-... https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:421: // Emulate next sign-in. Data should be removed first and ARC started after. On 2016/12/06 03:23:57, khmel wrote: > On 2016/12/02 08:54:55, hidehiko wrote: > > To emulate re-sign-in, shouldn't we re-instantiate the arc_session_manager()? > > Hmm, probably I did not get you. Second and next > OnPrimaryUserProfilePrepared(profile()) is very common pattern in this file and > used in many tests here. Oh, sorry for my poor explanation. I was wondering if re-creating the instance can help to remove the check only for testing at arc_session_manager:L236. If not, I'm fine to leave this as is in this CL. Note that, TBH, it's better to recreate ArcSessionManager instance (in any test to emulate re-sign-in) to make emulation more closer to the ARC's real behavior, but it looks out of focus of this CL, I think. Let's address it later. https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:207: MaybeReenableArc(); PostTask is the workaround for crbug.com/665316, and it is not yet fixed, so I think it still needs to be kept here. https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:97: DELETING_DATA_FOLDER, REMOVING_DATA_DIR or REMOVING_DATA_DIRECTORY for consistency. https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:106: ASSERT_EQ(ArcSessionManager::State::DELETING_DATA_FOLDER, This won't work. ASSERT_EQ is a macro to return from the current function. So, ASSERT_EQ in a utility method just returns from the utility method, but will not quit the test scenario in caller, unfortunately.
Note: could you run trybots? Also, in later updating, i'd appreciate if you split "rebase" and "your change" into another PS, so that it will be easier for me to take a look. Thank you for your cooperation in advance!
The CQ bit was checked by khmel@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:279: // No data removal request is expected on Arc boot. On 2016/12/06 05:05:30, hidehiko wrote: > On 2016/12/06 03:23:57, khmel wrote: > > On 2016/12/02 08:54:55, hidehiko wrote: > > > It is still unclear to me why this is guaranteed. > > > > > > # And, actually, consecutive > > > ArcEnterpriseReportingService::ReportManagementState(MANAGED_DO_LOST) and > > > ArcAuthService::OnSignInComplete() invocation looks to hit the case, at > least. > > > > > > Could you comment what guarantees the condition? > > > > ReportManagementState(MANAGED_DO_LOST) calls 2 functions: > > RemoveArcData and > > StopAndEnableArc > > > > Theoretically OnProvisioningFinished(ProvisioningResult::SUCCESS) may happen > > due async nature of bridge stopping. However we already have check > > DCHECK_EQ(state_, State::ACTIVE) at the beginning of this function; That means > > this check is also incorrect because StopAndEnableArc sets state to STOP, > right? > > From this they are both incorrect or correct. > > Oh, good catch! So, the DCHECK above is wrong... > Could you remove both DCHECK with comments? Done. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:516: // Don't start ARC if there is a pending request to remove the data. Restart On 2016/12/06 05:05:30, hidehiko wrote: > On 2016/12/06 03:23:57, khmel wrote: > > On 2016/12/02 08:54:55, hidehiko wrote: > > > I still think this is a part of OnPrimaryUserProfilePrepared, because this > > > should happen only when user logs in at ChromeOS. > > > Is there any other cases we need to take a look? Then, could you comment? > > > > I refactored this code based on state we introduced and moving analyzing this > > flag to OnPrimaryUserProfilePrepared > > Nice! Acknowledged. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:58: base::WrapUnique(new chromeos::FakeSessionManagerClient)); On 2016/12/06 05:05:30, hidehiko wrote: > nit: base::MakeUnique<chromeos::FakeSessionManagerClient>() > > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-... Done. https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:421: // Emulate next sign-in. Data should be removed first and ARC started after. On 2016/12/06 05:05:30, hidehiko wrote: > On 2016/12/06 03:23:57, khmel wrote: > > On 2016/12/02 08:54:55, hidehiko wrote: > > > To emulate re-sign-in, shouldn't we re-instantiate the > arc_session_manager()? > > > > Hmm, probably I did not get you. Second and next > > OnPrimaryUserProfilePrepared(profile()) is very common pattern in this file > and > > used in many tests here. > > Oh, sorry for my poor explanation. > I was wondering if re-creating the instance can help to remove the check only > for testing at arc_session_manager:L236. > If not, I'm fine to leave this as is in this CL. > > Note that, TBH, it's better to recreate ArcSessionManager instance (in any test > to emulate re-sign-in) to make emulation more closer to the ARC's real behavior, > but it looks out of focus of this CL, I think. Let's address it later. I did similar what you suggested in some other tests where ArcAppListPrefs also included into the testing and recreating managers is required. In this tests we can test this component more independently so this is not strictly required. Also this test does not affect check you mentioned above because we implemented wait cycles to test that state switches correctly. https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:207: MaybeReenableArc(); On 2016/12/06 05:05:30, hidehiko wrote: > PostTask is the workaround for crbug.com/665316, and it is not yet fixed, so I > think it still needs to be kept here. It seems that problem quite complicated based on task desc. Let address in separate CL. https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:97: DELETING_DATA_FOLDER, On 2016/12/06 05:05:30, hidehiko wrote: > REMOVING_DATA_DIR or REMOVING_DATA_DIRECTORY for consistency. Good catch. https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:106: ASSERT_EQ(ArcSessionManager::State::DELETING_DATA_FOLDER, On 2016/12/06 05:05:30, hidehiko wrote: > This won't work. > > ASSERT_EQ is a macro to return from the current function. > So, ASSERT_EQ in a utility method just returns from the utility method, but will > not quit the test scenario in caller, unfortunately. IIUC abort is called if any problem with ASSERT_EQ. Anyway I changed to returning bool result and checking it in test body.
lgtm! https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:421: // Emulate next sign-in. Data should be removed first and ARC started after. On 2016/12/06 18:16:12, khmel wrote: > On 2016/12/06 05:05:30, hidehiko wrote: > > On 2016/12/06 03:23:57, khmel wrote: > > > On 2016/12/02 08:54:55, hidehiko wrote: > > > > To emulate re-sign-in, shouldn't we re-instantiate the > > arc_session_manager()? > > > > > > Hmm, probably I did not get you. Second and next > > > OnPrimaryUserProfilePrepared(profile()) is very common pattern in this file > > and > > > used in many tests here. > > > > Oh, sorry for my poor explanation. > > I was wondering if re-creating the instance can help to remove the check only > > for testing at arc_session_manager:L236. > > If not, I'm fine to leave this as is in this CL. > > > > Note that, TBH, it's better to recreate ArcSessionManager instance (in any > test > > to emulate re-sign-in) to make emulation more closer to the ARC's real > behavior, > > but it looks out of focus of this CL, I think. Let's address it later. > > I did similar what you suggested in some other tests where ArcAppListPrefs also > included into the testing and recreating managers is required. In this tests we > can test this component more independently so this is not strictly required. > Also this test does not affect check you mentioned above because we implemented > wait cycles to test that state switches correctly. My understanding is, the check in the callback is for testing situation where the Shutdown() is called, but the instance is not yet deleted and the message loop is running (like L416). Because the callback is bound with ArcSessionManager's weak_ptr, if we can delete the instance after Shutdown always, the if check should not be there, as callback should never be called then. Anyway, I'm fine to leave it for now, and let's address it later. https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:106: ASSERT_EQ(ArcSessionManager::State::DELETING_DATA_FOLDER, On 2016/12/06 18:16:13, khmel wrote: > On 2016/12/06 05:05:30, hidehiko wrote: > > This won't work. > > > > ASSERT_EQ is a macro to return from the current function. > > So, ASSERT_EQ in a utility method just returns from the utility method, but > will > > not quit the test scenario in caller, unfortunately. > > IIUC abort is called if any problem with ASSERT_EQ. Anyway I changed to > returning bool result and checking it in test body. FYI: https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/internal/gte... is the impl, IIUC. https://codereview.chromium.org/2541173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:272: // Due asynchronous nature of stopping Arc bridge, OnProvisioningFinished may Nice commenting!
lgtm! https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:421: // Emulate next sign-in. Data should be removed first and ARC started after. On 2016/12/06 18:16:12, khmel wrote: > On 2016/12/06 05:05:30, hidehiko wrote: > > On 2016/12/06 03:23:57, khmel wrote: > > > On 2016/12/02 08:54:55, hidehiko wrote: > > > > To emulate re-sign-in, shouldn't we re-instantiate the > > arc_session_manager()? > > > > > > Hmm, probably I did not get you. Second and next > > > OnPrimaryUserProfilePrepared(profile()) is very common pattern in this file > > and > > > used in many tests here. > > > > Oh, sorry for my poor explanation. > > I was wondering if re-creating the instance can help to remove the check only > > for testing at arc_session_manager:L236. > > If not, I'm fine to leave this as is in this CL. > > > > Note that, TBH, it's better to recreate ArcSessionManager instance (in any > test > > to emulate re-sign-in) to make emulation more closer to the ARC's real > behavior, > > but it looks out of focus of this CL, I think. Let's address it later. > > I did similar what you suggested in some other tests where ArcAppListPrefs also > included into the testing and recreating managers is required. In this tests we > can test this component more independently so this is not strictly required. > Also this test does not affect check you mentioned above because we implemented > wait cycles to test that state switches correctly. My understanding is, the check in the callback is for testing situation where the Shutdown() is called, but the instance is not yet deleted and the message loop is running (like L416). Because the callback is bound with ArcSessionManager's weak_ptr, if we can delete the instance after Shutdown always, the if check should not be there, as callback should never be called then. Anyway, I'm fine to leave it for now, and let's address it later. https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2541173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:106: ASSERT_EQ(ArcSessionManager::State::DELETING_DATA_FOLDER, On 2016/12/06 18:16:13, khmel wrote: > On 2016/12/06 05:05:30, hidehiko wrote: > > This won't work. > > > > ASSERT_EQ is a macro to return from the current function. > > So, ASSERT_EQ in a utility method just returns from the utility method, but > will > > not quit the test scenario in caller, unfortunately. > > IIUC abort is called if any problem with ASSERT_EQ. Anyway I changed to > returning bool result and checking it in test body. FYI: https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/internal/gte... is the impl, IIUC. https://codereview.chromium.org/2541173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2541173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:272: // Due asynchronous nature of stopping Arc bridge, OnProvisioningFinished may Nice commenting!
Thank you for review. Trying to land it now.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2541173002/#ps140001 (title: "fix browser tests")
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": 1481049220633880,
"parent_rev": "72e1eb79243eb32d1cabcf2ad35a967c3e210231", "commit_rev":
"b07a1a23266dcb7f585ccbcc939563f5734e8980"}
Message was sent while issue was closed.
Description was changed from
==========
arc: Make request to remove Android's data folder persistent.
Before request to remove Android's data folder lived runtime
only during the current user session. This caused problem when
remove data folder request did not complete due interruption on
user sign out. As result on next sign up data folder was not
removed and this might cause problems with sign in. This CL
makes this request persistent by adding pref flag which is
reset by callback that data is actually removed or by signalling
that sing in completed successfully. In case user sings up,
Arc enabled and request to remove data is active then data folder
is removed first and Arc is restarted after this operation.
BUG=b/32500143
TEST=Manually on device. Simulate provisioning fail and sign up when
error window is active. Sign-in again and based on logs, data
folder was removed and Arc started after this.
Login normally, sign-out and sign in again. No data removal
occurred.
==========
to
==========
arc: Make request to remove Android's data folder persistent.
Before request to remove Android's data folder lived runtime
only during the current user session. This caused problem when
remove data folder request did not complete due interruption on
user sign out. As result on next sign up data folder was not
removed and this might cause problems with sign in. This CL
makes this request persistent by adding pref flag which is
reset by callback that data is actually removed or by signalling
that sing in completed successfully. In case user sings up,
Arc enabled and request to remove data is active then data folder
is removed first and Arc is restarted after this operation.
BUG=b/32500143
TEST=Manually on device. Simulate provisioning fail and sign up when
error window is active. Sign-in again and based on logs, data
folder was removed and Arc started after this.
Login normally, sign-out and sign in again. No data removal
occurred.
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
arc: Make request to remove Android's data folder persistent.
Before request to remove Android's data folder lived runtime
only during the current user session. This caused problem when
remove data folder request did not complete due interruption on
user sign out. As result on next sign up data folder was not
removed and this might cause problems with sign in. This CL
makes this request persistent by adding pref flag which is
reset by callback that data is actually removed or by signalling
that sing in completed successfully. In case user sings up,
Arc enabled and request to remove data is active then data folder
is removed first and Arc is restarted after this operation.
BUG=b/32500143
TEST=Manually on device. Simulate provisioning fail and sign up when
error window is active. Sign-in again and based on logs, data
folder was removed and Arc started after this.
Login normally, sign-out and sign in again. No data removal
occurred.
==========
to
==========
arc: Make request to remove Android's data folder persistent.
Before request to remove Android's data folder lived runtime
only during the current user session. This caused problem when
remove data folder request did not complete due interruption on
user sign out. As result on next sign up data folder was not
removed and this might cause problems with sign in. This CL
makes this request persistent by adding pref flag which is
reset by callback that data is actually removed or by signalling
that sing in completed successfully. In case user sings up,
Arc enabled and request to remove data is active then data folder
is removed first and Arc is restarted after this operation.
BUG=b/32500143
TEST=Manually on device. Simulate provisioning fail and sign up when
error window is active. Sign-in again and based on logs, data
folder was removed and Arc started after this.
Login normally, sign-out and sign in again. No data removal
occurred.
Committed: https://crrev.com/04471f8085185bbdeb796b5539844f8dcf257ba8
Cr-Commit-Position: refs/heads/master@{#436688}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/04471f8085185bbdeb796b5539844f8dcf257ba8 Cr-Commit-Position: refs/heads/master@{#436688} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
