Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2487783002:
Make Crashpad use the user data dir, rather than always default location (Closed)
|
Created:
4 years, 1 month ago by scottmg Modified:
3 years, 10 months ago Reviewers:
bcwhite, Mark Mentovai, tapted, penny, Alexei Svitkine (slow), robertshield, pastarmovj, grt (UTC plus 2) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake Crashpad use the user data dir, rather than always default location
This puts the crash database in the user data directory, rather than in
the global shared one. In order to accomplish this, the variable
expansion from policy needs to be moved to be suitable for use in
chrome_elf. This code handles variable expansions in a registry key that
can be set to override the user data dir (in either HKLM or HKCU).
We do not need to completely remove the usage of DLLs other than
kernel32 in the code despite the code being used in chrome_elf, as in
all cases, it will not execute until after the DllMain() of chrome_elf
has completed, and we load the used functions via GetProcAddress(), not
by import lib. Currently, chrome_elf is signalled from WinMain() of
chrome to start crash reporting. In the near future it will be started
earlier from a background thread spawned in DllMain(). However, that
thread cannot execute until DllMain has completed, so it's then safe to
cause the other DLL loads that the variable expansion requires.
(Regarding the future thread-based spawning, ref:
https://blogs.msdn.microsoft.com/oldnewthing/20070904-00/?p=25283/ which
explains why the creation of the thread with which we don't synchronize
is OK, because DllMain calls are serialized. But we can discuss that
more in https://codereview.chromium.org/2475863004/ too. )
BUG=565446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/7433a2b305bbfcb8c469cddab9a1a3ab2c5b70d1
Cr-Commit-Position: refs/heads/master@{#434855}
Patch Set 1 #Patch Set 2 : wip #Patch Set 3 : . #
Total comments: 17
Patch Set 4 : . #
Total comments: 1
Patch Set 5 : . #
Total comments: 29
Patch Set 6 : . #Patch Set 7 : rebase #Patch Set 8 : fixes after rebase #Patch Set 9 : use installdetails #Patch Set 10 : . #
Total comments: 1
Patch Set 11 : . #Patch Set 12 : gn check #Patch Set 13 : . #
Total comments: 24
Patch Set 14 : . #Patch Set 15 : . #
Total comments: 22
Patch Set 16 : . #Patch Set 17 : out of install_details #
Total comments: 1
Patch Set 18 : . #
Total comments: 8
Patch Set 19 : thunk #
Total comments: 4
Patch Set 20 : warning, wcsncpy_s #Patch Set 21 : browser_tests #Patch Set 22 : install_static for swarming #
Total comments: 1
Messages
Total messages: 108 (62 generated)
Description was changed from ========== WIP on making crashpad initialization go in user-data-dir rather than always default BUG=565446 ========== to ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. BUG=565446 ==========
Description was changed from ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. BUG=565446 ========== to ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. BUG=565446 ==========
scottmg@chromium.org changed reviewers: + mark@chromium.org, pastarmovj@chromium.org
scottmg@chromium.org changed reviewers: + pennymac@chromium.org
scottmg@chromium.org changed reviewers: + bcwhite@chromium.org
scottmg@chromium.org changed reviewers: + robertshield@chromium.org
mark: Please review. others: Mostly FYI in case you have any concerns (sorry for the big list, but it sort of touches a little bit of all over). This is mostly moving things around. There is some (noted) duplication in deriving the user data directory unfortunately, but this seemed the most reasonable given the lack of any base/ or other support in chrome_elf. This also only affects Windows at the moment. We should do other platforms too.
Description was changed from ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. BUG=565446 ========== to ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. (Regarding the future thread-based spawning, ref: https://blogs.msdn.microsoft.com/oldnewthing/20070904-00/?p=25283/ which explains why the creation of the thread with which we don't synchronize is OK, because DllMain calls are serialized. But we can discuss that more in https://codereview.chromium.org/2475863004/ too. ) BUG=565446 ==========
https://codereview.chromium.org/2487783002/diff/40001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:364: // sync. Can we add a test for equivalence? Or can we refactor for improved sharing? https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:399: nt::CloseRegKey(key); I’m surprised that OpenRegKey doesn’t return a key and provide a ScopedRegKey to deal with the close. Looks like that would improve every single call site including this one. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:534: std::unique_ptr<wchar_t> user_data_dir_path; This gets a new wchar_t[], so it should be a unique_ptr<wchar_t[]> https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:583: user_data_dir = user_data_dir.substr(0, user_data_dir.size() - 1); If C++11 is guaranteed (and it’s 2016, it should be, right?), just .pop_back()? Otherwise, .erase(use_data_dir.size() - 1, 1) or .resize(user_data_dir.size() - 1) ought to be better as they work with the already-existing string. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:586: const bool specified_directory_was_invalid = This says: If user_data_dir is empty, stop and set was_invalid to false. Otherwise, set was_invalid to !RecursiveDirectoryCreate(). Is that really what you want? It’s compatible (and bug-compatible) with the other file, but wouldn’t we want to try GetDefaultUserDatadirectory() if user_data_dir is empty? was_INvalid and the !s make this pretty double-negative-y, which probably contributed to the confusion. It should read more cleanly and be less bug-prone if you have a more positive attitude. was_invalid = empty() || !RecursiveDirectoryCreate() or was_valid = !empty() && RecursiveDirectoryCreate() …if this behavior is really intended. If not, the rationale for basically failing if user_data_dir is empty is lost on me, and someone needs a comment. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:590: if (!GetDefaultUserDataDirectory(&user_data_dir)) { This can be folded into the if that contains it. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/p... File chrome/install_static/policy_path_parser.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:47: template <typename T, size_t N> char (&ArraySizeHelper(T (&array)[N]))[N]; If C++11 is guaranteed, we can use the neater C++11 constexpr implementation I was messing with. Nobody uses this in a static_cast, so it should be OK.
https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:586: const bool specified_directory_was_invalid = On 2016/11/10 22:06:17, Mark Mentovai wrote: > This says: If user_data_dir is empty, stop and set was_invalid to false. > Otherwise, set was_invalid to !RecursiveDirectoryCreate(). > > Is that really what you want? It’s compatible (and bug-compatible) with the > other file, but wouldn’t we want to try GetDefaultUserDatadirectory() if > user_data_dir is empty? > > was_INvalid and the !s make this pretty double-negative-y, which probably > contributed to the confusion. It should read more cleanly and be less bug-prone > if you have a more positive attitude. > > was_invalid = empty() || !RecursiveDirectoryCreate() > or > was_valid = !empty() && RecursiveDirectoryCreate() > > …if this behavior is really intended. If not, the rationale for basically > failing if user_data_dir is empty is lost on me, and someone needs a comment. Yeah, you're right. I just copied and adapted the other function into this one. I didn't really process it too critically and assumed it was doing what you said above (i.e. if one wasn't supplied, use the default and create it if necessary). I'm not sure what the correct behaviour is now that I've started writing some tests for it. The original implementation alludes to the browser process handling it "later", but I'm not sure how/where that happens yet. Hmm...
https://codereview.chromium.org/2487783002/diff/40001/chrome/browser/policy/p... File chrome/browser/policy/policy_path_parser_win.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser_win.cc:46: return install_static::ExpandPathVariables(untranslated_string); nit: Please put a small comment here why the code lives in install_static instead of here. I know this now but when I read install_static -this sounds as if it part of the installer instead of the browser imo and I might wonder again when I read this a year from now.
On 2016/11/11 00:24:47, scottmg wrote: > https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... > File chrome/install_static/install_util.cc (right): > > https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... > chrome/install_static/install_util.cc:586: const bool > specified_directory_was_invalid = > On 2016/11/10 22:06:17, Mark Mentovai wrote: > > This says: If user_data_dir is empty, stop and set was_invalid to false. > > Otherwise, set was_invalid to !RecursiveDirectoryCreate(). > > > > Is that really what you want? It’s compatible (and bug-compatible) with the > > other file, but wouldn’t we want to try GetDefaultUserDatadirectory() if > > user_data_dir is empty? > > > > was_INvalid and the !s make this pretty double-negative-y, which probably > > contributed to the confusion. It should read more cleanly and be less > bug-prone > > if you have a more positive attitude. > > > > was_invalid = empty() || !RecursiveDirectoryCreate() > > or > > was_valid = !empty() && RecursiveDirectoryCreate() > > > > …if this behavior is really intended. If not, the rationale for basically > > failing if user_data_dir is empty is lost on me, and someone needs a comment. > > Yeah, you're right. I just copied and adapted the other function into this one. > I didn't really process it too critically and assumed it was doing what you said > above (i.e. if one wasn't supplied, use the default and create it if necessary). > > I'm not sure what the correct behaviour is now that I've started writing some > tests for it. The original implementation alludes to the browser process > handling it "later", but I'm not sure how/where that happens yet. Hmm... I dug into this. Here's what we do now: 1. Get command line --user-data-dir if set 2. Override with policy registry setting if set 3. If either of those are set (non-empty) but invalid, then instead call PathService to get a value for USER_DATA_DIR. This in turn gets the default user data dir (i.e. %LOCALAPPDATA%\Google\Chrome\User Data). 4. If that fails, then in the browser we blindly continue on and hope something else will save us. This seems only lightly considered, because it appends the (invalid) value of --user-data-dir to the command line to try to help child processes, but it doesn't yet *have* a valid user data dir. So that doesn't make any sense. Child processes will CHECK() at this point. 5. In the browser we continue on, and eventually display a dialog stating that we can't write to the data directory. 6. Then, because the retrieval of the user data dir failed, https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?q=... returns failure, we skip the rest of initialization and then deref a nullptr (g_browser_process). So I'm skeptical of the utility of this double fallback logic. So! My proposed solution is to: - Do 1-3. - If that 3 fails (we can't get any valid user data dir), then display the dialog right there and LOG(FATAL) immediately. The end user gets the same behaviour, our code gets clearer. - If the command line or policy directories were invalid, but the default user data was OK, then setting for child processes and continuing makes sense, so maintain that behaviour. Assuming that doesn't seem objectionable I'm going to try to write some tests for that now, and then make the chrome_elf version match.
robertshield@chromium.org changed reviewers: + grt@chromium.org
I'm adding grt@ to this, because there's some installer refactoring going on that has recently interacted badly with crashpad storage location.
https://codereview.chromium.org/2487783002/diff/40001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:364: // sync. On 2016/11/10 22:06:17, Mark Mentovai wrote: > Can we add a test for equivalence? Or can we refactor for improved sharing? Done. https://codereview.chromium.org/2487783002/diff/40001/chrome/browser/policy/p... File chrome/browser/policy/policy_path_parser_win.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser_win.cc:46: return install_static::ExpandPathVariables(untranslated_string); On 2016/11/14 09:42:50, pastarmovj wrote: > nit: Please put a small comment here why the code lives in install_static > instead of here. I know this now but when I read install_static -this sounds as > if it part of the installer instead of the browser imo and I might wonder again > when I read this a year from now. Done. (Agreed that the "install_static" library is terribly named. Someone was working on renaming/moving it to somewhere more sensible but hasn't happened yet.) https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:399: nt::CloseRegKey(key); On 2016/11/10 22:06:17, Mark Mentovai wrote: > I’m surprised that OpenRegKey doesn’t return a key and provide a ScopedRegKey to > deal with the close. Looks like that would improve every single call site > including this one. Agreed. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=665635 for a followup. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:534: std::unique_ptr<wchar_t> user_data_dir_path; On 2016/11/10 22:06:17, Mark Mentovai wrote: > This gets a new wchar_t[], so it should be a unique_ptr<wchar_t[]> Done. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:583: user_data_dir = user_data_dir.substr(0, user_data_dir.size() - 1); On 2016/11/10 22:06:17, Mark Mentovai wrote: > If C++11 is guaranteed (and it’s 2016, it should be, right?), just .pop_back()? > > Otherwise, .erase(use_data_dir.size() - 1, 1) or .resize(user_data_dir.size() - > 1) ought to be better as they work with the already-existing string. Done. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:586: const bool specified_directory_was_invalid = On 2016/11/11 00:24:46, scottmg wrote: > On 2016/11/10 22:06:17, Mark Mentovai wrote: > > This says: If user_data_dir is empty, stop and set was_invalid to false. > > Otherwise, set was_invalid to !RecursiveDirectoryCreate(). > > > > Is that really what you want? It’s compatible (and bug-compatible) with the > > other file, but wouldn’t we want to try GetDefaultUserDatadirectory() if > > user_data_dir is empty? > > > > was_INvalid and the !s make this pretty double-negative-y, which probably > > contributed to the confusion. It should read more cleanly and be less > bug-prone > > if you have a more positive attitude. > > > > was_invalid = empty() || !RecursiveDirectoryCreate() > > or > > was_valid = !empty() && RecursiveDirectoryCreate() > > > > …if this behavior is really intended. If not, the rationale for basically > > failing if user_data_dir is empty is lost on me, and someone needs a comment. > > Yeah, you're right. I just copied and adapted the other function into this one. > I didn't really process it too critically and assumed it was doing what you said > above (i.e. if one wasn't supplied, use the default and create it if necessary). > > I'm not sure what the correct behaviour is now that I've started writing some > tests for it. The original implementation alludes to the browser process > handling it "later", but I'm not sure how/where that happens yet. Hmm... I think the behaviour is semi-rational now. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:590: if (!GetDefaultUserDataDirectory(&user_data_dir)) { On 2016/11/10 22:06:17, Mark Mentovai wrote: > This can be folded into the if that contains it. Done. https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/p... File chrome/install_static/policy_path_parser.cc (right): https://codereview.chromium.org/2487783002/diff/40001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:47: template <typename T, size_t N> char (&ArraySizeHelper(T (&array)[N]))[N]; On 2016/11/10 22:06:17, Mark Mentovai wrote: > If C++11 is guaranteed, we can use the neater C++11 constexpr implementation I > was messing with. Nobody uses this in a static_cast, so it should be OK. Done. https://codereview.chromium.org/2487783002/diff/60001/chrome/install_static/i... File chrome/install_static/install_util.cc (left): https://codereview.chromium.org/2487783002/diff/60001/chrome/install_static/i... chrome/install_static/install_util.cc:221: if ((file_attributes != INVALID_FILE_ATTRIBUTES) && The "else" was at the wrong level (no error was returned if the directory failed to be created). This is caught by UserDataDir.InvalidResultsInDefault now.
https://codereview.chromium.org/2487783002/diff/80001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:399: // WARNING! It is important that this code match behavior with is it necessary to do the work in both places? i introduced a way to pass data from chrome_elf into chrome.exe and chrome.dll in r432270 (sorry, you're going to have to make some changes when you sync past that). does it make sense to put the user_data_dir into InstallDetails::Payload a la the channel name? https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:67: const wchar_t kUserDataDirSwitch[] = L"user-data-dir"; nit: either use "Swtich" on all of these constants that are switches, or on none of them. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:199: if (!_wfullpath(file_path, input.c_str(), MAX_PATH)) #include <stdlib.h> for _wfullpath https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:199: if (!_wfullpath(file_path, input.c_str(), MAX_PATH)) nit: MAX_PATH -> _countof(file_path) https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:206: if (file_attributes != INVALID_FILE_ATTRIBUTES) uber nit: either handle failure with an early return for consistency with the function above, or turn this into one return statement: return file_attributes != INVALID_FILE_ATTRIBUTES && (file_attributes & FILE_ATTRIBUTE_DIRECTORY) != 0; https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:596: bool have_valid_directory = i gently prefer this, but it's your call: bool have_valid_directory = (!user_data_dir.empty() && RecursiveDirectoryCreate(user_data_dir)) || GetDefaultUserDataDirectory(&user_data_dir); // The Chrome implementation CHECKs() here in the browser process. We don't as // this function is used to initialize crash reporting, so we would get no // report of this failure. assert(have_valid_directory); if (!have_valid_directory) return false; https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:621: // being nit: wrap https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:623: if (!GetUserDataDirectoryUsingProcessCommandLine(crash_dir)) { nit: omit braces https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.h:119: bool GetUserDataDirectory(const std::wstring& user_data_dir_from_command_line, could you add unit tests for these new functions? https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... File chrome/install_static/policy_path_parser.cc (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:48: constexpr size_t arraysize(const T (&array)[N]) { use _countof from stdlib.h instead of this https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:56: // should only be used after DllMain() has run. wdyt of adding something to enforce this? the first thing that comes to mind is an "EnablePathExpansion();" call at the end of DllMain that sets some state that is assert()ed on here. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:67: result = result.substr(1, result.length() - 2); i would think that result.pop_back(); result.erase(0, 1); would be less wasteful. if you want to keep it on one line: result.erase(0, 1).pop_back(); https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:71: LoadLibrary(L"shell32.dll"), "SHGetSpecialFolderPathW")); no matching calls to FreeLibrary for any of these. is this intentional? it seems bad. https://codereview.chromium.org/2487783002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:4919: "../app/user_data_dir_win_unittest.cc", missing file?
https://codereview.chromium.org/2487783002/diff/80001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:399: // WARNING! It is important that this code match behavior with On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > is it necessary to do the work in both places? i introduced a way to pass data > from chrome_elf into chrome.exe and chrome.dll in r432270 (sorry, you're going > to have to make some changes when you sync past that). does it make sense to put > the user_data_dir into InstallDetails::Payload a la the channel name? Possibly... (it's not quite an install detail", but oh well). While they're relatively similar, the differences are: 1. Handling non-Windows, which chrome_elf isn't going to do 2. Interacting with PathService 3. Notifying the user of failures, which has to be done later here, rather than in chrome_elf. So there's still going to be be some stuff here. But I'll rebase on your CL and take a look. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:67: const wchar_t kUserDataDirSwitch[] = L"user-data-dir"; On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > nit: either use "Swtich" on all of these constants that are switches, or on none > of them. Hrm, I don't like "Switch" on there, but there's 3 different "user data dirs" in here: "user-data-dir" for the command line, "UserDataDir" for the registry key and "User Data" for the directory name! So it was intended to be parallel to kUserDataDirRegKey and kUserDataDirname. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:199: if (!_wfullpath(file_path, input.c_str(), MAX_PATH)) On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > #include <stdlib.h> for _wfullpath Done. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:199: if (!_wfullpath(file_path, input.c_str(), MAX_PATH)) On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > nit: MAX_PATH -> _countof(file_path) Done. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:206: if (file_attributes != INVALID_FILE_ATTRIBUTES) On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > uber nit: either handle failure with an early return for consistency with the > function above, or turn this into one return statement: > return file_attributes != INVALID_FILE_ATTRIBUTES && (file_attributes & > FILE_ATTRIBUTE_DIRECTORY) != 0; Done. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:596: bool have_valid_directory = On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > i gently prefer this, but it's your call: > bool have_valid_directory = > (!user_data_dir.empty() && RecursiveDirectoryCreate(user_data_dir)) || > GetDefaultUserDataDirectory(&user_data_dir); > > // The Chrome implementation CHECKs() here in the browser process. We don't as > // this function is used to initialize crash reporting, so we would get no > // report of this failure. > assert(have_valid_directory); > if (!have_valid_directory) > return false; Done. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:621: // being On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > nit: wrap Done. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.cc:623: if (!GetUserDataDirectoryUsingProcessCommandLine(crash_dir)) { On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > nit: omit braces Done. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/i... chrome/install_static/install_util.h:119: bool GetUserDataDirectory(const std::wstring& user_data_dir_from_command_line, On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > could you add unit tests for these new functions? Yeah, oops, they're in the file I forgot to add! https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... File chrome/install_static/policy_path_parser.cc (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:48: constexpr size_t arraysize(const T (&array)[N]) { On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > use _countof from stdlib.h instead of this Done. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:56: // should only be used after DllMain() has run. On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > wdyt of adding something to enforce this? the first thing that comes to mind is > an "EnablePathExpansion();" call at the end of DllMain that sets some state that > is assert()ed on here. I think it's OK without that because it'll be pretty obvious because it'll deadlock on the LoadLibrary(). https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:67: result = result.substr(1, result.length() - 2); On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > i would think that > result.pop_back(); > result.erase(0, 1); > would be less wasteful. if you want to keep it on one line: > result.erase(0, 1).pop_back(); Maybe a little... I admit I only moved this code, I didn't read it. https://codereview.chromium.org/2487783002/diff/80001/chrome/install_static/p... chrome/install_static/policy_path_parser.cc:71: LoadLibrary(L"shell32.dll"), "SHGetSpecialFolderPathW")); On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > no matching calls to FreeLibrary for any of these. is this intentional? it seems > bad. I thought about that. But cpu@ always implied unloading a DLL was like casting a hex on yourself. But done. Four wrappers seemed like too many, so I overdid it a template instead. https://codereview.chromium.org/2487783002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:4919: "../app/user_data_dir_win_unittest.cc", On 2016/11/16 13:24:53, grt (UTC plus 1) wrote: > missing file? Done.
https://codereview.chromium.org/2487783002/diff/80001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2487783002/diff/80001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:399: // WARNING! It is important that this code match behavior with On 2016/11/16 21:01:18, scottmg wrote: > On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > > is it necessary to do the work in both places? i introduced a way to pass data > > from chrome_elf into chrome.exe and chrome.dll in r432270 (sorry, you're going > > to have to make some changes when you sync past that). does it make sense to > put > > the user_data_dir into InstallDetails::Payload a la the channel name? > > Possibly... (it's not quite an install detail", but oh well). While they're > relatively similar, the differences are: > 1. Handling non-Windows, which chrome_elf isn't going to do > 2. Interacting with PathService > 3. Notifying the user of failures, which has to be done later here, rather than > in chrome_elf. > > So there's still going to be be some stuff here. But I'll rebase on your CL and > take a look. Maybe for Windows this code can get the value from chrome_elf (via InstallDetails, which we can rename!) and then do the other processing with it. I think we should avoid fetching the value twice to avoid a race against a policy update or accidental refactorings that forget to update both locations.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/17 09:20:26, grt (UTC plus 1) wrote: > https://codereview.chromium.org/2487783002/diff/80001/chrome/app/chrome_main_... > File chrome/app/chrome_main_delegate.cc (right): > > https://codereview.chromium.org/2487783002/diff/80001/chrome/app/chrome_main_... > chrome/app/chrome_main_delegate.cc:399: // WARNING! It is important that this > code match behavior with > On 2016/11/16 21:01:18, scottmg wrote: > > On 2016/11/16 13:24:52, grt (UTC plus 1) wrote: > > > is it necessary to do the work in both places? i introduced a way to pass > data > > > from chrome_elf into chrome.exe and chrome.dll in r432270 (sorry, you're > going > > > to have to make some changes when you sync past that). does it make sense to > > put > > > the user_data_dir into InstallDetails::Payload a la the channel name? > > > > Possibly... (it's not quite an install detail", but oh well). While they're > > relatively similar, the differences are: > > 1. Handling non-Windows, which chrome_elf isn't going to do > > 2. Interacting with PathService > > 3. Notifying the user of failures, which has to be done later here, rather > than > > in chrome_elf. > > > > So there's still going to be be some stuff here. But I'll rebase on your CL > and > > take a look. > > Maybe for Windows this code can get the value from chrome_elf (via > InstallDetails, which we can rename!) and then do the other processing with it. > I think we should avoid fetching the value twice to avoid a race against a > policy update or accidental refactorings that forget to update both locations. Ready for another look. I didn't rename InstallDetails here to avoid this getting more sprawling, but we can do that as a standalone followup.
https://codereview.chromium.org/2487783002/diff/180001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2487783002/diff/180001/chrome/install_static/... chrome/install_static/install_details.h:202: static PrimaryInstallDetails* GetMutable(); I assume you will dislike this solution, but I didn't think of a much better one. The derivation of the user data dir needs to be able to call GetDefaultUserDataDirectory(), which needs AppendChromeInstallSubDirectory(), which requires an set up InstallDetails. Because of the create-and-move-into-place-as-immutable setup, I can't directly create the user data dir ahead of time, nor install it later after the rest of the InstallDetails have been set up. Maybe you have a better suggestion on how to set this up?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2487783002/diff/240001/chrome/app/user_data_d... File chrome/app/user_data_dir_win_unittest.cc (right): https://codereview.chromium.org/2487783002/diff/240001/chrome/app/user_data_d... chrome/app/user_data_dir_win_unittest.cc:28: install_static::GetUserDataDirectory(L"", &result, &invalid); can these tests be in install_static_unittests? https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:10: #include <windows.h> nit: i think we generally include windows.h first since it must be included before certain other system headers. i dunno how universal this is, but i thought it was a thing. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:42: const wchar_t kRegistryChromePolicyKey[] = please follow the strategy in ReportingIsEnforcedByPolicy to compute this path. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:388: if (nt::OpenRegKey(hive, kRegistryChromePolicyKey, KEY_READ, &key, nullptr)) { nit: if (nt::QueryRegValueSZ(hive, nt::NONE, policy_path, key_name, &value)) { https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:390: *dir = install_static::ExpandPathVariables(value); nit: omit "install_static::" https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:398: void CheckUserDataDirPolicy(std::wstring* user_data_dir) { please document this. the caller assumes that |user_data_dir| will only be modified if a value is found. also, "Check" isn't a great name since it actually Gets the policy value. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:400: // Policy from the HKLM hive has precedence over HKCU. i don't think it would be a bad thing for this to look more like ReportingIsEnforcedByPolicy below https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:548: bool GetUserDataDirectory(const std::wstring& user_data_dir_from_command_line, if i'm not mistaken, the only codepath for GetUserDataDirectoryUsingProcessCommandLine, GetUserDataDirectory, CheckUserDataDirPolicy, and GetDefaultUserDataDirectory is the one during startup when creating the ProductInstallDetails. i think it's cleaner to move all of the code for this into product_install_details.cc and rejigger as needed so that they can be called by MakeProductDetails before the global instance has been put into place. i think this will resolve the GetMutable thing. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:585: install_static::kUserDataDirSwitch), nit: omit "install_static::" https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:593: *crash_dir = install_static::InstallDetails::Get().user_data_dir(); nit: omit "install_static::" https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... File chrome/install_static/policy_path_parser.cc (right): https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/policy_path_parser.cc:35: const WinFolderNamesToCSIDLMapping win_folder_mapping[] = { nit: kWinFolderMapping https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... File chrome/install_static/policy_path_parser.h (right): https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/policy_path_parser.h:15: const std::wstring& untranslated_string); nit: un-wrap
Patchset #14 (id:260001) has been deleted
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Patchset #15 (id:300001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks Greg! https://codereview.chromium.org/2487783002/diff/240001/chrome/app/user_data_d... File chrome/app/user_data_dir_win_unittest.cc (right): https://codereview.chromium.org/2487783002/diff/240001/chrome/app/user_data_d... chrome/app/user_data_dir_win_unittest.cc:28: install_static::GetUserDataDirectory(L"", &result, &invalid); On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > can these tests be in install_static_unittests? Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:10: #include <windows.h> On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > nit: i think we generally include windows.h first since it must be included > before certain other system headers. i dunno how universal this is, but i > thought it was a thing. Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:42: const wchar_t kRegistryChromePolicyKey[] = On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > please follow the strategy in ReportingIsEnforcedByPolicy to compute this path. Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:388: if (nt::OpenRegKey(hive, kRegistryChromePolicyKey, KEY_READ, &key, nullptr)) { On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > nit: > if (nt::QueryRegValueSZ(hive, nt::NONE, policy_path, key_name, &value)) { Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:390: *dir = install_static::ExpandPathVariables(value); On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > nit: omit "install_static::" Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:398: void CheckUserDataDirPolicy(std::wstring* user_data_dir) { On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > please document this. the caller assumes that |user_data_dir| will only be > modified if a value is found. also, "Check" isn't a great name since it actually > Gets the policy value. Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:400: // Policy from the HKLM hive has precedence over HKCU. On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > i don't think it would be a bad thing for this to look more like > ReportingIsEnforcedByPolicy below Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:548: bool GetUserDataDirectory(const std::wstring& user_data_dir_from_command_line, On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > if i'm not mistaken, the only codepath for > GetUserDataDirectoryUsingProcessCommandLine, GetUserDataDirectory, > CheckUserDataDirPolicy, and GetDefaultUserDataDirectory is the one during > startup when creating the ProductInstallDetails. i think it's cleaner to move > all of the code for this into product_install_details.cc and rejigger as needed > so that they can be called by MakeProductDetails before the global instance has > been put into place. i think this will resolve the GetMutable thing. Done. It's bit ugly to have to pass the partial InstallDetails through things (it's needed for install_suffix in AppendChromeInstallSubdirectory() which I didn't explain very clearly). But I think overall this is probably nicer now. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:585: install_static::kUserDataDirSwitch), On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > nit: omit "install_static::" Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/install_util.cc:593: *crash_dir = install_static::InstallDetails::Get().user_data_dir(); On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > nit: omit "install_static::" Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... File chrome/install_static/policy_path_parser.cc (right): https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/policy_path_parser.cc:35: const WinFolderNamesToCSIDLMapping win_folder_mapping[] = { On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > nit: kWinFolderMapping Done. https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... File chrome/install_static/policy_path_parser.h (right): https://codereview.chromium.org/2487783002/diff/240001/chrome/install_static/... chrome/install_static/policy_path_parser.h:15: const std::wstring& untranslated_string); On 2016/11/21 10:29:54, grt (UTC plus 1) wrote: > nit: un-wrap Done.
https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/install_details.h:55: size_t user_data_dir_length; do you think storing these lengths is overkill? part of me figures "why incur a strlen when we know the length?" while another part of me figures "less code is better code." what do you think? https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/install_util.cc:373: std::wstring& AppendChromeInstallSubDirectory(std::wstring* path, oops, i goofd when i wrote this -- |path| should be the last arg since it is an in/out arg. would you be so kind? i think it's more natural for the args to be |details| (or |mode|), |include_suffix|, |path|. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/install_util.cc:436: bool GetCrashDumpLocation(std::wstring* crash_dir) { how about making this return the std::wstring since it no longer fails? return InstallDetails::Get().user_data_dir().append(L"\\Crashpad"); https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/policy_path_parser.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/policy_path_parser.cc:103: SCOPED_LOAD_FUNCTION(L"shell32.dll", ::SHGetSpecialFolderPathW); do you expect that these are already loaded in the process by the time this function is called? would it be safe to use GetModuleHandle instead of LoadLibrary? then you wouldn't need the FreeLibrary calls. just a thought. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:58: kUserDataDirRegistryKeyName, &value)) { nit: indentation https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:59: *user_data_dir = ExpandPathVariables(value); hmm. this is going to be called from within chrome_elf's DllMain now. is that not what you intended? oops, did i steer you in a bad direction? https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:202: constexpr wchar_t kUserDataDirname[] = L"User Data"; nit: inline the literal below? https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:256: GetUserDataDirectoryUsingProcessCommandLine(*details, &user_data_dir, rather than passing a partially-constructed InstallDetails around, what about passing around an InstallConstants (|mode|) a la DetermineChannel? https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/user_data_dir_win_unittest.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/user_data_dir_win_unittest.cc:31: sizeof(InstallConstants), 0, L"", nullptr, nullptr, barf. i just learned about: // clang-format off // clang-format on wdyt about wrapping these constants with them so that they are readable? https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/user_data_dir_win_unittest.cc:71: ASSERT_TRUE(nt::SetTestingOverride(nt::HKLM, temp)); this needs to be paired with SetTestingOverride(nt::HKLM, base::string16()); could you make a test fixture for these that does the overriding (and resetting) in its ctor and dtor?
(not sure what to do about the chrome_elf DllMain yet, but just posting fixes on the off chance you're still around and have time to do another iteration on this part at least.) https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/install_details.h:55: size_t user_data_dir_length; On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > do you think storing these lengths is overkill? part of me figures "why incur a > strlen when we know the length?" while another part of me figures "less code is > better code." what do you think? Yeah, was just copying channel/channel_length. But it's a bit ugly and since it's unnecessary for current usage, I'll drop the lengths. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/install_util.cc:373: std::wstring& AppendChromeInstallSubDirectory(std::wstring* path, On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > oops, i goofd when i wrote this -- |path| should be the last arg since it is an > in/out arg. would you be so kind? i think it's more natural for the args to be > |details| (or |mode|), |include_suffix|, |path|. Done. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/install_util.cc:436: bool GetCrashDumpLocation(std::wstring* crash_dir) { On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > how about making this return the std::wstring since it no longer fails? > return InstallDetails::Get().user_data_dir().append(L"\\Crashpad"); Done. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/policy_path_parser.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/policy_path_parser.cc:103: SCOPED_LOAD_FUNCTION(L"shell32.dll", ::SHGetSpecialFolderPathW); On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > do you expect that these are already loaded in the process by the time this > function is called? would it be safe to use GetModuleHandle instead of > LoadLibrary? then you wouldn't need the FreeLibrary calls. just a thought. No, I don't think there's any reason for these to be already loaded in the future. In fact, they're unlikely to be, as this will be run very early in the context of chrome_elf. (As you pointed out elsewhere, too early!) https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:58: kUserDataDirRegistryKeyName, &value)) { On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > nit: indentation Done. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:59: *user_data_dir = ExpandPathVariables(value); On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > hmm. this is going to be called from within chrome_elf's DllMain now. is that > not what you intended? oops, did i steer you in a bad direction? Yup, $#@#! I was thinking it would be ok after https://codereview.chromium.org/2475863004/ (which was the point of doing this one) but no, this will hang under the same conditions and re-induce https://bugs.chromium.org/p/chromium/issues/detail?id=655788. Hmm. I really wish I'd got this landed before your CL now. :-) https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:202: constexpr wchar_t kUserDataDirname[] = L"User Data"; On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > nit: inline the literal below? Done. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:256: GetUserDataDirectoryUsingProcessCommandLine(*details, &user_data_dir, On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > rather than passing a partially-constructed InstallDetails around, what about > passing around an InstallConstants (|mode|) a la DetermineChannel? Done. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/user_data_dir_win_unittest.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/user_data_dir_win_unittest.cc:31: sizeof(InstallConstants), 0, L"", nullptr, nullptr, On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > barf. i just learned about: > // clang-format off > // clang-format on > wdyt about wrapping these constants with them so that they are readable? I don't think this is bad enough to warrant that. But I can get away with removing (default initializing) some of the fields so that there's less ugly. https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/user_data_dir_win_unittest.cc:71: ASSERT_TRUE(nt::SetTestingOverride(nt::HKLM, temp)); On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > this needs to be paired with SetTestingOverride(nt::HKLM, base::string16()); > could you make a test fixture for these that does the overriding (and resetting) > in its ctor and dtor? Yucky, done. I made it a scoped class here rather than a test fixture as I've got an outstanding CL to do similar for the registry key access in https://codereview.chromium.org/2507263002/. So I move this one there afterwards.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
The CQ bit was unchecked by scottmg@chromium.org
Description was changed from ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. (Regarding the future thread-based spawning, ref: https://blogs.msdn.microsoft.com/oldnewthing/20070904-00/?p=25283/ which explains why the creation of the thread with which we don't synchronize is OK, because DllMain calls are serialized. But we can discuss that more in https://codereview.chromium.org/2475863004/ too. ) BUG=565446 ========== to ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. (Regarding the future thread-based spawning, ref: https://blogs.msdn.microsoft.com/oldnewthing/20070904-00/?p=25283/ which explains why the creation of the thread with which we don't synchronize is OK, because DllMain calls are serialized. But we can discuss that more in https://codereview.chromium.org/2475863004/ too. ) BUG=565446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:59: *user_data_dir = ExpandPathVariables(value); On 2016/11/22 17:16:07, scottmg wrote: > On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > > hmm. this is going to be called from within chrome_elf's DllMain now. is that > > not what you intended? oops, did i steer you in a bad direction? > > Yup, $#@#! > > I was thinking it would be ok after https://codereview.chromium.org/2475863004/ > (which was the point of doing this one) but no, this will hang under the same > conditions and re-induce > https://bugs.chromium.org/p/chromium/issues/detail?id=655788. > > Hmm. > > I really wish I'd got this landed before your CL now. :-) Wow, yeah, I suck. :( Sigh. This has to be initialized later. OK, so I think I'll not use InstallDetails after all. Since it's only used from chrome_elf and chrome.exe (not chrome.dll) it's reasonable to just ... call a function. So I've done that. https://codereview.chromium.org/2487783002/diff/360001/chrome/install_static/... File chrome/install_static/user_data_dir.h (right): https://codereview.chromium.org/2487783002/diff/360001/chrome/install_static/... chrome/install_static/user_data_dir.h:32: const InstallConstants& mode, I maintained passing this in rather than using the global one because it makes it easier to test, rather than requiring global initialization.
Sorry about leading you down the garden path. Taking another look now.
https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... chrome/install_static/product_install_details.cc:59: *user_data_dir = ExpandPathVariables(value); On 2016/11/22 20:40:41, scottmg wrote: > On 2016/11/22 17:16:07, scottmg wrote: > > On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > > > hmm. this is going to be called from within chrome_elf's DllMain now. is > that > > > not what you intended? oops, did i steer you in a bad direction? > > > > Yup, $#@#! > > > > I was thinking it would be ok after > https://codereview.chromium.org/2475863004/ > > (which was the point of doing this one) but no, this will hang under the same > > conditions and re-induce > > https://bugs.chromium.org/p/chromium/issues/detail?id=655788. > > > > Hmm. > > > > I really wish I'd got this landed before your CL now. :-) > > Wow, yeah, I suck. :( Sigh. This has to be initialized later. > > OK, so I think I'll not use InstallDetails after all. Since it's only used from > chrome_elf and chrome.exe (not chrome.dll) Does this mean that the user data dir will be computed twice: once in chrome_elf.dll and once in chrome.exe? https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... chrome/install_static/install_details.h:75: const InstallConstants* mode() const { return payload_->mode; } nit: return a const-ref since this can never be null https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... chrome/install_static/install_util.h:16: #include "chrome/install_static/install_details.h" is this needed? https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... File chrome/install_static/user_data_dir.cc (right): https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... chrome/install_static/user_data_dir.cc:19: std::wstring g_user_data_dir; this will introduce a global dtor, which is banned. maybe make it a ptr that's nullptr during static initialization and leaked after it's set? https://codereview.chromium.org/2487783002/diff/380001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_main.cc (left): https://codereview.chromium.org/2487783002/diff/380001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_main.cc:42: // CRT on initialization installs an exception filter which calls i never liked the way this was indented...
On 2016/11/22 20:53:26, grt (UTC plus 1) wrote: > Sorry about leading you down the garden path. Taking another look now. Thanks for looking so late. My fault for not using my "brain".
On 2016/11/22 21:06:59, grt (UTC plus 1) wrote: > https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... > File chrome/install_static/product_install_details.cc (right): > > https://codereview.chromium.org/2487783002/diff/320001/chrome/install_static/... > chrome/install_static/product_install_details.cc:59: *user_data_dir = > ExpandPathVariables(value); > On 2016/11/22 20:40:41, scottmg wrote: > > On 2016/11/22 17:16:07, scottmg wrote: > > > On 2016/11/22 11:37:09, grt (UTC plus 1) wrote: > > > > hmm. this is going to be called from within chrome_elf's DllMain now. is > > that > > > > not what you intended? oops, did i steer you in a bad direction? > > > > > > Yup, $#@#! > > > > > > I was thinking it would be ok after > > https://codereview.chromium.org/2475863004/ > > > (which was the point of doing this one) but no, this will hang under the > same > > > conditions and re-induce > > > https://bugs.chromium.org/p/chromium/issues/detail?id=655788. > > > > > > Hmm. > > > > > > I really wish I'd got this landed before your CL now. :-) > > > > Wow, yeah, I suck. :( Sigh. This has to be initialized later. > > > > OK, so I think I'll not use InstallDetails after all. Since it's only used > from > > chrome_elf and chrome.exe (not chrome.dll) > > Does this mean that the user data dir will be computed twice: once in > chrome_elf.dll and once in chrome.exe? Oops, yes. I meant to do the thunk dance, and then forgot. Done. > > https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... > File chrome/install_static/install_details.h (right): > > https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... > chrome/install_static/install_details.h:75: const InstallConstants* mode() const > { return payload_->mode; } > nit: return a const-ref since this can never be null > > https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... > File chrome/install_static/install_util.h (right): > > https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... > chrome/install_static/install_util.h:16: #include > "chrome/install_static/install_details.h" > is this needed? > > https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... > File chrome/install_static/user_data_dir.cc (right): > > https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... > chrome/install_static/user_data_dir.cc:19: std::wstring g_user_data_dir; > this will introduce a global dtor, which is banned. maybe make it a ptr that's > nullptr during static initialization and leaked after it's set? > > https://codereview.chromium.org/2487783002/diff/380001/chrome_elf/chrome_elf_... > File chrome_elf/chrome_elf_main.cc (left): > > https://codereview.chromium.org/2487783002/diff/380001/chrome_elf/chrome_elf_... > chrome_elf/chrome_elf_main.cc:42: // CRT on initialization installs an exception > filter which calls > i never liked the way this was indented...
https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... chrome/install_static/install_details.h:75: const InstallConstants* mode() const { return payload_->mode; } On 2016/11/22 21:06:58, grt (UTC plus 1) wrote: > nit: return a const-ref since this can never be null Done. https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... chrome/install_static/install_util.h:16: #include "chrome/install_static/install_details.h" On 2016/11/22 21:06:58, grt (UTC plus 1) wrote: > is this needed? Done. https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... File chrome/install_static/user_data_dir.cc (right): https://codereview.chromium.org/2487783002/diff/380001/chrome/install_static/... chrome/install_static/user_data_dir.cc:19: std::wstring g_user_data_dir; On 2016/11/22 21:06:58, grt (UTC plus 1) wrote: > this will introduce a global dtor, which is banned. maybe make it a ptr that's > nullptr during static initialization and leaked after it's set? Done. [That rule is pretty lax on Windows tbh, they're everywhere. :(] https://codereview.chromium.org/2487783002/diff/380001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_main.cc (left): https://codereview.chromium.org/2487783002/diff/380001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_main.cc:42: // CRT on initialization installs an exception filter which calls On 2016/11/22 21:06:58, grt (UTC plus 1) wrote: > i never liked the way this was indented... ocd ftw!
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
i'm not a big fan of having the thunk live so far away from the rest of the code, but this looks like it'll get the job done. lgtm. https://codereview.chromium.org/2487783002/diff/400001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/400001/chrome/install_static/... chrome/install_static/install_util.cc:442: assert(ret); the strict compiler is complaining about |ret| being unused in NDEBUG builds. yikes. is there something more attractive than: #ifndef NDEBUG bool ret = #endif GetUserDataDirectory(&user_data_dir, nullptr); assert(ret); https://codereview.chromium.org/2487783002/diff/400001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2487783002/diff/400001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_main.cc:40: wcscpy_s(user_data_dir, user_data_dir_length, user_data_dir_str.c_str()); looks like this is counting on the crt's parameter validation to kick in if the destination buffers are somehow too small. could you take a look at base/win/process_startup_helper.cc to be sure that it will do the right thing (comments there still reference breakpad -- is it correct)?
Thanks for slogging through this with me Greg. https://codereview.chromium.org/2487783002/diff/400001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2487783002/diff/400001/chrome/install_static/... chrome/install_static/install_util.cc:442: assert(ret); On 2016/11/23 09:47:37, grt (UTC plus 1) wrote: > the strict compiler is complaining about |ret| being unused in NDEBUG builds. > yikes. is there something more attractive than: > #ifndef NDEBUG > bool ret = > #endif > GetUserDataDirectory(&user_data_dir, nullptr); > assert(ret); There's ignore_result in base/ and USE() in v8, but nothing in install_static I could see. Added IgnoreUnused() to install_util.h. https://codereview.chromium.org/2487783002/diff/400001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2487783002/diff/400001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_main.cc:40: wcscpy_s(user_data_dir, user_data_dir_length, user_data_dir_str.c_str()); On 2016/11/23 09:47:37, grt (UTC plus 1) wrote: > looks like this is counting on the crt's parameter validation to kick in if the > destination buffers are somehow too small. could you take a look at > base/win/process_startup_helper.cc to be sure that it will do the right thing > (comments there still reference breakpad -- is it correct)? I think process_startup_helper.cc is fine (other than the comment referring to breakpad). wcsncpy_s(..., _TRUNCATE) is probably more sensible here though, so switched to that.
More owners reviews please :) robertshield: chrome_elf/ pastarmovj: chrome/browser/policy/policy_path_parser_win.cc asvitkine: chrome/browser/metrics/chrome_metrics_service_client.cc Thanks!
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
chrome_metrics_service_client.cc LGTM
policy lgtm
On 2016/11/25 06:54:11, pastarmovj wrote: > policy lgtm quick question re: > We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. We're not adding any non-kernel32 dll exports to chrome_elf's imports right? That would break elf's early loading premise. Looks like https://codereview.chromium.org/2487783002/diff/420001/chrome/install_static/... is using explicit loading which should avoid this, but want to double check. If we're not then LGTM with a request to update the CL description to mention that the reason we can get away with using non-kernel32 exports is that "the code will not execute until after the DllMain of chrome_elf has completed AND we're doing explicit loading with LoadLibrary".
Description was changed from ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. (Regarding the future thread-based spawning, ref: https://blogs.msdn.microsoft.com/oldnewthing/20070904-00/?p=25283/ which explains why the creation of the thread with which we don't synchronize is OK, because DllMain calls are serialized. But we can discuss that more in https://codereview.chromium.org/2475863004/ too. ) BUG=565446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed, and we load the used functions via GetProcAddress(), not by import lib. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. (Regarding the future thread-based spawning, ref: https://blogs.msdn.microsoft.com/oldnewthing/20070904-00/?p=25283/ which explains why the creation of the thread with which we don't synchronize is OK, because DllMain calls are serialized. But we can discuss that more in https://codereview.chromium.org/2475863004/ too. ) BUG=565446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
On 2016/11/25 07:51:58, robertshield_slow_reviews wrote: > On 2016/11/25 06:54:11, pastarmovj wrote: > > policy lgtm > > quick question re: > > > We do not need to completely remove the usage of DLLs other than > kernel32 in the code despite the code being used in chrome_elf, as in > all cases, it will not execute until after the DllMain() of chrome_elf > has completed. > > We're not adding any non-kernel32 dll exports to chrome_elf's imports right? > That would break elf's early loading premise. > > Looks like > https://codereview.chromium.org/2487783002/diff/420001/chrome/install_static/... > is using explicit loading which should avoid this, but want to double check. > > If we're not then LGTM with a request to update the CL description to mention > that the reason we can get away with using non-kernel32 exports is that "the > code will not execute until after the DllMain of chrome_elf has completed AND > we're doing explicit loading with LoadLibrary". Yes, that's right. Updated description.
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2487783002/#ps420001 (title: "warning, wcsncpy_s")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(LGTM)
On 2016/11/28 18:17:52, scottmg (slow on 25nov) wrote: > On 2016/11/25 07:51:58, robertshield_slow_reviews wrote: > > On 2016/11/25 06:54:11, pastarmovj wrote: > > > policy lgtm > > > > quick question re: > > > > > We do not need to completely remove the usage of DLLs other than > > kernel32 in the code despite the code being used in chrome_elf, as in > > all cases, it will not execute until after the DllMain() of chrome_elf > > has completed. > > > > We're not adding any non-kernel32 dll exports to chrome_elf's imports right? > > That would break elf's early loading premise. > > > > Looks like > > > https://codereview.chromium.org/2487783002/diff/420001/chrome/install_static/... > > is using explicit loading which should avoid this, but want to double check. > > > > If we're not then LGTM with a request to update the CL description to mention > > that the reason we can get away with using non-kernel32 exports is that "the > > code will not execute until after the DllMain of chrome_elf has completed AND > > we're doing explicit loading with LoadLibrary". > > Yes, that's right. Updated description. For completeness, should have noted this -- with this CL, this is the imports for a release chrome_elf: c:\src\cr\src>dumpbin /imports out\release\chrome_elf.dll | grep "Section contains\|\.dll" Dump of file out\release\chrome_elf.dll Section contains the following imports: KERNEL32.dll VERSION.dll Section contains the following delay load imports: ADVAPI32.dll dbghelp.dll WINMM.dll RPCRT4.dll (I guess I could have delayloaded the policy stuff instead, but feels a bit too subtle and easy to break accidentally.)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #21 (id:440001) has been deleted
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #21 (id:460001) has been deleted
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/11/28 20:19:29, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) [[[ I'm getting odd failures from swarming runs in SHGetSpecialFolderPath causing failures in the policy_path_parser code that I mostly just relocated... e.g. https://chromium-swarm.appspot.com/task?id=32c648287f41ca10&refresh=10&show_r... where it returns GetLastError() == "An attempt was made to reference a token that does not exist." (or possibly SHGetSpecialFolderPath doesn't set last error, but in any case, it's unexpectedly failing trying to retrieve the Windows directory.) Still slowly debugging, but just in case someone knows what's going on... ]]]
On 2016/11/29 00:33:08, scottmg wrote: > On 2016/11/28 20:19:29, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > [[[ > > I'm getting odd failures from swarming runs in SHGetSpecialFolderPath causing > failures in the policy_path_parser code that I mostly just relocated... > > e.g. > https://chromium-swarm.appspot.com/task?id=32c648287f41ca10&refresh=10&show_r... > where it returns GetLastError() == "An attempt was made to reference a token > that does not exist." (or possibly SHGetSpecialFolderPath doesn't set last > error, but in any case, it's unexpectedly failing trying to retrieve the Windows > directory.) > > Still slowly debugging, but just in case someone knows what's going on... Since the otherwise-identical test passes for HKCU https://chromium-swarm.appspot.com/task?id=32c65a69e4bf4110&refresh=10&show_r..., I guess it must be some sort of access-based restriction that swarming does. I think I'll just remove that test then, as it's not too important. Reading from HKLM in general is tested by another test, and the substitution of variables is tested by the HKCU one. > ]]]
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, asvitkine@chromium.org, robertshield@chromium.org, pastarmovj@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2487783002/#ps500001 (title: "install_static for swarming")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1480380958705460,
"parent_rev": "8625a0a73ff1511899d3fcae9f3b0eccdf367d0b", "commit_rev":
"7a2c4c0c73fc7330a11fb74b26a3286bb38a2077"}
Message was sent while issue was closed.
Committed patchset #22 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed, and we load the used functions via GetProcAddress(), not by import lib. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. (Regarding the future thread-based spawning, ref: https://blogs.msdn.microsoft.com/oldnewthing/20070904-00/?p=25283/ which explains why the creation of the thread with which we don't synchronize is OK, because DllMain calls are serialized. But we can discuss that more in https://codereview.chromium.org/2475863004/ too. ) BUG=565446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Make Crashpad use the user data dir, rather than always default location This puts the crash database in the user data directory, rather than in the global shared one. In order to accomplish this, the variable expansion from policy needs to be moved to be suitable for use in chrome_elf. This code handles variable expansions in a registry key that can be set to override the user data dir (in either HKLM or HKCU). We do not need to completely remove the usage of DLLs other than kernel32 in the code despite the code being used in chrome_elf, as in all cases, it will not execute until after the DllMain() of chrome_elf has completed, and we load the used functions via GetProcAddress(), not by import lib. Currently, chrome_elf is signalled from WinMain() of chrome to start crash reporting. In the near future it will be started earlier from a background thread spawned in DllMain(). However, that thread cannot execute until DllMain has completed, so it's then safe to cause the other DLL loads that the variable expansion requires. (Regarding the future thread-based spawning, ref: https://blogs.msdn.microsoft.com/oldnewthing/20070904-00/?p=25283/ which explains why the creation of the thread with which we don't synchronize is OK, because DllMain calls are serialized. But we can discuss that more in https://codereview.chromium.org/2475863004/ too. ) BUG=565446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/7433a2b305bbfcb8c469cddab9a1a3ab2c5b70d1 Cr-Commit-Position: refs/heads/master@{#434855} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/7433a2b305bbfcb8c469cddab9a1a3ab2c5b70d1 Cr-Commit-Position: refs/heads/master@{#434855}
Message was sent while issue was closed.
On 2016/11/29 00:33:08, scottmg wrote: > On 2016/11/28 20:19:29, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > [[[ > > I'm getting odd failures from swarming runs in SHGetSpecialFolderPath causing > failures in the policy_path_parser code that I mostly just relocated... > > e.g. > https://chromium-swarm.appspot.com/task?id=32c648287f41ca10&refresh=10&show_r... > where it returns GetLastError() == "An attempt was made to reference a token > that does not exist." (or possibly SHGetSpecialFolderPath doesn't set last > error, but in any case, it's unexpectedly failing trying to retrieve the Windows > directory.) > > Still slowly debugging, but just in case someone knows what's going on... > > ]]] Two thoughts: 1. Is this policy path stuff happening in (sandboxed) child procs? If so, this is problematic (see, for example, https://crbug.com/502416). If this is the case, is it necessary? I thought we passed the user data dir to child procs explicitly. 2. I forgot what the second one was. Hopefully the first one is the problem. :-)
Message was sent while issue was closed.
On 2016/11/29 08:40:50, grt (UTC plus 1) wrote: > On 2016/11/29 00:33:08, scottmg wrote: > > On 2016/11/28 20:19:29, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > > > [[[ > > > > I'm getting odd failures from swarming runs in SHGetSpecialFolderPath causing > > failures in the policy_path_parser code that I mostly just relocated... > > > > e.g. > > > https://chromium-swarm.appspot.com/task?id=32c648287f41ca10&refresh=10&show_r... > > where it returns GetLastError() == "An attempt was made to reference a token > > that does not exist." (or possibly SHGetSpecialFolderPath doesn't set last > > error, but in any case, it's unexpectedly failing trying to retrieve the > Windows > > directory.) > > > > Still slowly debugging, but just in case someone knows what's going on... > > > > ]]] > > Two thoughts: > 1. Is this policy path stuff happening in (sandboxed) child procs? If so, this > is problematic (see, for example, https://crbug.com/502416). If this is the > case, is it necessary? I thought we passed the user data dir to child procs > explicitly. It would happen in sandboxed processes pre-lockdown, yeah. I believe that would have happened before too (as --user-data-dir only got appended when an invalid directory was used and it had to be overridden to a valid one), but I'll confirm that now. > 2. I forgot what the second one was. Hopefully the first one is the problem. :-) It seems to be working OK in normal usage (i.e. chrome) and it tests locally, just not for HKLM on swarming. Maybe there's some variable I overlooked in local testing though.
Message was sent while issue was closed.
On 2016/11/29 16:20:38, scottmg wrote: > On 2016/11/29 08:40:50, grt (UTC plus 1) wrote: > > On 2016/11/29 00:33:08, scottmg wrote: > > > On 2016/11/28 20:19:29, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > > > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > > > > > [[[ > > > > > > I'm getting odd failures from swarming runs in SHGetSpecialFolderPath > causing > > > failures in the policy_path_parser code that I mostly just relocated... > > > > > > e.g. > > > > > > https://chromium-swarm.appspot.com/task?id=32c648287f41ca10&refresh=10&show_r... > > > where it returns GetLastError() == "An attempt was made to reference a token > > > that does not exist." (or possibly SHGetSpecialFolderPath doesn't set last > > > error, but in any case, it's unexpectedly failing trying to retrieve the > > Windows > > > directory.) > > > > > > Still slowly debugging, but just in case someone knows what's going on... > > > > > > ]]] > > > > Two thoughts: > > 1. Is this policy path stuff happening in (sandboxed) child procs? If so, this > > is problematic (see, for example, https://crbug.com/502416). If this is the > > case, is it necessary? I thought we passed the user data dir to child procs > > explicitly. > > It would happen in sandboxed processes pre-lockdown, yeah. I believe that would > have happened before too (as --user-data-dir only got appended when an invalid > directory was used and it had to be overridden to a valid one), but I'll confirm > that now. The only things that appear to get access to the profile dir are browser, service, nacl-broker, and nacl-loader, per chrome::ProcessNeedsProfileDir(). Others don't even call InitializeUserDataDir() on Windows. I confirmed manually that --type=service correctly retrieves the user data dir from HKLM registry key and is able to correctly expand ${windows} in that context. I'm less certain about the NaCl process types, as ... um, enable_nacl=false. A wild guess is that they're not likely sandboxed, but I'm not actually sure about that. Working on confirming that one way or the other now. > > > 2. I forgot what the second one was. Hopefully the first one is the problem. > :-) > > It seems to be working OK in normal usage (i.e. chrome) and it tests locally, > just not for HKLM on swarming. Maybe there's some variable I overlooked in local > testing though.
Message was sent while issue was closed.
On 2016/11/29 18:39:00, scottmg wrote: > On 2016/11/29 16:20:38, scottmg wrote: > > On 2016/11/29 08:40:50, grt (UTC plus 1) wrote: > > > On 2016/11/29 00:33:08, scottmg wrote: > > > > On 2016/11/28 20:19:29, commit-bot: I haz the power wrote: > > > > > Try jobs failed on following builders: > > > > > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > > > > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > > > > > > > [[[ > > > > > > > > I'm getting odd failures from swarming runs in SHGetSpecialFolderPath > > causing > > > > failures in the policy_path_parser code that I mostly just relocated... > > > > > > > > e.g. > > > > > > > > > > https://chromium-swarm.appspot.com/task?id=32c648287f41ca10&refresh=10&show_r... > > > > where it returns GetLastError() == "An attempt was made to reference a > token > > > > that does not exist." (or possibly SHGetSpecialFolderPath doesn't set last > > > > error, but in any case, it's unexpectedly failing trying to retrieve the > > > Windows > > > > directory.) > > > > > > > > Still slowly debugging, but just in case someone knows what's going on... > > > > > > > > ]]] > > > > > > Two thoughts: > > > 1. Is this policy path stuff happening in (sandboxed) child procs? If so, > this > > > is problematic (see, for example, https://crbug.com/502416). If this is the > > > case, is it necessary? I thought we passed the user data dir to child procs > > > explicitly. > > > > It would happen in sandboxed processes pre-lockdown, yeah. I believe that > would > > have happened before too (as --user-data-dir only got appended when an invalid > > directory was used and it had to be overridden to a valid one), but I'll > confirm > > that now. > > The only things that appear to get access to the profile dir are browser, > service, nacl-broker, and nacl-loader, per chrome::ProcessNeedsProfileDir(). > Others don't even call InitializeUserDataDir() on Windows. > > I confirmed manually that --type=service correctly retrieves the user data dir > from HKLM registry key and is able to correctly expand ${windows} in that > context. > > I'm less certain about the NaCl process types, as ... um, enable_nacl=false. A > wild guess is that they're not likely sandboxed, but I'm not actually sure about > that. Working on confirming that one way or the other now. Gurgle. So, ... not sure nacl passes --user-data-dir through to the subprocess https://cs.chromium.org/chromium/src/components/nacl/browser/nacl_process_hos... And sometimes (when on wow64) it's actually running nacl64.exe, not chrome.exe. I'm still trying to actually find some nacl content that I can try to run. ... Oh, I remember looking into a crash reported in https://bugs.chromium.org/p/chromium/issues/detail?id=467025#c28 that was NaCl x86! :) ... Debugging has so far proved not overly effective. nacl64.exe when run as --nacl-broker doesn't pass --user-data-dir to nacl-loader. So I guess loader is not a problem. Looking way back at https://chromiumcodereview.appspot.com/10390003/ it seems it has something to do with user-data-dir on network shares. I tried setting --user-data-dir to a network share and running NaCl wow64 content and I believe it's working as expected. I don't really see NaCl (broker) really doing anything with the user data dir though, so I'm not sure what it was actually needed for. It's possible it's not needed any longer. Julian, do you happen to remember from that CL long ago? Since it's getting passed in on the command line, and nacl64 doesn't use chrome_elf anyway, I'm going to somewhat cautiously declare it "fine". > > > > > 2. I forgot what the second one was. Hopefully the first one is the problem. > > :-) > > > > It seems to be working OK in normal usage (i.e. chrome) and it tests locally, > > just not for HKLM on swarming. Maybe there's some variable I overlooked in > local > > testing though.
Message was sent while issue was closed.
A revert of this CL (patchset #22 id:500001) has been created in https://codereview.chromium.org/2549593002/ by scottmg@chromium.org. The reason for reverting is: Reverting for now until https://codereview.chromium.org/2543503003/ lands (making the install_static command line parser smarter), and then I'll reland this unchanged. BUG=670012 .
Message was sent while issue was closed.
On 2016/12/01 18:16:18, scottmg wrote: > A revert of this CL (patchset #22 id:500001) has been created in > https://codereview.chromium.org/2549593002/ by mailto:scottmg@chromium.org. > > The reason for reverting is: Reverting for now until > https://codereview.chromium.org/2543503003/ lands (making the install_static > command line parser smarter), and then I'll reland this unchanged. > > BUG=670012 > > > . Hrm, doesn't revert cleanly. Maybe I'll just "hurry" instead after all.
Message was sent while issue was closed.
tapted@chromium.org changed reviewers: + tapted@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2487783002/diff/500001/chrome/install_static/... File chrome/install_static/user_data_dir_win_unittest.cc (right): https://codereview.chromium.org/2487783002/diff/500001/chrome/install_static/... chrome/install_static/user_data_dir_win_unittest.cc:120: rv = key1.WriteValue(kUserDataDirRegistryKey, L"111"); Some tests have started failing with output like ComponentCloudPolicyTest.SignOutAndBackIn (run #1): [ RUN ] ComponentCloudPolicyTest.SignOutAndBackIn sending server_data: {"host": "127.0.0.1", "port": 50365} (36 bytes) [6020:1352:0205/030741.381:WARNING:policy_test_utils.cc(52)] There are pre-existing policies in this machine: { "MetricsReportingEnabled": false, "UserDataDir": "111" } c:\c\win\src\chromerowser\policy\cloud\component_cloud_policy_browsertest.cc(125): error: Value of: PolicyServiceIsEmpty(g_browser_process->policy_service()) Actual: false Expected: true Pre-existing policies in this machine will make this test fail. [ FAILED ] ComponentCloudPolicyTest.SignOutAndBackIn, where TypeParam = and GetParam() = (849 ms) That "111" string seems to come from here. Filed http://crbug.com/688878 . I'm going to start by disabling tests here that appear to be writing to global persistent state. #sheriff
Message was sent while issue was closed.
On 2016/11/29 20:11:09, scottmg wrote: > On 2016/11/29 18:39:00, scottmg wrote: > > On 2016/11/29 16:20:38, scottmg wrote: > > > On 2016/11/29 08:40:50, grt (UTC plus 1) wrote: > > > > On 2016/11/29 00:33:08, scottmg wrote: > > > > > On 2016/11/28 20:19:29, commit-bot: I haz the power wrote: > > > > > > Try jobs failed on following builders: > > > > > > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > > > > > win_chromium_x64_rel_ng on master.tryserver.chromium.win > (JOB_FAILED, > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > > > > > > > > > [[[ > > > > > > > > > > I'm getting odd failures from swarming runs in SHGetSpecialFolderPath > > > causing > > > > > failures in the policy_path_parser code that I mostly just relocated... > > > > > > > > > > e.g. > > > > > > > > > > > > > > > https://chromium-swarm.appspot.com/task?id=32c648287f41ca10&refresh=10&show_r... > > > > > where it returns GetLastError() == "An attempt was made to reference a > > token > > > > > that does not exist." (or possibly SHGetSpecialFolderPath doesn't set > last > > > > > error, but in any case, it's unexpectedly failing trying to retrieve the > > > > Windows > > > > > directory.) > > > > > > > > > > Still slowly debugging, but just in case someone knows what's going > on... > > > > > > > > > > ]]] > > > > > > > > Two thoughts: > > > > 1. Is this policy path stuff happening in (sandboxed) child procs? If so, > > this > > > > is problematic (see, for example, https://crbug.com/502416). If this is > the > > > > case, is it necessary? I thought we passed the user data dir to child > procs > > > > explicitly. > > > > > > It would happen in sandboxed processes pre-lockdown, yeah. I believe that > > would > > > have happened before too (as --user-data-dir only got appended when an > invalid > > > directory was used and it had to be overridden to a valid one), but I'll > > confirm > > > that now. > > > > The only things that appear to get access to the profile dir are browser, > > service, nacl-broker, and nacl-loader, per chrome::ProcessNeedsProfileDir(). > > Others don't even call InitializeUserDataDir() on Windows. > > > > I confirmed manually that --type=service correctly retrieves the user data dir > > from HKLM registry key and is able to correctly expand ${windows} in that > > context. > > > > I'm less certain about the NaCl process types, as ... um, enable_nacl=false. A > > wild guess is that they're not likely sandboxed, but I'm not actually sure > about > > that. Working on confirming that one way or the other now. > > Gurgle. So, ... not sure nacl passes --user-data-dir through to the subprocess > https://cs.chromium.org/chromium/src/components/nacl/browser/nacl_process_hos... > And sometimes (when on wow64) it's actually running nacl64.exe, not chrome.exe. > > I'm still trying to actually find some nacl content that I can try to run. > > ... > > Oh, I remember looking into a crash reported in > https://bugs.chromium.org/p/chromium/issues/detail?id=467025#c28 that was NaCl > x86! :) > > ... > > Debugging has so far proved not overly effective. > > nacl64.exe when run as --nacl-broker doesn't pass --user-data-dir to > nacl-loader. So I guess loader is not a problem. > > Looking way back at https://chromiumcodereview.appspot.com/10390003/?_ga=1.96993942.806489194.146... it seems it > has something to do with user-data-dir on network shares. > > I tried setting --user-data-dir to a network share and running NaCl wow64 > content and I believe it's working as expected. I don't really see NaCl (broker) > really doing anything with the user data dir though, so I'm not sure what it was > actually needed for. It's possible it's not needed any longer. Julian, do you > happen to remember from that CL long ago? The issue back then was that a sandboxed process can not get full access network filesystems with its stripped process token but there were many sandboxed processes which would try to create the user data directory and fail and terminate. So I had to come up with a way to decide which process was not ok to do that and prevent it from doing so. If you have concerns any of this might have regressed now you can test it by sharing a folder from another computer. Mount it locally on yours and run with --user-data-dir pointing to the shared mount point and see if Chrome will run ok. I am not sure anymore if sharing a folder on your own machine and mounting it locally was good enough. > > Since it's getting passed in on the command line, and nacl64 doesn't use > chrome_elf anyway, I'm going to somewhat cautiously declare it "fine". > > > > > > > > > 2. I forgot what the second one was. Hopefully the first one is the > problem. > > > :-) > > > > > > It seems to be working OK in normal usage (i.e. chrome) and it tests > locally, > > > just not for HKLM on swarming. Maybe there's some variable I overlooked in > > local > > > testing though. |
