Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2021253002:
Skip profiles in GetLastOpenedProfiles that fail to initialize (Closed)
DescriptionSkip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
Committed: https://crrev.com/4552c5134b38e43f1169e10810820bd10e459aa4
Cr-Commit-Position: refs/heads/master@{#399102}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed the bad DCHECK, updated comments #
Total comments: 12
Patch Set 3 : Fix variable name clash (stupid me) #Patch Set 4 : Fix bugs (the bug in #2 was not properly fixed) #Patch Set 5 : Update comments in code % pkasting's comments #
Total comments: 2
Patch Set 6 : Add test, fix nits #
Total comments: 35
Patch Set 7 : AddAceToObjectsSecurityDescriptor removed, nits fixed #Patch Set 8 : Removed test code #
Dependent Patchsets: Messages
Total messages: 61 (18 generated)
Description was changed from ========== Skip profiles in GetLastOpenedProfiles that fail to initialize If a profile fails to initialize, a nullptr is returned when we create that profile, causing a DCHECK failure. However, if this happens in Release, the DCHECK is bypassed, and a vector with nullptrs is returned. This results in crashes because callsites of GetLastOpenedProfiles does not check for nullptrs. BUG=614753 TEST=unknown (needs help to reproduce the failure) ========== to ========== Skip profiles in GetLastOpenedProfiles that fail to initialize If a profile fails to initialize, a nullptr is returned when we create that profile, causing a DCHECK failure. However, if this happens in Release, the DCHECK is bypassed, and a vector with nullptrs is returned. This results in crashes because callsites of GetLastOpenedProfiles does not check for nullptrs. See #17 of the bug for a detailed analysis of the crash. BUG=614753 TEST=unknown (needs help to reproduce the failure) ==========
lwchkg@chromium.org changed reviewers: + anthonyvd@chromium.org, pkasting@chromium.org, rogerta@chromium.org
Anthony: PTAL. Roger: PTAL. You own nothing by now, but you're likely the one who knows the most about the bugs involved. Peter: PTAL on startup_browser_creator.cc (the only changes are the comments)
https://codereview.chromium.org/2021253002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2021253002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:1344: if (profile) { This code violates the prohibition on handling DCHECK failure. Furthermore, based on your comments, it sounds like it's possible for some of the DCHECKs in this function to fail (since otherwise GetProfile() can't return null, AFAICT). Please update this code, determining which checks can fail and which cannot. Checks which cannot fail should not have conditional code to handle failure. Checks which can fail should not be DCHECKs to begin with. Please also update the comments for this function and/or GetProfile() to make it clear when they can fail, if ever. https://codereview.chromium.org/2021253002/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2021253002/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator.cc:751: // other problems. See crbug.com/614753. Nit: I'm not a fan of "see bug X" comments, because the bug contains a lot of information and it's not instantly obvious what about it the reader is supposed to see. Instead, if a reader needs to know more than what you've described here (do they?), then go ahead and write the additional info in the comment here directly.
lgtm
lgtm % pkasting@'s comments. FWIW it looks like the DCHECK(profile) in ProfileManager::CreateAndInitializeProfile is wrong because CreateProfile can indeed return null, so I'd personally advocate removing it. As for a repro case, from the code it looks like the conditions would be: - Create a profile and use it last (so that it's in last_used_profiles) - Shutdown Chrome - Delete the profile directory in the User Data Dir - Make the User Data Dir read only - Relaunch Chrome Or alternately, just launch Chrome with a read-only user_data_dir when last_used_profiles isn't empty (so you'd still need a local_state file in there). What'll happen is: - user_data_dir.AppendASCII(profile) will be "read_only_dir/profile_path" - CreateProfile will fail (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...)
On 2016/05/31 14:38:27, anthonyvd wrote: > lgtm % pkasting@'s comments. > > FWIW it looks like the DCHECK(profile) in > ProfileManager::CreateAndInitializeProfile is wrong because CreateProfile can > indeed return null, so I'd personally advocate removing it. I see. Thanks. And unfortunately it means that GetProfile() can return a nullptr now, and almost all callsites (there are about 10 of them) did not expect that it can legitimately happen. Every of these cases results in segfault by dereferencing the nullptr. The good news is... nothing becomes worse by this CL. BTW, I'm not the only one discovering that GetProfile() can return nullptr. jamescook@ has documented this in https://chromium.googlesource.com/chromium/src/+/8094dc81060e8d642d2d1e130671... > > As for a repro case, from the code it looks like the conditions would be: > > - Create a profile and use it last (so that it's in last_used_profiles) > - Shutdown Chrome > - Delete the profile directory in the User Data Dir > - Make the User Data Dir read only > - Relaunch Chrome > > Or alternately, just launch Chrome with a read-only user_data_dir when > last_used_profiles isn't empty (so you'd still need a local_state file in > there). What'll happen is: > > - user_data_dir.AppendASCII(profile) will be "read_only_dir/profile_path" > - CreateProfile will fail > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) Thanks. Hope that I can make a browser test with these ideas.
Patch #2 uploaded to address Peter's comments. PTAL. https://codereview.chromium.org/2021253002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2021253002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:1344: if (profile) { On 2016/05/31 11:41:09, Peter Kasting wrote: > This code violates the prohibition on handling DCHECK failure. > > Furthermore, based on your comments, it sounds like it's possible for some of > the DCHECKs in this function to fail (since otherwise GetProfile() can't return > null, AFAICT). > > Please update this code, determining which checks can fail and which cannot. > Checks which cannot fail should not have conditional code to handle failure. > Checks which can fail should not be DCHECKs to begin with. > > Please also update the comments for this function and/or GetProfile() to make it > clear when they can fail, if ever. Done. https://codereview.chromium.org/2021253002/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2021253002/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator.cc:751: // other problems. See crbug.com/614753. On 2016/05/31 11:41:09, Peter Kasting wrote: > Nit: I'm not a fan of "see bug X" comments, because the bug contains a lot of > information and it's not instantly obvious what about it the reader is supposed > to see. Instead, if a reader needs to know more than what you've described here > (do they?), then go ahead and write the additional info in the comment here > directly. Done.
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021253002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021253002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
LGTM https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile.h (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile.h:133: // profile directory fails, nullptr is returned. You said there are many other callers that presume this never returns null. Will you be fixing them separately? If not, who will be? We definitely want to address that issue now that we're aware of it. Nit: Avoid passive voice: // This can return null if |create_mode| is CREATE_MODE_SYNCHRONOUS and the // creation of the profile directory fails. https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1344: bool result = AddProfile(profile); Nit: You don't need to do this in this change, but touching this code makes me notice that AddProfile() cannot ever actually fail: it only returns false in a NOTREACHED() case, which means that code is also violating the rule on handling DCHECK failure. It would be nice to change that NOTREACHED to a DCHECK, remove the return value, and then simplify this to: if (profile) AddProfile(profile); (No other places in the code check the return value of that function anyway.) https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:91: // crbug.com/383019 and crbug.com/614753 for related crashes. Nit: This last sentence is unnecessary given that your previous sentence explicitly says the function can return null. As I noted before, I'm not a big fan of bug links in comments, and it doesn't seem like "yes, there have been crashes in the past" is something so important we need to link to the bugs, so I'd remove this. Super picky: I'd also use the word "null" in place of "nullptr" just because it reads more nicely, here and in the other places you do this below. https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:146: // errors) are skipped. My comments on this sentence from the last patch still apply. https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:306: // nullptr when fails. Nit: when -> if creation https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:751: // nullptr returns in Profile::CreateProfile. Nit: I'd probably just leave off the second clause here since it discusses an implementation detail of a function that's elsewhere; if we modify how that function works or what things are named this comment could go out of date, and the first clause seems descriptive enough. If you really feel like more detail is required, perhaps instead say something like "i.e. all such profiles encounter disk errors on creation".
On 2016/05/31 16:54:22, WC Leung wrote: > On 2016/05/31 14:38:27, anthonyvd wrote: > > > > As for a repro case, from the code it looks like the conditions would be: > > > > - Create a profile and use it last (so that it's in last_used_profiles) > > - Shutdown Chrome > > - Delete the profile directory in the User Data Dir > > - Make the User Data Dir read only > > - Relaunch Chrome > > > > Or alternately, just launch Chrome with a read-only user_data_dir when > > last_used_profiles isn't empty (so you'd still need a local_state file in > > there). What'll happen is: > > > > - user_data_dir.AppendASCII(profile) will be "read_only_dir/profile_path" > > - CreateProfile will fail > > > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) > > Thanks. Hope that I can make a browser test with these ideas. First attempt to reproduce: failed. Here's the result: F:\chromium>"runchromium_debug - Copy (4)" [8328:6412:0601/222244:ERROR:cache_util_win.cc(20)] Unable to move the cache: 5 [8328:6412:0601/222244:ERROR:cache_util.cc(134)] Unable to move cache folder e:\chromium_profiles_4\ShaderCache\GPUCache to e:\chromium_profiles_4\ShaderCache\old_GPUCache_000 [8328:6412:0601/222244:ERROR:cache_creator.cc(134)] Unable to create cache [8328:6412:0601/222244:ERROR:shader_disk_cache.cc(589)] Shader Cache Creation failed: -2 [8328:5928:0601/222244:ERROR:process_singleton_win.cc(419)] Lock file can not be created! Error code: 5 [8328:5928:0601/222244:ERROR:chrome_browser_main.cc(1477)] Failed to create a ProcessSingleton for your profile directory. This means that running multiple instances would start multiple browser processes rather than opening a new window in the existing process. Aborting now to avoid profile corruption. [8328:3900:0601/222244:WARNING:file_util_win.cc(333)] Failed to get temporary file name in e:\chromium_profiles_4: Access is denied. (0x5) [8328:3900:0601/222244:WARNING:important_file_writer.cc(55)] temp file failure: e:\chromium_profiles_4\Local State : could not create temporary file: Access is denied. (0x5) [8328:5928:0601/222244:WARNING:message_queue.cc(38)] Leaking 1 ports in unreceived messages [8328:5928:0601/222244:WARNING:message_queue.cc(38)] Leaking 1 ports in unreceived messages Looks like a read-only dir doesn't work, because a lock file of the directory cannot be generated. Is there any other way creating a directory fails? (My guess: disk full, intervention of bad anti-virus software. But those are not reproducible.)
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021253002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021253002/60001
Finally I've successful repro'ed the bug. Turn out I need to disable only "create folder" right on the user-data-dir (but NOT it's subdirectories.) Anyway, this may be too involving for creation of unit tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/01 16:44:33, WC Leung wrote: > Finally I've successful repro'ed the bug. Turn out I need to disable only > "create folder" right on the user-data-dir (but NOT it's subdirectories.) > > Anyway, this may be too involving for creation of unit tests. Just tested the fix here. (1) If the bad profile is not the |last_used_profile|, then the fix does it correct: just skip the bad profile. (Note: AFAIK the |last_used_profile| should be one of the |last_opened_profiles|. Correct me if I'm wrong.) (2) If the bad profile is indeed the |last_used_profile|, then Chromium does not start, but have no crashes. Here's the (useless) log: F:\chromium\src>..\"runchromium_debug - Copy (4)" [10196:10192:0602/012859:WARNING:file_util_win.cc(442)] Failed to create directory e:\chromium_profiles_4\Profile 1, last error is 5. [10196:10192:0602/012859:WARNING:message_queue.cc(38)] Leaking 1 ports in unreceived messages [10196:10192:0602/012859:WARNING:message_queue.cc(38)] Leaking 1 ports in unreceived messages In Release version we don't honor --enable-logging, so exactly nothing appears. Anyway, we need some better fallback behavior for case (2), possible one or more of the following: (a) crash nicely. (i.e. with CHECK(false) or LOG(error), plus a descriptive message.) (b) try to open other profiles. (If |last_opened_profiles| is not empty, I think we're going to do this.) (c) use a guest profile. (If we use this one, should we output a message in Chrome?) WDYT?
On 2016/06/01 17:41:32, WC Leung wrote: > Anyway, we need some better fallback behavior for case (2), possible one or more > of the following: > (a) crash nicely. (i.e. with CHECK(false) or LOG(error), plus a descriptive > message.) Don't do this. > (b) try to open other profiles. (If |last_opened_profiles| is not empty, I think > we're going to do this.) Yes, I would do this if there are any other previous profiles to open. > (c) use a guest profile. (If we use this one, should we output a message in > Chrome?) If there are no other previous profiles to open, either use guest mode or create a new profile (what if either/both of these fail?), and tell the user we couldn't open (names of profiles). I'd maybe add a Learn More link to a help center article about what to do here, and show this message in both cases above.
New patch uploaded. I'll try to create some Windows-only tests for the failure. No guarantee to succeed though. On 2016/06/01 17:49:19, Peter Kasting wrote: > On 2016/06/01 17:41:32, WC Leung wrote: > > Anyway, we need some better fallback behavior for case (2), possible one or > more > > of the following: > > (a) crash nicely. (i.e. with CHECK(false) or LOG(error), plus a descriptive > > message.) > > Don't do this. Acknowledged. > > > (b) try to open other profiles. (If |last_opened_profiles| is not empty, I > think > > we're going to do this.) > > Yes, I would do this if there are any other previous profiles to open. BTW, should we open other profiles if there is no |last_opened_profiles| to open? > > > (c) use a guest profile. (If we use this one, should we output a message in > > Chrome?) > > If there are no other previous profiles to open, either use guest mode or create > a new profile (what if either/both of these fail?), and tell the user we > couldn't open (names of profiles). I'd maybe add a Learn More link to a help > center article about what to do here, and show this message in both cases above. Guest mode: sounds good to me. Hopefully this always succeed. If fail, then this is a bug (and unfortunately needs to crash). New profile: won't bother trying because creating a profile is more likely to fail than succeed. When we reach this step, we DID try to create other profiles, but failed anyway. Show message: sounds good to me. Help center article: I'm not familiar with this. Anthony: WDYT? https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile.h (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile.h:133: // profile directory fails, nullptr is returned. On 2016/05/31 19:12:30, Peter Kasting wrote: > You said there are many other callers that presume this never returns null. > Will you be fixing them separately? If not, who will be? We definitely want to > address that issue now that we're aware of it. > I'll continue to fix them, most likely in separate CLs. Without fixing those related problems, the crashes simply continue in a different form. Anyway, I need to discuss with Anthony and maybe other people in the profile team about what to do in the other cases, particularly what to do if the |last_used_profile| cannot be opened. > Nit: Avoid passive voice: > > // This can return null if |create_mode| is CREATE_MODE_SYNCHRONOUS and the > // creation of the profile directory fails. Done. https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1344: bool result = AddProfile(profile); On 2016/05/31 19:12:30, Peter Kasting wrote: > Nit: You don't need to do this in this change, but touching this code makes me > notice that AddProfile() cannot ever actually fail: it only returns false in a > NOTREACHED() case, which means that code is also violating the rule on handling > DCHECK failure. It would be nice to change that NOTREACHED to a DCHECK, remove > the return value, and then simplify this to: > > if (profile) > AddProfile(profile); > > (No other places in the code check the return value of that function anyway.) You're right. Anyway the code here is having an age of 5+ years, and it seems that the code quality at that time is not as good as the new code. Anyway, will do it in another CL. The change needs a review. https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:91: // crbug.com/383019 and crbug.com/614753 for related crashes. On 2016/05/31 19:12:30, Peter Kasting wrote: > Nit: This last sentence is unnecessary given that your previous sentence > explicitly says the function can return null. As I noted before, I'm not a big > fan of bug links in comments, and it doesn't seem like "yes, there have been > crashes in the past" is something so important we need to link to the bugs, so > I'd remove this. > > Super picky: I'd also use the word "null" in place of "nullptr" just because it > reads more nicely, here and in the other places you do this below. Done. Anyway, I'll likely be as picky as you if I've got a chance to work in Google. https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:146: // errors) are skipped. On 2016/05/31 19:12:30, Peter Kasting wrote: > My comments on this sentence from the last patch still apply. Can't find the comment in the last patch. Are you referring to the comment in another file? https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:751: // nullptr returns in Profile::CreateProfile. On 2016/05/31 19:12:30, Peter Kasting wrote: > Nit: I'd probably just leave off the second clause here since it discusses an > implementation detail of a function that's elsewhere; if we modify how that > function works or what things are named this comment could go out of date, and > the first clause seems descriptive enough. > > If you really feel like more detail is required, perhaps instead say something > like "i.e. all such profiles encounter disk errors on creation". I see. Now the second clause is removed.
LGTM https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2021253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:146: // errors) are skipped. On 2016/06/01 18:56:55, WC Leung wrote: > On 2016/05/31 19:12:30, Peter Kasting wrote: > > My comments on this sentence from the last patch still apply. > > Can't find the comment in the last patch. Are you referring to the comment in > another file? Don't worry, your newest version of the comment is good. https://codereview.chromium.org/2021253002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2021253002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:299: // null if fails. Nit: "if creation fails" (it is the creation stage that would fail, right?)
lwchkg@chromium.org changed reviewers: + brettw@chromium.org
New patch with test uploaded. Anthony and Peter Kasting: PTAL. brettw@: Need your opinion: the function AddAceToObjectsSecurityDescriptor in chrome/browser/profiles/profile_manager_browsertest.cc is very similar to DenyPermission in base/test/test_file_util_win.cc. So I want to know whether it's a good idea to use the currently local DenyPermission (with no modification) instead. If yes, please advice on where to write the function declaration. Thanks! https://codereview.chromium.org/2021253002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2021253002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:299: // null if fails. On 2016/06/01 19:36:17, Peter Kasting wrote: > Nit: "if creation fails" (it is the creation stage that would fail, right?) Done.
Still LGTM % that browser test (It looks good but I'm not familiar enough with those mechanics to comment so I'll defer to the judgment of more knowledgeable people in that area).
https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:515: bool AddAceToObjectsSecurityDescriptor(LPTSTR filename, LPTSTR trustee, I'm OK duplicating this code. It's a bit yucky but it seems a bit difficult to generalize and it's just a wrapper around a single syscall. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:581: if (testname == "LastOpenedProfileMissing") { This seems potentially very confusing since people don't expect the test behavior to change based on the name of the test. Can you use this test harness only for the one test you need? It looks like otherwise this class is almost a no-op.
https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:47: #endif Nit: There's a lot of stuff in this block and added below, that's all Windows-specific. Does it make sense to move this to a separate _browsertest_win.cc file? https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:515: bool AddAceToObjectsSecurityDescriptor(LPTSTR filename, LPTSTR trustee, On 2016/06/03 21:47:24, brettw wrote: > I'm OK duplicating this code. It's a bit yucky but it seems a bit difficult to > generalize and it's just a wrapper around a single syscall. It looks to me as if these two are so close that just a few fields on the TRUSTEE are different. If that's true, I think you should indeed try to avoid the code duplication. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:516: DWORD access_rights, ACCESS_MODE access_mode, DWORD inheritance) { Nit: All lines of args must be indented the same. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:525: LOG(WARNING) << "GetNamedSecurityInfo Error " << result; Are you going to collect and analyze this log data? If not, don't log; logging statements bloat the binary and make it unclear whether we think the statement is even reachable. (3 places) https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:534: explicit_access.Trustee.ptstrName = trustee; Nit: Try to use initializer values if possible, e.g.: EXPLICIT_ACCESS explicit_access = {access_rights, access_mode, inheritance, {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_UNKNOWN, trustee}}; (Not sure how clang-format would wrap/indent this) This results in being explicit about all the fields in the TRUSTEE struct and is more efficient to boot. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:550: if (new_dacl) Don't conditionalize these LocalFree() calls; it should be valid to free a null pointer. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:551: LocalFree((HLOCAL)new_dacl); Consider creating a scoping object that frees these automatically to avoid the possibility of forgetting to free on exit/later refactoring breaking this. This is more compelling if added to base/win as another ScopedXXX object, and then used across the rest of the codebase where LocalFrees are currently performed, in which case you'd want to split this to a separate CL. The class could be something like ScopedLocal<T> where T is e.g. ACL or PSECURITY_DESCRIPTOR, and there should be a Receive() method as on ScopedComPtr() to allow initialization via outparam. Nit: No C-style casts. I don't think you need to cast at all; the code in acl.cc frees these types with no casting. (2 places) https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:568: // Tests may be reported as PASS even if the test has never been run. To I have no idea what this is talking about :(. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:569: // mitigate the issue, a "red flag" is raised for suspectable tests. These Do you mean "susceptible" or "suspicious"? https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:603: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:606: PRE_LastOpenedProfileMissing) { I'm not terribly familiar with using PRE_ in tests. Why can't the code in this testcase be unified with the testcase below? https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:637: ProfileManager* profile_manager = g_browser_process->profile_manager(); Why do you need this variable? https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:647: #endif // defined(OS_WIN) Nit: Blank line above this, or no blank below the opening #ifdef.
Thank you for your review. It's time to me to report the bug in the browser test (see below for details). I didn't do that "yesterday" because it was already 5 a.m. for me. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:47: #endif On 2016/06/04 01:24:25, Peter Kasting wrote: > Nit: There's a lot of stuff in this block and added below, that's all > Windows-specific. Does it make sense to move this to a separate > _browsertest_win.cc file? SGTM https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:515: bool AddAceToObjectsSecurityDescriptor(LPTSTR filename, LPTSTR trustee, On 2016/06/04 01:24:25, Peter Kasting wrote: > On 2016/06/03 21:47:24, brettw wrote: > > I'm OK duplicating this code. It's a bit yucky but it seems a bit difficult to > > generalize and it's just a wrapper around a single syscall. > > It looks to me as if these two are so close that just a few fields on the > TRUSTEE are different. If that's true, I think you should indeed try to avoid > the code duplication. I see. Then I'll do the deduplication. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:516: DWORD access_rights, ACCESS_MODE access_mode, DWORD inheritance) { On 2016/06/04 01:24:25, Peter Kasting wrote: > Nit: All lines of args must be indented the same. Acknowledged. Anyway, if that deduplication succeeds we don't need the code here. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:550: if (new_dacl) On 2016/06/04 01:24:25, Peter Kasting wrote: > Don't conditionalize these LocalFree() calls; it should be valid to free a null > pointer. Noted and thanks. https://msdn.microsoft.com/en-us/library/windows/desktop/aa366730%28v=vs.85%2... did confirm that it is valid to pass NULL to LocalFree. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:551: LocalFree((HLOCAL)new_dacl); On 2016/06/04 01:24:25, Peter Kasting wrote: > Consider creating a scoping object that frees these automatically to avoid the > possibility of forgetting to free on exit/later refactoring breaking this. > > This is more compelling if added to base/win as another ScopedXXX object, and > then used across the rest of the codebase where LocalFrees are currently > performed, in which case you'd want to split this to a separate CL. The class > could be something like ScopedLocal<T> where T is e.g. ACL or > PSECURITY_DESCRIPTOR, and there should be a Receive() method as on > ScopedComPtr() to allow initialization via outparam. > > Nit: No C-style casts. I don't think you need to cast at all; the code in > acl.cc frees these types with no casting. (2 places) Acknowledged. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:568: // Tests may be reported as PASS even if the test has never been run. To On 2016/06/04 01:24:25, Peter Kasting wrote: > I have no idea what this is talking about :(. You've stumpled on a bug in the browser test. :-) The main body of the test isn't run at all, but the test is shown to PASS. To reproduce: 1. change "Profile 1" into "Profile 2" in line 588 (dir_to_delete variable). 2. run the tests. Now it should fail on line 597 (i.e. EXPECT_FALSE(red_flag_);) 3. remove the line EXPECT_FALSE(red_flag_); 4. run the tests. The test should still FAIL, but actually it is a PASS! https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:569: // mitigate the issue, a "red flag" is raised for suspectable tests. These On 2016/06/04 01:24:25, Peter Kasting wrote: > Do you mean "susceptible" or "suspicious"? Susceptble. Sorry for wrong spelling. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:581: if (testname == "LastOpenedProfileMissing") { On 2016/06/03 21:47:24, brettw wrote: > This seems potentially very confusing since people don't expect the test > behavior to change based on the name of the test. Can you use this test harness > only for the one test you need? It looks like otherwise this class is almost a > no-op. We need to do remove a profile directory and set the ACL of another directory after the end of the PRE_ test and before the start of the main test. And I tried to do it in the TearDown step of the PRE_ test, and sadly failed. So sadly I have no other choices. Anyway, in gtest even the PRE_ test counts as a separate test, so (sadly again) the condition is necessary. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:603: }; On 2016/06/04 01:24:25, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Acknowledged. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:606: PRE_LastOpenedProfileMissing) { On 2016/06/04 01:24:25, Peter Kasting wrote: > I'm not terribly familiar with using PRE_ in tests. Why can't the code in this > testcase be unified with the testcase below? The test (PRE_ and non-PRE_ as a whole) do the following: (1) The PRE_ test simply set up 2 extra profiles for the main test. (2) Then the directory "Profile 1" is removed (in SetUpUserDataDirectory()) and also |user_data_dir| is set to deny creation of new directories. (3) Then start the main test with it's browser process. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:637: ProfileManager* profile_manager = g_browser_process->profile_manager(); On 2016/06/04 01:24:25, Peter Kasting wrote: > Why do you need this variable? I don't need it, sorry for that. Forgot to remove dead code that doesn't work. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:647: #endif // defined(OS_WIN) On 2016/06/04 01:24:25, Peter Kasting wrote: > Nit: Blank line above this, or no blank below the opening #ifdef. Acknowledged.
https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:568: // Tests may be reported as PASS even if the test has never been run. To On 2016/06/04 04:55:22, WC Leung wrote: > On 2016/06/04 01:24:25, Peter Kasting wrote: > > I have no idea what this is talking about :(. > > You've stumpled on a bug in the browser test. :-) The main body of the test > isn't run at all, but the test is shown to PASS. > > To reproduce: > 1. change "Profile 1" into "Profile 2" in line 588 (dir_to_delete variable). > 2. run the tests. Now it should fail on line 597 (i.e. EXPECT_FALSE(red_flag_);) > 3. remove the line EXPECT_FALSE(red_flag_); > 4. run the tests. The test should still FAIL, but actually it is a PASS! Please understand this bug completely before proceeding with this CL -- the workaround you have here isn't OK unless we understand much more deeply what's going on. I wonder if this is connected with the extremely confusing design of this test (see reply below). https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:606: PRE_LastOpenedProfileMissing) { On 2016/06/04 04:55:22, WC Leung wrote: > On 2016/06/04 01:24:25, Peter Kasting wrote: > > I'm not terribly familiar with using PRE_ in tests. Why can't the code in > this > > testcase be unified with the testcase below? > > The test (PRE_ and non-PRE_ as a whole) do the following: > (1) The PRE_ test simply set up 2 extra profiles for the main test. > (2) Then the directory "Profile 1" is removed (in SetUpUserDataDirectory()) and > also |user_data_dir| is set to deny creation of new directories. > (3) Then start the main test with it's browser process. That didn't really answer my question. Are you saying that the main test's browser process needs stuff from the PRE test but reads that stuff before we have an opportunity to do SetUp() etc.? This is all very unclear, which is why both Brett and I seem confused.
Description was changed from ========== Skip profiles in GetLastOpenedProfiles that fail to initialize If a profile fails to initialize, a nullptr is returned when we create that profile, causing a DCHECK failure. However, if this happens in Release, the DCHECK is bypassed, and a vector with nullptrs is returned. This results in crashes because callsites of GetLastOpenedProfiles does not check for nullptrs. See #17 of the bug for a detailed analysis of the crash. BUG=614753 TEST=unknown (needs help to reproduce the failure) ========== to ========== Skip profiles in GetLastOpenedProfiles that fail to initialize If a profile fails to initialize, a nullptr is returned when we create that profile, causing a DCHECK failure. However, if this happens in Release, the DCHECK is bypassed, and a vector with nullptrs is returned. This results in crashes because callsites of GetLastOpenedProfiles does not check for nullptrs. See #17 of the bug for a detailed analysis of the crash. BUG=614753 TEST= 1. Use a Windows version. 2. Start chromium with a new user data directory, e.g. chromium --user-data-dir=d:\datadir1 3. Create two extra profiles, so you have "Person 1", "Person 2" and "Person 3" 4. Exit by Ctrl+Shift+Q while the browser windows are still open. 5. Now remove the directory "Profile 1" in the user data directory. 6. Set the permission of the user data directory to deny creating a new folder. (a) Right-click the directory, select "Properties". (b) Click "Advanced" (c) Click "Add" in "Permission" tab. (d) Click "Select a principal", type "EVERYONE" in the box, then press "Okay". (e) In the combo boxes, select "Type: Deny" and "Applies To: This folder only". (f) Select "Show advanced permissions". (g) Uncheck all, then check "Create folders / append data", which is at the lower left corner. (h) Click Ok in the three dialog boxes. 7. Start chromium again (repeat step (1)). Expected behavior: Before patch - crash. After patch - chromium starts with two browser windows in step 7. If step 6 is not taken - chromium starts with three browser windows in step 7.created. ==========
Description was changed from
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g. chromium
--user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and "Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating a new folder.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press "Okay".
(e) In the combo boxes, select "Type: Deny" and "Applies To: This folder
only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is at the
lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with two browser windows in step 7.
If step 6 is not taken - chromium starts with three browser windows in step
7.created.
==========
to
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
==========
https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:568: // Tests may be reported as PASS even if the test has never been run. To On 2016/06/04 08:01:39, Peter Kasting wrote: > Please understand this bug completely before proceeding with this CL -- the > workaround you have here isn't OK unless we understand much more deeply what's > going on. Agree that the bug needs more investigation. The perfect way to land the CL is to wait for the fix, so we don't need a special flag in the text fixture. But bug 614753 is getting pushed. So I think we'd move the test into a separate CL and commit the rest. > > I wonder if this is connected with the extremely confusing design of this test > (see reply below). I think it's related to paragraph (2) in comment #20 of this CL. Somehow chromium didn't start, but there's no crash in that case. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:606: PRE_LastOpenedProfileMissing) { On 2016/06/04 08:01:39, Peter Kasting wrote: > On 2016/06/04 04:55:22, WC Leung wrote: > > On 2016/06/04 01:24:25, Peter Kasting wrote: > > > I'm not terribly familiar with using PRE_ in tests. Why can't the code in > > this > > > testcase be unified with the testcase below? > > > > The test (PRE_ and non-PRE_ as a whole) do the following: > > (1) The PRE_ test simply set up 2 extra profiles for the main test. > > (2) Then the directory "Profile 1" is removed (in SetUpUserDataDirectory()) > and > > also |user_data_dir| is set to deny creation of new directories. > > (3) Then start the main test with it's browser process. > > That didn't really answer my question. Are you saying that the main test's > browser process needs stuff from the PRE test but reads that stuff before we > have an opportunity to do SetUp() etc.? This is all very unclear, which is why > both Brett and I seem confused. Oh sorry for my bad explanation. What you're saying is close. The purpose of the PRE test is to set up some states for the main test. After the PRE test finish, the main test starts with a new process, but using same user data directory as the PRE test. The "Tests Spanning Restarts" in https://www.chromium.org/developers/testing/browser-tests has made a brief introduction to PRE tests, but the details are not available unless we read the source. In this test the information shared are a few profiles and their respective browser windows. The switch switches::kRestoreLastSession was used to make the main test starts with the same set of windows as the PRE test. BTW, SetUpUserDataDir is called during SetUp, and is said to be the place to do something in the user data directory before the browser process starts. https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... In this test, some profile(s) are deleted in SetUpUserDataDir, and the user data directory is also set to deny creation of new directories (profiles). In this way the browser process will crash in the unpatched code.
Patchset #7 (id:120001) has been deleted
New patch uploaded with AddAceToObjectsSecurityDescriptor removed and a few nits fixed. I'll move the test code to another CL as soon as the try bots are green. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:569: // mitigate the issue, a "red flag" is raised for suspectable tests. These On 2016/06/04 04:55:22, WC Leung wrote: > On 2016/06/04 01:24:25, Peter Kasting wrote: > > Do you mean "susceptible" or "suspicious"? > > Susceptble. Sorry for wrong spelling. Done. https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:603: }; On 2016/06/04 04:55:22, WC Leung wrote: > On 2016/06/04 01:24:25, Peter Kasting wrote: > > Nit: DISALLOW_COPY_AND_ASSIGN > > Acknowledged. Unable to do this in test fixtures, because compilation fails if this statement is added.
https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:603: }; On 2016/06/04 17:34:43, WC Leung wrote: > On 2016/06/04 04:55:22, WC Leung wrote: > > On 2016/06/04 01:24:25, Peter Kasting wrote: > > > Nit: DISALLOW_COPY_AND_ASSIGN > > > > Acknowledged. > > > Unable to do this in test fixtures, because compilation fails if this statement > is added. Why?
https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:603: }; > > Unable to do this in test fixtures, because compilation fails if this > statement > > is added. > > Why? Here's the compiler log: f:\chromium\src\chrome\browser\profiles\profile_manager_browsertest.cc(558): error C2512: 'ProfileManagerWinBrowserTest': no appropriate default constructor available f:\chromium\src\chrome\browser\profiles\profile_manager_browsertest.cc(517): note: see declaration of 'ProfileManagerWinBrowserTest' And adding "public ProfileManagerWinBrowserTest() {}" in the public section solves the problem.
On 2016/06/04 15:48:33, WC Leung wrote: > https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... > File chrome/browser/profiles/profile_manager_browsertest.cc (right): > > https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... > chrome/browser/profiles/profile_manager_browsertest.cc:568: // Tests may be > reported as PASS even if the test has never been run. To > On 2016/06/04 08:01:39, Peter Kasting wrote: > > Please understand this bug completely before proceeding with this CL -- the > > workaround you have here isn't OK unless we understand much more deeply what's > > going on. > > Agree that the bug needs more investigation. The perfect way to land the CL is > to wait for the fix, so we don't need a special flag in the text fixture. > > But bug 614753 is getting pushed. So I think we'd move the test into a separate > CL and commit the rest. > > > > > I wonder if this is connected with the extremely confusing design of this test > > (see reply below). > > I think it's related to paragraph (2) in comment #20 of this CL. Somehow > chromium didn't start, but there's no crash in that case. > > https://codereview.chromium.org/2021253002/diff/100001/chrome/browser/profile... > chrome/browser/profiles/profile_manager_browsertest.cc:606: > PRE_LastOpenedProfileMissing) { > On 2016/06/04 08:01:39, Peter Kasting wrote: > > On 2016/06/04 04:55:22, WC Leung wrote: > > > On 2016/06/04 01:24:25, Peter Kasting wrote: > > > > I'm not terribly familiar with using PRE_ in tests. Why can't the code in > > > this > > > > testcase be unified with the testcase below? > > > > > > The test (PRE_ and non-PRE_ as a whole) do the following: > > > (1) The PRE_ test simply set up 2 extra profiles for the main test. > > > (2) Then the directory "Profile 1" is removed (in SetUpUserDataDirectory()) > > and > > > also |user_data_dir| is set to deny creation of new directories. > > > (3) Then start the main test with it's browser process. > > > > That didn't really answer my question. Are you saying that the main test's > > browser process needs stuff from the PRE test but reads that stuff before we > > have an opportunity to do SetUp() etc.? This is all very unclear, which is > why > > both Brett and I seem confused. > > Oh sorry for my bad explanation. What you're saying is close. > > The purpose of the PRE test is to set up some states for the main test. After > the PRE test finish, the main test starts with a new process, but using same > user data directory as the PRE test. The "Tests Spanning Restarts" in > https://www.chromium.org/developers/testing/browser-tests has made a brief > introduction to PRE tests, but the details are not available unless we read the > source. > > In this test the information shared are a few profiles and their respective > browser windows. The switch switches::kRestoreLastSession was used to make the > main test starts with the same set of windows as the PRE test. > > BTW, SetUpUserDataDir is called during SetUp, and is said to be the place to do > something in the user data directory before the browser process starts. > https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... > https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... > > In this test, some profile(s) are deleted in SetUpUserDataDir, and the user data > directory is also set to deny creation of new directories (profiles). In this > way the browser process will crash in the unpatched code. You can't structure the tests this way which is why you're having all of these problems. The tests don't necessarily run in order, and on the bots, may not run on the same machine. It should be possible for a developer to run only one test with --gtest_filter. Please express this as just one test.
> You can't structure the tests this way which is why you're having all of these > problems. The tests don't necessarily run in order, and on the bots, may not run > on the same machine. It should be possible for a developer to run only one test > with --gtest_filter. > > Please express this as just one test. Puzzled. Just to show that the tests aren't meant to be run alone with a log below. While the harness counts the PRE_ test and the main test as two tests, they're only runnable as one whole set. For the rest of your claim, I need to produce a detailed read of the browser test harness verify your claim. Anyway, the section "Tests Spanning Restarts" appears to suggest PRE_ test and the main test are run using a single user_data_dir and the PRE_ test guarantees to run before the main test. [I7-4790] Mon 06/06/2016 6:42:51.13 F:\chromium\src\out\Release>browser_tests --gtest_filter=ProfileManagerWinBrowserTest.LastOpenedProfileMissing IMPORTANT DEBUGGING NOTE: each test is run inside its own process. For debugging a test inside a debugger, use the --gtest_filter=<your_test_name> flag along with either --single_process (to run the test in one launcher/browser process) or --single-process (to do the above, and also run Chrome in single-process mode). Using sharding settings from environment. This is shard 0/1 Using 1 parallel jobs. Note: Google Test filter = ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ProfileManagerWinBrowserTest, where TypeParam = [ RUN ] ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing [18012:10768:0606/064309:WARNING:sqlite_persistent_cookie_store.cc(1360)] Failed to post task from net::SQLitePersistentCookieStore::Backend::FinishedLoadingCookies@f:\chromium\src\net\extras\sqlite\sqlite_persistent_cookie_store.cc:1368 to client_task_runner_. [18012:10768:0606/064309:WARNING:sqlite_persistent_cookie_store.cc(1360)] Failed to post task from net::SQLitePersistentCookieStore::Backend::LoadKeyAndNotifyInBackground@f:\chromium\src\net\extras\sqlite\sqlite_persistent_cookie_store.cc:545 to client_task_runner_. [ OK ] ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing (4962 ms) [----------] 1 test from ProfileManagerWinBrowserTest (4969 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (4984 ms total) [ PASSED ] 1 test. [1/2] ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing (5083 ms) Note: Google Test filter = ProfileManagerWinBrowserTest.LastOpenedProfileMissing [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ProfileManagerWinBrowserTest, where TypeParam = [ RUN ] ProfileManagerWinBrowserTest.LastOpenedProfileMissing [ OK ] ProfileManagerWinBrowserTest.LastOpenedProfileMissing (1431 ms) [----------] 1 test from ProfileManagerWinBrowserTest (1435 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (1451 ms total) [ PASSED ] 1 test. [2/2] ProfileManagerWinBrowserTest.LastOpenedProfileMissing (1539 ms) SUCCESS: all tests passed. [I7-4790] Mon 06/06/2016 6:43:13.27 F:\chromium\src\out\Release>browser_tests --gtest_filter=ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing IMPORTANT DEBUGGING NOTE: each test is run inside its own process. For debugging a test inside a debugger, use the --gtest_filter=<your_test_name> flag along with either --single_process (to run the test in one launcher/browser process) or --single-process (to do the above, and also run Chrome in single-process mode). Using sharding settings from environment. This is shard 0/1 Using 1 parallel jobs. 0 tests run [I7-4790] Mon 06/06/2016 6:44:34.59 F:\chromium\src\out\Release>
> You can't structure the tests this way which is why you're having all of these > problems. The tests don't necessarily run in order, and on the bots, may not run > on the same machine. It should be possible for a developer to run only one test > with --gtest_filter. > > Please express this as just one test. Puzzled. Just to show that the tests aren't meant to be run alone with a log below. While the harness counts the PRE_ test and the main test as two tests, they're only runnable as one whole set. For the rest of your claim, I need to produce a detailed read of the browser test harness verify your claim. Anyway, the section "Tests Spanning Restarts" appears to suggest PRE_ test and the main test are run using a single user_data_dir and the PRE_ test guarantees to run before the main test. [I7-4790] Mon 06/06/2016 6:42:51.13 F:\chromium\src\out\Release>browser_tests --gtest_filter=ProfileManagerWinBrowserTest.LastOpenedProfileMissing IMPORTANT DEBUGGING NOTE: each test is run inside its own process. For debugging a test inside a debugger, use the --gtest_filter=<your_test_name> flag along with either --single_process (to run the test in one launcher/browser process) or --single-process (to do the above, and also run Chrome in single-process mode). Using sharding settings from environment. This is shard 0/1 Using 1 parallel jobs. Note: Google Test filter = ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ProfileManagerWinBrowserTest, where TypeParam = [ RUN ] ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing [18012:10768:0606/064309:WARNING:sqlite_persistent_cookie_store.cc(1360)] Failed to post task from net::SQLitePersistentCookieStore::Backend::FinishedLoadingCookies@f:\chromium\src\net\extras\sqlite\sqlite_persistent_cookie_store.cc:1368 to client_task_runner_. [18012:10768:0606/064309:WARNING:sqlite_persistent_cookie_store.cc(1360)] Failed to post task from net::SQLitePersistentCookieStore::Backend::LoadKeyAndNotifyInBackground@f:\chromium\src\net\extras\sqlite\sqlite_persistent_cookie_store.cc:545 to client_task_runner_. [ OK ] ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing (4962 ms) [----------] 1 test from ProfileManagerWinBrowserTest (4969 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (4984 ms total) [ PASSED ] 1 test. [1/2] ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing (5083 ms) Note: Google Test filter = ProfileManagerWinBrowserTest.LastOpenedProfileMissing [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ProfileManagerWinBrowserTest, where TypeParam = [ RUN ] ProfileManagerWinBrowserTest.LastOpenedProfileMissing [ OK ] ProfileManagerWinBrowserTest.LastOpenedProfileMissing (1431 ms) [----------] 1 test from ProfileManagerWinBrowserTest (1435 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (1451 ms total) [ PASSED ] 1 test. [2/2] ProfileManagerWinBrowserTest.LastOpenedProfileMissing (1539 ms) SUCCESS: all tests passed. [I7-4790] Mon 06/06/2016 6:43:13.27 F:\chromium\src\out\Release>browser_tests --gtest_filter=ProfileManagerWinBrowserTest.PRE_LastOpenedProfileMissing IMPORTANT DEBUGGING NOTE: each test is run inside its own process. For debugging a test inside a debugger, use the --gtest_filter=<your_test_name> flag along with either --single_process (to run the test in one launcher/browser process) or --single-process (to do the above, and also run Chrome in single-process mode). Using sharding settings from environment. This is shard 0/1 Using 1 parallel jobs. 0 tests run [I7-4790] Mon 06/06/2016 6:44:34.59 F:\chromium\src\out\Release>
> For the rest of your claim, I need to produce a detailed read of the browser > test harness verify your claim. Anyway, the section "Tests Spanning Restarts" > appears to suggest PRE_ test and the main test are run using a single > user_data_dir and the PRE_ test guarantees to run before the main test. You can research what you want. But to be clear and to save you time, each test must be independent. Even if it happens to work on your computer, this is from a design principle perspective. Not lgtm (for the bot).
On 2016/06/06 05:19:28, brettw wrote: > > For the rest of your claim, I need to produce a detailed read of the browser > > test harness verify your claim. Anyway, the section "Tests Spanning Restarts" > > appears to suggest PRE_ test and the main test are run using a single > > user_data_dir and the PRE_ test guarantees to run before the main test. > > You can research what you want. But to be clear and to save you time, each test > must be independent. Even if it happens to work on your computer, this is from a > design principle perspective. > > Not lgtm (for the bot). Brett: Thank you for your clarification. As planned in #33, the tests are moved to crrev.com/2040893002 which is meant to be run but not to be committed. I'll redesign the test when I have time. Anyway, Brett: PTAL of the newest patch (with tests removed). Peter: if you want to have a look, please compare it with patch 5. Regards, WC Leung.
LGTM
Probably other people's reviews are sufficient so I'll say LGTM to undo my previous message (although I didn't really look). Somebody did point out to me that the PRE_ thing you did is actually a real thing. I assumed you invented that so sorry about the confusion. Nobody I have talked to had ever heard of it though, so I'm not surprised if it has bugs, and it seems confusing. I suspect we should have implemented that feature differently, although I don't have any context of what was considered when we added it.
On 2016/06/09 22:26:47, brettw wrote: > Probably other people's reviews are sufficient so I'll say LGTM to undo my > previous message (although I didn't really look). > > Somebody did point out to me that the PRE_ thing you did is actually a real > thing. I assumed you invented that so sorry about the confusion. Nobody I have > talked to had ever heard of it though, so I'm not surprised if it has bugs, and > it seems confusing. I suspect we should have implemented that feature > differently, although I don't have any context of what was considered when we > added it. Thanks everyone. Now I'll commit. Anyway, not sure if the PRE_ run in this test can be replaced by a mock just to create config files, so the test can be made more readable. I'll come back to the test when I finish the patch about a similar crash in ProfileChooserView/avatar menu.
On 2016/06/09 22:26:47, brettw wrote: > Probably other people's reviews are sufficient so I'll say LGTM to undo my > previous message (although I didn't really look). > > Somebody did point out to me that the PRE_ thing you did is actually a real > thing. I assumed you invented that so sorry about the confusion. Nobody I have > talked to had ever heard of it though, so I'm not surprised if it has bugs, and > it seems confusing. I suspect we should have implemented that feature > differently, although I don't have any context of what was considered when we > added it. Thanks everyone. Now I'll commit. Anyway, not sure if the PRE_ run in this test can be replaced by a mock just to create config files, so the test can be made more readable. I'll come back to the test when I finish the patch about a similar crash in ProfileChooserView/avatar menu.
The CQ bit was checked by lwchkg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2021253002/#ps160001 (title: "Removed test code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021253002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lwchkg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021253002/160001
Message was sent while issue was closed.
Description was changed from
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
==========
to
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
==========
to
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
Committed: https://crrev.com/4552c5134b38e43f1169e10810820bd10e459aa4
Cr-Commit-Position: refs/heads/master@{#399102}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4552c5134b38e43f1169e10810820bd10e459aa4 Cr-Commit-Position: refs/heads/master@{#399102}
Message was sent while issue was closed.
Description was changed from
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
Committed: https://crrev.com/4552c5134b38e43f1169e10810820bd10e459aa4
Cr-Commit-Position: refs/heads/master@{#399102}
==========
to
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 2" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
Committed: https://crrev.com/4552c5134b38e43f1169e10810820bd10e459aa4
Cr-Commit-Position: refs/heads/master@{#399102}
==========
Message was sent while issue was closed.
Updated the descriptions. Step 5 of the test instructions was wrong. Sorry for that.
Message was sent while issue was closed.
Description was changed from
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 2" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
Committed: https://crrev.com/4552c5134b38e43f1169e10810820bd10e459aa4
Cr-Commit-Position: refs/heads/master@{#399102}
==========
to
==========
Skip profiles in GetLastOpenedProfiles that fail to initialize
If a profile fails to initialize, a nullptr is returned when we create
that profile, causing a DCHECK failure. However, if this happens in
Release, the DCHECK is bypassed, and a vector with nullptrs is returned.
This results in crashes because callsites of GetLastOpenedProfiles does
not check for nullptrs.
See #17 of the bug for a detailed analysis of the crash.
BUG=614753
TEST=
1. Use a Windows version.
2. Start chromium with a new user data directory, e.g.
chromium --user-data-dir=d:\datadir1
3. Create two extra profiles, so you have "Person 1", "Person 2" and
"Person 3"
4. Exit by Ctrl+Shift+Q while the browser windows are still open.
5. Now remove the directory "Profile 1" in the user data directory.
6. Set the permission of the user data directory to deny creating new
directories.
(a) Right-click the directory, select "Properties".
(b) Click "Advanced"
(c) Click "Add" in "Permission" tab.
(d) Click "Select a principal", type "EVERYONE" in the box, then press
"Okay".
(e) In the combo boxes, select "Type: Deny" and
"Applies To: This folder only".
(f) Select "Show advanced permissions".
(g) Uncheck all, then check "Create folders / append data", which is
at the lower left corner.
(h) Click Ok in the three dialog boxes.
7. Start chromium again (repeat step (1)).
Expected behavior:
Before patch - crash.
After patch - chromium starts with TWO browser windows in step 7.
If step 6 is not taken - chromium starts with THREE browser windows in
step 7
Committed: https://crrev.com/4552c5134b38e43f1169e10810820bd10e459aa4
Cr-Commit-Position: refs/heads/master@{#399102}
==========
Message was sent while issue was closed.
On 2016/06/15 12:33:16, WC Leung wrote: > Updated the descriptions. Step 5 of the test instructions was wrong. Sorry for > that. The update is bogus (terribly sorry for that). The original instruction is correct. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
