Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2145813002:
Clear old ARC instance before opt-in. (Closed)
|
Created:
4 years, 5 months ago by dspaid Modified:
4 years, 5 months ago Reviewers:
hidehiko 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. |
DescriptionShow error during opt-in if arc-bridge not stopped.
If a user attempts to disable and re-enable ARC in quick succession user
data from the former instance will not be cleared by the normal wipe
process because kArcEnabled is true again when the bridge is stopped.
To work around this the opt-in process will display an error if
attempting to opt-in before the previous ARC instance has shut down and
it will wipe data explicitly.
BUG=627664
TEST=Manually test with following steps:
Enable ARC
Disable ARC
Immediately re-enable ARC
Verify error message displayed
Verify user data directory removed
Click Try again
Verify ARC starts up appropriately
Committed: https://crrev.com/fe1a51e9faf942ace5613490077fefba8f592621
Cr-Commit-Position: refs/heads/master@{#406216}
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase #
Total comments: 1
Patch Set 3 : Move forced wipe logic to arc_user_data_service #Patch Set 4 : update comment #Patch Set 5 : Fixed test #
Total comments: 1
Patch Set 6 : Use prefs() from PrefMember instead of passing a new parameter #
Total comments: 4
Patch Set 7 : Address Comments #
Messages
Total messages: 32 (20 generated)
dspaid@chromium.org changed reviewers: + hidehiko@chromium.org
The Subject looks not matching to the what this CL does? https://codereview.chromium.org/2145813002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2145813002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:666: if (arc_bridge_service()->state() != ArcBridgeService::State::STOPPED) { Could you rebase and adapt to the newest code?
Description was changed from ========== Clear old ARC instance before opt-in. If a user attempts to disable and re-enable ARC in quick succession user data from the former instance will not be cleared by the normal wipe process because kArcEnabled is true again when the bridge is stopped. To work around this the opt-in process will display an error if attempting to opt-in before the previous ARC instance has shut down and it will wipe data explicitly. BUG=627664 TEST=Manually test with following steps: Enable ARC Disable ARC Immediately re-enable ARC Verify error message displayed Verify user data directory removed Click Try again Verify ARC starts up appropriately ========== to ========== Show error during opt-in if arc-bridge not stopped. If a user attempts to disable and re-enable ARC in quick succession user data from the former instance will not be cleared by the normal wipe process because kArcEnabled is true again when the bridge is stopped. To work around this the opt-in process will display an error if attempting to opt-in before the previous ARC instance has shut down and it will wipe data explicitly. BUG=627664 TEST=Manually test with following steps: Enable ARC Disable ARC Immediately re-enable ARC Verify error message displayed Verify user data directory removed Click Try again Verify ARC starts up appropriately ==========
rebased and updated title.
https://codereview.chromium.org/2145813002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2145813002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:670: clear_required_ = true; For the record of the offline chat. Could you take a look at an alternative way to clear the data? (Having an error UI here sounds reasonable to me). E.g., remembering that the user explicitly disabled the ARC, until the ABS reports that ARC is actually stopped, etc.?
On 2016/07/14 05:11:26, hidehiko wrote: > https://codereview.chromium.org/2145813002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > https://codereview.chromium.org/2145813002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_auth_service.cc:670: clear_required_ = true; > For the record of the offline chat. > > Could you take a look at an alternative way to clear the data? (Having an error > UI here sounds reasonable to me). > E.g., remembering that the user explicitly disabled the ARC, until the ABS > reports that ARC is actually stopped, etc.? Done
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 checked by dspaid@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_compile_dbg_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 dspaid@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.
https://codereview.chromium.org/2145813002/diff/80001/components/arc/user_dat... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/2145813002/diff/80001/components/arc/user_dat... components/arc/user_data/arc_user_data_service.cc:30: pref_change_registrar_.Init(pref_service); arc_enabled_pref_->prefs() should work?
On 2016/07/15 14:39:23, hidehiko wrote: > https://codereview.chromium.org/2145813002/diff/80001/components/arc/user_dat... > File components/arc/user_data/arc_user_data_service.cc (right): > > https://codereview.chromium.org/2145813002/diff/80001/components/arc/user_dat... > components/arc/user_data/arc_user_data_service.cc:30: > pref_change_registrar_.Init(pref_service); > arc_enabled_pref_->prefs() should work? Thanks, forgot about that. Much cleaner now.
The CQ bit was checked by dspaid@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 with minor comments. https://codereview.chromium.org/2145813002/diff/100001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/2145813002/diff/100001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:18: class PrefService; Unused. https://codereview.chromium.org/2145813002/diff/100001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:65: base::WeakPtrFactory<ArcUserDataService> weak_ptr_factory_; Could you add #include?
https://codereview.chromium.org/2145813002/diff/100001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/2145813002/diff/100001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:18: class PrefService; On 2016/07/19 02:08:29, hidehiko wrote: > Unused. Done. https://codereview.chromium.org/2145813002/diff/100001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:65: base::WeakPtrFactory<ArcUserDataService> weak_ptr_factory_; On 2016/07/19 02:08:29, hidehiko wrote: > Could you add #include? Done.
The CQ bit was checked by dspaid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2145813002/#ps120001 (title: "Address Comments")
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.
Description was changed from ========== Show error during opt-in if arc-bridge not stopped. If a user attempts to disable and re-enable ARC in quick succession user data from the former instance will not be cleared by the normal wipe process because kArcEnabled is true again when the bridge is stopped. To work around this the opt-in process will display an error if attempting to opt-in before the previous ARC instance has shut down and it will wipe data explicitly. BUG=627664 TEST=Manually test with following steps: Enable ARC Disable ARC Immediately re-enable ARC Verify error message displayed Verify user data directory removed Click Try again Verify ARC starts up appropriately ========== to ========== Show error during opt-in if arc-bridge not stopped. If a user attempts to disable and re-enable ARC in quick succession user data from the former instance will not be cleared by the normal wipe process because kArcEnabled is true again when the bridge is stopped. To work around this the opt-in process will display an error if attempting to opt-in before the previous ARC instance has shut down and it will wipe data explicitly. BUG=627664 TEST=Manually test with following steps: Enable ARC Disable ARC Immediately re-enable ARC Verify error message displayed Verify user data directory removed Click Try again Verify ARC starts up appropriately ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Show error during opt-in if arc-bridge not stopped. If a user attempts to disable and re-enable ARC in quick succession user data from the former instance will not be cleared by the normal wipe process because kArcEnabled is true again when the bridge is stopped. To work around this the opt-in process will display an error if attempting to opt-in before the previous ARC instance has shut down and it will wipe data explicitly. BUG=627664 TEST=Manually test with following steps: Enable ARC Disable ARC Immediately re-enable ARC Verify error message displayed Verify user data directory removed Click Try again Verify ARC starts up appropriately ========== to ========== Show error during opt-in if arc-bridge not stopped. If a user attempts to disable and re-enable ARC in quick succession user data from the former instance will not be cleared by the normal wipe process because kArcEnabled is true again when the bridge is stopped. To work around this the opt-in process will display an error if attempting to opt-in before the previous ARC instance has shut down and it will wipe data explicitly. BUG=627664 TEST=Manually test with following steps: Enable ARC Disable ARC Immediately re-enable ARC Verify error message displayed Verify user data directory removed Click Try again Verify ARC starts up appropriately Committed: https://crrev.com/fe1a51e9faf942ace5613490077fefba8f592621 Cr-Commit-Position: refs/heads/master@{#406216} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fe1a51e9faf942ace5613490077fefba8f592621 Cr-Commit-Position: refs/heads/master@{#406216} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
