Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
953453002:
Wait until a new profile has been created before deleting the active profile. (Closed)
|
Created:
5 years, 10 months ago by Bernhard Bauer Modified:
5 years, 10 months ago CC:
chromium-reviews, rginda+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWait until a new profile has been created before deleting the active profile.
BUG=460859
Committed: https://crrev.com/944361e0a8808cc0c82dcb0ef4e58440f5d1c0fa
Cr-Commit-Position: refs/heads/master@{#318288}
Patch Set 1 #Patch Set 2 : x #
Total comments: 4
Patch Set 3 : test #
Total comments: 4
Patch Set 4 : fix! #Patch Set 5 : comment #Patch Set 6 : fix!!!!11111oneoneoneoneargh #
Messages
Total messages: 25 (8 generated)
bauerb@chromium.org changed reviewers: + noms@chromium.org
Please review!
I think this looks ok. Is it possible to add a test for this scenario? https://codereview.chromium.org/953453002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/953453002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:703: // On the Mac, the browser process is not killed when all browser windows are I think this may be true of windows in background mode nowadays too :( Maybe we should enable this, and the ActiveProfileDeletedNextProfileDeletedToo test for all the things? https://codereview.chromium.org/953453002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/953453002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.h:268: // Schedules the profile at the given path to be deleted on shutdown, nit: profile at |profile_dir| to be deleted, marks |new_active...| as active.
On 2015/02/23 23:54:49, Monica Dinculescu wrote: > I think this looks ok. Is it possible to add a test for this scenario? Hm, I think I can change AppController so it will test the particular code path that is failing in the bug (See https://codereview.chromium.org/948003004/ for where it fails without the fix). https://codereview.chromium.org/953453002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/953453002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:703: // On the Mac, the browser process is not killed when all browser windows are On 2015/02/23 23:54:49, Monica Dinculescu wrote: > I think this may be true of windows in background mode nowadays too :( > Maybe we should enable this, and the ActiveProfileDeletedNextProfileDeletedToo > test for all the things? Hm... but then again, if the browser does indeed quit, we don't want to start loading a profile right beforehand. Do you know under which circumstances we go into background mode? Are those circumstances going to apply if we are deleting the only loaded profile?
https://codereview.chromium.org/953453002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/953453002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:703: // On the Mac, the browser process is not killed when all browser windows are On 2015/02/24 15:13:32, Bernhard (back on Feb 26) wrote: > On 2015/02/23 23:54:49, Monica Dinculescu wrote: > > I think this may be true of windows in background mode nowadays too :( > > Maybe we should enable this, and the ActiveProfileDeletedNextProfileDeletedToo > > test for all the things? > > Hm... but then again, if the browser does indeed quit, we don't want to start > loading a profile right beforehand. > > Do you know under which circumstances we go into background mode? Are those > circumstances going to apply if we are deleting the only loaded profile? That makes sense.Let's ignore it for now, and if/when it comes up on Windows, we'll deal with it then. https://codereview.chromium.org/953453002/diff/40001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/953453002/diff/40001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:900: if (!lastProfile_ || profilePath == lastProfile_->GetPath()) i'm confused. is this a real change or a test? :(
https://codereview.chromium.org/953453002/diff/40001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/953453002/diff/40001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:900: if (!lastProfile_ || profilePath == lastProfile_->GetPath()) On 2015/02/24 19:37:37, Monica Dinculescu wrote: > i'm confused. is this a real change or a test? :( Allow me to explain: |lastProfile_| starts out as null and is set when a browser window becomes active, or the application is brought to the front, which is guaranteed to happen during regular browser startup. In a browser_test, the application is never brought to front though (see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/_QA9sH_UEnw/dis...), so this would stay null. Doing this change will make sure that GetLastUsedProfile() is called in the next line, which is what CHECKS if an uninitialized profile is returned. The effect is that this tests the code path that is failing in the linked bug.
mlerman@chromium.org changed reviewers: + mlerman@chromium.org
https://codereview.chromium.org/953453002/diff/40001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/953453002/diff/40001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:900: if (!lastProfile_ || profilePath == lastProfile_->GetPath()) On 2015/02/24 22:28:24, Bernhard (back on Feb 26) wrote: > On 2015/02/24 19:37:37, Monica Dinculescu wrote: > > i'm confused. is this a real change or a test? :( > > Allow me to explain: |lastProfile_| starts out as null and is set when a browser > window becomes active, or the application is brought to the front, which is > guaranteed to happen during regular browser startup. In a browser_test, the > application is never brought to front though (see > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/_QA9sH_UEnw/dis...), > so this would stay null. Doing this change will make sure that > GetLastUsedProfile() is called in the next line, which is what CHECKS if an > uninitialized profile is returned. The effect is that this tests the code path > that is failing in the linked bug. Makes sense - can you modify the above comment block to reflect this?
Fixed the issue that Monica pointed out over chat. PTAL? https://codereview.chromium.org/953453002/diff/40001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/953453002/diff/40001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:900: if (!lastProfile_ || profilePath == lastProfile_->GetPath()) On 2015/02/25 18:32:10, Mike Lerman wrote: > On 2015/02/24 22:28:24, Bernhard (back on Feb 26) wrote: > > On 2015/02/24 19:37:37, Monica Dinculescu wrote: > > > i'm confused. is this a real change or a test? :( > > > > Allow me to explain: |lastProfile_| starts out as null and is set when a > browser > > window becomes active, or the application is brought to the front, which is > > guaranteed to happen during regular browser startup. In a browser_test, the > > application is never brought to front though (see > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/_QA9sH_UEnw/dis...), > > so this would stay null. Doing this change will make sure that > > GetLastUsedProfile() is called in the next line, which is what CHECKS if an > > uninitialized profile is returned. The effect is that this tests the code path > > that is failing in the linked bug. > > Makes sense - can you modify the above comment block to reflect this? Done.
lgtm! thanks for putting up with this! :)
(removing me as reviewer)
mlerman@chromium.org changed reviewers: - mlerman@chromium.org
bauerb@chromium.org changed reviewers: + rsesek@chromium.org
+Robert for app_controller_mac OWNERS review.
LGTM
The CQ bit was checked by bauerb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/953453002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, noms@chromium.org Link to the patchset: https://codereview.chromium.org/953453002/#ps100001 (title: "fix!!!!11111oneoneoneoneargh")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/953453002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/944361e0a8808cc0c82dcb0ef4e58440f5d1c0fa Cr-Commit-Position: refs/heads/master@{#318288}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/967453002/ by qyearsley@chromium.org. The reason for reverting is: Suspected of causing flaky failure of ProfileManagerBrowserTests.DeleteSingletonProfile and ProfileManagerBrowserTests.DeletePasswords on Mac. Examples: https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/4785 https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/4789 https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/4790 http://build.chromium.org/p/chromium.mac/builders/Mac10.8%20Tests/builds/4858 http://build.chromium.org/p/chromium.mac/builders/Mac10.8%20Tests/builds/4859 http://build.chromium.org/p/chromium.mac/builders/Mac10.8%20Tests/builds/4860. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
