Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507753002:
Install the chrome event log provider together with the browser. (Closed)
|
Created:
4 years, 1 month ago by pastarmovj Modified:
3 years, 11 months ago CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, wfh+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInstall the chrome event log provider together with the browser.
The dll will always be present but only registered when running
a system install because we need access to the LOCAL_MACHINE hive.
BUG=642115
TEST=setup_util_unittest,mini_installer,manual
Committed: https://crrev.com/e064666e44e4f3821559ce188cfaf4d696dd4668
Cr-Commit-Position: refs/heads/master@{#439788}
Patch Set 1 : . #
Total comments: 20
Patch Set 2 : Address comments. #
Total comments: 16
Patch Set 3 : Address comments. #Patch Set 4 : Add tests. #Patch Set 5 : Fix wrong append call. #
Total comments: 20
Patch Set 6 : Fix formatting. #
Total comments: 8
Patch Set 7 : Address comments. #
Total comments: 2
Patch Set 8 : Make the provider registry key track the product branding. #
Total comments: 10
Patch Set 9 : Address comments. #
Total comments: 2
Patch Set 10 : Clean-up InstallFullName. #Patch Set 11 : Fix prop files. #Patch Set 12 : Fix RegisterEventLogProvider unit test. #
Total comments: 1
Messages
Total messages: 46 (17 generated)
Patchset #1 (id:1) has been deleted
pastarmovj@chromium.org changed reviewers: + grt@chromium.org
Hi Greg, this is my attempt at adding the eventlog provider to the installer. Please let me know if this is the right way to do this since I have never touched this code before. I have to test this more extensively as well before I vet it done as well. Btw I tested the size effect of this change on the installer and here are the numbers: - Without this CL the mini_installer.exe file size is: 44,424bytes. - With the code but without the file included: 44,407bytes. - With the code and the file: 44,419 bytes. It seems that this file causes some alignment changes which even reduce the size but without relying on this effect I think it is safer to say that we have no measurable impact on the installer size. Do you think this way of measuring the impact is right? Thanks! Julian
grt@chromium.org changed reviewers: + sebmarchand@chromium.org
Looks pretty good. I think the new file should also be mentioned in chrome/tools/build/win/FILES.cfg so that it ends up in the default archived .zip file. I broke the build for a week or so the last time I edited that file, so +sebmarchand to comment on the right way to add to it. :-) Seb: please redirect to someone else if you'd like. Maybe Bruce? I don't think we need this new dll to be put on the symbol server since it doesn't contain code, but I think it does belong in the .zip archive. Thanks! https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:145: void RegisterChromesEventlogProvider(const base::FilePath& chrome_exe) { nit: RegisterChromeEventLogProvider or maybe just RegisterEventLogProvider is there is only Chrome these days. it's your call. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:145: void RegisterChromesEventlogProvider(const base::FilePath& chrome_exe) { i think it makes sense to move this into setup_util.{cc,h} along with a function to unregister the provider (called by the code in uninstall.cc) so that the code for these two operations can be physically close should any changes ever be needed to both. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:150: VLOG(1) << "Registering Chrome's event log provider at " << reg_path; nit: remove extra space in log message https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:151: std::unique_ptr<WorkItem> work_item(WorkItem::CreateCreateRegKeyWorkItem( rather than creating and using individual items like this, create a WorkItemList, add the items you care about to it, then: if (!work_item_list->Do()) work_item_list->Rollback(); this will ensure that any failure will return the state of the machine to the way it was before the first mutation. if certain items should not result in failure of the whole operation, you can call set_best_effort(true) on them. if some items shouldn't be rolled back even in case of failure, you can call set_rollback_enabled(false) on them. my guess is that the default behavior is probably okay for this https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:179: const wchar_t* const file_keys[] = { nit: static constexpr const wchar_t* kFileKeys[] = {...} https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:184: for (int i = 0; i < arraysize(file_keys); ++i) { nit: for (const wchar_t* file_key : file_keys) { https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... File chrome/installer/setup/setup_constants.h (right): https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_constants.h:22: extern const wchar_t kEventlogProvidersRegPath[]; if you follow my request to move both bits of code into setup_util, i think these constants should move there, too. unless they need to be referenced by other code, i don't see a benefit to having them out here like this. for any constant that is used only once, i think it's okay to inline the literal. if it's used more than once in the same function, "static constexpr" at function scope is good. if it's used in more than one function, "constexpr" in an unnamed namespace at the top of the .cc file is good. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/uninstall.cc:1228: base::string16 media_player_reg_path(installer::kMediaPlayerRegPath); personal preference: i would keep this as reg_path and reuse the same string instance below to reduce stack and heap usage. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/uninstall.cc:1233: // Remove the event log provider registration as we are going to delete nit: blank line before this
Adding this file to FILES.cfg should be enough! https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/mini_i... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/chrome.release:28: kasko.dll: %(VersionDir)s\ Could you fix the order here while you're at it? (kasko should go after icudtl.dat).
On 2016/11/16 16:46:29, Sébastien Marchand wrote: > Adding this file to FILES.cfg should be enough! Is there an existing stanza in FILES.cfg that you suggest Julian copies? One for a file that just ends up in the normal chrome-win32.zip file. Or do you think we even need to archive it separately like that (outside of mini_installer.exe)?
sebmarchand@chromium.org changed reviewers: + mmoss@chromium.org
I think that something like this should be enough:
{
'filename': 'eventlog_provider.dll',
'buildtype': ['dev', 'official'],
'filegroup': ['default'],
},
but I'm not 100% sure, +mmoss@ to confirm this.
On 2016/11/16 17:08:30, Sébastien Marchand wrote:
> I think that something like this should be enough:
> {
> 'filename': 'eventlog_provider.dll',
> 'buildtype': ['dev', 'official'],
> 'filegroup': ['default'],
> },
>
> but I'm not 100% sure, +mmoss@ to confirm this.
That sounds right.
Thanks for all the great comments! :) Btw what would be your suggestion for testing this CL along running the setup.exe from the branded build and seeing that it installs and uninstalls well both as system install and as user install? https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/mini_i... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/chrome.release:28: kasko.dll: %(VersionDir)s\ On 2016/11/16 16:46:29, Sébastien Marchand wrote: > Could you fix the order here while you're at it? (kasko should go after > icudtl.dat). Done. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:145: void RegisterChromesEventlogProvider(const base::FilePath& chrome_exe) { On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > nit: RegisterChromeEventLogProvider or maybe just RegisterEventLogProvider is > there is only Chrome these days. it's your call. Done. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:145: void RegisterChromesEventlogProvider(const base::FilePath& chrome_exe) { On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > i think it makes sense to move this into setup_util.{cc,h} along with a function > to unregister the provider (called by the code in uninstall.cc) so that the code > for these two operations can be physically close should any changes ever be > needed to both. Done. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:150: VLOG(1) << "Registering Chrome's event log provider at " << reg_path; On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > nit: remove extra space in log message Done. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:151: std::unique_ptr<WorkItem> work_item(WorkItem::CreateCreateRegKeyWorkItem( On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > rather than creating and using individual items like this, create a > WorkItemList, add the items you care about to it, then: > if (!work_item_list->Do()) > work_item_list->Rollback(); > this will ensure that any failure will return the state of the machine to the > way it was before the first mutation. > > if certain items should not result in failure of the whole operation, you can > call set_best_effort(true) on them. if some items shouldn't be rolled back even > in case of failure, you can call set_rollback_enabled(false) on them. my guess > is that the default behavior is probably okay for this Done. You are right that those make sense only as a whole so the default transaction logic is just right. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:179: const wchar_t* const file_keys[] = { On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > nit: static constexpr const wchar_t* kFileKeys[] = {...} Done. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:184: for (int i = 0; i < arraysize(file_keys); ++i) { On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > nit: for (const wchar_t* file_key : file_keys) { Done. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... File chrome/installer/setup/setup_constants.h (right): https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_constants.h:22: extern const wchar_t kEventlogProvidersRegPath[]; On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > if you follow my request to move both bits of code into setup_util, i think > these constants should move there, too. unless they need to be referenced by > other code, i don't see a benefit to having them out here like this. for any > constant that is used only once, i think it's okay to inline the literal. if > it's used more than once in the same function, "static constexpr" at function > scope is good. if it's used in more than one function, "constexpr" in an unnamed > namespace at the top of the .cc file is good. Done. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/uninstall.cc:1228: base::string16 media_player_reg_path(installer::kMediaPlayerRegPath); On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > personal preference: i would keep this as reg_path and reuse the same string > instance below to reduce stack and heap usage. Done. https://codereview.chromium.org/2507753002/diff/20001/chrome/installer/setup/... chrome/installer/setup/uninstall.cc:1233: // Remove the event log provider registration as we are going to delete On 2016/11/16 15:04:44, grt (UTC plus 1) wrote: > nit: blank line before this Done.
Looking pretty good. A unit-test would be a good start. Please also add to chrome/test/mini_installer/config/chrome_system_installed.prop and chrome_system_updated.prop so that they verify the new registry keys. To test manually, run "mini_installer.exe --system-level" from an elevated cmd prompt. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:54: constexpr wchar_t kEventLogProvidersRegPath[] = nit: kEventLogProviderRegPath https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:55: L"\\SYSTEM\\CurrentControlSet\\Services\\EventLog\\Application\\chrome"; can "chrome" be any string, or does it need to be "chrome"? as-is, multiple installs will collide in this registry key (i.e., stable, dev, beta), as will Chromium. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:681: work_item_list->AddSetRegValueWorkItem( nit: could you put a simple comment above each of these explaining what they accomplish? what does "7" mean? https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:686: provider.Append(FILE_PATH_LITERAL("eventlog_provider.dll")); the provider is in the version directory, so you need to Append the version # being installed, no? i think all callers of RegisterChromeOnMachine know the version, so you could pass it on down into here. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:701: if (!work_item_list->Do()) are you intentionally not calling Rollback? if so, please explain why in the comment above. or is that what you're trying to convey in the comment? https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_util.h (right): https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.h:148: void RegisterEventLogProvider(const base::FilePath& chrome_exe); please add a unittest for these functions. in the test, use registry_util::RegistryOverrideManager so that the registry modifications don't tamper with the meaty bits of the registry. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.h:150: // Register Chrome's EventLog message provider dll. nit: fix comment
https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:55: L"\\SYSTEM\\CurrentControlSet\\Services\\EventLog\\Application\\chrome"; On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > can "chrome" be any string, or does it need to be "chrome"? as-is, multiple > installs will collide in this registry key (i.e., stable, dev, beta), as will > Chromium. Maybe use BrowserDistribution::GetBaseAppName() for the last component of this path? It returns something different for each install.
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Thanks for the hints. I made some tests which actually caught a few issues! :) https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:54: constexpr wchar_t kEventLogProvidersRegPath[] = On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > nit: kEventLogProviderRegPath Done. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:55: L"\\SYSTEM\\CurrentControlSet\\Services\\EventLog\\Application\\chrome"; On 2016/11/18 12:10:34, grt (UTC plus 1) wrote: > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > can "chrome" be any string, or does it need to be "chrome"? as-is, multiple > > installs will collide in this registry key (i.e., stable, dev, beta), as will > > Chromium. > > Maybe use BrowserDistribution::GetBaseAppName() for the last component of this > path? It returns something different for each install. Unfortunately this has to coincide with the costant kEventSource defined here https://cs.chromium.org/chromium/src/base/syslog_logging.cc?rcl=0&l=28 which is in base and therefore would not be suitable to depend on other components. I think however this is not a big deal since worst case a degraded UI experience in the Event Viewer will appear but no crashes or anything will happen. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:681: work_item_list->AddSetRegValueWorkItem( On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > nit: could you put a simple comment above each of these explaining what they > accomplish? what does "7" mean? Done. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:686: provider.Append(FILE_PATH_LITERAL("eventlog_provider.dll")); On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > the provider is in the version directory, so you need to Append the version # > being installed, no? i think all callers of RegisterChromeOnMachine know the > version, so you could pass it on down into here. Done. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:701: if (!work_item_list->Do()) On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > are you intentionally not calling Rollback? if so, please explain why in the > comment above. or is that what you're trying to convey in the comment? No sorry I mistakenly assumed that the rollback is the default behavior but now I read what you were telling me is that I can change the default on a task by task basis not that it will magically happen for the list. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_util.h (right): https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.h:150: // Register Chrome's EventLog message provider dll. On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > nit: fix comment Done.
looking good. still some issues to be ironed out, but we're going in the right direction! https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:55: L"\\SYSTEM\\CurrentControlSet\\Services\\EventLog\\Application\\chrome"; On 2016/11/21 15:48:40, pastarmovj wrote: > On 2016/11/18 12:10:34, grt (UTC plus 1) wrote: > > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > > can "chrome" be any string, or does it need to be "chrome"? as-is, multiple > > > installs will collide in this registry key (i.e., stable, dev, beta), as > will > > > Chromium. > > > > Maybe use BrowserDistribution::GetBaseAppName() for the last component of this > > path? It returns something different for each install. > > Unfortunately this has to coincide with the costant kEventSource defined here > https://cs.chromium.org/chromium/src/base/syslog_logging.cc?rcl=0&l=28 which is > in base and therefore would not be suitable to depend on other components. I > think however this is not a big deal since worst case a degraded UI experience > in the Event Viewer will appear but no crashes or anything will happen. ah, this is pretty bad. any Chromium-based browser co-installed with Google Chrome will use Chrome's log provider dll. i think we can do better. you could introduce a function in logging:: to set the event source name, and then call it from within Chrome somewhere after Chrome figures out which channel it is. each Chrome channel should have its own event source name. it would be natural to add the event source name to install_static/install_constants.h, and then in chromium_install_modes.cc and google_chrome_install_modes.cc. you would then add an accessor for it to InstallDetails (a la app_guid()). InitChromeLogging could then set the event source name with something like: logging::SetEventSourceName(install_static::InstallDetails::Get().event_source_name()); this scales better across multiple distributions. the installer can't use this part of install_static until i land https://codereview.chromium.org/2459583002/, so you would have to be a little hacky in here for now. add a TODO(grt) to clean it up. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:455: // Try to register the event log provider for Chrome, but only if it is a nit: // Register the event log provider for system-level installs only, as it // requires admin privileges. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:458: RegisterEventLogProvider(installer_state.target_path().Append( nit: AppendASCII(version.GetString()) https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:670: void RegisterEventLogProvider(const base::FilePath& install_directory) { nit: either rename |install_directory| to |version_directory|, or have this take |target_path| and |version| as two args and compose them here. i'm leaning toward the latter since it hides an implementation detail from the caller. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:690: base::FilePath provider(install_directory); nit: base::FilePath provider( install_directory.Append(FILE_PATH_LITERAL("eventlog_provider.dll"); (or whatever git cl format says) and remove the next line https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:693: static const wchar_t* file_keys[] = { nit: static constexpr const wchar_t* kFileKeys[] = { https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:706: LOG(ERROR) << "Could not register Chrome's event log provider."; WorkitemList will emit a log message very similar to this. i propose you remove this line (and the braces around the "if" body) and insert something like: work_item_list->set_log_message("Register event log provider"); up around line 675 https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/setup_util_unittest.cc (right): https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:204: constexpr wchar_t registry_location[] = nit: static constexpr wchar_t kRegistryLocation[] = https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:208: ASSERT_TRUE(key.HasValue(L"CategoryCount")); nit: make these all EXPECT_TRUE so that the test will continue in case of failure. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:217: ASSERT_NE(ERROR_SUCCESS, this could be EXPECT_NE as well https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:1236: DeRegisterEventLogProvider(); could you try uninstalling while eventvwr is showing a Chrome event? i suspect it will make DeleteChromeFilesAndFolders return DELETE_FAILED, which would be unfortunate. if it does, we may need to move the provider dll into a temp dir and schedule it for delete after reboot.
On 2016/11/21 15:48:41, pastarmovj wrote: > Thanks for the hints. I made some tests which actually caught a few issues! :) > > https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... > File chrome/installer/setup/setup_util.cc (right): > > https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_util.cc:54: constexpr wchar_t > kEventLogProvidersRegPath[] = > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > nit: kEventLogProviderRegPath > > Done. > > https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_util.cc:55: > L"\\SYSTEM\\CurrentControlSet\\Services\\EventLog\\Application\\chrome"; > On 2016/11/18 12:10:34, grt (UTC plus 1) wrote: > > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > > can "chrome" be any string, or does it need to be "chrome"? as-is, multiple > > > installs will collide in this registry key (i.e., stable, dev, beta), as > will > > > Chromium. > > > > Maybe use BrowserDistribution::GetBaseAppName() for the last component of this > > path? It returns something different for each install. > > Unfortunately this has to coincide with the costant kEventSource defined here > https://cs.chromium.org/chromium/src/base/syslog_logging.cc?rcl=0&l=28 which is > in base and therefore would not be suitable to depend on other components. I > think however this is not a big deal since worst case a degraded UI experience > in the Event Viewer will appear but no crashes or anything will happen. > > https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_util.cc:681: > work_item_list->AddSetRegValueWorkItem( > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > nit: could you put a simple comment above each of these explaining what they > > accomplish? what does "7" mean? > > Done. > > https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_util.cc:686: > provider.Append(FILE_PATH_LITERAL("eventlog_provider.dll")); > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > the provider is in the version directory, so you need to Append the version # > > being installed, no? i think all callers of RegisterChromeOnMachine know the > > version, so you could pass it on down into here. > > Done. > > https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_util.cc:701: if (!work_item_list->Do()) > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > are you intentionally not calling Rollback? if so, please explain why in the > > comment above. or is that what you're trying to convey in the comment? > > No sorry I mistakenly assumed that the rollback is the default behavior but now > I read what you were telling me is that I can change the default on a task by > task basis not that it will magically happen for the list. > > https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... > File chrome/installer/setup/setup_util.h (right): > > https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_util.h:150: // Register Chrome's EventLog message > provider dll. > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > nit: fix comment > > Done. Manual verification in a VM was successful. Both installing and uninstalling do the expected respective actions. Automatic test coverage has been added too.
Description was changed from ========== Install the chrome event log provider together with the browser. The dll will always be present but only registered when running a system install because we need access to the LOCAL_MACHINE hive. BUG=642115 TEST=manual ========== to ========== Install the chrome event log provider together with the browser. The dll will always be present but only registered when running a system install because we need access to the LOCAL_MACHINE hive. BUG=642115 TEST=setup_util_unittest,mini_installer,manual ==========
On 2016/11/22 10:49:14, pastarmovj wrote: > Manual verification in a VM was successful. Both installing and uninstalling do > the expected respective actions. Automatic test coverage has been added too. Do you know what process loads the provider dll? Is it not eventvwr?
Patchset #6 (id:160001) has been deleted
PTAL. Thanks! PS. The dll is held open by a svchost process hosting the EventLog service. It is however released when the Event Viewer is closed. As discussed this can lead to partial file cleanup when the Event Viewer is open while the browser is uninstalled and http://crbug.com/668120 tracks the work needed to close that gap. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:455: // Try to register the event log provider for Chrome, but only if it is a On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > nit: > // Register the event log provider for system-level installs only, as it > // requires admin privileges. Done. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:458: RegisterEventLogProvider(installer_state.target_path().Append( On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > nit: AppendASCII(version.GetString()) Obsoleted after moving this to a parameter of RegisterEventLogProvider. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:670: void RegisterEventLogProvider(const base::FilePath& install_directory) { On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > nit: either rename |install_directory| to |version_directory|, or have this take > |target_path| and |version| as two args and compose them here. i'm leaning > toward the latter since it hides an implementation detail from the caller. Done. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:690: base::FilePath provider(install_directory); On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > nit: > base::FilePath provider( > install_directory.Append(FILE_PATH_LITERAL("eventlog_provider.dll"); > (or whatever git cl format says) and remove the next line Done. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:693: static const wchar_t* file_keys[] = { On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > nit: static constexpr const wchar_t* kFileKeys[] = { Done. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:706: LOG(ERROR) << "Could not register Chrome's event log provider."; On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > WorkitemList will emit a log message very similar to this. i propose you remove > this line (and the braces around the "if" body) and insert something like: > work_item_list->set_log_message("Register event log provider"); > up around line 675 Done. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/setup_util_unittest.cc (right): https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:204: constexpr wchar_t registry_location[] = On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > nit: static constexpr wchar_t kRegistryLocation[] = Done. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:208: ASSERT_TRUE(key.HasValue(L"CategoryCount")); On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > nit: make these all EXPECT_TRUE so that the test will continue in case of > failure. Done. Fell in the copy/paste trap here :) https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:217: ASSERT_NE(ERROR_SUCCESS, On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > this could be EXPECT_NE as well Done. https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2507753002/diff/140001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:1236: DeRegisterEventLogProvider(); On 2016/11/22 10:41:26, grt (UTC plus 1) wrote: > could you try uninstalling while eventvwr is showing a Chrome event? i suspect > it will make DeleteChromeFilesAndFolders return DELETE_FAILED, which would be > unfortunate. if it does, we may need to move the provider dll into a temp dir > and schedule it for delete after reboot. I checked that actually - the file is indeed held hostage of the event viewer while it is running. I thought we were already doing this for the whole contents of the versioned folder and therefore didn't really thought about doing extra work in this regard here. Can you please advice me what do I need to do in this regard?
Looks good. Just a few minor comments plus the name collision to iron out. https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_util.cc:55: L"\\SYSTEM\\CurrentControlSet\\Services\\EventLog\\Application\\chrome"; On 2016/11/22 10:41:25, grt (UTC plus 1) wrote: > On 2016/11/21 15:48:40, pastarmovj wrote: > > On 2016/11/18 12:10:34, grt (UTC plus 1) wrote: > > > On 2016/11/18 11:38:12, grt (UTC plus 1) wrote: > > > > can "chrome" be any string, or does it need to be "chrome"? as-is, > multiple > > > > installs will collide in this registry key (i.e., stable, dev, beta), as > > will > > > > Chromium. > > > > > > Maybe use BrowserDistribution::GetBaseAppName() for the last component of > this > > > path? It returns something different for each install. > > > > Unfortunately this has to coincide with the costant kEventSource defined here > > https://cs.chromium.org/chromium/src/base/syslog_logging.cc?rcl=0&l=28 which > is > > in base and therefore would not be suitable to depend on other components. I > > think however this is not a big deal since worst case a degraded UI experience > > in the Event Viewer will appear but no crashes or anything will happen. > > ah, this is pretty bad. any Chromium-based browser co-installed with Google > Chrome will use Chrome's log provider dll. Also anything else built using Chromium's base, which is a growing body of software (CEF and whatnot). > i think we can do better. you could > introduce a function in logging:: to set the event source name, and then call it > from within Chrome somewhere after Chrome figures out which channel it is. each > Chrome channel should have its own event source name. it would be natural to add > the event source name to install_static/install_constants.h, and then in > chromium_install_modes.cc and google_chrome_install_modes.cc. you would then add > an accessor for it to InstallDetails (a la app_guid()). InitChromeLogging could > then set the event source name with something like: > > > logging::SetEventSourceName(install_static::InstallDetails::Get().event_source_name()); > > this scales better across multiple distributions. the installer can't use this > part of install_static until i land https://codereview.chromium.org/2459583002/, > so you would have to be a little hacky in here for now. add a TODO(grt) to clean > it up. https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... chrome/installer/setup/install.cc:447: const base::FilePath chrome_exe( nit: move this back down to line 465 where it was before? https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... chrome/installer/setup/install.cc:459: version.GetString()); nit: pass |version| by constref rather than stringifying it here. https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:695: static constexpr const wchar_t* file_keys[] = { nit: kFileKeys https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/setup_util_unittest.cc (right): https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:210: EXPECT_TRUE(key.HasValue(L"CategoryMessageFile")); please add an expectation that the path was composed properly for these
Patchset #7 (id:200001) has been deleted
PTAL. I added a bug to track the investigation of how to make the branding go into the event log naming. https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... chrome/installer/setup/install.cc:447: const base::FilePath chrome_exe( On 2016/11/24 07:49:34, grt (UTC plus 1) wrote: > nit: move this back down to line 465 where it was before? Done. https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... chrome/installer/setup/install.cc:459: version.GetString()); On 2016/11/24 07:49:34, grt (UTC plus 1) wrote: > nit: pass |version| by constref rather than stringifying it here. Done. https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:695: static constexpr const wchar_t* file_keys[] = { On 2016/11/24 07:49:34, grt (UTC plus 1) wrote: > nit: kFileKeys Done. https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/setup_util_unittest.cc (right): https://codereview.chromium.org/2507753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:210: EXPECT_TRUE(key.HasValue(L"CategoryMessageFile")); On 2016/11/24 07:49:34, grt (UTC plus 1) wrote: > please add an expectation that the path was composed properly for these Done.
i think it's best to address the branding issue now. otherwise, you'll need to add extra code to detect and delete the old style of registration. i don't think it will be too difficult to do now. https://codereview.chromium.org/2507753002/diff/220001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2507753002/diff/220001/chrome/installer/setup... chrome/installer/setup/install.cc:454: if (installer_state.system_install()) { nit: you can get rid of these braces now. :-)
Now that we have per channel SYSLOG I think I am done with this CL. One question though in the light of https://bugs.chromium.org/p/chromium/issues/detail?id=671917 do I have to either make mini_installer depend on eventlog_provider (sounds more right) or add the eventlog_provider to the chrome_official... target too to get it on the build servers? https://codereview.chromium.org/2507753002/diff/220001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2507753002/diff/220001/chrome/installer/setup... chrome/installer/setup/install.cc:454: if (installer_state.system_install()) { On 2016/11/24 14:26:13, grt (UTC plus 1) wrote: > nit: you can get rid of these braces now. :-) Done.
On 2016/12/08 15:33:55, pastarmovj wrote: > Now that we have per channel SYSLOG I think I am done with this CL. > > One question though in the light of > https://bugs.chromium.org/p/chromium/issues/detail?id=671917 do I have to either > make mini_installer depend on eventlog_provider (sounds more right) or add the > eventlog_provider to the chrome_official... target too to get it on the build > servers? chrome_official_builder_no_unittests depends on mini_installer, so i think the right place to add a dep is in the archive() rule in the generate_mini_installer template in chrome/installer/mini_installer/BUILD.gn.
https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:36: #include "chrome/install_static/install_details.h" remove this for now https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:54: // Event log provider registry location and value names. nit: value names are not defined here https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:55: // TODO(http://crbug.com/668397): Make this string depend on the branding then remove this TODO? https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:676: reg_path.append(install_static::InstallDetails::Get().install_full_name()); this is ultimately the right thing, but it won't work until https://codereview.chromium.org/2459583002/ lands. for now, we'll need to hack something with a TODO. how about: // TODO(grt): use install_static::InstallDetails::Get().install_full_name() when // InstallDetails is initialized in the installer. base::string16 reg_path(kEventLogProvidersRegPath); #if defined(GOOGLE_CHROME_BUILD) reg_path += "Chrome"; if (InstallUtil::IsChromeSxSProcess()) reg_path += " SxS"; #else reg_path += "Chromium"; #endif https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:717: reg_path.append(install_static::InstallDetails::Get().install_full_name()); same as above. maybe put the ugly logic in a helper function in the unnamed namespace (AppendInstallFullName?)
https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:36: #include "chrome/install_static/install_details.h" On 2016/12/13 13:21:52, grt (UTC plus 1) wrote: > remove this for now Done. https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:54: // Event log provider registry location and value names. On 2016/12/13 13:21:52, grt (UTC plus 1) wrote: > nit: value names are not defined here Done. https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:55: // TODO(http://crbug.com/668397): Make this string depend on the branding then On 2016/12/13 13:21:52, grt (UTC plus 1) wrote: > remove this TODO? Done. https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:676: reg_path.append(install_static::InstallDetails::Get().install_full_name()); On 2016/12/13 13:21:52, grt (UTC plus 1) wrote: > this is ultimately the right thing, but it won't work until > https://codereview.chromium.org/2459583002/ lands. for now, we'll need to hack > something with a TODO. how about: > > // TODO(grt): use install_static::InstallDetails::Get().install_full_name() > when > // InstallDetails is initialized in the installer. > base::string16 reg_path(kEventLogProvidersRegPath); > #if defined(GOOGLE_CHROME_BUILD) > reg_path += "Chrome"; > if (InstallUtil::IsChromeSxSProcess()) > reg_path += " SxS"; > #else > reg_path += "Chromium"; > #endif Done. https://codereview.chromium.org/2507753002/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:717: reg_path.append(install_static::InstallDetails::Get().install_full_name()); On 2016/12/13 13:21:52, grt (UTC plus 1) wrote: > same as above. maybe put the ugly logic in a helper function in the unnamed > namespace (AppendInstallFullName?) Done.
lgtm w/ a nit https://codereview.chromium.org/2507753002/diff/260001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/260001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:60: base::string16 reg_path(kEventLogProvidersRegPath); remove
https://codereview.chromium.org/2507753002/diff/260001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2507753002/diff/260001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:60: base::string16 reg_path(kEventLogProvidersRegPath); On 2016/12/16 13:03:58, grt (UTC plus 1) wrote: > remove Done.
The CQ bit was checked by pastarmovj@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/2507753002/#ps280001 (title: "Clean-up InstallFullName.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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 pastarmovj@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/2507753002/#ps320001 (title: "Fix RegisterEventLogProvider unit test.")
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": 320001, "attempt_start_ts": 1482239192370500,
"parent_rev": "551ea82c87f73269c6f9115bb156c215af926ac5", "commit_rev":
"1c437630a8f474cd35b2531601b580b81fae449f"}
Message was sent while issue was closed.
Description was changed from ========== Install the chrome event log provider together with the browser. The dll will always be present but only registered when running a system install because we need access to the LOCAL_MACHINE hive. BUG=642115 TEST=setup_util_unittest,mini_installer,manual ========== to ========== Install the chrome event log provider together with the browser. The dll will always be present but only registered when running a system install because we need access to the LOCAL_MACHINE hive. BUG=642115 TEST=setup_util_unittest,mini_installer,manual Review-Url: https://codereview.chromium.org/2507753002 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Install the chrome event log provider together with the browser. The dll will always be present but only registered when running a system install because we need access to the LOCAL_MACHINE hive. BUG=642115 TEST=setup_util_unittest,mini_installer,manual Review-Url: https://codereview.chromium.org/2507753002 ========== to ========== Install the chrome event log provider together with the browser. The dll will always be present but only registered when running a system install because we need access to the LOCAL_MACHINE hive. BUG=642115 TEST=setup_util_unittest,mini_installer,manual Committed: https://crrev.com/e064666e44e4f3821559ce188cfaf4d696dd4668 Cr-Commit-Position: refs/heads/master@{#439788} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e064666e44e4f3821559ce188cfaf4d696dd4668 Cr-Commit-Position: refs/heads/master@{#439788}
Message was sent while issue was closed.
https://codereview.chromium.org/2507753002/diff/320001/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2507753002/diff/320001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:1236: DeRegisterEventLogProvider(); i just noticed that this shouldn't be within the "if (remove_all)" block. it just so happens that !remove_all is never true for system-level installs, so DeRegisterEventLogProvider will get called at the right time. i think it makes sense to move this up so that it comes before the "if (remove_all)" block. |
