Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1869473002:
Fixed crash on double profile delete operation. (Closed)
DescriptionFixed crash on double profile delete operation.
BUG=601049
R=anthonyvd@chromium.org, bauerb@chromium.org, dbeam@chromium.org, stevenjb@chromium.org, achuith@chromium.org, mlerman@chromium.org
Committed: https://crrev.com/6b5e981164b2e466e023d960fa1452ebfb85eaf7
Cr-Commit-Position: refs/heads/master@{#393836}
Patch Set 1 #
Total comments: 1
Patch Set 2 : patch redone #
Total comments: 1
Patch Set 3 : Moved double deletion detection to earlier stage. #Patch Set 4 : Added log message. #
Total comments: 1
Patch Set 5 : DeleteProfileAtPath use MaybeScheduleProfileForDeletion #
Total comments: 2
Patch Set 6 : #
Total comments: 2
Patch Set 7 : Review fixes. #
Total comments: 6
Patch Set 8 : review fixes. #Patch Set 9 : review fixes. #
Total comments: 1
Patch Set 10 : param renaming. #Messages
Total messages: 49 (10 generated)
On 2016/04/06 15:50:51, palar wrote: ping.
Sorry for the delay, I didn't see this review come in. https://codereview.chromium.org/1869473002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1869473002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:1377: size_t profile_index = cache.GetIndexOfProfileWithPath(profile_dir); Can you use the new ProfileAttributesStorage interface (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) please? Should be a drop-in replacement.
On 2016/04/12 17:29:16, anthonyvd wrote: > Sorry for the delay, I didn't see this review come in. > > https://codereview.chromium.org/1869473002/diff/1/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/1869473002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.cc:1377: size_t profile_index = > cache.GetIndexOfProfileWithPath(profile_dir); > Can you use the new ProfileAttributesStorage interface > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) > please? Should be a drop-in replacement. Sorry for the delay. Code was moved over fresh codebase. On double delete operation the only issue will be reaching NOTREACHED() at the ProfileInfoCache::DeleteProfileFromCache(), which will not cause any side-effects on the release builds.
On 2016/04/28 16:09:06, palar wrote: > On 2016/04/12 17:29:16, anthonyvd wrote: > > Sorry for the delay, I didn't see this review come in. > > > > > https://codereview.chromium.org/1869473002/diff/1/chrome/browser/profiles/pro... > > File chrome/browser/profiles/profile_manager.cc (right): > > > > > https://codereview.chromium.org/1869473002/diff/1/chrome/browser/profiles/pro... > > chrome/browser/profiles/profile_manager.cc:1377: size_t profile_index = > > cache.GetIndexOfProfileWithPath(profile_dir); > > Can you use the new ProfileAttributesStorage interface > > > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) > > please? Should be a drop-in replacement. > > Sorry for the delay. Code was moved over fresh codebase. > On double delete operation the only issue will be reaching NOTREACHED() at the > ProfileInfoCache::DeleteProfileFromCache(), which will not cause any > side-effects on the release builds. Ping.
lgtm
The CQ bit was checked by palar@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869473002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869473002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Fixed crash on double profile delete operation. BUG=601049 R=anthonyvd@chromium.org ========== to ========== Fixed crash on double profile delete operation. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org ==========
On 2016/05/04 12:25:33, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Oops, mine more reviwers.
palar@yandex-team.ru changed reviewers: + bauerb@chromium.org
Ping.
Sorry, adding a reviewer does not automatically send out emails, so I didn't see this. https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:357: if (is_removing_) I'm not sure if silently returning is the right thing to do here. We should disable UI that allows clearing browsing data while that is in progress, and then DCHECK that we are not trying to clear data twice (which already happens in SetRemoving()). The same probably goes for profile deletion -- ProfileManager::ScheduleProfileForDeletion() and/or ScheduleQueueProfileDirectoryForDeletion should DCHECK that the profile isn't already scheduled for deletion, and deleting a profile should disable the UI while that is in progress.
On 2016/05/05 10:06:47, Bernhard Bauer wrote: > Sorry, adding a reviewer does not automatically send out emails, so I didn't see > this. > > https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... > File chrome/browser/browsing_data/browsing_data_remover.cc (right): > > https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... > chrome/browser/browsing_data/browsing_data_remover.cc:357: if (is_removing_) > I'm not sure if silently returning is the right thing to do here. We should > disable UI that allows clearing browsing data while that is in progress, and > then DCHECK that we are not trying to clear data twice (which already happens in > SetRemoving()). > > The same probably goes for profile deletion -- > ProfileManager::ScheduleProfileForDeletion() and/or > ScheduleQueueProfileDirectoryForDeletion should DCHECK that the profile isn't > already scheduled for deletion, and deleting a profile should disable the UI > while that is in progress. I agree, double delete operation still should treated as a bug. But crashing browser when error can be processed isn't right thing to do either. Maybe then "if (is_removing_) { NOTREACHED(); return; }" will be sufficient?
On 2016/05/05 10:17:17, palar wrote: > On 2016/05/05 10:06:47, Bernhard Bauer wrote: > > Sorry, adding a reviewer does not automatically send out emails, so I didn't > see > > this. > > > > > https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... > > File chrome/browser/browsing_data/browsing_data_remover.cc (right): > > > > > https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... > > chrome/browser/browsing_data/browsing_data_remover.cc:357: if (is_removing_) > > I'm not sure if silently returning is the right thing to do here. We should > > disable UI that allows clearing browsing data while that is in progress, and > > then DCHECK that we are not trying to clear data twice (which already happens > in > > SetRemoving()). > > > > The same probably goes for profile deletion -- > > ProfileManager::ScheduleProfileForDeletion() and/or > > ScheduleQueueProfileDirectoryForDeletion should DCHECK that the profile isn't > > already scheduled for deletion, and deleting a profile should disable the UI > > while that is in progress. > > I agree, double delete operation still should treated as a bug. But crashing > browser when error can be processed isn't right thing to do either. Maybe then > "if (is_removing_) { NOTREACHED(); return; }" will be sufficient? Keep in mind that a DCHECK will be compiled out in a Release build. What you suggest is specifically called out as an antipattern in http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-: Basically, if you are stating that something should not happen (in the absence of a bug), you shouldn't have code in a Release build that deals with it. Also, a crash in case of a bug is not the Worst Thing: It will get reported, brought to someone's attention, and eventually fixed. Silently handling conditions that are supposed to never happen means no one will ever learn about it if they do happen. It also makes it more difficult to reason about the code: if you see a DCHECK asserting a condition, you can assume that condition is the case. If you have code that handles failure of the condition, you have to assume that that's possible.
On 2016/05/05 10:27:54, Bernhard Bauer wrote: > On 2016/05/05 10:17:17, palar wrote: > > On 2016/05/05 10:06:47, Bernhard Bauer wrote: > > > Sorry, adding a reviewer does not automatically send out emails, so I didn't > > see > > > this. > > > > > > > > > https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... > > > File chrome/browser/browsing_data/browsing_data_remover.cc (right): > > > > > > > > > https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... > > > chrome/browser/browsing_data/browsing_data_remover.cc:357: if (is_removing_) > > > I'm not sure if silently returning is the right thing to do here. We should > > > disable UI that allows clearing browsing data while that is in progress, and > > > then DCHECK that we are not trying to clear data twice (which already > happens > > in > > > SetRemoving()). > > > > > > The same probably goes for profile deletion -- > > > ProfileManager::ScheduleProfileForDeletion() and/or > > > ScheduleQueueProfileDirectoryForDeletion should DCHECK that the profile > isn't > > > already scheduled for deletion, and deleting a profile should disable the UI > > > while that is in progress. > > > > I agree, double delete operation still should treated as a bug. But crashing > > browser when error can be processed isn't right thing to do either. Maybe then > > "if (is_removing_) { NOTREACHED(); return; }" will be sufficient? > > Keep in mind that a DCHECK will be compiled out in a Release build. What you > suggest is specifically called out as an antipattern in > http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-: > Basically, if you are stating that something should not happen (in the absence > of a bug), you shouldn't have code in a Release build that deals with it. Also, > a crash in case of a bug is not the Worst Thing: It will get reported, brought > to someone's attention, and eventually fixed. Silently handling conditions that > are supposed to never happen means no one will ever learn about it if they do > happen. It also makes it more difficult to reason about the code: if you see a > DCHECK asserting a condition, you can assume that condition is the case. If you > have code that handles failure of the condition, you have to assume that that's > possible. Ok. NOTREACHED() is not an option, didn't fond of this anyway. The only reason I propose this was same practice at ProfileInfoCache::DeleteProfileFromCache(...); So what do we have: 1. A time-consuming async operation, which remove data from profile. 2. Profile should be available till operation complete. 3. While profile is available it can be deleted*, unless it will be flagged as is_removing_. 4. BrowsingDataRemover class have such flag. 5. Not sure if BrowsingDataRemover can actualy do it's work if called for same profile twice, but DCHECKS make me suggest it doesn't, or at least do not expect this. Questions: 1. Why BrowsingDataRemover::Remove(...) allowed to process on profile which already being processed? 2. If silently returning is not right thing to do, what do we have as an option? ------------- *) Even we block UI while activating profile removal, nothing can prevent attempt to open chrome://tune at another tab/window and attempt delete profile from there.
On 2016/05/05 11:37:44, palar wrote: > On 2016/05/05 10:27:54, Bernhard Bauer wrote: > > On 2016/05/05 10:17:17, palar wrote: > > > On 2016/05/05 10:06:47, Bernhard Bauer wrote: > > > > Sorry, adding a reviewer does not automatically send out emails, so I > didn't > > > see > > > > this. > > > > > > > > > > > > > > https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... > > > > File chrome/browser/browsing_data/browsing_data_remover.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1869473002/diff/20001/chrome/browser/browsing... > > > > chrome/browser/browsing_data/browsing_data_remover.cc:357: if > (is_removing_) > > > > I'm not sure if silently returning is the right thing to do here. We > should > > > > disable UI that allows clearing browsing data while that is in progress, > and > > > > then DCHECK that we are not trying to clear data twice (which already > > happens > > > in > > > > SetRemoving()). > > > > > > > > The same probably goes for profile deletion -- > > > > ProfileManager::ScheduleProfileForDeletion() and/or > > > > ScheduleQueueProfileDirectoryForDeletion should DCHECK that the profile > > isn't > > > > already scheduled for deletion, and deleting a profile should disable the > UI > > > > while that is in progress. > > > > > > I agree, double delete operation still should treated as a bug. But crashing > > > browser when error can be processed isn't right thing to do either. Maybe > then > > > "if (is_removing_) { NOTREACHED(); return; }" will be sufficient? > > > > Keep in mind that a DCHECK will be compiled out in a Release build. What you > > suggest is specifically called out as an antipattern in > > > http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-: > > Basically, if you are stating that something should not happen (in the absence > > of a bug), you shouldn't have code in a Release build that deals with it. > Also, > > a crash in case of a bug is not the Worst Thing: It will get reported, brought > > to someone's attention, and eventually fixed. Silently handling conditions > that > > are supposed to never happen means no one will ever learn about it if they do > > happen. It also makes it more difficult to reason about the code: if you see a > > DCHECK asserting a condition, you can assume that condition is the case. If > you > > have code that handles failure of the condition, you have to assume that > that's > > possible. > > Ok. NOTREACHED() is not an option, didn't fond of this anyway. The only reason I > propose this was same practice at ProfileInfoCache::DeleteProfileFromCache(...); > So what do we have: > 1. A time-consuming async operation, which remove data from profile. > 2. Profile should be available till operation complete. > 3. While profile is available it can be deleted*, unless it will be flagged as > is_removing_. > 4. BrowsingDataRemover class have such flag. > 5. Not sure if BrowsingDataRemover can actualy do it's work if called for same > profile twice, but DCHECKS make me suggest it doesn't, or at least do not expect > this. > Questions: > 1. Why BrowsingDataRemover::Remove(...) allowed to process on profile which > already being processed? It's not. Remove() itself doesn't DCHECK that, but SetRemoving() does. > 2. If silently returning is not right thing to do, what do we have as an option? > > ------------- > *) Even we block UI while activating profile removal, nothing can prevent > attempt to open chrome://tune at another tab/window and attempt delete profile > from there. Actually, I don't think that can happen: ScheduleProfileForDeletion() removes the profile from ProfileAttributesStorage, so it wouldn't show up in the list of profiles anymore. The only reason why the double deletion can happen is because ScheduleProfileForDeletion() itself doesn't have an early check whether the profile actually exists. There is a lookup in line 1378 which does fail, and it looks like you've noticed that as well :) So, here's what I would suggest: 1) ScheduleProfileForDeletion() can only be called with a profile that is in the ProfileAttributesStorage, and it DCHECKs that at the beginning. Because it also removes the profile from ProfileAttributesStorage, it means that ScheduleProfileForDeletion() can not be called more than once with the same profile. 2) In order not to trigger the DCHECK in 1), we need to stop double profile deletions somewhere. We could do that by disabling the UI once we kick off a delete, or by checking in chrome/browser/ui/webui/profile_helper.cc whether the profile still exists. (The reason why I'm okay with silently handling it here is that here we know this will only be called by UI code.)
On 2016/05/05 16:17:54, Bernhard Bauer wrote: > > So, here's what I would suggest: > 1) ScheduleProfileForDeletion() can only be called with a profile that is in the > ProfileAttributesStorage, and it DCHECKs that at the beginning. Because it also > removes the profile from ProfileAttributesStorage, it means that > ScheduleProfileForDeletion() can not be called more than once with the same > profile. > 2) In order not to trigger the DCHECK in 1), we need to stop double profile > deletions somewhere. We could do that by disabling the UI once we kick off a > delete, or by checking in chrome/browser/ui/webui/profile_helper.cc whether the > profile still exists. (The reason why I'm okay with silently handling it here is > that here we know this will only be called by UI code.) Must agree with you, it is better to prevent double deletion than hunt any consequences it's bring. ScheduleProfileForDeletion() is highest common point to such behaviour, so I placed silent return there in case of profile already scheduled for deletion.
On 2016/05/06 09:58:59, palar wrote: > On 2016/05/05 16:17:54, Bernhard Bauer wrote: > > > > So, here's what I would suggest: > > 1) ScheduleProfileForDeletion() can only be called with a profile that is in > the > > ProfileAttributesStorage, and it DCHECKs that at the beginning. Because it > also > > removes the profile from ProfileAttributesStorage, it means that > > ScheduleProfileForDeletion() can not be called more than once with the same > > profile. > > 2) In order not to trigger the DCHECK in 1), we need to stop double profile > > deletions somewhere. We could do that by disabling the UI once we kick off a > > delete, or by checking in chrome/browser/ui/webui/profile_helper.cc whether > the > > profile still exists. (The reason why I'm okay with silently handling it here > is > > that here we know this will only be called by UI code.) > > Must agree with you, it is better to prevent double deletion than hunt any > consequences it's bring. > ScheduleProfileForDeletion() is highest common point to such behaviour, so I > placed silent return there in case of profile already scheduled for deletion. Let's do this in chrome/browser/ui/webui/profile_helper.cc -- there are other places that might end up calling ScheduleProfileForDeletion(), but doing so twice would be a bug, so we don't want to silently handle this in all code paths, just the ones from Web UI. We don't have IsProfileMarkedForDeletion() outside of ProfileManager, but we can instead check whether the profile path is in ProfileAttributesStorage, which has the additional advantage that it would prevent us from trying to delete a profile at a random path (i.e. where we don't actually have a profile) as well.
On 2016/05/06 11:07:05, Bernhard Bauer wrote: > On 2016/05/06 09:58:59, palar wrote: > > On 2016/05/05 16:17:54, Bernhard Bauer wrote: > > > > > > So, here's what I would suggest: > > > 1) ScheduleProfileForDeletion() can only be called with a profile that is in > > the > > > ProfileAttributesStorage, and it DCHECKs that at the beginning. Because it > > also > > > removes the profile from ProfileAttributesStorage, it means that > > > ScheduleProfileForDeletion() can not be called more than once with the same > > > profile. > > > 2) In order not to trigger the DCHECK in 1), we need to stop double profile > > > deletions somewhere. We could do that by disabling the UI once we kick off a > > > delete, or by checking in chrome/browser/ui/webui/profile_helper.cc whether > > the > > > profile still exists. (The reason why I'm okay with silently handling it > here > > is > > > that here we know this will only be called by UI code.) > > > > Must agree with you, it is better to prevent double deletion than hunt any > > consequences it's bring. > > ScheduleProfileForDeletion() is highest common point to such behaviour, so I > > placed silent return there in case of profile already scheduled for deletion. > > Let's do this in chrome/browser/ui/webui/profile_helper.cc -- there are other > places that might end up calling ScheduleProfileForDeletion(), but doing so > twice would be a bug, so we don't want to silently handle this in all code > paths, just the ones from Web UI. > > We don't have IsProfileMarkedForDeletion() outside of ProfileManager, but we can > instead check whether the profile path is in ProfileAttributesStorage, which has > the additional advantage that it would prevent us from trying to delete a > profile at a random path (i.e. where we don't actually have a profile) as well. I think about it, but there is ScheduleProfileForDeletion() call from UserManagerScreenHandler::HandleRemoveUser() with same vulnerability.
On 2016/05/06 11:07:05, Bernhard Bauer wrote: > We don't have IsProfileMarkedForDeletion() outside of ProfileManager, but we can > instead check whether the profile path is in ProfileAttributesStorage, which has > the additional advantage that it would prevent us from trying to delete a > profile at a random path (i.e. where we don't actually have a profile) as well. And we can't relay on ProfileAttributesStorage, as far I can see. There is a way to delete profile path which is not in storage. On ProfileCreation fail for example. Correct me if I wrong.
On 2016/05/06 11:12:36, palar wrote: > On 2016/05/06 11:07:05, Bernhard Bauer wrote: > > On 2016/05/06 09:58:59, palar wrote: > > > On 2016/05/05 16:17:54, Bernhard Bauer wrote: > > > > > > > > So, here's what I would suggest: > > > > 1) ScheduleProfileForDeletion() can only be called with a profile that is > in > > > the > > > > ProfileAttributesStorage, and it DCHECKs that at the beginning. Because it > > > also > > > > removes the profile from ProfileAttributesStorage, it means that > > > > ScheduleProfileForDeletion() can not be called more than once with the > same > > > > profile. > > > > 2) In order not to trigger the DCHECK in 1), we need to stop double > profile > > > > deletions somewhere. We could do that by disabling the UI once we kick off > a > > > > delete, or by checking in chrome/browser/ui/webui/profile_helper.cc > whether > > > the > > > > profile still exists. (The reason why I'm okay with silently handling it > > here > > > is > > > > that here we know this will only be called by UI code.) > > > > > > Must agree with you, it is better to prevent double deletion than hunt any > > > consequences it's bring. > > > ScheduleProfileForDeletion() is highest common point to such behaviour, so I > > > placed silent return there in case of profile already scheduled for > deletion. > > > > Let's do this in chrome/browser/ui/webui/profile_helper.cc -- there are other > > places that might end up calling ScheduleProfileForDeletion(), but doing so > > twice would be a bug, so we don't want to silently handle this in all code > > paths, just the ones from Web UI. > > > > We don't have IsProfileMarkedForDeletion() outside of ProfileManager, but we > can > > instead check whether the profile path is in ProfileAttributesStorage, which > has > > the additional advantage that it would prevent us from trying to delete a > > profile at a random path (i.e. where we don't actually have a profile) as > well. > > I think about it, but there is ScheduleProfileForDeletion() call from > UserManagerScreenHandler::HandleRemoveUser() with same vulnerability. Oh, good catch. That one probably should go through DeleteProfileAtPath() as well, as they're both in Web UI code. (We might have to refactor the method a little bit so it can record the correct action depending on where it's called from). On 2016/05/06 11:41:05, palar wrote: > On 2016/05/06 11:07:05, Bernhard Bauer wrote: > > We don't have IsProfileMarkedForDeletion() outside of ProfileManager, but we > can > > instead check whether the profile path is in ProfileAttributesStorage, which > has > > the additional advantage that it would prevent us from trying to delete a > > profile at a random path (i.e. where we don't actually have a profile) as > well. > > And we can't relay on ProfileAttributesStorage, as far I can see. There is a way > to delete profile path which is not in storage. On ProfileCreation fail for > example. Correct me if I wrong. That one definitely should not silently handle failure. If we try to delete a profile where creation has failed, we really only should do that once, and only for a profile that we've created in the first place.
On 2016/05/09 14:04:04, Bernhard Bauer wrote: > Oh, good catch. That one probably should go through DeleteProfileAtPath() as > well, as they're both in Web UI code. (We might have to refactor the method a > little bit so it can record the correct action depending on where it's called > from). Ok. Code is the same actualy. > That one definitely should not silently handle failure. If we try to delete a > profile where creation has failed, we really only should do that once, and only > for a profile that we've created in the first place. Currently failed-to-create profiles are handled correctly. They not added to ProfileAttributesStorage, and deleted by webui::DeleteProfileAtPath() as result ProfileAttributesStorage can't be used as validator for such operation. And I still think IsProfileMarkedForDeletion() is the only good validator for this case. I can suggest to create function ProfileManager::ScheduleProfileForDeletionForUI() wich will check IsProfileMarkedForDeletion() and call ScheduleProfileForDeletion() if it doesn't. Overkill, IMO, but it will be as close to your suggestion as it can be.
On 2016/05/10 11:07:24, palar wrote: > On 2016/05/09 14:04:04, Bernhard Bauer wrote: > > Oh, good catch. That one probably should go through DeleteProfileAtPath() as > > well, as they're both in Web UI code. (We might have to refactor the method a > > little bit so it can record the correct action depending on where it's called > > from). > Ok. Code is the same actualy. The constant used to record metrics is different (DELETE_PROFILE_SETTINGS vs. DELETE_PROFILE_USER_MANAGER), but we can refactor that out. > > That one definitely should not silently handle failure. If we try to delete a > > profile where creation has failed, we really only should do that once, and > only > > for a profile that we've created in the first place. > Currently failed-to-create profiles are handled correctly. > They not added to ProfileAttributesStorage, and deleted by > webui::DeleteProfileAtPath() as result ProfileAttributesStorage can't be used as > validator for such operation. And I still think IsProfileMarkedForDeletion() is > the only good validator for this case. I can suggest to create function > ProfileManager::ScheduleProfileForDeletionForUI() wich will check > IsProfileMarkedForDeletion() and call ScheduleProfileForDeletion() if it > doesn't. Overkill, IMO, but it will be as close to your suggestion as it can be. OK... could we name the method something like MaybeScheduleProfileForDeletion() though? The fact that it would be used for UI would be more of an implementation detail of said UI.
Description was changed from ========== Fixed crash on double profile delete operation. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org ========== to ========== Fixed crash on double profile delete operation. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org, dbeam@chromium.org, stevenjb@chromium.org, achuith@chromium.org, mlerman@chromium.org ==========
palar@yandex-team.ru changed reviewers: + achuith@chromium.org, dbeam@chromium.org, mlerman@chromium.org, stevenjb@chromium.org
On 2016/05/11 17:56:56, Bernhard Bauer wrote: > OK... could we name the method something like MaybeScheduleProfileForDeletion() > though? The fact that it would be used for UI would be more of an implementation > detail of said UI. Made new patch-set, added reviewers.
Nice! Just some nits now: https://codereview.chromium.org/1869473002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1869473002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:733: LOG(WARNING) << "Profile already marked for deletion: " I would not warn here if we are returning success as a flag. Customers of this function can decide how to handle it. https://codereview.chromium.org/1869473002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right): https://codereview.chromium.org/1869473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:419: if (profile) If the body is now more than one line, add braces.
On 2016/05/12 12:06:23, Bernhard Bauer wrote: > Nice! Just some nits now: > > https://codereview.chromium.org/1869473002/diff/80001/chrome/browser/profiles... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/1869473002/diff/80001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:733: LOG(WARNING) << "Profile already > marked for deletion: " > I would not warn here if we are returning success as a flag. Customers of this > function can decide how to handle it. > > https://codereview.chromium.org/1869473002/diff/80001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right): > > https://codereview.chromium.org/1869473002/diff/80001/chrome/browser/ui/webui... > chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:419: if > (profile) > If the body is now more than one line, add braces. Fixed.
lgtm
lgtm w/ nit+suggestion https://codereview.chromium.org/1869473002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1869473002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:741: DCHECK(profiles::IsMultipleProfilesEnabled()); We should probably add CHECK(!IsProfileMarkedForDeletion(profile_dir)) here to document the difference and ensure the correct method is being used. https://codereview.chromium.org/1869473002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/1869473002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager.h:195: const CreateCallback& callback); blank line
On 2016/05/12 15:53:00, stevenjb wrote: > lgtm w/ nit+suggestion > > https://codereview.chromium.org/1869473002/diff/100001/chrome/browser/profile... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/1869473002/diff/100001/chrome/browser/profile... > chrome/browser/profiles/profile_manager.cc:741: > DCHECK(profiles::IsMultipleProfilesEnabled()); > We should probably add CHECK(!IsProfileMarkedForDeletion(profile_dir)) here to > document the difference and ensure the correct method is being used. > > https://codereview.chromium.org/1869473002/diff/100001/chrome/browser/profile... > File chrome/browser/profiles/profile_manager.h (right): > > https://codereview.chromium.org/1869473002/diff/100001/chrome/browser/profile... > chrome/browser/profiles/profile_manager.h:195: const CreateCallback& callback); > blank line Done.
https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_manager.h:194: bool MaybeScheduleProfileForDeletion(const base::FilePath& profile_dir, comment should say something about the return value https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/create_profile_handler.cc (right): https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/create_profile_handler.cc:235: if (profile) I believe you need {} in the multi-line case? https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/sync_setup_handler.cc (right): https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/sync_setup_handler.cc:696: webui::DeleteProfileAtPath(GetProfile()->GetPath(), {} https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/profile_helper.cc (right): https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/profile_helper.cc:36: if (g_browser_process->profile_manager()->MaybeScheduleProfileForDeletion( This seems a bit awkward. Can you pass user into MaybeScheduleProfileForDeletion and call LogProfileDeleteUser in that function instead?
https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:742: CHECK(!IsProfileMarkedForDeletion(profile_dir)); sooooo, why can't we just do: if (IsProfileMarkedForDeletion(profile_dir)) { // do something with callback, i.e. append to a list or Run() return; } this method is just asking that it be scheduled for deletion. if it's already scheduled, i don't really see the harm...
https://codereview.chromium.org/1869473002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1869473002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:737: return; i vote for this solution but without error and using callback
On 2016/05/12 22:33:59, achuithb wrote: > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... > File chrome/browser/profiles/profile_manager.h (right): > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... > chrome/browser/profiles/profile_manager.h:194: bool > MaybeScheduleProfileForDeletion(const base::FilePath& profile_dir, > comment should say something about the return value > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/options/create_profile_handler.cc (right): > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... > chrome/browser/ui/webui/options/create_profile_handler.cc:235: if (profile) > I believe you need {} in the multi-line case? > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/options/sync_setup_handler.cc (right): > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... > chrome/browser/ui/webui/options/sync_setup_handler.cc:696: > webui::DeleteProfileAtPath(GetProfile()->GetPath(), > {} Isn't it already have one? > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/profile_helper.cc (right): > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/ui/webu... > chrome/browser/ui/webui/profile_helper.cc:36: if > (g_browser_process->profile_manager()->MaybeScheduleProfileForDeletion( > This seems a bit awkward. Can you pass user into MaybeScheduleProfileForDeletion > and call LogProfileDeleteUser in that function instead? Done.
On 2016/05/13 03:37:40, Dan Beam wrote: > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... > chrome/browser/profiles/profile_manager.cc:742: > CHECK(!IsProfileMarkedForDeletion(profile_dir)); > sooooo, why can't we just do: > > if (IsProfileMarkedForDeletion(profile_dir)) { > // do something with callback, i.e. append to a list or Run() > return; > } > > this method is just asking that it be scheduled for deletion. if it's already > scheduled, i don't really see the harm... I'd like to, but Bernhard Bauer was against it. He insist on profile deletion logic to be as strict as possible, with only exception for UI.
On 2016/05/13 09:21:39, palar wrote: > On 2016/05/13 03:37:40, Dan Beam wrote: > > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... > > File chrome/browser/profiles/profile_manager.cc (right): > > > > > https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... > > chrome/browser/profiles/profile_manager.cc:742: > > CHECK(!IsProfileMarkedForDeletion(profile_dir)); > > sooooo, why can't we just do: > > > > if (IsProfileMarkedForDeletion(profile_dir)) { > > // do something with callback, i.e. append to a list or Run() > > return; > > } > > > > this method is just asking that it be scheduled for deletion. if it's already > > scheduled, i don't really see the harm... > > I'd like to, but Bernhard Bauer was against it. He insist on profile deletion > logic to be as strict as possible, with only exception for UI. Yup -- my reasoning was, we shouldn't try to delete a profile more than once, unless we absolutely know it can legitimately happen. https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1869473002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:742: CHECK(!IsProfileMarkedForDeletion(profile_dir)); I would make this a DCHECK though.
lgtm https://codereview.chromium.org/1869473002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/profile_helper.cc (right): https://codereview.chromium.org/1869473002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/profile_helper.cc:31: ProfileMetrics::ProfileDelete user) { nit: user's not the best name for the variable. This is moreso the ui_surface, deletion_source, or similar.
On 2016/05/13 13:21:33, Mike Lerman wrote: > lgtm > > https://codereview.chromium.org/1869473002/diff/160001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/profile_helper.cc (right): > > https://codereview.chromium.org/1869473002/diff/160001/chrome/browser/ui/webu... > chrome/browser/ui/webui/profile_helper.cc:31: ProfileMetrics::ProfileDelete > user) { > nit: user's not the best name for the variable. This is moreso the ui_surface, > deletion_source, or similar. deletion_source is fine.
The CQ bit was checked by palar@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org, bauerb@chromium.org, stevenjb@chromium.org, mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/1869473002/#ps180001 (title: "param renaming.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869473002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869473002/180001
Message was sent while issue was closed.
Description was changed from ========== Fixed crash on double profile delete operation. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org, dbeam@chromium.org, stevenjb@chromium.org, achuith@chromium.org, mlerman@chromium.org ========== to ========== Fixed crash on double profile delete operation. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org, dbeam@chromium.org, stevenjb@chromium.org, achuith@chromium.org, mlerman@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Fixed crash on double profile delete operation. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org, dbeam@chromium.org, stevenjb@chromium.org, achuith@chromium.org, mlerman@chromium.org ========== to ========== Fixed crash on double profile delete operation. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org, dbeam@chromium.org, stevenjb@chromium.org, achuith@chromium.org, mlerman@chromium.org Committed: https://crrev.com/6b5e981164b2e466e023d960fa1452ebfb85eaf7 Cr-Commit-Position: refs/heads/master@{#393836} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6b5e981164b2e466e023d960fa1452ebfb85eaf7 Cr-Commit-Position: refs/heads/master@{#393836} |
