Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2952883002:
Chrome Cleaner: Fix issue where a tagged profile could be reset multiple times (Closed)
|
Created:
3 years, 6 months ago by alito Modified:
3 years, 6 months ago CC:
chromium-reviews, vakh+watch_chromium.org, joenotcharles+watch_chromium.org, grt+watch_chromium.org, timvolodine, csharp+watch_chromium.org, alito+watch_chromium.org, ftirelo+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChrome Cleaner: Fix issue where a tagged profile could be reset multiple times
The ProfileResetter object in the SettingsResetter class needs to
be kept alive until its reset operation has been completed,
otherwise the SettingsResetter object could be destroyed before
it has had a chance to untag profiles that were already
reset. This is now done by keeping a vector of ProfileResetter
objects until the SettingsResetter class is deleted.
Review-Url: https://codereview.chromium.org/2952883002
Cr-Commit-Position: refs/heads/master@{#481614}
Committed: https://chromium.googlesource.com/chromium/src/+/dac87b8db8fa7f9999bed9e1c37d99bea5596563
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add sequence DCHECKs. #Messages
Total messages: 18 (10 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Chrome Cleaner: Fix issue where ProfileResetter could be deleted too early BUG= ========== to ========== Chrome Cleaner: Fix issue where a tagged profile could be reset multiple times The ProfileResetter object in the SettingsResetter class needs to be kept alive until the reset operation has been completed. This is now done by keeping a vector of such objects until the SettingsResetter class is deleted. ==========
Description was changed from ========== Chrome Cleaner: Fix issue where a tagged profile could be reset multiple times The ProfileResetter object in the SettingsResetter class needs to be kept alive until the reset operation has been completed. This is now done by keeping a vector of such objects until the SettingsResetter class is deleted. ========== to ========== Chrome Cleaner: Fix issue where a tagged profile could be reset multiple times The ProfileResetter object in the SettingsResetter class needs to be kept alive until its reset operation has been completed, otherwise the SettingsResetter object could be destroyed before untagging profiles that were already reset. This is now done by keeping a vector of ProfileResetter objects until the SettingsResetter class is deleted. ==========
Description was changed from ========== Chrome Cleaner: Fix issue where a tagged profile could be reset multiple times The ProfileResetter object in the SettingsResetter class needs to be kept alive until its reset operation has been completed, otherwise the SettingsResetter object could be destroyed before untagging profiles that were already reset. This is now done by keeping a vector of ProfileResetter objects until the SettingsResetter class is deleted. ========== to ========== Chrome Cleaner: Fix issue where a tagged profile could be reset multiple times The ProfileResetter object in the SettingsResetter class needs to be kept alive until its reset operation has been completed, otherwise the SettingsResetter object could be destroyed before it has had a chance to untag profiles that were already reset. This is now done by keeping a vector of ProfileResetter objects until the SettingsResetter class is deleted. ==========
alito@chromium.org changed reviewers: + ftirelo@chromium.org
PTAL.
alito@chromium.org changed reviewers: + robertshield@chromium.org
Forgot to add OWNER... adding Robert.
LGTM with a nit. https://codereview.chromium.org/2952883002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2952883002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:148: profile_resetters_.back()->Reset( Would it make sense to DCHECK that this always happens on the same sequence?
Nice finding btw!
lgtm
All done. Landing. https://codereview.chromium.org/2952883002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2952883002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:148: profile_resetters_.back()->Reset( On 2017/06/22 11:04:39, ftirelo wrote: > Would it make sense to DCHECK that this always happens on the same sequence? Done.
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ftirelo@chromium.org, robertshield@chromium.org Link to the patchset: https://codereview.chromium.org/2952883002/#ps40001 (title: "Add sequence DCHECKs.")
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": 40001, "attempt_start_ts": 1498153457158710,
"parent_rev": "452f0fd1a8564e39ca732c1818083ec53959aa6f", "commit_rev":
"dac87b8db8fa7f9999bed9e1c37d99bea5596563"}
Message was sent while issue was closed.
Description was changed from ========== Chrome Cleaner: Fix issue where a tagged profile could be reset multiple times The ProfileResetter object in the SettingsResetter class needs to be kept alive until its reset operation has been completed, otherwise the SettingsResetter object could be destroyed before it has had a chance to untag profiles that were already reset. This is now done by keeping a vector of ProfileResetter objects until the SettingsResetter class is deleted. ========== to ========== Chrome Cleaner: Fix issue where a tagged profile could be reset multiple times The ProfileResetter object in the SettingsResetter class needs to be kept alive until its reset operation has been completed, otherwise the SettingsResetter object could be destroyed before it has had a chance to untag profiles that were already reset. This is now done by keeping a vector of ProfileResetter objects until the SettingsResetter class is deleted. Review-Url: https://codereview.chromium.org/2952883002 Cr-Commit-Position: refs/heads/master@{#481614} Committed: https://chromium.googlesource.com/chromium/src/+/dac87b8db8fa7f9999bed9e1c37d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/dac87b8db8fa7f9999bed9e1c37d... |
