Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2061593002:
Fix crash when switching to a profile that cannot be opened (Closed)
DescriptionFix crash for switching to a profile that cannot be opened
When a profile fails to initialize, it was deleted immediately. This was
causing use-after-free problems. This CL defers the destruction of such
profiles to browser shutdown, which is the same time as other profiles.
BUG=620312
R=pkasting@chromium.org
R=anthonyvd@chromium.org
R=phajdan.jr@chromium.org
1. Use a Windows version of Chromium.
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" and "Person 2".
4. Close the window "Person 2".
5. Then close the window "Person 1".
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.
8. Right-click on the text "Profile 1".
9. Select "Profile 2".
Expected behavior:
Before patch - crash.
After patch - an dialog box is displayed, and the profile is NOT
opened (of course we cannot do it). Anyway, if the text
in the dialog box is not useful enough, please report.
Patch Set 1 #
Total comments: 28
Patch Set 2 : Rebase #Patch Set 3 : Respond to nits, fix tests #
Total comments: 21
Patch Set 4 : WIP, do not commit #Patch Set 5 : Updated test_file_util_win.cc % pkasting@'s comment. #Patch Set 6 : Move DenyFilePermission to match the place in test_file_util.h #
Total comments: 24
Patch Set 7 : Respond to comments. #Patch Set 8 : Respond to pkasting@'s comments #
Total comments: 3
Messages
Total messages: 54 (11 generated)
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/2061593002/1
This is the third (hopefully last) CL on the crash in issue 614753. Anthony: PTAL. Peter: PTAL. You own chrome/browser/ui/* but I guess you're interested in other parts of the CL as well. Paweł: PTAL on base/test/*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:82: if (GetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()), Nit: Avoid const_cast. Store this in a local and pass the address of the local. (2 locals covering 3 places) https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:84: DACL_SECURITY_INFORMATION, NULL, NULL, &old_dacl, Nit: nullptr (several places) https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:89: EXPLICIT_ACCESS change; Stolen from my comments on one of your previous CLs: Nit: Try to use initializer values if possible, e.g.: EXPLICIT_ACCESS explicit_access = {permission, DENY_ACCESS, 0, {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_USER, trustee}}; (Not sure how clang-format would wrap/indent this) https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:101: LocalFree(security_descriptor); Stolen from my comments on one of your previous CLs: 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. https://codereview.chromium.org/2061593002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2061593002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11275: + <message name="IDS_COULDNT_SWITCH_PROFILE_ERROR" desc="Error displayed when the profile to be switched to can not be opened correctly due to problems reading or writing files in it."> Why does this message need to be separate from IDS_COULDNT_OPEN_PROFILE_ERROR? The two are nearly identical. Can't we unify on a single message for both? The reason I could see to have two would be if this one was more like "The profile '<username>' could not be opened." But it might make sense to provide the profile name on the other message anyway. https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:1108: failed_profiles_.emplace_back(std::move(iter->second->profile)); Nit: There's no real reason here to use emplace_back() over push_back(). https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:404: // Owns profiles that fails initialization. Like normal profiles, they are not Nit: fail https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:405: // safe to destroy from memory before shutdown. Nit: "...they must not be deleted before shutdown." But it would be nice to explain why, since it's not obvious why a failed profile can't just be deleted. https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager_browsertest.cc:361: #if defined(OS_WIN) It would be nice to be able to run this test on all platforms. Is it possible to implement DenyFilePermission() on non-Win? https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_window.cc:221: return; Nit: Not necessary (next conditional does this for us)
Replies... Anyway, I'll do a rebase in my computer to see if the test also fail in my computer. https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:82: if (GetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()), On 2016/06/13 04:43:20, Peter Kasting wrote: > Nit: Avoid const_cast. Store this in a local and pass the address of the local. > (2 locals covering 3 places) Acknowledged. https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:84: DACL_SECURITY_INFORMATION, NULL, NULL, &old_dacl, On 2016/06/13 04:43:20, Peter Kasting wrote: > Nit: nullptr (several places) Acknowledged. https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:89: EXPLICIT_ACCESS change; On 2016/06/13 04:43:20, Peter Kasting wrote: > Stolen from my comments on one of your previous CLs: > > Nit: Try to use initializer values if possible, e.g.: > > EXPLICIT_ACCESS explicit_access = > {permission, DENY_ACCESS, 0, > {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_USER, > trustee}}; > > (Not sure how clang-format would wrap/indent this) Pawel: WDYT about the proposed changes? I'll let you decide on this one. (Anyway, both the current way and the initializer values looks good to me.) https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:101: LocalFree(security_descriptor); On 2016/06/13 04:43:20, Peter Kasting wrote: > Stolen from my comments on one of your previous CLs: > > 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. SGTM. BTW, I'll check against the existing ScopedXXX objects before adding one. https://codereview.chromium.org/2061593002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2061593002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11275: + <message name="IDS_COULDNT_SWITCH_PROFILE_ERROR" desc="Error displayed when the profile to be switched to can not be opened correctly due to problems reading or writing files in it."> On 2016/06/13 04:43:20, Peter Kasting wrote: > Why does this message need to be separate from IDS_COULDNT_OPEN_PROFILE_ERROR? > The two are nearly identical. Can't we unify on a single message for both? > > The reason I could see to have two would be if this one was more like "The > profile '<username>' could not be opened." But it might make sense to provide > the profile name on the other message anyway. I see. Let me check the context of the other message. (It appears the error that results in the other message is currently being discussed in chrome-dev group.) https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:1108: failed_profiles_.emplace_back(std::move(iter->second->profile)); On 2016/06/13 04:43:20, Peter Kasting wrote: > Nit: There's no real reason here to use emplace_back() over push_back(). Acknowledged. https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:404: // Owns profiles that fails initialization. Like normal profiles, they are not On 2016/06/13 04:43:20, Peter Kasting wrote: > Nit: fail Acknowledged. https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:405: // safe to destroy from memory before shutdown. On 2016/06/13 04:43:20, Peter Kasting wrote: > Nit: "...they must not be deleted before shutdown." > > But it would be nice to explain why, since it's not obvious why a failed profile > can't just be deleted. Acknowledged. https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager_browsertest.cc:361: #if defined(OS_WIN) On 2016/06/13 04:43:20, Peter Kasting wrote: > It would be nice to be able to run this test on all platforms. Is it possible > to implement DenyFilePermission() on non-Win? Sadly I don't think that is possible. I've done a web search, and found no equivalent of FILE_ADD_SUBDIRECTORY permission in linux. I don't think there's an equivalent in Mac too, because the "rwx" system is also used. The good news is : the bug appears to be Windows only. (Peter, can you confirm this for me?) https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_window.cc:221: return; On 2016/06/13 04:43:20, Peter Kasting wrote: > Nit: Not necessary (next conditional does this for us) Acknowledged.
On 2016/06/13 08:06:45, WC Leung wrote: > Replies... > Anyway, I'll do a rebase in my computer to see if the test also fail in my > computer. > Update: test failed after rebase. So my computer is still reliable. :-)
Patchset #2 (id:20001) has been deleted
base/test LGTM
c/b/profiles/* lgtm
Patchset #2 (id:40001) has been deleted
New patch uploaded. Peter: PTAL. Pawel: PTAL again to see if the changes to base/test is good. If not I'm willing to revert to an LGTMed patch. Anthony: PTAL on ProfileIOData and the updated tests. https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:82: if (GetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()), On 2016/06/13 08:06:45, WC Leung wrote: > On 2016/06/13 04:43:20, Peter Kasting wrote: > > Nit: Avoid const_cast. Store this in a local and pass the address of the > local. > > (2 locals covering 3 places) > > Acknowledged. Do you know of any good method to copy the contents a std::wstring into a local C string? https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:84: DACL_SECURITY_INFORMATION, NULL, NULL, &old_dacl, On 2016/06/13 08:06:45, WC Leung wrote: > On 2016/06/13 04:43:20, Peter Kasting wrote: > > Nit: nullptr (several places) > > Acknowledged. Done. https://codereview.chromium.org/2061593002/diff/1/base/test/test_file_util_wi... base/test/test_file_util_win.cc:89: EXPLICIT_ACCESS change; Done. Pawel: please see if this is good. On 2016/06/13 08:06:45, WC Leung wrote: > On 2016/06/13 04:43:20, Peter Kasting wrote: > > Stolen from my comments on one of your previous CLs: > > > > Nit: Try to use initializer values if possible, e.g.: > > > > EXPLICIT_ACCESS explicit_access = > > {permission, DENY_ACCESS, 0, > > {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_USER, > > trustee}}; > > > > (Not sure how clang-format would wrap/indent this) > > Pawel: WDYT about the proposed changes? I'll let you decide on this one. > (Anyway, both the current way and the initializer values looks good to me.) https://codereview.chromium.org/2061593002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2061593002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:11275: + <message name="IDS_COULDNT_SWITCH_PROFILE_ERROR" desc="Error displayed when the profile to be switched to can not be opened correctly due to problems reading or writing files in it."> On 2016/06/13 08:06:45, WC Leung wrote: > On 2016/06/13 04:43:20, Peter Kasting wrote: > > Why does this message need to be separate from IDS_COULDNT_OPEN_PROFILE_ERROR? > > > The two are nearly identical. Can't we unify on a single message for both? > > > > The reason I could see to have two would be if this one was more like "The > > profile '<username>' could not be opened." But it might make sense to provide > > the profile name on the other message anyway. > > I see. Let me check the context of the other message. (It appears the error that > results in the other message is currently being discussed in chrome-dev group.) In the other CL Brett is giving similar comments. I'll just use the current message unmodified in this CL, because adding some $1 into the string results in non-trivial modifications of two files. See https://cs.chromium.org/search/?q=IDS_COULDNT_OPEN_PROFILE_ERROR&sq=package:c... https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:1108: failed_profiles_.emplace_back(std::move(iter->second->profile)); On 2016/06/13 08:06:45, WC Leung wrote: > On 2016/06/13 04:43:20, Peter Kasting wrote: > > Nit: There's no real reason here to use emplace_back() over push_back(). > > Acknowledged. Done. https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:404: // Owns profiles that fails initialization. Like normal profiles, they are not On 2016/06/13 08:06:45, WC Leung wrote: > On 2016/06/13 04:43:20, Peter Kasting wrote: > > Nit: fail > > Acknowledged. Done. https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:405: // safe to destroy from memory before shutdown. On 2016/06/13 04:43:20, Peter Kasting wrote: > Nit: "...they must not be deleted before shutdown." > > But it would be nice to explain why, since it's not obvious why a failed profile > can't just be deleted. Actually some services (e.g. pref store) that Profile is depending on appears to be creating async tasks that outlives itself. My syzyasan report points to JSONPrefStore. BTW, I'd rather not having this written in the source code. Unfortunately I do not have enough understanding of the pref store as a whole, so I cannot fix it myself. https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager_browsertest.cc:361: #if defined(OS_WIN) > The good news is : the bug appears to be Windows only. (Peter, can you confirm > this for me?) I mean confirm with the crash logs (I don't have access to things internal to Google.)
Description was changed from ========== Fix crash for switching to a profile that cannot be opened When a profile fails to initialize, it was deleted immediately. This was causing use-after-free problems. This CL defers the destruction of such profiles to browser shutdown, which is the same time as other profiles. BUG=614753 R=pkasting@chromium.org R=anthonyvd@chromium.org R=phajdan.jr@chromium.org ========== to ========== Fix crash for switching to a profile that cannot be opened When a profile fails to initialize, it was deleted immediately. This was causing use-after-free problems. This CL defers the destruction of such profiles to browser shutdown, which is the same time as other profiles. BUG=614753 R=pkasting@chromium.org R=anthonyvd@chromium.org R=phajdan.jr@chromium.org 1. Use a Windows version of Chromium. 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" and "Person 2". 4. Close the window "Person 2". 5. Then close the window "Person 1". 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. 8. Right-click on the text "Profile 1". 9. Select "Profile 2". Expected behavior: Before patch - crash. After patch - an dialog box is displayed, and the profile is NOT opened (of course we cannot do it). Anyway, if the text in the dialog box is not useful enough, please report. ==========
Description was changed from ========== Fix crash for switching to a profile that cannot be opened When a profile fails to initialize, it was deleted immediately. This was causing use-after-free problems. This CL defers the destruction of such profiles to browser shutdown, which is the same time as other profiles. BUG=614753 R=pkasting@chromium.org R=anthonyvd@chromium.org R=phajdan.jr@chromium.org 1. Use a Windows version of Chromium. 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" and "Person 2". 4. Close the window "Person 2". 5. Then close the window "Person 1". 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. 8. Right-click on the text "Profile 1". 9. Select "Profile 2". Expected behavior: Before patch - crash. After patch - an dialog box is displayed, and the profile is NOT opened (of course we cannot do it). Anyway, if the text in the dialog box is not useful enough, please report. ========== to ========== Fix crash for switching to a profile that cannot be opened When a profile fails to initialize, it was deleted immediately. This was causing use-after-free problems. This CL defers the destruction of such profiles to browser shutdown, which is the same time as other profiles. BUG=614753, 620312 R=pkasting@chromium.org R=anthonyvd@chromium.org R=phajdan.jr@chromium.org 1. Use a Windows version of Chromium. 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" and "Person 2". 4. Close the window "Person 2". 5. Then close the window "Person 1". 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. 8. Right-click on the text "Profile 1". 9. Select "Profile 2". Expected behavior: Before patch - crash. After patch - an dialog box is displayed, and the profile is NOT opened (of course we cannot do it). Anyway, if the text in the dialog box is not useful enough, please report. ==========
Description was changed from ========== Fix crash for switching to a profile that cannot be opened When a profile fails to initialize, it was deleted immediately. This was causing use-after-free problems. This CL defers the destruction of such profiles to browser shutdown, which is the same time as other profiles. BUG=614753, 620312 R=pkasting@chromium.org R=anthonyvd@chromium.org R=phajdan.jr@chromium.org 1. Use a Windows version of Chromium. 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" and "Person 2". 4. Close the window "Person 2". 5. Then close the window "Person 1". 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. 8. Right-click on the text "Profile 1". 9. Select "Profile 2". Expected behavior: Before patch - crash. After patch - an dialog box is displayed, and the profile is NOT opened (of course we cannot do it). Anyway, if the text in the dialog box is not useful enough, please report. ========== to ========== Fix crash for switching to a profile that cannot be opened When a profile fails to initialize, it was deleted immediately. This was causing use-after-free problems. This CL defers the destruction of such profiles to browser shutdown, which is the same time as other profiles. BUG=620312 R=pkasting@chromium.org R=anthonyvd@chromium.org R=phajdan.jr@chromium.org 1. Use a Windows version of Chromium. 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" and "Person 2". 4. Close the window "Person 2". 5. Then close the window "Person 1". 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. 8. Right-click on the text "Profile 1". 9. Select "Profile 2". Expected behavior: Before patch - crash. After patch - an dialog box is displayed, and the profile is NOT opened (of course we cannot do it). Anyway, if the text in the dialog box is not useful enough, please report. ==========
Created a new issue (623012) for tracking purpose, and moved the CL to the new issue (but still blocking on the original 614753).
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/2061593002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... base/test/test_file_util_win.cc:78: // Deny |permission| on the file |path|, for the current user. Nit: Definition order in .cc file must match declaration order in .h. https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... base/test/test_file_util_win.cc:82: if (GetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()), My comments about const_cast stand (3 places). In the case of the fixed constant below this is easy, you can just declare a local wchar_t* = L"..." directly. The other two places you will need to declare a wchar_t[] on the stack with sufficient size to hold path.value(), then copy in with wcscpy(). (Or use wcscpy_s() if you want to be really safe.) https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... base/test/test_file_util_win.cc:98: LocalFree(security_descriptor); I assume you decided to investigate the scoping object in a separate CL? https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1311: if (cert_transparency_verifier_) When can these be null? Unless it's super obvious to the observer, consider adding a comment about it. https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:405: // be deleted before shutdown. I'm uncomfortable with having this code and comment when you say you don't understand the pref store well enough to fix this (or, apparently, even comment in detail about what's going on here). If you don't understand well enough to do that, how do I know the code you're writing here is correct, and not just a band-aid? In general my reply to such things is "go back and keep investigating until you do understand it better, or find someone who does and work with them to determine the correct long term fix". https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_browsertest.cc:50: // expected. Nit: Not grammatical, and mostly restates the code anyway. Does this get called repeatedly, once with CREATE_STATUS_CREATED and then again after that? If so maybe: A callback that does not return control to the test until profile creation has returned the expected final status, or failed with some unexpected status along the way. https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_browsertest.cc:385: base::string16(), Nit: Could combine all these on one line, but whatever https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_browsertest.cc:391: run_loop2.RunUntilIdle(); What is this doing? https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/ui/profi... File chrome/browser/ui/profile_error_dialog.h (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/ui/profi... chrome/browser/ui/profile_error_dialog.h:21: PROFILE_ERROR_SWITCH_TO_FAILURE, Nit: This sounds like "we switched to failure" rather than "failure to switch to this profile." Consider changing this enum to an enum class named ProfileError so you can remove PROFILE_ERROR_ from in front of every value, then naming these more descriptively. For example, is HISTORY for errors opening the history DB, or does it cover more than that? A name like COULDNT_OPEN_HISTORY_DB would be way better than HISTORY, just as something like TRIED_TO_SWITCH_TO_UNOPENABLE_PROFILE would be better than SWITCH_TO_FAILURE.
lwchkg@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb as owner of component/perfs Peter: just some replies to your most important comments. Bauerb: adding you here because you may know what is happening with pref store. One of the reason of the crash in this CL is that JSONPrefStore is being used after freed. All I know about the use-after-free is that JSONPrefStore is freed when the owning profile object is deleted. Both Peter and me are wondering whether something can be done in the pref store classes solve the root cause of the crash. Thanks! (Currently the fix is a workaround deferring the deletion of the failed-to-initialize profile.) https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... base/test/test_file_util_win.cc:98: LocalFree(security_descriptor); On 2016/06/16 07:02:10, Peter Kasting wrote: > I assume you decided to investigate the scoping object in a separate CL? Yes. https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:405: // be deleted before shutdown. On 2016/06/16 07:02:11, Peter Kasting wrote: > I'm uncomfortable with having this code and comment when you say you don't > understand the pref store well enough to fix this (or, apparently, even comment > in detail about what's going on here). > > If you don't understand well enough to do that, how do I know the code you're > writing here is correct, and not just a band-aid? In general my reply to such > things is "go back and keep investigating until you do understand it better, or > find someone who does and work with them to determine the correct long term > fix". Actually there was some discussion about deleting profiles in the design document "Profile Architecture". It reads like this: > We absolutely have to address the interdependency of services. As it stands > today, we do not shut down profiles after they are no longer needed in > multiprofile mode because our crash rate when shutting down a profile is too > high to ship to users... So this is the "reason" that normal profiles are deleted when chrome is shutting down, instead of when the last browser windows of the profile is closed. The ugly thing that this CL does is to extend this workaround to profiles that fails to initialize. Since this workaround is already being used and documented I have grounds to believe (sadly not know) that it will not make things worse instead of better. For the crash part, I have found two services (one is JSONPrefStore) is crashing when the profile is deleted. Since I was unable to fix JSONPrefStore, I don't know whether there are other services crashing on such a deletion. I would like to say "probably" for the existing of the third crashing service. Anyway, it is worth asking owners of pref store to have a discussion. Bauer: WDYT? https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_browsertest.cc:391: run_loop2.RunUntilIdle(); On 2016/06/16 07:02:11, Peter Kasting wrote: > What is this doing? To wait for async initialization of profile is complete. Otherwise by finishing the test we delete the profile immediately and this leads to a crash. Anyway, I forgot about the actual stack trace of the crash. (Need to retry and see it again.) And well, hopefully nothing big is happening on non-UI threads. If there is indeed something big happening then RunUntilIdle makes tests flaky. https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/ui/profi... File chrome/browser/ui/profile_error_dialog.h (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/ui/profi... chrome/browser/ui/profile_error_dialog.h:21: PROFILE_ERROR_SWITCH_TO_FAILURE, On 2016/06/16 07:02:11, Peter Kasting wrote: > Nit: This sounds like "we switched to failure" rather than "failure to switch to > this profile." > > Consider changing this enum to an enum class named ProfileError so you can > remove PROFILE_ERROR_ from in front of every value, then naming these more > descriptively. For example, is HISTORY for errors opening the history DB, or > does it cover more than that? A name like COULDNT_OPEN_HISTORY_DB would be way > better than HISTORY, just as something like > TRIED_TO_SWITCH_TO_UNOPENABLE_PROFILE would be better than SWITCH_TO_FAILURE. Good point. I'll make my enum more descriptive. BTW, adding a class makes the enum shorter, but does not make the code in the callsites shorter (the class name is just prefixed).
https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:405: // be deleted before shutdown. On 2016/06/16 10:04:10, WC Leung wrote: > On 2016/06/16 07:02:11, Peter Kasting wrote: > > I'm uncomfortable with having this code and comment when you say you don't > > understand the pref store well enough to fix this (or, apparently, even > comment > > in detail about what's going on here). > > > > If you don't understand well enough to do that, how do I know the code you're > > writing here is correct, and not just a band-aid? In general my reply to such > > things is "go back and keep investigating until you do understand it better, > or > > find someone who does and work with them to determine the correct long term > > fix". > > Actually there was some discussion about deleting profiles in the design > document "Profile Architecture". It reads like this: > > > We absolutely have to address the interdependency of services. As it stands > > today, we do not shut down profiles after they are no longer needed in > > multiprofile mode because our crash rate when shutting down a profile is too > > high to ship to users... > > So this is the "reason" that normal profiles are deleted when chrome is shutting > down, instead of when the last browser windows of the profile is closed. The > ugly thing that this CL does is to extend this workaround to profiles that fails > to initialize. Since this workaround is already being used and documented I have > grounds to believe (sadly not know) that it will not make things worse instead > of better. > > For the crash part, I have found two services (one is JSONPrefStore) is crashing > when the profile is deleted. Since I was unable to fix JSONPrefStore, I don't > know whether there are other services crashing on such a deletion. I would like > to say "probably" for the existing of the third crashing service. > > Anyway, it is worth asking owners of pref store to have a discussion. > > Bauer: WDYT? FWIW, it seems very wrong to me for anyone to be depending-until-chrome-shutdown on any part of a profile that couldn't even be initialized. https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_browsertest.cc:391: run_loop2.RunUntilIdle(); On 2016/06/16 10:04:10, WC Leung wrote: > On 2016/06/16 07:02:11, Peter Kasting wrote: > > What is this doing? > > To wait for async initialization of profile is complete. Isn't that what the first run loop does? > Otherwise by finishing > the test we delete the profile immediately and this leads to a crash. > > Anyway, I forgot about the actual stack trace of the crash. (Need to retry and > see it again.) This sounds like it's related to the other issue. We really should ensure that you can immediately delete a profile that wasn't initialized. https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/ui/profi... File chrome/browser/ui/profile_error_dialog.h (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/ui/profi... chrome/browser/ui/profile_error_dialog.h:21: PROFILE_ERROR_SWITCH_TO_FAILURE, On 2016/06/16 10:04:10, WC Leung wrote: > On 2016/06/16 07:02:11, Peter Kasting wrote: > > Nit: This sounds like "we switched to failure" rather than "failure to switch > to > > this profile." > > > > Consider changing this enum to an enum class named ProfileError so you can > > remove PROFILE_ERROR_ from in front of every value, then naming these more > > descriptively. For example, is HISTORY for errors opening the history DB, or > > does it cover more than that? A name like COULDNT_OPEN_HISTORY_DB would be way > > better than HISTORY, just as something like > > TRIED_TO_SWITCH_TO_UNOPENABLE_PROFILE would be better than SWITCH_TO_FAILURE. > > Good point. I'll make my enum more descriptive. > > BTW, adding a class makes the enum shorter, but does not make the code in the > callsites shorter (the class name is just prefixed). I know.
https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:405: // be deleted before shutdown. On 2016/06/16 10:04:10, WC Leung wrote: > On 2016/06/16 07:02:11, Peter Kasting wrote: > > I'm uncomfortable with having this code and comment when you say you don't > > understand the pref store well enough to fix this (or, apparently, even > comment > > in detail about what's going on here). > > > > If you don't understand well enough to do that, how do I know the code you're > > writing here is correct, and not just a band-aid? In general my reply to such > > things is "go back and keep investigating until you do understand it better, > or > > find someone who does and work with them to determine the correct long term > > fix". > > Actually there was some discussion about deleting profiles in the design > document "Profile Architecture". It reads like this: > > > We absolutely have to address the interdependency of services. As it stands > > today, we do not shut down profiles after they are no longer needed in > > multiprofile mode because our crash rate when shutting down a profile is too > > high to ship to users... > > So this is the "reason" that normal profiles are deleted when chrome is shutting > down, instead of when the last browser windows of the profile is closed. The > ugly thing that this CL does is to extend this workaround to profiles that fails > to initialize. Since this workaround is already being used and documented I have > grounds to believe (sadly not know) that it will not make things worse instead > of better. > > For the crash part, I have found two services (one is JSONPrefStore) is crashing > when the profile is deleted. Since I was unable to fix JSONPrefStore, I don't > know whether there are other services crashing on such a deletion. I would like > to say "probably" for the existing of the third crashing service. > > Anyway, it is worth asking owners of pref store to have a discussion. > > Bauer: WDYT? We use JSONPrefStore in a couple of different places. Do you know which one crashes? Do you have a stack trace or something?
> We use JSONPrefStore in a couple of different places. Do you know which one > crashes? Do you have a stack trace or something? Please see https://drive.google.com/open?id=0BxDfZwJpDb1IVEdqZVdiSmVJNDA for the stack traces. Sadly I cannot produce a stack with a syzyasan report at the same run. And please note that you have to download https://codereview.chromium.org/2061563002/ to reproduce the crash (it fixes a few crashes that are triggered before pref store). And the crash does not generate a dump if a debugger is not used.
bauerb@chromium.org changed reviewers: + gab@chromium.org
+Gab, tracked prefs pro Okay, here's what I think is happening: * JSONPrefStore reports an error while reading prefs * InterceptablePrefFilter::FilterOnLoad() calls its callback * The callback eventually (but synchronously!) ends up calling into ProfileManager::OnProfileCreated() * ProfileManager::OnProfileCreated() deletes the profile because the profile load was unsuccessful * Everything transitively owned by the profile (including the filter) is deleted * Afterwards, control returns to InterceptablePrefFilter::FilterOnLoad(), which tries to reset the callback member that has been deleted. I think preventing accesses after the callback has been called should be sufficient to fix the crash (although there could be other instances hidden somewhere). There is a neat convenience function base::ResetAndReturn() that can be used to reset the callback member _before_ we call it, so we can still ensure we don't hang on to the callback for longer than necessary.
On 2016/06/16 16:22:18, Bernhard Bauer wrote: > +Gab, tracked prefs pro > > Okay, here's what I think is happening: > > * JSONPrefStore reports an error while reading prefs > * InterceptablePrefFilter::FilterOnLoad() calls its callback > * The callback eventually (but synchronously!) ends up calling into > ProfileManager::OnProfileCreated() > * ProfileManager::OnProfileCreated() deletes the profile because the profile > load was unsuccessful > * Everything transitively owned by the profile (including the filter) is deleted > * Afterwards, control returns to InterceptablePrefFilter::FilterOnLoad(), which > tries to reset the callback member that has been deleted. > > I think preventing accesses after the callback has been called should be > sufficient to fix the crash (although there could be other instances hidden > somewhere). There is a neat convenience function base::ResetAndReturn() that can > be used to reset the callback member _before_ we call it, so we can still ensure > we don't hang on to the callback for longer than necessary. Interesting, that indeed would cause a crash, the tracked prefs code doesn't handle being deleted while it runs and deleting the Profile which owns it would definitely do this. Making tracked pref support this might not be trivial and overall I don't think it's correct to delete the Profile* from a stack on which references to it and its members exist. Here's a way to avoid deletion while in-use @ https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager...., replace: profiles_info_.erase(iter); with: // Unmap the ProfileInfo, but delete its Profile* asynchronously as it may still // be referenced on the current stack. SequencedTaskRunnerHandle::Get()->DeleteSoon(info->profile.release()); profiles_info_.erase(iter);
On 2016/06/17 18:14:06, gab wrote: > On 2016/06/16 16:22:18, Bernhard Bauer wrote: > > +Gab, tracked prefs pro > > > > Okay, here's what I think is happening: > > > > * JSONPrefStore reports an error while reading prefs > > * InterceptablePrefFilter::FilterOnLoad() calls its callback > > * The callback eventually (but synchronously!) ends up calling into > > ProfileManager::OnProfileCreated() > > * ProfileManager::OnProfileCreated() deletes the profile because the profile > > load was unsuccessful > > * Everything transitively owned by the profile (including the filter) is > deleted > > * Afterwards, control returns to InterceptablePrefFilter::FilterOnLoad(), > which > > tries to reset the callback member that has been deleted. > > > > I think preventing accesses after the callback has been called should be > > sufficient to fix the crash (although there could be other instances hidden > > somewhere). There is a neat convenience function base::ResetAndReturn() that > can > > be used to reset the callback member _before_ we call it, so we can still > ensure > > we don't hang on to the callback for longer than necessary. > > Interesting, that indeed would cause a crash, the tracked prefs code doesn't > handle being deleted while it runs and deleting the Profile which owns it would > definitely do this. Making tracked pref support this might not be trivial and > overall I don't think it's correct to delete the Profile* from a stack on which > references to it and its members exist. > > Here's a way to avoid deletion while in-use @ > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager...., > replace: > > profiles_info_.erase(iter); > > with: > > // Unmap the ProfileInfo, but delete its Profile* asynchronously as it may > still > // be referenced on the current stack. > SequencedTaskRunnerHandle::Get()->DeleteSoon(info->profile.release()); > profiles_info_.erase(iter); Yeah, there are probably a bunch of places where we could run callbacks asynchronously, to break up this reentrancy issue…
On 2016/06/17 18:17:04, Bernhard Bauer wrote: > > Here's a way to avoid deletion while in-use @ > > > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager...., > > replace: > > > > profiles_info_.erase(iter); > > > > with: > > > > // Unmap the ProfileInfo, but delete its Profile* asynchronously as it may > > still > > // be referenced on the current stack. > > SequencedTaskRunnerHandle::Get()->DeleteSoon(info->profile.release()); > > profiles_info_.erase(iter); Is any initialization going in other threads, especially the FILE thread? If yes this is not correct. After the object is deleted by DeleteSoon, it can still be referenced in the reply part of PostTaskAndReply. > > Yeah, there are probably a bunch of places where we could run callbacks > asynchronously, to break up this reentrancy issue… Actually I wish that all objects referenced by callbacks were properly wrapped in smart pointers. It means WeakPointer if using in the same thread, and RefCountedThreadSafe/CancelableTaskTracker for cancellable tasks in other threads.
On 2016/06/20 16:22:02, WC Leung wrote: > On 2016/06/17 18:17:04, Bernhard Bauer wrote: > > > Here's a way to avoid deletion while in-use @ > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager...., > > > replace: > > > > > > profiles_info_.erase(iter); > > > > > > with: > > > > > > // Unmap the ProfileInfo, but delete its Profile* asynchronously as it may > > > still > > > // be referenced on the current stack. > > > SequencedTaskRunnerHandle::Get()->DeleteSoon(info->profile.release()); > > > profiles_info_.erase(iter); > > Is any initialization going in other threads, especially the FILE thread? If yes > this is not correct. After the object is deleted by DeleteSoon, it can still be > referenced in the reply part of PostTaskAndReply. > > > > > Yeah, there are probably a bunch of places where we could run callbacks > > asynchronously, to break up this reentrancy issue… > > Actually I wish that all objects referenced by callbacks were properly wrapped > in smart pointers. It means WeakPointer if using in the same thread, and > RefCountedThreadSafe/CancelableTaskTracker for cancellable tasks in other > threads. In depends IMO, clear ownership is better than weak ownership when possible as it makes things clearer. In this case though, it doesn't have to do with a callback. The object being deleted has an active member function on the stack (i.e. Foo is owned by Profile, Foo::Bar() calls Profile::Baz() which decides to delete itself (and thus Foo by owning it), when the callstack unwinds Foo::Bar() will crash if accessing any member state which is a reasonable thing to do from its point of view. The solution is that Profile::Baz() should schedule itself for deletion (as per suggestion above) instead of synchronously deleting itself on a stack that might have references to its state.
https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... base/test/test_file_util_win.cc:98: LocalFree(security_descriptor); On 2016/06/16 10:04:10, WC Leung wrote: > On 2016/06/16 07:02:10, Peter Kasting wrote: > > I assume you decided to investigate the scoping object in a separate CL? > > Yes. Created issue 621559 to track this. https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/ui/profi... File chrome/browser/ui/profile_error_dialog.h (right): https://codereview.chromium.org/2061593002/diff/80001/chrome/browser/ui/profi... chrome/browser/ui/profile_error_dialog.h:21: PROFILE_ERROR_SWITCH_TO_FAILURE, On 2016/06/16 11:00:00, Peter Kasting wrote: > On 2016/06/16 10:04:10, WC Leung wrote: > > On 2016/06/16 07:02:11, Peter Kasting wrote: > > > Nit: This sounds like "we switched to failure" rather than "failure to > switch > > to > > > this profile." > > > > > > Consider changing this enum to an enum class named ProfileError so you can > > > remove PROFILE_ERROR_ from in front of every value, then naming these more > > > descriptively. For example, is HISTORY for errors opening the history DB, > or > > > does it cover more than that? A name like COULDNT_OPEN_HISTORY_DB would be > way > > > better than HISTORY, just as something like > > > TRIED_TO_SWITCH_TO_UNOPENABLE_PROFILE would be better than > SWITCH_TO_FAILURE. > > > > Good point. I'll make my enum more descriptive. > > > > BTW, adding a class makes the enum shorter, but does not make the code in the > > callsites shorter (the class name is just prefixed). > > I know. Created issue 621555 to track this task.
On 2016/06/20 16:52:10, gab (OOO back Monday) wrote: > On 2016/06/20 16:22:02, WC Leung wrote: > > On 2016/06/17 18:17:04, Bernhard Bauer wrote: > > > > Here's a way to avoid deletion while in-use @ > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager...., > > > > replace: > > > > > > > > profiles_info_.erase(iter); > > > > > > > > with: > > > > > > > > // Unmap the ProfileInfo, but delete its Profile* asynchronously as it > may > > > > still > > > > // be referenced on the current stack. > > > > SequencedTaskRunnerHandle::Get()->DeleteSoon(info->profile.release()); > > > > profiles_info_.erase(iter); > > > > Is any initialization going in other threads, especially the FILE thread? If > yes > > this is not correct. After the object is deleted by DeleteSoon, it can still > be > > referenced in the reply part of PostTaskAndReply. > > > > > > > > Yeah, there are probably a bunch of places where we could run callbacks > > > asynchronously, to break up this reentrancy issue… > > > > Actually I wish that all objects referenced by callbacks were properly wrapped > > in smart pointers. It means WeakPointer if using in the same thread, and > > RefCountedThreadSafe/CancelableTaskTracker for cancellable tasks in other > > threads. > > In depends IMO, clear ownership is better than weak ownership when possible as > it makes things clearer. > > In this case though, it doesn't have to do with a callback. The object being > deleted has an active member function on the stack (i.e. Foo is owned by > Profile, Foo::Bar() calls Profile::Baz() which decides to delete itself (and > thus Foo by owning it), when the callstack unwinds Foo::Bar() will crash if > accessing any member state which is a reasonable thing to do from its point of > view. Noted with thanks! > > The solution is that Profile::Baz() should schedule itself for deletion (as per > suggestion above) instead of synchronously deleting itself on a stack that might > have references to its state. Just checked about JSONPrefStore. The file is read in the UI thread (I assumed file reads are done in the FILE thread, silly me), so apparently nothing goes out-of-thread. So DeleteSoon() sounds good. Anyway, I've been able to delete a profile now. But sadly I have another crash :-). Notice the line "Unable to terminate process: Access is denied. (0x5)", which should be where the problem lies. I can only reproduce the crash with DeleteSoon, but that LOG(ERROR) line appears in both my original patch and the proposed new patch. The crash log is here: F:\chromium>"runchromium_debug - Copy (4).bat" [12996:12132:0626/195106:WARNING:message_queue.cc(38)] Leaking 2 ports in unreceived messages [12996:12132:0626/195106:WARNING:extension_service.cc(2022)] Found external version of extension apdfllckaahabafndbhieahigkjlhalfthat is older than current version. Current version is: 14.1. New version is: 6.3. Keeping current version. [12996:12132:0626/195106:WARNING:external_registry_loader_win.cc(213)] Error observing HKLM. [12996:12132:0626/195111:INFO:CONSOLE(6)] "SW registered", source: https://www.google.com.hk/_/chrome/newtab?espv=2&ie=UTF-8 (6) [12996:9552:0626/195114:WARNING:file_util_win.cc(442)] Failed to create directory e:\chromium_profiles_4\Profile 1, last error is 5. [12996:9472:0626/195119:WARNING:file_util_win.cc(442)] Failed to create directory e:\chromium_profiles_4\Profile 1, last error is 5. [12996:9472:0626/195123:WARNING:file_util_win.cc(442)] Failed to create directory e:\chromium_profiles_4\Profile 1, last error is 5. [12996:10008:0626/195125:ERROR:process_win.cc(134)] Unable to terminate process: Access is denied. (0x5) [12996:8812:0626/195133:WARNING:file_util_win.cc(442)] Failed to create directory e:\chromium_profiles_4\Profile 1, last error is 5. [12996:12132:0626/195134:FATAL:dependency_manager.cc(111)] Check failed: false. Attempted to access a context that was ShutDown(). This is most likely a heap smasher in progress. After KeyedService::Shutdown() completes, your service MUST NOT refer to depended services again. Backtrace: base::debug::StackTrace::StackTrace [0x1009ED31+33] (f:\chromium\src\base\debug\stack_trace_win.cc:215) logging::LogMessage::~LogMessage [0x100FB83B+75] (f:\chromium\src\base\logging.cc:524) DependencyManager::AssertContextWasntDestroyed [0x0E45AB0A+298] (f:\chromium\src\components\keyed_service\core\dependency_manager.cc:111) KeyedServiceBaseFactory::AssertContextWasntDestroyed [0x0E45E744+276] (f:\chromium\src\components\keyed_service\core\keyed_service_base_factory.cc:87) BrowserContextKeyedServiceFactory::GetBrowserContextToUse [0x1A50C3C1+273] (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:58) BrowserContextKeyedServiceFactory::GetContextToUse [0x1A50C472+34] (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:105) KeyedServiceFactory::GetServiceForContext [0x0E4675B7+231] (f:\chromium\src\components\keyed_service\core\keyed_service_factory.cc:65) BrowserContextKeyedServiceFactory::GetServiceForBrowserContext [0x1A50C4BF+31] (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:46) ...
On 2016/06/26 20:04:05, WC Leung wrote: > On 2016/06/20 16:52:10, gab (OOO back Monday) wrote: > > On 2016/06/20 16:22:02, WC Leung wrote: > > > On 2016/06/17 18:17:04, Bernhard Bauer wrote: > > > > > Here's a way to avoid deletion while in-use @ > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager...., > > > > > replace: > > > > > > > > > > profiles_info_.erase(iter); > > > > > > > > > > with: > > > > > > > > > > // Unmap the ProfileInfo, but delete its Profile* asynchronously as it > > may > > > > > still > > > > > // be referenced on the current stack. > > > > > SequencedTaskRunnerHandle::Get()->DeleteSoon(info->profile.release()); > > > > > profiles_info_.erase(iter); > > > > > > Is any initialization going in other threads, especially the FILE thread? If > > yes > > > this is not correct. After the object is deleted by DeleteSoon, it can still > > be > > > referenced in the reply part of PostTaskAndReply. > > > > > > > > > > > Yeah, there are probably a bunch of places where we could run callbacks > > > > asynchronously, to break up this reentrancy issue… > > > > > > Actually I wish that all objects referenced by callbacks were properly > wrapped > > > in smart pointers. It means WeakPointer if using in the same thread, and > > > RefCountedThreadSafe/CancelableTaskTracker for cancellable tasks in other > > > threads. > > > > In depends IMO, clear ownership is better than weak ownership when possible as > > it makes things clearer. > > > > In this case though, it doesn't have to do with a callback. The object being > > deleted has an active member function on the stack (i.e. Foo is owned by > > Profile, Foo::Bar() calls Profile::Baz() which decides to delete itself (and > > thus Foo by owning it), when the callstack unwinds Foo::Bar() will crash if > > accessing any member state which is a reasonable thing to do from its point of > > view. > > Noted with thanks! > > > > > The solution is that Profile::Baz() should schedule itself for deletion (as > per > > suggestion above) instead of synchronously deleting itself on a stack that > might > > have references to its state. > > Just checked about JSONPrefStore. The file is read in the UI thread (I assumed > file reads are done in the FILE thread, silly me), so apparently nothing goes > out-of-thread. So DeleteSoon() sounds good. > > Anyway, I've been able to delete a profile now. But sadly I have another crash > :-). Notice the line "Unable to terminate process: Access is denied. (0x5)", > which should be where the problem lies. I can only reproduce the crash with > DeleteSoon, but that LOG(ERROR) line appears in both my original patch and the > proposed new patch. > > The crash log is here: > > F:\chromium>"runchromium_debug - Copy (4).bat" > [12996:12132:0626/195106:WARNING:message_queue.cc(38)] Leaking 2 ports in > unreceived messages > [12996:12132:0626/195106:WARNING:extension_service.cc(2022)] Found external > version of extension apdfllckaahabafndbhieahigkjlhalfthat is older than current > version. Current version is: 14.1. New version is: 6.3. Keeping current version. > [12996:12132:0626/195106:WARNING:external_registry_loader_win.cc(213)] Error > observing HKLM. > [12996:12132:0626/195111:INFO:CONSOLE(6)] "SW registered", source: > https://www.google.com.hk/_/chrome/newtab?espv=2&ie=UTF-8 (6) > [12996:9552:0626/195114:WARNING:file_util_win.cc(442)] Failed to create > directory e:\chromium_profiles_4\Profile 1, last error is 5. > [12996:9472:0626/195119:WARNING:file_util_win.cc(442)] Failed to create > directory e:\chromium_profiles_4\Profile 1, last error is 5. > [12996:9472:0626/195123:WARNING:file_util_win.cc(442)] Failed to create > directory e:\chromium_profiles_4\Profile 1, last error is 5. > [12996:10008:0626/195125:ERROR:process_win.cc(134)] Unable to terminate process: > Access is denied. (0x5) > [12996:8812:0626/195133:WARNING:file_util_win.cc(442)] Failed to create > directory e:\chromium_profiles_4\Profile 1, last error is 5. > [12996:12132:0626/195134:FATAL:dependency_manager.cc(111)] Check failed: false. > Attempted to access a context that was ShutDown(). This is most likely a heap > smasher in progress. After KeyedService::Shutdown() completes, your service MUST > NOT refer to depended services again. > Backtrace: > base::debug::StackTrace::StackTrace [0x1009ED31+33] > (f:\chromium\src\base\debug\stack_trace_win.cc:215) > logging::LogMessage::~LogMessage [0x100FB83B+75] > (f:\chromium\src\base\logging.cc:524) > DependencyManager::AssertContextWasntDestroyed [0x0E45AB0A+298] > (f:\chromium\src\components\keyed_service\core\dependency_manager.cc:111) > KeyedServiceBaseFactory::AssertContextWasntDestroyed [0x0E45E744+276] > (f:\chromium\src\components\keyed_service\core\keyed_service_base_factory.cc:87) > BrowserContextKeyedServiceFactory::GetBrowserContextToUse > [0x1A50C3C1+273] > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:58) > BrowserContextKeyedServiceFactory::GetContextToUse [0x1A50C472+34] > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:105) > KeyedServiceFactory::GetServiceForContext [0x0E4675B7+231] > (f:\chromium\src\components\keyed_service\core\keyed_service_factory.cc:65) > BrowserContextKeyedServiceFactory::GetServiceForBrowserContext > [0x1A50C4BF+31] > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:46) > ... You've left out the most important part of the stack trace ;-) Where is that call coming from?
On 2016/06/27 08:33:40, Bernhard Bauer wrote: > On 2016/06/26 20:04:05, WC Leung wrote: > > On 2016/06/20 16:52:10, gab (OOO back Monday) wrote: > > > On 2016/06/20 16:22:02, WC Leung wrote: > > > > On 2016/06/17 18:17:04, Bernhard Bauer wrote: > > > > > > Here's a way to avoid deletion while in-use @ > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager...., > > > > > > replace: > > > > > > > > > > > > profiles_info_.erase(iter); > > > > > > > > > > > > with: > > > > > > > > > > > > // Unmap the ProfileInfo, but delete its Profile* asynchronously as > it > > > may > > > > > > still > > > > > > // be referenced on the current stack. > > > > > > > SequencedTaskRunnerHandle::Get()->DeleteSoon(info->profile.release()); > > > > > > profiles_info_.erase(iter); > > > > > > > > Is any initialization going in other threads, especially the FILE thread? > If > > > yes > > > > this is not correct. After the object is deleted by DeleteSoon, it can > still > > > be > > > > referenced in the reply part of PostTaskAndReply. > > > > > > > > > > > > > > Yeah, there are probably a bunch of places where we could run callbacks > > > > > asynchronously, to break up this reentrancy issue… > > > > > > > > Actually I wish that all objects referenced by callbacks were properly > > wrapped > > > > in smart pointers. It means WeakPointer if using in the same thread, and > > > > RefCountedThreadSafe/CancelableTaskTracker for cancellable tasks in other > > > > threads. > > > > > > In depends IMO, clear ownership is better than weak ownership when possible > as > > > it makes things clearer. > > > > > > In this case though, it doesn't have to do with a callback. The object being > > > deleted has an active member function on the stack (i.e. Foo is owned by > > > Profile, Foo::Bar() calls Profile::Baz() which decides to delete itself (and > > > thus Foo by owning it), when the callstack unwinds Foo::Bar() will crash if > > > accessing any member state which is a reasonable thing to do from its point > of > > > view. > > > > Noted with thanks! > > > > > > > > The solution is that Profile::Baz() should schedule itself for deletion (as > > per > > > suggestion above) instead of synchronously deleting itself on a stack that > > might > > > have references to its state. > > > > Just checked about JSONPrefStore. The file is read in the UI thread (I assumed > > file reads are done in the FILE thread, silly me), so apparently nothing goes > > out-of-thread. So DeleteSoon() sounds good. Not quite, the notification that the file was read (i.e. JsonPrefStore::OnFileRead()), which triggers the cascade where this issue occurs, is on the UI thread. But the file itself is read asynchronously (on the BlockingPool but that's irrelevant to this bug). > > > > Anyway, I've been able to delete a profile now. But sadly I have another crash > > :-). Notice the line "Unable to terminate process: Access is denied. (0x5)", > > which should be where the problem lies. I can only reproduce the crash with > > DeleteSoon, but that LOG(ERROR) line appears in both my original patch and the > > proposed new patch. > > > > The crash log is here: > > > > F:\chromium>"runchromium_debug - Copy (4).bat" > > [12996:12132:0626/195106:WARNING:message_queue.cc(38)] Leaking 2 ports in > > unreceived messages > > [12996:12132:0626/195106:WARNING:extension_service.cc(2022)] Found external > > version of extension apdfllckaahabafndbhieahigkjlhalfthat is older than > current > > version. Current version is: 14.1. New version is: 6.3. Keeping current > version. > > [12996:12132:0626/195106:WARNING:external_registry_loader_win.cc(213)] Error > > observing HKLM. > > [12996:12132:0626/195111:INFO:CONSOLE(6)] "SW registered", source: > > https://www.google.com.hk/_/chrome/newtab?espv=2&ie=UTF-8 (6) > > [12996:9552:0626/195114:WARNING:file_util_win.cc(442)] Failed to create > > directory e:\chromium_profiles_4\Profile 1, last error is 5. > > [12996:9472:0626/195119:WARNING:file_util_win.cc(442)] Failed to create > > directory e:\chromium_profiles_4\Profile 1, last error is 5. > > [12996:9472:0626/195123:WARNING:file_util_win.cc(442)] Failed to create > > directory e:\chromium_profiles_4\Profile 1, last error is 5. > > [12996:10008:0626/195125:ERROR:process_win.cc(134)] Unable to terminate > process: > > Access is denied. (0x5) > > [12996:8812:0626/195133:WARNING:file_util_win.cc(442)] Failed to create > > directory e:\chromium_profiles_4\Profile 1, last error is 5. > > [12996:12132:0626/195134:FATAL:dependency_manager.cc(111)] Check failed: > false. > > Attempted to access a context that was ShutDown(). This is most likely a heap > > smasher in progress. After KeyedService::Shutdown() completes, your service > MUST > > NOT refer to depended services again. > > Backtrace: > > base::debug::StackTrace::StackTrace [0x1009ED31+33] > > (f:\chromium\src\base\debug\stack_trace_win.cc:215) > > logging::LogMessage::~LogMessage [0x100FB83B+75] > > (f:\chromium\src\base\logging.cc:524) > > DependencyManager::AssertContextWasntDestroyed [0x0E45AB0A+298] > > (f:\chromium\src\components\keyed_service\core\dependency_manager.cc:111) > > KeyedServiceBaseFactory::AssertContextWasntDestroyed [0x0E45E744+276] > > > (f:\chromium\src\components\keyed_service\core\keyed_service_base_factory.cc:87) > > BrowserContextKeyedServiceFactory::GetBrowserContextToUse > > [0x1A50C3C1+273] > > > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:58) > > BrowserContextKeyedServiceFactory::GetContextToUse [0x1A50C472+34] > > > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:105) > > KeyedServiceFactory::GetServiceForContext [0x0E4675B7+231] > > (f:\chromium\src\components\keyed_service\core\keyed_service_factory.cc:65) > > BrowserContextKeyedServiceFactory::GetServiceForBrowserContext > > [0x1A50C4BF+31] > > > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:46) > > ... > > You've left out the most important part of the stack trace ;-) Where is that > call coming from? Indeed :-)
On 2016/06/27 18:53:04, gab wrote: > > > Just checked about JSONPrefStore. The file is read in the UI thread (I > assumed > > > file reads are done in the FILE thread, silly me), so apparently nothing > goes > > > out-of-thread. So DeleteSoon() sounds good. > > Not quite, the notification that the file was read (i.e. > JsonPrefStore::OnFileRead()), which triggers the cascade where this issue > occurs, is on the UI thread. But the file itself is read asynchronously (on the > BlockingPool but that's irrelevant to this bug). > Thanks for pointing out. I've read it wrong. :-( > > > > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:46) > > > ... > > > > You've left out the most important part of the stack trace ;-) Where is that > > call coming from? > > Indeed :-) The full log is here: https://drive.google.com/drive/u/0/folders/0BxDfZwJpDb1IVEdqZVdiSmVJNDA And it exceeded the 10000 bytes limit. Anyway, to reproduce the crash I kept clicking the failing profile in the avatar menu. And you're right... one of the crashes I've made does not come with the process exit fail message, so they're independent.
On 2016/06/27 21:39:44, WC Leung wrote: > On 2016/06/27 18:53:04, gab wrote: > > > > Just checked about JSONPrefStore. The file is read in the UI thread (I > > assumed > > > > file reads are done in the FILE thread, silly me), so apparently nothing > > goes > > > > out-of-thread. So DeleteSoon() sounds good. > > > > Not quite, the notification that the file was read (i.e. > > JsonPrefStore::OnFileRead()), which triggers the cascade where this issue > > occurs, is on the UI thread. But the file itself is read asynchronously (on > the > > BlockingPool but that's irrelevant to this bug). > > > > Thanks for pointing out. I've read it wrong. :-( > > > > > > > > > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:46) > > > > ... > > > > > > You've left out the most important part of the stack trace ;-) Where is that > > > call coming from? > > > > Indeed :-) > > > The full log is here: > https://drive.google.com/drive/u/0/folders/0BxDfZwJpDb1IVEdqZVdiSmVJNDA > > And it exceeded the 10000 bytes limit. > > Anyway, to reproduce the crash I kept clicking the failing profile in the avatar > menu. And you're right... one of the crashes I've made does not come with the > process exit fail message, so they're independent. Oh... sorry for wrong link. Here's the right one https://drive.google.com/open?id=0BxDfZwJpDb1ISFB4OHp0cWNfQUE And after I removed the ProfileError message, it became https://drive.google.com/open?id=0BxDfZwJpDb1IanFsRFdCVGVKZlU Sadly it appears that the profile create/delete cycles has got the same address for some contexts, and hence the assertion failure.
On 2016/06/27 22:07:23, WC Leung wrote: > On 2016/06/27 21:39:44, WC Leung wrote: > > On 2016/06/27 18:53:04, gab wrote: > > > > > Just checked about JSONPrefStore. The file is read in the UI thread (I > > > assumed > > > > > file reads are done in the FILE thread, silly me), so apparently nothing > > > goes > > > > > out-of-thread. So DeleteSoon() sounds good. > > > > > > Not quite, the notification that the file was read (i.e. > > > JsonPrefStore::OnFileRead()), which triggers the cascade where this issue > > > occurs, is on the UI thread. But the file itself is read asynchronously (on > > the > > > BlockingPool but that's irrelevant to this bug). > > > > > > > Thanks for pointing out. I've read it wrong. :-( > > > > > > > > > > > > > > > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:46) > > > > > ... > > > > > > > > You've left out the most important part of the stack trace ;-) Where is > that > > > > call coming from? > > > > > > Indeed :-) > > > > > > The full log is here: > > https://drive.google.com/drive/u/0/folders/0BxDfZwJpDb1IVEdqZVdiSmVJNDA > > > > And it exceeded the 10000 bytes limit. > > > > Anyway, to reproduce the crash I kept clicking the failing profile in the > avatar > > menu. And you're right... one of the crashes I've made does not come with the > > process exit fail message, so they're independent. > > > Oh... sorry for wrong link. Here's the right one > https://drive.google.com/open?id=0BxDfZwJpDb1ISFB4OHp0cWNfQUE So this crash is because ProfileImpl's constructor posts a DelayedTask with a raw pointer to itself the profile error dialog happens to run a nested message loop which picked up that task after the profile was deleted (the nested message loop processed the deletion before it processed that). So the problem here is that: 1) The nested message loop of the dialog makes the "async" deletion no longer async (i.e. we're still on the PrefHashFilter::FinalizeFilterOnLoad(),etc. stack). 2) ProfileImpl binds a raw Profile* in a delayed task (that's a separate issue but should also be filed and fixed as it's clearly wrong given a profile can be deleted at any point and there's thus no guarantee that it will be alive when the task is executed). A WeakPtr is probably what's appropriate here. (2) should be fixed separately. (1) will be a problem for this change. It happens because you added the dialog in OpenBrowserWindowForProfile which runs synchronously as part of RunCallbacks(). We can't pop the error dialog synchronously because a nested MessageLoop will be an issue. EDIT: Actually... DeleteSoon is using non-nestable tasks [1] so the deletion shouldn't run in the nested MessageLoop... now I'm confused why the context thinks it's destroyed, is this the same bug as explained below with that assert (using raw pointer values is wrong). [1] https://cs.chromium.org/chromium/src/base/sequenced_task_runner.cc?rcl=0&l=21 > > And after I removed the ProfileError message, it became > https://drive.google.com/open?id=0BxDfZwJpDb1IanFsRFdCVGVKZlU By "remove the ProfileError message" you mean comment out the code that pops the dialog to avoid the first crash caused by the nested MessageLoop? This crash doesn't make sense to me if we assume Profile* are uniquely assigned as the deleted Profile* should only be considered dead (and cause that assertion) *after* the shutdown phase yet the assertion occurs during the shutdown phase in your crash stack (ref.: DependencyManager::DestroyContextServices(): https://cs.chromium.org/chromium/src/components/keyed_service/core/dependency...). > Sadly it appears that the profile create/delete cycles has got the same address > for some contexts, and hence the assertion failure. I agree that this is probably what's happening? Did you have to delete multiple profiles to get to this crash? Using raw pointer values here seems wrong. I think we should change DependencyManager to use something actually unique (Profile::GetDebugName()? or from a BrowserContext (BrowserContext::GetPath()?) to track dead profiles. DependencyManager should also be updated to use new style #if DCHECK_IS_ON() instead of #ifndef NDEBUG for assertion code.
On 2016/06/30 16:06:40, gab wrote: > On 2016/06/27 22:07:23, WC Leung wrote: > > On 2016/06/27 21:39:44, WC Leung wrote: > > > On 2016/06/27 18:53:04, gab wrote: > > > > > > Just checked about JSONPrefStore. The file is read in the UI thread (I > > > > assumed > > > > > > file reads are done in the FILE thread, silly me), so apparently > nothing > > > > goes > > > > > > out-of-thread. So DeleteSoon() sounds good. > > > > > > > > Not quite, the notification that the file was read (i.e. > > > > JsonPrefStore::OnFileRead()), which triggers the cascade where this issue > > > > occurs, is on the UI thread. But the file itself is read asynchronously > (on > > > the > > > > BlockingPool but that's irrelevant to this bug). > > > > > > > > > > Thanks for pointing out. I've read it wrong. :-( > > > > > > > > > > > > > > > > > > > > > > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:46) > > > > > > ... > > > > > > > > > > You've left out the most important part of the stack trace ;-) Where is > > that > > > > > call coming from? > > > > > > > > Indeed :-) > > > > > > > > > The full log is here: > > > https://drive.google.com/drive/u/0/folders/0BxDfZwJpDb1IVEdqZVdiSmVJNDA > > > > > > And it exceeded the 10000 bytes limit. > > > > > > Anyway, to reproduce the crash I kept clicking the failing profile in the > > avatar > > > menu. And you're right... one of the crashes I've made does not come with > the > > > process exit fail message, so they're independent. > > > > > > Oh... sorry for wrong link. Here's the right one > > https://drive.google.com/open?id=0BxDfZwJpDb1ISFB4OHp0cWNfQUE > > So this crash is because ProfileImpl's constructor posts a DelayedTask with a > raw pointer to itself > the profile error dialog happens to run a nested message loop which picked up > that task after the > profile was deleted (the nested message loop processed the deletion before it > processed that). > > So the problem here is that: > 1) The nested message loop of the dialog makes the "async" deletion no longer > async (i.e. we're still on the PrefHashFilter::FinalizeFilterOnLoad(),etc. > stack). > 2) ProfileImpl binds a raw Profile* in a delayed task (that's a separate issue > but should also be filed and fixed as it's clearly wrong given a profile can be > deleted at any point and there's thus no guarantee that it will be alive when > the task is executed). A WeakPtr is probably what's appropriate here. > > (2) should be fixed separately. > > (1) will be a problem for this change. It happens because you added the dialog > in OpenBrowserWindowForProfile which runs synchronously as part of > RunCallbacks(). We can't pop the error dialog synchronously because a nested > MessageLoop will be an issue. > EDIT: Actually... DeleteSoon is using non-nestable tasks [1] so the deletion > shouldn't run in the nested MessageLoop... now I'm confused why the context > thinks it's destroyed, is this the same bug as explained below with that assert > (using raw pointer values is wrong). > > [1] > https://cs.chromium.org/chromium/src/base/sequenced_task_runner.cc?rcl=0&l=21 > > > > > And after I removed the ProfileError message, it became > > https://drive.google.com/open?id=0BxDfZwJpDb1IanFsRFdCVGVKZlU > > By "remove the ProfileError message" you mean comment out the code that pops the > dialog to avoid the first crash caused by the nested MessageLoop? > > This crash doesn't make sense to me if we assume Profile* are uniquely assigned > as the deleted Profile* should only be considered dead (and cause that > assertion) *after* the shutdown phase yet the assertion occurs during the > shutdown phase in your crash stack (ref.: > DependencyManager::DestroyContextServices(): > https://cs.chromium.org/chromium/src/components/keyed_service/core/dependency...). > > > Sadly it appears that the profile create/delete cycles has got the same > address > > for some contexts, and hence the assertion failure. > > I agree that this is probably what's happening? Did you have to delete multiple > profiles to get to this crash? Using raw pointer values here seems wrong. > > I think we should change DependencyManager to use something actually unique > (Profile::GetDebugName()? or from a BrowserContext (BrowserContext::GetPath()?) > to track dead profiles. > DependencyManager should also be updated to use new style #if DCHECK_IS_ON() > instead of #ifndef NDEBUG for assertion code. 1. Yes I'm trying to create the same profile multiple times. Create fails, delete the failed profile, create again... etc. Note that this can be done overlapped (if the task runner nested) because DeleteSoon takes time to run. 2. And yes I've commented the code that creates the dialog. And this only made the crash more reproducible. I just need to do the silly thing in (1) a few times in quick succession to crash it. And that's why I think that the root cause of this crash is DependencyManager, regardless of the presence or absence of the dialog. 3. Somehow the failed DCHECK is about not allowing the same KeyedService of a profile to be deallocated twice. Using raw pointers should be okay if we can bind the pointer to a Profile*. Of course, Profile* has the same deficiency as DependencyManager (see Nick's comment about ProfileManager::IsValidProfile() in issue 557916). So we need to be very careful about how to fix DependencyManager. I have no definitive answer by now, but am thinking about having some observer/callback that can reset DependencyManager during profile destruction. Anyway, by (2), anything related to GetPath() or GetDebugName() is bad for uniqueness. Anyway, a good fix of (3) is at least non-trivial, so it is not a good idea to include it in this CL. For this CL we either wait for the fix, or to defer failed profile deletion to the end of the chromium process so the services do not use the same memory address. I'm okay with both of them now, because this is sadly found NOT related the cause to the crash in 614753.
On 2016/06/30 18:15:51, WC Leung wrote: > On 2016/06/30 16:06:40, gab wrote: > > On 2016/06/27 22:07:23, WC Leung wrote: > > > On 2016/06/27 21:39:44, WC Leung wrote: > > > > On 2016/06/27 18:53:04, gab wrote: > > > > > > > Just checked about JSONPrefStore. The file is read in the UI thread > (I > > > > > assumed > > > > > > > file reads are done in the FILE thread, silly me), so apparently > > nothing > > > > > goes > > > > > > > out-of-thread. So DeleteSoon() sounds good. > > > > > > > > > > Not quite, the notification that the file was read (i.e. > > > > > JsonPrefStore::OnFileRead()), which triggers the cascade where this > issue > > > > > occurs, is on the UI thread. But the file itself is read asynchronously > > (on > > > > the > > > > > BlockingPool but that's irrelevant to this bug). > > > > > > > > > > > > > Thanks for pointing out. I've read it wrong. :-( > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (f:\chromium\src\components\keyed_service\content\browser_context_keyed_service_factory.cc:46) > > > > > > > ... > > > > > > > > > > > > You've left out the most important part of the stack trace ;-) Where > is > > > that > > > > > > call coming from? > > > > > > > > > > Indeed :-) > > > > > > > > > > > > The full log is here: > > > > https://drive.google.com/drive/u/0/folders/0BxDfZwJpDb1IVEdqZVdiSmVJNDA > > > > > > > > And it exceeded the 10000 bytes limit. > > > > > > > > Anyway, to reproduce the crash I kept clicking the failing profile in the > > > avatar > > > > menu. And you're right... one of the crashes I've made does not come with > > the > > > > process exit fail message, so they're independent. > > > > > > > > > Oh... sorry for wrong link. Here's the right one > > > https://drive.google.com/open?id=0BxDfZwJpDb1ISFB4OHp0cWNfQUE > > > > So this crash is because ProfileImpl's constructor posts a DelayedTask with a > > raw pointer to itself > > the profile error dialog happens to run a nested message loop which picked up > > that task after the > > profile was deleted (the nested message loop processed the deletion before it > > processed that). > > > > So the problem here is that: > > 1) The nested message loop of the dialog makes the "async" deletion no longer > > async (i.e. we're still on the PrefHashFilter::FinalizeFilterOnLoad(),etc. > > stack). > > 2) ProfileImpl binds a raw Profile* in a delayed task (that's a separate > issue > > but should also be filed and fixed as it's clearly wrong given a profile can > be > > deleted at any point and there's thus no guarantee that it will be alive when > > the task is executed). A WeakPtr is probably what's appropriate here. > > > > (2) should be fixed separately. > > > > (1) will be a problem for this change. It happens because you added the dialog > > in OpenBrowserWindowForProfile which runs synchronously as part of > > RunCallbacks(). We can't pop the error dialog synchronously because a nested > > MessageLoop will be an issue. > > EDIT: Actually... DeleteSoon is using non-nestable tasks [1] so the deletion > > shouldn't run in the nested MessageLoop... now I'm confused why the context > > thinks it's destroyed, is this the same bug as explained below with that > assert > > (using raw pointer values is wrong). > > > > [1] > > https://cs.chromium.org/chromium/src/base/sequenced_task_runner.cc?rcl=0&l=21 > > > > > > > > And after I removed the ProfileError message, it became > > > https://drive.google.com/open?id=0BxDfZwJpDb1IanFsRFdCVGVKZlU > > > > By "remove the ProfileError message" you mean comment out the code that pops > the > > dialog to avoid the first crash caused by the nested MessageLoop? > > > > This crash doesn't make sense to me if we assume Profile* are uniquely > assigned > > as the deleted Profile* should only be considered dead (and cause that > > assertion) *after* the shutdown phase yet the assertion occurs during the > > shutdown phase in your crash stack (ref.: > > DependencyManager::DestroyContextServices(): > > > https://cs.chromium.org/chromium/src/components/keyed_service/core/dependency...). > > > > > Sadly it appears that the profile create/delete cycles has got the same > > address > > > for some contexts, and hence the assertion failure. > > > > I agree that this is probably what's happening? Did you have to delete > multiple > > profiles to get to this crash? Using raw pointer values here seems wrong. > > > > I think we should change DependencyManager to use something actually unique > > (Profile::GetDebugName()? or from a BrowserContext > (BrowserContext::GetPath()?) > > to track dead profiles. > > DependencyManager should also be updated to use new style #if DCHECK_IS_ON() > > instead of #ifndef NDEBUG for assertion code. > > 1. Yes I'm trying to create the same profile multiple times. Create fails, > delete the failed profile, create again... etc. Note that this can be done > overlapped (if the task runner nested) because DeleteSoon takes time to run. Note that it's not necessarily that DeleteSoon "takes time to run" it's that it's intentionally posted as a NonNestableTask so it won't run until the main loop picks it up (after the nested loop exits). > > 2. And yes I've commented the code that creates the dialog. And this only made > the crash more reproducible. I just need to do the silly thing in (1) a few > times in quick succession to crash it. And that's why I think that the root > cause of this crash is DependencyManager, regardless of the presence or absence > of the dialog. > > 3. Somehow the failed DCHECK is about not allowing the same KeyedService of a > profile to be deallocated twice. Using raw pointers should be okay if we can > bind the pointer to a Profile*. Of course, Profile* has the same deficiency as > DependencyManager (see Nick's comment about ProfileManager::IsValidProfile() in > issue 557916). So we need to be very careful about how to fix DependencyManager. > I have no definitive answer by now, but am thinking about having some > observer/callback that can reset DependencyManager during profile destruction. > Anyway, by (2), anything related to GetPath() or GetDebugName() is bad for > uniqueness. > > Anyway, a good fix of (3) is at least non-trivial, so it is not a good idea to > include it in this CL. > > For this CL we either wait for the fix, or to defer failed profile deletion to > the end of the chromium process so the services do not use the same memory > address. I'm okay with both of them now, because this is sadly found NOT related > the cause to the crash in 614753. I'd rather we fix DependencyManager first and check this one in cleanly after but could be convinced otherwise (e.g. say you comment out the erroneous assertion in DependencyManager, does everything work fine?)
Updated test_file_util_win.cc in patches #5 (for delta) and #6. Peter and Paweł: PTAL. If this is okay I'll move the edits to https://codereview.chromium.org/2047483003/ https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... base/test/test_file_util_win.cc:78: // Deny |permission| on the file |path|, for the current user. On 2016/06/16 07:02:10, Peter Kasting wrote: > Nit: Definition order in .cc file must match declaration order in .h. Acknowledged. https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... base/test/test_file_util_win.cc:82: if (GetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()), On 2016/06/16 07:02:10, Peter Kasting wrote: > My comments about const_cast stand (3 places). In the case of the fixed > constant below this is easy, you can just declare a local wchar_t* = L"..." > directly. The other two places you will need to declare a wchar_t[] on the > stack with sufficient size to hold path.value(), then copy in with wcscpy(). > (Or use wcscpy_s() if you want to be really safe.) Thanks a lot. I'll use wcsncpy() because - wcscpy() is too dangerous for possible buffer overruns. - wcscpy_s() is not available if __STDC_WANT_LIB_EXT1__ is not defined. But to define it it seems we need to modify the whole chromium source tree. BTW, do you know why that LPTSTR was not an LPCTSTR at the first place?
base/test still looks good with comments https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util.h:47: // Deny |permission| on the file |path|, for the current user. Consider documenting what are valid values for |permission| . https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util.h:48: bool DenyFilePermission(const FilePath& path, DWORD permission); nit: add WARN_UNUSED_RESULT .
LGTM https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util.h:48: bool DenyFilePermission(const FilePath& path, DWORD permission); On 2016/07/05 09:28:08, Paweł Hajdan Jr. wrote: > nit: add WARN_UNUSED_RESULT . Do we need this to be WARN_UNUSED_RESULT? That's used pretty rarely, and I'm not sure I understand what mandates it here. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util_win.cc:123: int path_size = path.value().size(); Should be size_t. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util_win.cc:124: std::unique_ptr<TCHAR[]> path_ptr(new TCHAR[path_size + 1]); Nit: Prefer "= base::MakeUnique" to raw new. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util_win.cc:126: path_ptr[path_size] = L'\0'; I don't see why this line is needed, since the source string is shorter than the third arg above and thus wcsncpy() should always append the trailing null (shouldn't it?). https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:50: // expected. Nit: check -> checks, but really this comment doesn't seem to match very well with the function name (e.g. the comment says nothing about profiles or creation). Not sure what would be a good comment. https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:55: if (status != Profile::CREATE_STATUS_CREATED) { Nit: Should this explain why the conditional is here? https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:390: base::RunLoop run_loop2; Nit: Should probably explain why you need a second runloop. https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_window.cc:238: return; Nit: Not necessary, subsequent condition handles this
https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:390: base::RunLoop run_loop2; On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > Nit: Should probably explain why you need a second runloop. I'd go further than that: I don't think you need to run the current message loop until it's idle when you've just run it. There should be no task left, unless something gets posted from a background thread between the time the first run loop quits and the second run loop runs, which would be a race condition. (If there was need for the second runloop, you could use it inline, BTW: base::RunLoop().RunUntilIdle()).
Patch uploaded mainly to respond to the comments in base/test. The other parts were not being worked on, so there's no point for reviewing yet. Sorry for being slow. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util.h:47: // Deny |permission| on the file |path|, for the current user. On 2016/07/05 09:28:08, Paweł Hajdan Jr. wrote: > Consider documenting what are valid values for |permission| . It's too much to document. The constants are listed in https://msdn.microsoft.com/en-us/library/aa822867(v=vs.85).aspx , but even that page is very confusing because I don't know what are the "types of objects" are described in the document, and whether a right apply to each type of object. If you search MSDN yourself, probably you'll land on even more confusing pages instead, like the ones below: https://msdn.microsoft.com/en-us/library/aa364399(v=vs.85).aspx https://msdn.microsoft.com/en-us/library/aa379607(v=vs.85).aspx https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util.h:48: bool DenyFilePermission(const FilePath& path, DWORD permission); On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > On 2016/07/05 09:28:08, Paweł Hajdan Jr. wrote: > > nit: add WARN_UNUSED_RESULT . > The idea sounds good, but I'm not sure whether this is really used for the compiles. (Yes if win_clang bot compiles browser tests.) WARN_UNUSED_RESULT is simply ignored in MSVC. > Do we need this to be WARN_UNUSED_RESULT? That's used pretty rarely, and I'm > not sure I understand what mandates it here. We do not want callsites to assume that the call is successful, so the check is helpful. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util_win.cc:123: int path_size = path.value().size(); On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > Should be size_t. Done. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util_win.cc:124: std::unique_ptr<TCHAR[]> path_ptr(new TCHAR[path_size + 1]); On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > Nit: Prefer "= base::MakeUnique" to raw new. Done. Thanks for making base::MakeUnique known to me. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util_win.cc:126: path_ptr[path_size] = L'\0'; On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > I don't see why this line is needed, since the source string is shorter than the > third arg above and thus wcsncpy() should always append the trailing null > (shouldn't it?). I'm super-paranoid here because a missing '\0' causes a security problem instead of a normal bug. So I don't trust the weak guarantee above. :-) Here's my concern: 1. the string length may not be calculated correctly (either due to my carelessness or someone else carelessly modifying my code). 2. wcsncpy() isn't coded by us. Basically we're trusting the code in MSVC, gcc and clang, and these aren't tested by us. https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:50: // expected. On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > Nit: check -> checks, but really this comment doesn't seem to match very well > with the function name (e.g. the comment says nothing about profiles or > creation). Not sure what would be a good comment. Acknowledged. https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:55: if (status != Profile::CREATE_STATUS_CREATED) { On 2016/07/11 02:35:54, Peter Kasting (OOO til Jul 18) wrote: > Nit: Should this explain why the conditional is here? Yes. Profile::CREATE_STATUS_CREATED means the profile is created successfully but not yet fully initialized. Another call with status CREATE_STATUS_INITIALIZED will be made in this case. BTW, still thinking about what to write exactly in this comment. https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:390: base::RunLoop run_loop2; On 2016/07/11 08:40:14, Bernhard Bauer wrote: > On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > > Nit: Should probably explain why you need a second runloop. > > I'd go further than that: I don't think you need to run the current message loop > until it's idle when you've just run it. There should be no task left, unless > something gets posted from a background thread between the time the first run > loop quits and the second run loop runs, which would be a race condition. > > (If there was need for the second runloop, you could use it inline, BTW: > base::RunLoop().RunUntilIdle()). I'll wait until the code does really not to crash before the final decision. BTW, agree that the solution we're proposing does not require this run loop. https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/2061593002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_window.cc:238: return; On 2016/07/11 02:35:54, Peter Kasting (OOO til Jul 18) wrote: > Nit: Not necessary, subsequent condition handles this Done.
https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util.h:48: bool DenyFilePermission(const FilePath& path, DWORD permission); On 2016/07/18 09:41:35, WC Leung wrote: > On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > > Do we need this to be WARN_UNUSED_RESULT? That's used pretty rarely, and I'm > > not sure I understand what mandates it here. > > We do not want callsites to assume that the call is successful, so the check is > helpful. That's not really the criteria for that macro. In general, for any function that returns a value, callers "should" check the value. WARN_UNUSED_RESULT is more for the case where not checking a return value will lead to serious problems like security holes. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util_win.cc:126: path_ptr[path_size] = L'\0'; On 2016/07/18 09:41:35, WC Leung wrote: > On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > > I don't see why this line is needed, since the source string is shorter than > the > > third arg above and thus wcsncpy() should always append the trailing null > > (shouldn't it?). > > I'm super-paranoid here because a missing '\0' causes a security problem instead > of a normal bug. So I don't trust the weak guarantee above. :-) > > Here's my concern: > 1. the string length may not be calculated correctly (either due to my > carelessness or someone else carelessly modifying my code). > 2. wcsncpy() isn't coded by us. Basically we're trusting the code in MSVC, gcc > and clang, and these aren't tested by us. I'm opposed to adding something to account for (2). wcsncpy() is part of the language's library. We trust that we compile on platforms where the library is implemented correctly. Otherwise we'd need checks all over the codebase. Readers will likewise assume that the library functions work and wonder why you have this. For (1), I don't see a way that you were careless and miscalculated the path length. That leaves future code modifiers. The right way to handle such cases, if they need be handled at all, is with a DCHECK that there's no problem to be handled, not by actually handling the (phantom) problem. But I wouldn't bother with this either. After all, if a future modifier might change the path calculation to be in error, they might remove a DCHECK as well, or do any other silly thing. So trying to plan for this makes no sense to me. We should assume that people won't introduce bugs into the internals of functions when they're currently correct and there's no motivating reason why someone would be prone to make that mistake. In the meantime, having the termination happen here is really misleading, because it makes it look like wcsncpy() doesn't null-terminate, which will make a reader go start checking any other such calls in the codebase. We don't want people misled about how library functions work.
New patch uploaded with small changes to base/test/*. Peter: PTAL. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util.h:48: bool DenyFilePermission(const FilePath& path, DWORD permission); On 2016/07/18 17:34:30, Peter Kasting (slow) wrote: > On 2016/07/18 09:41:35, WC Leung wrote: > > On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > > > Do we need this to be WARN_UNUSED_RESULT? That's used pretty rarely, and > I'm > > > not sure I understand what mandates it here. > > > > We do not want callsites to assume that the call is successful, so the check > is > > helpful. > > That's not really the criteria for that macro. In general, for any function > that returns a value, callers "should" check the value. WARN_UNUSED_RESULT is > more for the case where not checking a return value will lead to serious > problems like security holes. I have no opinion on this one. BTW, adding WARN_UNUSED_RESULT for just one function makes the file inconsistent, so I'm okay with not adding it by now. https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/140001/base/test/test_file_ut... base/test/test_file_util_win.cc:126: path_ptr[path_size] = L'\0'; On 2016/07/18 17:34:30, Peter Kasting (slow) wrote: > On 2016/07/18 09:41:35, WC Leung wrote: > > On 2016/07/11 02:35:53, Peter Kasting (OOO til Jul 18) wrote: > > > I don't see why this line is needed, since the source string is shorter than > > the > > > third arg above and thus wcsncpy() should always append the trailing null > > > (shouldn't it?). > > > > I'm super-paranoid here because a missing '\0' causes a security problem > instead > > of a normal bug. So I don't trust the weak guarantee above. :-) > > > > Here's my concern: > > 1. the string length may not be calculated correctly (either due to my > > carelessness or someone else carelessly modifying my code). > > 2. wcsncpy() isn't coded by us. Basically we're trusting the code in MSVC, gcc > > and clang, and these aren't tested by us. > > I'm opposed to adding something to account for (2). wcsncpy() is part of the > language's library. We trust that we compile on platforms where the library is > implemented correctly. Otherwise we'd need checks all over the codebase. > Readers will likewise assume that the library functions work and wonder why you > have this. > > For (1), I don't see a way that you were careless and miscalculated the path > length. That leaves future code modifiers. The right way to handle such cases, > if they need be handled at all, is with a DCHECK that there's no problem to be > handled, not by actually handling the (phantom) problem. But I wouldn't bother > with this either. After all, if a future modifier might change the path > calculation to be in error, they might remove a DCHECK as well, or do any other > silly thing. So trying to plan for this makes no sense to me. We should assume > that people won't introduce bugs into the internals of functions when they're > currently correct and there's no motivating reason why someone would be prone to > make that mistake. > > In the meantime, having the termination happen here is really misleading, > because it makes it look like wcsncpy() doesn't null-terminate, which will make > a reader go start checking any other such calls in the codebase. We don't want > people misled about how library functions work. I see. I do buy in the readability part. So the line is removed.
LGTM https://codereview.chromium.org/2061593002/diff/180001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2061593002/diff/180001/base/test/test_file_ut... base/test/test_file_util.h:47: // Deny |permission| on the file |path|, for the current user. See comment in https://codereview.chromium.org/2047483003/diff/300001/base/test/test_file_ut... . https://codereview.chromium.org/2061593002/diff/180001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2061593002/diff/180001/base/test/test_file_ut... base/test/test_file_util_win.cc:119: // Deny |permission| on the file |path|, for the current user. Nit: Don't repeat .h comments in the .cc file. https://codereview.chromium.org/2061593002/diff/180001/chrome/browser/profile... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2061593002/diff/180001/chrome/browser/profile... chrome/browser/profiles/profile_manager_browsertest.cc:50: // expected. Nit: Copy-and-paste what you did in https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... , as I think it's slightly better (using closures instead of passing runloops, omitting the above-function comments but adding some inside the function, etc.)
What's the status here? Looks like nothing has happened since my sign-off.
On 2016/08/20 03:04:42, Peter Kasting wrote: > What's the status here? Looks like nothing has happened since my sign-off. Sorry for being slow. Didn't manage to get the spare time to work on this CL because family life is too busy with kids. Anyway, back to the topic: some problem with the CL (crash with deleting profiles) was not yet solved in the way we want. BTW, the profile error dialog had a facelift in https://codereview.chromium.org/2107493002/ and this CL as affected. Which this CL is ready I'll invite afakhry@ for taking a look.
On 2016/08/22 18:31:11, WC Leung wrote: > On 2016/08/20 03:04:42, Peter Kasting wrote: > > What's the status here? Looks like nothing has happened since my sign-off. > > Sorry for being slow. Didn't manage to get the spare time to work on this CL > because family life is too busy with kids. > > Anyway, back to the topic: some problem with the CL (crash with deleting > profiles) was not yet solved in the way we want. BTW, the profile error dialog > had a facelift in https://codereview.chromium.org/2107493002/ and this CL as > affected. Which this CL is ready I'll invite afakhry@ for taking a look. It's been almost six months here. The CL mentioned above has landed. Can this either move forward or close?
On 2017/02/11 02:00:43, Peter Kasting wrote: > On 2016/08/22 18:31:11, WC Leung wrote: > > On 2016/08/20 03:04:42, Peter Kasting wrote: > > > What's the status here? Looks like nothing has happened since my sign-off. > > > > Sorry for being slow. Didn't manage to get the spare time to work on this CL > > because family life is too busy with kids. > > > > Anyway, back to the topic: some problem with the CL (crash with deleting > > profiles) was not yet solved in the way we want. BTW, the profile error dialog > > had a facelift in https://codereview.chromium.org/2107493002/ and this CL as > > affected. Which this CL is ready I'll invite afakhry@ for taking a look. > > It's been almost six months here. The CL mentioned above has landed. Can this > either move forward or close? This CL was supposed to be a part of the "fix" of bug 614753, but actually my analyzation of that bug was wrong. So the "fix" may actually targets a problem that no real person faces. (See bug 614753 for updates.) Anyway, when someone clicks on the fail-to-open profile in the avatar menu, Chromium will crash. Since some time has passed without a crash report filed to me, I believe that no one is facing the crash. The proposed solution (by gab in #42) is to fix DependencyManager first, and that involves some design changes to DependencyManager. Unless we know that someone is crashing on this bug, I don't think that it is worth the effort to fix DependencyManager and move forward. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
