Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698683002:
Forced ephemeral profile deletion on browser removal crash fix. (Closed)
|
Created:
3 years, 10 months ago by palar Modified:
3 years, 9 months ago Reviewers:
Bernhard Bauer, Mike Lerman, Elliot Glaysher, anthonyvd, Mr4D (OOO till 08-26), pastarmovj CC:
chromium-reviews, rginda+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForced ephemeral profile deletion on browser removal crash fix.
Skiped all unnecessary activity actual to profile deletion from UI.
All async activity is gone, and race on browser shutdown with it.
BUG=691774
R=anthonyvd@chromium.org, bauerb@chromium.org
Review-Url: https://codereview.chromium.org/2698683002
Cr-Commit-Position: refs/heads/master@{#454586}
Committed: https://chromium.googlesource.com/chromium/src/+/47cf44bdd2b1242e75a2bb5945a9412c41f4073d
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed comment. #
Total comments: 11
Patch Set 3 : Review fixes. #Patch Set 4 : Removed extra line. #Patch Set 5 : Added DCHECK #
Total comments: 1
Patch Set 6 : Moved IsProfileEphemeral() declaration. #Patch Set 7 : Fixed compilation and unittests. #Patch Set 8 : Fixed ProfileManagerBrowserTest.EphemeralProfile checks. #Patch Set 9 : Reverted last patch. #Patch Set 10 : Changed active profile selection logic upon ephemeral profile deletion. #
Messages
Total messages: 47 (14 generated)
bauerb@chromium.org changed reviewers: + pastarmovj@chromium.org
+pastarmovj Thanks for the fast turnaround! https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:818: FinishDeletingProfile(profile_dir, GenerateNextProfileDirectoryPath()); Just to confirm: This updates the last used profile to a new one, but doesn't create it yet, correct? If so, a comment to that effect would be nice. https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:205: // Schedules the forced ephemeral profile at the given path to be deleted on Nit: Empty line before comments please :) https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:207: // profileless. I don't understand that sentence. When all browser windows are closed, that's when this method is called, right?
On 2017/02/15 18:03:30, Bernhard Bauer wrote: > +pastarmovj > > Thanks for the fast turnaround! > > https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.cc:818: > FinishDeletingProfile(profile_dir, GenerateNextProfileDirectoryPath()); > Just to confirm: This updates the last used profile to a new one, but doesn't > create it yet, correct? If so, a comment to that effect would be nice. > > https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_manager.h (right): > > https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.h:205: // Schedules the forced ephemeral > profile at the given path to be deleted on > Nit: Empty line before comments please :) > > https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.h:207: // profileless. > I don't understand that sentence. When all browser windows are closed, that's > when this method is called, right? Thanks for looping me in Bernhard! I will leave stamping to you. As long as Chrome doesn't crash and ephemeral profiles are deleted as expected I am happy :). Please let me know if I can help with anything else and once this lands I will test on my end to confirm it works on windows too but I will need some help to verify this works on Mac as I have none.
https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:818: FinishDeletingProfile(profile_dir, GenerateNextProfileDirectoryPath()); On 2017/02/15 18:03:30, Bernhard Bauer wrote: > Just to confirm: This updates the last used profile to a new one, but doesn't > create it yet, correct? If so, a comment to that effect would be nice. Exacly. Upon browser start a new profile at the specified path will be seamlessly created. Or if there any other active profiles exists - they overwrite last used profile setting on window activation or on profile deletion (other profiles expected to be ephemeral too). https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:207: // profileless. On 2017/02/15 18:03:30, Bernhard Bauer wrote: > I don't understand that sentence. When all browser windows are closed, that's > when this method is called, right? When all browser windows for exact ephemeral profile. There can be other active profiles actualy.
https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:207: // profileless. On 2017/02/16 10:39:04, palar wrote: > On 2017/02/15 18:03:30, Bernhard Bauer wrote: > > I don't understand that sentence. When all browser windows are closed, that's > > when this method is called, right? > > When all browser windows for exact ephemeral profile. There can be other active > profiles actualy. Since the ephemeral profile policy applies across all profiles you can safely assume there will be no profile spared. Dunno if this info is relevant but I agree with Bernhard that the sentence as is doesn't convey much meaning and the name of the function plus the first sentence above to be enough documentation.
On 2017/02/16 10:43:26, pastarmovj wrote: > https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_manager.h (right): > > https://codereview.chromium.org/2698683002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.h:207: // profileless. > On 2017/02/16 10:39:04, palar wrote: > > On 2017/02/15 18:03:30, Bernhard Bauer wrote: > > > I don't understand that sentence. When all browser windows are closed, > that's > > > when this method is called, right? > > > > When all browser windows for exact ephemeral profile. There can be other > active > > profiles actualy. > > Since the ephemeral profile policy applies across all profiles you can safely > assume there will be no profile spared. Dunno if this info is relevant but I > agree with Bernhard that the sentence as is doesn't convey much meaning and the > name of the function plus the first sentence above to be enough documentation. Fixed comment.
lgtm
Anthony, can you look at this issue.
+skuhne Still need owner approval.
Description was changed from ========== Forced ephemeral profile deletion on browser removal crash fix. Skiped all unnecessary activity actual to profile deletion from UI. All async activity is gone, and race on browser shutdown with it. BUG=691774 R=anthonyvd@chromium.org, bauerb@chromium.org ========== to ========== Forced ephemeral profile deletion on browser removal crash fix. Skiped all unnecessary activity actual to profile deletion from UI. All async activity is gone, and race on browser shutdown with it. BUG=691774 R=anthonyvd@chromium.org, bauerb@chromium.org ==========
palar@yandex-team.ru changed reviewers: + skuhne@chromium.org
On 2017/02/20 10:07:48, palar wrote: Monday was a public holiday in the US. I hope you will get your review today :)
On 2017/02/21 10:15:17, pastarmovj wrote: > On 2017/02/20 10:07:48, palar wrote: > > Monday was a public holiday in the US. I hope you will get your review today :) ping
On 2017/02/24 09:31:01, palar wrote: > On 2017/02/21 10:15:17, pastarmovj wrote: > > On 2017/02/20 10:07:48, palar wrote: > > > > Monday was a public holiday in the US. I hope you will get your review today > :) > > ping @skuhne, @anthonyvd, come on guys, it has been over a week with no reply on this CL! If you are overwhelmed with other work or ooo please say so, so that the author can find another reviewer. Not doing codereviews in time is a pretty ungoogly/unchromy behaviour. :( This is a crasher bug in the current stable version and as such reviewing the fix for it is pretty important.
palar@yandex-team.ru changed reviewers: + erg@chromium.org, mlerman@chromium.org
On 2017/02/27 10:02:44, pastarmovj wrote: > On 2017/02/24 09:31:01, palar wrote: > > On 2017/02/21 10:15:17, pastarmovj wrote: > > > On 2017/02/20 10:07:48, palar wrote: > > > > > > Monday was a public holiday in the US. I hope you will get your review today > > :) > > > > ping > > @skuhne, @anthonyvd, come on guys, it has been over a week with no reply on this > CL! If you are overwhelmed with other work or ooo please say so, so that the > author can find another reviewer. Not doing codereviews in time is a pretty > ungoogly/unchromy behaviour. :( > > This is a crasher bug in the current stable version and as such reviewing the > fix for it is pretty important. Added all other owners. Still need one approval from owner.
https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:817: const base::FilePath& profile_dir) { If this is supposed to only be used for Ephemeral profiles, can we add a DCHECK here that the profile specified by profile_dir is in fact ephemeral? https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1674: // Delete if the profile is an ephemeral profile. We're skipping the call to BrowserList::CloseAllBrowsersWithProfile. That seems unwise - we should make sure we do this call, I think? Bernhard, wdyt? https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:206: // Schedules the forced ephemeral profile at the given path to be deleted on Bernhard, do Ephemeral Profiles exist on CHROME_OS? Should this call also be in a !defined(OS_CHROMEOS) block? https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:208: void ScheduleForcedEphemeralProfileForDeletion( Why is this method public?
https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:817: const base::FilePath& profile_dir) { On 2017/03/01 14:24:29, Mike Lerman wrote: > If this is supposed to only be used for Ephemeral profiles, can we add a DCHECK > here that the profile specified by profile_dir is in fact ephemeral? It would be an overkill, but hey, why not. https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1674: // Delete if the profile is an ephemeral profile. On 2017/03/01 14:24:29, Mike Lerman wrote: > We're skipping the call to BrowserList::CloseAllBrowsersWithProfile. That seems > unwise - we should make sure we do this call, I think? Bernhard, wdyt? This call happened only on last browser with this profile is about to be closed. https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:208: void ScheduleForcedEphemeralProfileForDeletion( On 2017/03/01 14:24:29, Mike Lerman wrote: > Why is this method public? Good catch, thx.
https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:206: // Schedules the forced ephemeral profile at the given path to be deleted on On 2017/03/01 14:24:29, Mike Lerman wrote: > Bernhard, do Ephemeral Profiles exist on CHROME_OS? Should this call also be in > a !defined(OS_CHROMEOS) block? I can answer this one. On chromeos we offer a much better version of this conveniently also called ephemeral profiles but which uses proper RamFS based profiles. So this policy is not offered and used on ChromeOS at all.
https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1674: // Delete if the profile is an ephemeral profile. On 2017/03/01 16:51:44, palar wrote: > On 2017/03/01 14:24:29, Mike Lerman wrote: > > We're skipping the call to BrowserList::CloseAllBrowsersWithProfile. That > seems > > unwise - we should make sure we do this call, I think? Bernhard, wdyt? > > This call happened only on last browser with this profile is about to be closed. Ok. If we're not going to make sure programmatically that all the Browsers are closed (either by calling CloseAll, or by DCHECKing that the list of browsers for this profile has size 0), then please at least leave a comment. Although I've often been of the opinion that writing: // This state should be true is a poor man's version of DCHECK(state is true). https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:206: // Schedules the forced ephemeral profile at the given path to be deleted on On 2017/03/01 16:54:25, pastarmovj wrote: > On 2017/03/01 14:24:29, Mike Lerman wrote: > > Bernhard, do Ephemeral Profiles exist on CHROME_OS? Should this call also be > in > > a !defined(OS_CHROMEOS) block? > > I can answer this one. On chromeos we offer a much better version of this > conveniently also called ephemeral profiles but which uses proper RamFS based > profiles. So this policy is not offered and used on ChromeOS at all. Ok. Palar, can you make sure this method is also in !defined(OS_CHROMEOS)?
(fyi: i can't review this; i'm a per-file reviewer in the owners file.)
On 2017/03/01 18:36:03, Elliot Glaysher wrote: > (fyi: i can't review this; i'm a per-file reviewer in the owners file.) Got it. Someone should fix the src/chrome/browser/profiles/OWNERS file then.
On 2017/03/01 17:44:30, Mike Lerman wrote: > Ok. Palar, can you make sure this method is also in !defined(OS_CHROMEOS)? This is a bad idea. The policy may be not used on ChromeOS, but it neither disabled by preprocessor.
https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2698683002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1674: // Delete if the profile is an ephemeral profile. On 2017/03/01 17:44:30, Mike Lerman wrote: > On 2017/03/01 16:51:44, palar wrote: > > On 2017/03/01 14:24:29, Mike Lerman wrote: > > > We're skipping the call to BrowserList::CloseAllBrowsersWithProfile. That > > seems > > > unwise - we should make sure we do this call, I think? Bernhard, wdyt? > > > > This call happened only on last browser with this profile is about to be > closed. > > Ok. If we're not going to make sure programmatically that all the Browsers are > closed (either by calling CloseAll, or by DCHECKing that the list of browsers > for this profile has size 0), then please at least leave a comment. > > Although I've often been of the opinion that writing: > // This state should be true > is a poor man's version of > DCHECK(state is true). DCHECK I can do.
On 2017/03/01 19:11:10, palar wrote: > On 2017/03/01 17:44:30, Mike Lerman wrote: > > Ok. Palar, can you make sure this method is also in !defined(OS_CHROMEOS)? > This is a bad idea. > The policy may be not used on ChromeOS, but it neither disabled by preprocessor. I agree with Palar on that one. It technically will work on CrOS it just makes little sense in the presence of something better.
LGTM https://codereview.chromium.org/2698683002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2698683002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1731: namespace { style nit, no strong preference: shouldn't this go in the anonymous namespace declaration at the top of this file?
On 2017/03/01 19:21:20, Mike Lerman wrote: > LGTM > > https://codereview.chromium.org/2698683002/diff/80001/chrome/browser/profiles... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/2698683002/diff/80001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:1731: namespace { > style nit, no strong preference: shouldn't this go in the anonymous namespace > declaration at the top of this file? done.
The CQ bit was checked by palar@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/2698683002/#ps100001 (title: "Moved IsProfileEphemeral() declaration.")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by palar@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/2698683002/#ps120001 (title: "Fixed compilation and unittests.")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/03/02 12:15:39, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) @bauerb, @mlerman, can you check last patch set? Forcing the new profile path is not the wisest decision, searching existing one prior is much better.
LGTM.
The CQ bit was checked by palar@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/2698683002/#ps180001 (title: "Changed active profile selection logic upon ephemeral profile deletion.")
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": 180001, "attempt_start_ts": 1488552952691570,
"parent_rev": "eacee6c8bf6fe847bf6b5b08a260badd7ed64930", "commit_rev":
"47cf44bdd2b1242e75a2bb5945a9412c41f4073d"}
Message was sent while issue was closed.
Description was changed from ========== Forced ephemeral profile deletion on browser removal crash fix. Skiped all unnecessary activity actual to profile deletion from UI. All async activity is gone, and race on browser shutdown with it. BUG=691774 R=anthonyvd@chromium.org, bauerb@chromium.org ========== to ========== Forced ephemeral profile deletion on browser removal crash fix. Skiped all unnecessary activity actual to profile deletion from UI. All async activity is gone, and race on browser shutdown with it. BUG=691774 R=anthonyvd@chromium.org, bauerb@chromium.org Review-Url: https://codereview.chromium.org/2698683002 Cr-Commit-Position: refs/heads/master@{#454586} Committed: https://chromium.googlesource.com/chromium/src/+/47cf44bdd2b1242e75a2bb5945a9... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/47cf44bdd2b1242e75a2bb5945a9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
