Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1247993002:
Return Windows error code when create-process fails. (Closed)
DescriptionReturn Windows error code when create-process fails.
COULD_NOT_CREATE_PROCESS makes up about 10% of the setup errors; include the actual DWORD result code returned by ::CreateProcess by placing all result information into the proper location of the Registry for reading by Omaha.
BUG=
Committed: https://crrev.com/135f5f669574bd07eaedaf6b57503984aeb13be8
Cr-Commit-Position: refs/heads/master@{#341938}
Committed: https://crrev.com/41465440adac9c4ca529d42bc685eedb640c8eb7
Cr-Commit-Position: refs/heads/master@{#342528}
Committed: https://crrev.com/4e19d36202c2d3efcded709c2ac67c48f5665334
Cr-Commit-Position: refs/heads/master@{#342683}
Patch Set 1 #Patch Set 2 : report exit-code and last-error through registry #
Total comments: 9
Patch Set 3 : addressed review comments by grt #Patch Set 4 : clean up of comments and formatting #
Total comments: 1
Patch Set 5 : only write result if error doesn't exist #Patch Set 6 : don't write success results #
Total comments: 16
Patch Set 7 : complete rebuild based on grt suggestions #Patch Set 8 : fixed setup_unittests build #Patch Set 9 : fixed configuration tests and added new ones #
Total comments: 23
Patch Set 10 : addressed review comments by robertshield #Patch Set 11 : remove stale comment #Patch Set 12 : fix additional call to OpenClientStateKey #Patch Set 13 : add regkey to new build system #Patch Set 14 : rebased #Patch Set 15 : added mini_installer_constants referenced via configuration to .gn file #Patch Set 16 : fixed uninitialized values in some tests #Patch Set 17 : untabify and rebase #Messages
Total messages: 60 (11 generated)
bcwhite@chromium.org changed reviewers: + grt@chromium.org
grt@chromium.org changed reviewers: + ganesh@chromium.org
I like the idea, but I'm concerned that this doesn't scale for associating error codes with different locations. As luck would have it, Google Update has a contract with app installers via the registry. I think this is a good use for it. Ganesh: if Chrome's mini_installer were to write a last error code and/or an HRESULT into InstallerExtraCode1, would it be picked up by Google Update and sent back along with the process exit code? The mini_installer doesn't currently set InstallerResult or InstallerError, but rather relies on the process exit code being reported by Google Update. Would it need to switch to reporting all status by the registry in order to take advantage of InstallerExtraCode1?
On 2015/07/23 02:30:36, grt wrote: > Ganesh: if Chrome's mini_installer were to write a last error code and/or an > HRESULT into InstallerExtraCode1, would it be picked up by Google Update and > sent back along with the process exit code? The mini_installer doesn't currently > set InstallerResult or InstallerError, but rather relies on the process exit > code being reported by Google Update. Would it need to switch to reporting all > status by the registry in order to take advantage of InstallerExtraCode1? Hi Greg, it would be a good idea to switch all reporting to the registry to keep it in one place.
On 2015/07/23 05:50:35, S. Ganesh wrote: > On 2015/07/23 02:30:36, grt wrote: > > Ganesh: if Chrome's mini_installer were to write a last error code and/or an > > HRESULT into InstallerExtraCode1, would it be picked up by Google Update and > > sent back along with the process exit code? The mini_installer doesn't > currently > > set InstallerResult or InstallerError, but rather relies on the process exit > > code being reported by Google Update. Would it need to switch to reporting all > > status by the registry in order to take advantage of InstallerExtraCode1? > > Hi Greg, it would be a good idea to switch all reporting to the registry to keep > it in one place. I'll look into it.
New CL using registry has been uploaded. PTAL.
https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:77: LONG WriteValue(const wchar_t* value_name, const wchar_t* value); some suggestions: - change this to WriteSZValue - change WriteDword to WriteDWValue https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:896: const HKEY root_key = please move this into a helper function (WriteInstallerResults?) and wrap the call to it in #if defined(GOOGLE_CHROME_BUILD) since Google Update integration is not supported for Chromium. https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:898: const wchar_t* app_guid = configuration.chrome_app_guid(); this and SetInstallerFlags need to use the same ClientState key. please factor the search logic out of SetInstallerFlags somehow so that they use the same key. one way would be to make a helper that takes a Configuration as input and returns the app_guid to be used for interactions with Google Update. Then that app_guid can be passed to both SetInstallerFlags and WriteInstallerResults. https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:899: const REGSAM key_access = KEY_QUERY_VALUE | KEY_SET_VALUE; looks like KEY_QUERY_VALUE isn't needed here. https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:905: key.WriteDword(kInstallerExtraCode1RegistryValue, ::GetLastError()); GetLastError must be called immediately after a failed call to provide a useful result.
please update the CR subject and CL description as well, thanks
> > please update the CR subject and CL description as well, thanks > That's already done. Are you not seeing it? Actually... I guess I left the title the same since it still applies; it never mentioned "exit code" specifically. Brian bcwhite@google.com ----------------------------------------------------------------------------------------- *Treat someone as they are and they will remain that way.Treat someone as they can be and they will become that way.* To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:77: LONG WriteValue(const wchar_t* value_name, const wchar_t* value); On 2015/07/24 14:49:10, grt wrote: > some suggestions: > - change this to WriteSZValue > - change WriteDword to WriteDWValue Done. https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:896: const HKEY root_key = On 2015/07/24 14:49:11, grt wrote: > please move this into a helper function (WriteInstallerResults?) and wrap the > call to it in #if defined(GOOGLE_CHROME_BUILD) since Google Update integration > is not supported for Chromium. Done. https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:898: const wchar_t* app_guid = configuration.chrome_app_guid(); On 2015/07/24 14:49:11, grt wrote: > this and SetInstallerFlags need to use the same ClientState key. please factor > the search logic out of SetInstallerFlags somehow so that they use the same key. > one way would be to make a helper that takes a Configuration as input and > returns the app_guid to be used for interactions with Google Update. Then that > app_guid can be passed to both SetInstallerFlags and WriteInstallerResults. Done. Complicated logic, though. Please check thoroughly. https://codereview.chromium.org/1247993002/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:905: key.WriteDword(kInstallerExtraCode1RegistryValue, ::GetLastError()); On 2015/07/24 14:49:11, grt wrote: > GetLastError must be called immediately after a failed call to provide a useful > result. Weird. I wrote that for initial testing and changed it in a later commit. Must not have gotten sync'd to my office machine from where I did the upload.
https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:929: WriteInstallResults(configuration, exit_code, LastWindowsError); i had a thought over the weekend: mini_installer should not write results when RunSetup succeeds in launching setup.exe. setup.exe is responsible for writing its own results to the registry. we wouldn't want mini_installer to overwrite it.
On 2015/07/27 15:21:37, grt wrote: > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > File chrome/installer/mini_installer/mini_installer.cc (right): > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > chrome/installer/mini_installer/mini_installer.cc:929: > WriteInstallResults(configuration, exit_code, LastWindowsError); > i had a thought over the weekend: mini_installer should not write results when > RunSetup succeeds in launching setup.exe. setup.exe is responsible for writing > its own results to the registry. we wouldn't want mini_installer to overwrite > it. Fair enough. Omaha renames the registry keys to "Last..." once they've processed it. How about I have the mini_installer set those Registry values only if kInstallerResultRegistryValue has not already been set?
On 2015/07/27 16:37:54, bcwhite wrote: > On 2015/07/27 15:21:37, grt wrote: > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > File chrome/installer/mini_installer/mini_installer.cc (right): > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > chrome/installer/mini_installer/mini_installer.cc:929: > > WriteInstallResults(configuration, exit_code, LastWindowsError); > > i had a thought over the weekend: mini_installer should not write results when > > RunSetup succeeds in launching setup.exe. setup.exe is responsible for writing > > its own results to the registry. we wouldn't want mini_installer to overwrite > > it. > > Fair enough. Omaha renames the registry keys to "Last..." once they've > processed it. How about I have the mini_installer set those Registry values > only if kInstallerResultRegistryValue has not already been set? sgtm!
On 2015/07/27 17:05:34, grt wrote: > On 2015/07/27 16:37:54, bcwhite wrote: > > On 2015/07/27 15:21:37, grt wrote: > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > File chrome/installer/mini_installer/mini_installer.cc (right): > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > chrome/installer/mini_installer/mini_installer.cc:929: > > > WriteInstallResults(configuration, exit_code, LastWindowsError); > > > i had a thought over the weekend: mini_installer should not write results > when > > > RunSetup succeeds in launching setup.exe. setup.exe is responsible for > writing > > > its own results to the registry. we wouldn't want mini_installer to > overwrite > > > it. > > > > Fair enough. Omaha renames the registry keys to "Last..." once they've > > processed it. How about I have the mini_installer set those Registry values > > only if kInstallerResultRegistryValue has not already been set? > > sgtm! Problem... If the mini-installer gets an error but the setup run was successful, I think the error needs to override the success. Otherwise, if the previous setup.exe runs successfully and the mini-installer fails thereafter, no error will be presented.
On 2015/07/27 17:49:49, bcwhite wrote: > On 2015/07/27 17:05:34, grt wrote: > > On 2015/07/27 16:37:54, bcwhite wrote: > > > On 2015/07/27 15:21:37, grt wrote: > > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > > File chrome/installer/mini_installer/mini_installer.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > > chrome/installer/mini_installer/mini_installer.cc:929: > > > > WriteInstallResults(configuration, exit_code, LastWindowsError); > > > > i had a thought over the weekend: mini_installer should not write results > > when > > > > RunSetup succeeds in launching setup.exe. setup.exe is responsible for > > writing > > > > its own results to the registry. we wouldn't want mini_installer to > > overwrite > > > > it. > > > > > > Fair enough. Omaha renames the registry keys to "Last..." once they've > > > processed it. How about I have the mini_installer set those Registry values > > > only if kInstallerResultRegistryValue has not already been set? > > > > sgtm! > > Problem... If the mini-installer gets an error but the setup run was > successful, I think the error needs to override the success. Otherwise, if the > previous setup.exe runs successfully and the mini-installer fails thereafter, no > error will be presented. Just to clarify... Because setup.exe runs twice, if the first run saves "success" then a failure of the second run would not get recorded. New code... If mini_installer detects an error, only save the result to the registry if there is no existing result or the previous result was "success".
On 2015/07/27 18:00:58, bcwhite wrote: > On 2015/07/27 17:49:49, bcwhite wrote: > > On 2015/07/27 17:05:34, grt wrote: > > > On 2015/07/27 16:37:54, bcwhite wrote: > > > > On 2015/07/27 15:21:37, grt wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > > > File chrome/installer/mini_installer/mini_installer.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > > > chrome/installer/mini_installer/mini_installer.cc:929: > > > > > WriteInstallResults(configuration, exit_code, LastWindowsError); > > > > > i had a thought over the weekend: mini_installer should not write > results > > > when > > > > > RunSetup succeeds in launching setup.exe. setup.exe is responsible for > > > writing > > > > > its own results to the registry. we wouldn't want mini_installer to > > > overwrite > > > > > it. > > > > > > > > Fair enough. Omaha renames the registry keys to "Last..." once they've > > > > processed it. How about I have the mini_installer set those Registry > values > > > > only if kInstallerResultRegistryValue has not already been set? > > > > > > sgtm! > > > > Problem... If the mini-installer gets an error but the setup run was > > successful, I think the error needs to override the success. Otherwise, if > the > > previous setup.exe runs successfully and the mini-installer fails thereafter, > no > > error will be presented. > > Just to clarify... Because setup.exe runs twice, if the first run saves > "success" then a failure of the second run would not get recorded. > > New code... If mini_installer detects an error, only save the result to the > registry if there is no existing result or the previous result was "success". how about this: mini_installer writes values to the registry only if CreateProcess(setup) or GetExitCodeProcess(setup) doesn't happen or fails. this is pretty straightforward and doesn't rely on testing whether or not setup wrote to the registry. what do you think?
On 2015/07/27 19:47:51, grt wrote: > On 2015/07/27 18:00:58, bcwhite wrote: > > On 2015/07/27 17:49:49, bcwhite wrote: > > > On 2015/07/27 17:05:34, grt wrote: > > > > On 2015/07/27 16:37:54, bcwhite wrote: > > > > > On 2015/07/27 15:21:37, grt wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > > > > File chrome/installer/mini_installer/mini_installer.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > > > > chrome/installer/mini_installer/mini_installer.cc:929: > > > > > > WriteInstallResults(configuration, exit_code, LastWindowsError); > > > > > > i had a thought over the weekend: mini_installer should not write > > results > > > > when > > > > > > RunSetup succeeds in launching setup.exe. setup.exe is responsible for > > > > writing > > > > > > its own results to the registry. we wouldn't want mini_installer to > > > > overwrite > > > > > > it. > > > > > > > > > > Fair enough. Omaha renames the registry keys to "Last..." once they've > > > > > processed it. How about I have the mini_installer set those Registry > > values > > > > > only if kInstallerResultRegistryValue has not already been set? > > > > > > > > sgtm! > > > > > > Problem... If the mini-installer gets an error but the setup run was > > > successful, I think the error needs to override the success. Otherwise, if > > the > > > previous setup.exe runs successfully and the mini-installer fails > thereafter, > > no > > > error will be presented. > > > > Just to clarify... Because setup.exe runs twice, if the first run saves > > "success" then a failure of the second run would not get recorded. > > > > New code... If mini_installer detects an error, only save the result to the > > registry if there is no existing result or the previous result was "success". > > how about this: mini_installer writes values to the registry only if > CreateProcess(setup) or GetExitCodeProcess(setup) doesn't happen or fails. this > is pretty straightforward and doesn't rely on testing whether or not setup wrote > to the registry. what do you think? There are other Windows calls that can fail and cause the mini_installer to fail. The GetLastError code from those would not be reported, only the non-specific exit code about which call failed (not why it failed). Also, this could lead to returning a "success" result via the Registry but a "failure" result via the exit-code. I guess this is acceptable given that it's the current state of things but I think the mini-installer should be consistent in how it returns results, either always exit-code or always registry (albeit with setup.exe failure codes taking precedence).
On 2015/07/27 20:17:00, bcwhite wrote: > On 2015/07/27 19:47:51, grt wrote: > > On 2015/07/27 18:00:58, bcwhite wrote: > > > On 2015/07/27 17:49:49, bcwhite wrote: > > > > On 2015/07/27 17:05:34, grt wrote: > > > > > On 2015/07/27 16:37:54, bcwhite wrote: > > > > > > On 2015/07/27 15:21:37, grt wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > > > > > File chrome/installer/mini_installer/mini_installer.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1247993002/diff/60001/chrome/installer/mini_i... > > > > > > > chrome/installer/mini_installer/mini_installer.cc:929: > > > > > > > WriteInstallResults(configuration, exit_code, LastWindowsError); > > > > > > > i had a thought over the weekend: mini_installer should not write > > > results > > > > > when > > > > > > > RunSetup succeeds in launching setup.exe. setup.exe is responsible > for > > > > > writing > > > > > > > its own results to the registry. we wouldn't want mini_installer to > > > > > overwrite > > > > > > > it. > > > > > > > > > > > > Fair enough. Omaha renames the registry keys to "Last..." once > they've > > > > > > processed it. How about I have the mini_installer set those Registry > > > values > > > > > > only if kInstallerResultRegistryValue has not already been set? > > > > > > > > > > sgtm! > > > > > > > > Problem... If the mini-installer gets an error but the setup run was > > > > successful, I think the error needs to override the success. Otherwise, > if > > > the > > > > previous setup.exe runs successfully and the mini-installer fails > > thereafter, > > > no > > > > error will be presented. > > > > > > Just to clarify... Because setup.exe runs twice, if the first run saves > > > "success" then a failure of the second run would not get recorded. > > > > > > New code... If mini_installer detects an error, only save the result to the > > > registry if there is no existing result or the previous result was > "success". > > > > how about this: mini_installer writes values to the registry only if > > CreateProcess(setup) or GetExitCodeProcess(setup) doesn't happen or fails. apologies for phrasing my idea poorly there: by the above i meant that all errors anywhere in m_i would result in writing details to the registry except in the two cases where the failure was actually in setup.exe, where it is setup's responsibility to write to the registry. it's still correct for m_i to pass along the exit code from RunSetup as its own exit code, and m_i shouldn't do anything to the registry if the call to RunSetup results in getting an actual exit code from setup.exe since either the success or failure reported by setup.exe should be read by Google Update. following that logic, m_i should never write success info to the registry, as success implies that the final call to setup.exe succeeded. m_i should write all of its own failures to the registry, but not write setup.exe's failures (either the --update-setup-exe failures or the actual installation's failures). > this > > is pretty straightforward and doesn't rely on testing whether or not setup > wrote > > to the registry. what do you think? > > There are other Windows calls that can fail and cause the mini_installer to > fail. The GetLastError code from those would not be reported, only the > non-specific exit code about which call failed (not why it failed). > > Also, this could lead to returning a "success" result via the Registry but a > "failure" result via the exit-code. I guess this is acceptable given that it's > the current state of things but I think the mini-installer should be consistent > in how it returns results, either always exit-code or always registry (albeit > with setup.exe failure codes taking precedence).
> > > how about this: mini_installer writes values to the registry only if > > > CreateProcess(setup) or GetExitCodeProcess(setup) doesn't happen or fails. > > apologies for phrasing my idea poorly there: by the above i meant that all > errors anywhere in m_i would result in writing details to the registry except in > the two cases where the failure was actually in setup.exe, where it is setup's > responsibility to write to the registry. it's still correct for m_i to pass > along the exit code from RunSetup as its own exit code, and m_i shouldn't do > anything to the registry if the call to RunSetup results in getting an actual > exit code from setup.exe since either the success or failure reported by > setup.exe should be read by Google Update. following that logic, m_i should > never write success info to the registry, as success implies that the final call > to setup.exe succeeded. m_i should write all of its own failures to the > registry, but not write setup.exe's failures (either the --update-setup-exe > failures or the actual installation's failures). As I read this, it's patch #5 (don't overwrite failures in the registry) with an additional test so as to not write success results (which would already be there). Done.
https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:42: DWORD LastWindowsError = 0; can you do away with this? i don't think it's necessary to have a global variable. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:190: bool OpenInstallStateKey(const Configuration& configuration, RegKey* key) { the logic in here that figures out which app_guid to use to communicate with Google Update could be moved into Configuration and performed during its initialization. this would make it testable in configuration_test.cc (setup_unittests), which is finally being run on the trybots and waterfall builders. the one hiccup that i see is that OpenClientStateKey (and maybe other things) would need to be moved into some sort of utility file, which means adding new source(s) to the build target. this requires an extra step, but i think it's worth it due to the simplification here and the testability (that TODO can become a DONE without too much effort). Configuration::app_guid() could be the accessor that returns the guid by which the installer is to interact with Google Update. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:246: if (key.ReadDWValue(kInstallerResultRegistryValue, &value) this adds a dependency on an implementation detail of Google Update, which seems risky. can this be rejiggered so that it doesn't check for existing values? https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:362: if (WAIT_OBJECT_0 != wr || !::GetExitCodeProcess(pi.hProcess, &exit_code)) { please add a comment that this code assumes that wr == WAIT_FAILED if it != WAIT_OBJECT_0. i think that this is a safe assumption, but if it isn't we'll get noise from GetLastError. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:515: LastWindowsError = ::GetLastError(); the last error code is generally not reset to ERROR_SUCCESS (0) when an arbitrary Win32 call succeeds, so GetLastError shouldn't be called for cases where EnumResourceNames succeeds but the expected resource wasn't there. it'll provide random noise which we won't be able to distinguish from the actual signal. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:646: return exit_code; if this is hit, the return value from the various registry functions (which are "last error codes", just not using GetLastError) would be informative. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:930: if (!GetWorkDir(module, &base_path)) failures in here should be captured, too
https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:42: DWORD LastWindowsError = 0; On 2015/07/28 02:30:07, grt wrote: > can you do away with this? i don't think it's necessary to have a global > variable. Yes, though the global was the easiest. I could change all the return codes to be 64-bits and combine both exit-code and last-error DWORDs into the single value. Or return a pair<>. Any preference? https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:190: bool OpenInstallStateKey(const Configuration& configuration, RegKey* key) { On 2015/07/28 02:30:06, grt wrote: > the logic in here that figures out which app_guid to use to communicate with > Google Update could be moved into Configuration and performed during its > initialization. this would make it testable in configuration_test.cc > (setup_unittests), which is finally being run on the trybots and waterfall > builders. the one hiccup that i see is that OpenClientStateKey (and maybe other > things) would need to be moved into some sort of utility file, which means > adding new source(s) to the build target. this requires an extra step, but i > think it's worth it due to the simplification here and the testability (that > TODO can become a DONE without too much effort). Configuration::app_guid() could > be the accessor that returns the guid by which the installer is to interact with > Google Update. Okay. Any objection to RegKey becoming more sophisticated and moving those methods into it within a new file? https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:246: if (key.ReadDWValue(kInstallerResultRegistryValue, &value) On 2015/07/28 02:30:06, grt wrote: > this adds a dependency on an implementation detail of Google Update, which seems > risky. can this be rejiggered so that it doesn't check for existing values? I don't think so. We have to check if there is a non-success value present before writing our own. These keys and values, as well as their behavior, are documented. We could instead make the decision based on the exit-code of setup.exe but that would also be an implementation detail and an undocumented one at that. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:362: if (WAIT_OBJECT_0 != wr || !::GetExitCodeProcess(pi.hProcess, &exit_code)) { On 2015/07/28 02:30:07, grt wrote: > please add a comment that this code assumes that wr == WAIT_FAILED if it != > WAIT_OBJECT_0. i think that this is a safe assumption, but if it isn't we'll get > noise from GetLastError. Done. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:515: LastWindowsError = ::GetLastError(); On 2015/07/28 02:30:07, grt wrote: > the last error code is generally not reset to ERROR_SUCCESS (0) when an > arbitrary Win32 call succeeds, so GetLastError shouldn't be called for cases > where EnumResourceNames succeeds but the expected resource wasn't there. it'll > provide random noise which we won't be able to distinguish from the actual > signal. Done. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:646: return exit_code; On 2015/07/28 02:30:07, grt wrote: > if this is hit, the return value from the various registry functions (which are > "last error codes", just not using GetLastError) would be informative. Okay. This will be easier when the return-value includes both exit-code and last-error.
https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:42: DWORD LastWindowsError = 0; On 2015/07/28 13:14:01, bcwhite wrote: > On 2015/07/28 02:30:07, grt wrote: > > can you do away with this? i don't think it's necessary to have a global > > variable. > > Yes, though the global was the easiest. I could change all the return codes to > be 64-bits and combine both exit-code and last-error DWORDs into the single > value. Or return a pair<>. Any preference? Pair or a POD struct w/ value semantics both sound fine. A struct may be more clear since the reader won't need to remember who's on first. https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:246: if (key.ReadDWValue(kInstallerResultRegistryValue, &value) On 2015/07/28 13:14:01, bcwhite wrote: > On 2015/07/28 02:30:06, grt wrote: > > this adds a dependency on an implementation detail of Google Update, which > seems > > risky. can this be rejiggered so that it doesn't check for existing values? > > I don't think so. We have to check if there is a non-success value present > before writing our own. Why? Does it not suffice to suppress writing when setup.exe is successfully launched and exits? > These keys and values, as well as their behavior, are > documented. Google Update is not under our control. Who knows when it will decide to move the values to LastFoo? > We could instead make the decision based on the exit-code of > setup.exe but that would also be an implementation detail and an undocumented > one at that. setup.exe is part of Chrome, so we can establish any contract we want here.
https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:246: if (key.ReadDWValue(kInstallerResultRegistryValue, &value) > > I don't think so. We have to check if there is a non-success value present > > before writing our own. > > Why? Does it not suffice to suppress writing when setup.exe is successfully > launched and exits? If we set a constraint that the MI isn't allowed to have any kind of (reported) failure after RunSetup completes, then sure. It's complicated, though, by the fact that setup runs twice and the "don't report" flag needs to be stored somewhere that is either global or passed around. > > These keys and values, as well as their behavior, are > > documented. > > Google Update is not under our control. Who knows when it will decide to move > the values to LastFoo? Does it matter? In the extremely unlikely event that Omaha reports in between when setup completes and MI completes, we would report two results. Omaha would either show one additional successful install or two separate failures, both of which could provide useful information. Even ignoring the fact that most updates are started by Omaha which presumably waits for completion before reporting, the number of split results would so small as to be statistically insignificant, on the order of 0.01% (assuming 8s of MI activity after setup completes and completely random update/omaha runs throughout the day). I don't think it warrants any additional complexity in order to try to mitigate that.
Okay... - regkey moved into separate file - moved some static "helper" registry functions to regkey - guid determination moved into configuration - ProcessExitCode changed to struct to also hold windows last-error - registry access results returned in ProcessExitCode - still working on test ... and it started as such a small change ...
On 2015/07/28 19:24:51, bcwhite wrote: > Okay... > - regkey moved into separate file > - moved some static "helper" registry functions to regkey > - guid determination moved into configuration > - ProcessExitCode changed to struct to also hold windows last-error > - registry access results returned in ProcessExitCode > - still working on test > > ... and it started as such a small change ... Added test.
bcwhite@chromium.org changed reviewers: + robertshield@chromium.org
+robertshield Greg is away until late August. Would you mind continuing the review?
Looking good, mostly minor nits https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/configuration.cc (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/configuration.cc:41: has_chrome_frame_ ? Maybe out of scope for your change, but why does this Chrome Frame stuff still exist? --chrome-frame and its effects will never happen and should be deleted anywhere it is seen. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/configuration.h (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/configuration.h:47: // Returns the app guid used in as the registry key. Stale comment? https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/configuration.h:83: // Abstract registry access for testing purposes. Nit: Slightly ambiguous use of the word "abstract" and this isn't a pure virtual. I've commonly seen the comment "Virtual for testing" used elsewhere in the codebase. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:42: struct ProcessExitCode { This isn't a just an exit code anymore, suggest calling it ProcessExitResults or similar. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:44: DWORD windows_code; It isn't clear to a Windows dev what "windows_code" means. If this is always the Last Error Code then last_error_code would be clearer. If it is a generic holder for error related codes, then "error_code" would be clearer. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:51: return exit_code == ERROR_SUCCESS; Nit: ERROR_SUCCESS is a windows error code, not a process exit code. Do you mean SUCCESS_EXIT_CODE? https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/regkey.cc (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.cc:7: #include "chrome/installer/mini_installer/regkey.h" This #include should go first. Do git cl format or git cl presubmit disagree? https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.cc:19: DWORD type; please initialize type to 0 https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.cc:40: DWORD type; initialize https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/regkey.h (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.h:13: // return Windows last-error codes a la the Win32 registry API. s/a la the/a la/ https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.h:50: static bool OpenClientStateKey(HKEY root_key, const wchar_t* app_guid, Layering-wise, it seems a little odd to tack this on to a RegKey class since it contains application logic. Maybe keep this function in an anonymous namespace in mini_installer.cc?
https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/configuration.cc (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/configuration.cc:41: has_chrome_frame_ ? On 2015/08/04 03:11:36, robertshield wrote: > Maybe out of scope for your change, but why does this Chrome Frame stuff still > exist? --chrome-frame and its effects will never happen and should be deleted > anywhere it is seen. It was existing code. At one point I had removed it but restored it because I wasn't removing all other references to chrome_frame_. It seemed bad from to remove only one piece of the whole. I'm happy to clean up all references to chrome_frame_ but would rather do so in a different CL. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/configuration.h (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/configuration.h:83: // Abstract registry access for testing purposes. On 2015/08/04 03:11:36, robertshield wrote: > Nit: Slightly ambiguous use of the word "abstract" and this isn't a pure > virtual. I've commonly seen the comment "Virtual for testing" used elsewhere in > the codebase. Done. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:42: struct ProcessExitCode { On 2015/08/04 03:11:36, robertshield wrote: > This isn't a just an exit code anymore, suggest calling it ProcessExitResults or > similar. Done. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:44: DWORD windows_code; On 2015/08/04 03:11:36, robertshield wrote: > It isn't clear to a Windows dev what "windows_code" means. If this is always the > Last Error Code then last_error_code would be clearer. If it is a generic holder > for error related codes, then "error_code" would be clearer. It's usually the result of ::GetLastError() but sometimes the return code from Windows registry functions. It's always Windows specific, though. "windows_error"? https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:51: return exit_code == ERROR_SUCCESS; On 2015/08/04 03:11:36, robertshield wrote: > Nit: ERROR_SUCCESS is a windows error code, not a process exit code. Do you mean > SUCCESS_EXIT_CODE? Done. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/regkey.cc (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.cc:7: #include "chrome/installer/mini_installer/regkey.h" On 2015/08/04 03:11:36, robertshield wrote: > This #include should go first. Do git cl format or git cl presubmit disagree? Done. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.cc:19: DWORD type; On 2015/08/04 03:11:36, robertshield wrote: > please initialize type to 0 Done. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.cc:40: DWORD type; On 2015/08/04 03:11:36, robertshield wrote: > initialize Done. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/regkey.h (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.h:13: // return Windows last-error codes a la the Win32 registry API. On 2015/08/04 03:11:36, robertshield wrote: > s/a la the/a la/ Done. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/regkey.h:50: static bool OpenClientStateKey(HKEY root_key, const wchar_t* app_guid, On 2015/08/04 03:11:36, robertshield wrote: > Layering-wise, it seems a little odd to tack this on to a RegKey class since it > contains application logic. Maybe keep this function in an anonymous namespace > in mini_installer.cc? It needs to be someplace accessible by both configuration.cc and mini_installer.cc. I could move it into another shared file but since it's still registry related, I tacked it on here. Or I could just move it out of the class but keep it in the same file as a related global helper function. I'll do that.
lgtm https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/configuration.cc (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/configuration.cc:41: has_chrome_frame_ ? On 2015/08/04 19:34:30, bcwhite wrote: > On 2015/08/04 03:11:36, robertshield wrote: > > Maybe out of scope for your change, but why does this Chrome Frame stuff still > > exist? --chrome-frame and its effects will never happen and should be deleted > > anywhere it is seen. > > It was existing code. At one point I had removed it but restored it because I > wasn't removing all other references to chrome_frame_. It seemed bad from to > remove only one piece of the whole. > > I'm happy to clean up all references to chrome_frame_ but would rather do so in > a different CL. SGTM. https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1247993002/diff/160001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:44: DWORD windows_code; On 2015/08/04 19:34:31, bcwhite wrote: > On 2015/08/04 03:11:36, robertshield wrote: > > It isn't clear to a Windows dev what "windows_code" means. If this is always > the > > Last Error Code then last_error_code would be clearer. If it is a generic > holder > > for error related codes, then "error_code" would be clearer. > > It's usually the result of ::GetLastError() but sometimes the return code from > Windows registry functions. It's always Windows specific, though. > "windows_error"? Ok.
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247993002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247993002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/135f5f669574bd07eaedaf6b57503984aeb13be8 Cr-Commit-Position: refs/heads/master@{#341938}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1277833002/ by vabr@chromium.org. The reason for reverting is: Broke compilation on Win 64 GN. Info at http://crbug.com/517326..
I've updated the .gn file that should allow "Win 64 GN" to build. However, the bot won't apply the patch in order for me to test it because the CL also contains a patch to a .gypi file that apparently does not exist on the bot. @@@STEP_LOG_LINE@patch error@ Checking patch chrome/chrome_installer.gypi...@@@ @@@STEP_LOG_LINE@patch error@ error: chrome/chrome_installer.gypi: does not exist in index@@@ I'm going to have to submit this and hope that it fixes the problem that caused it to be previously reverted.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org Link to the patchset: https://codereview.chromium.org/1247993002/#ps260001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247993002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247993002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
win_chromium_x64_rel_ng appears to be flaky with a lot of builds failing at the same point. Since my change doesn't touch anything outside of the mini_installer, I going to point the finger else where.
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247993002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247993002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/41465440adac9c4ca529d42bc685eedb640c8eb7 Cr-Commit-Position: refs/heads/master@{#342528}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1285473002/ by bcwhite@chromium.org. The reason for reverting is: Looks like some other file needs to be added to the .gn as well. <sigh>.
Message was sent while issue was closed.
Sigh. Revert in progress. Some other file other than regkey.cc needs to be added as well, probably mini_installer_constants. configuration.obj : error LNK2019: unresolved external symbol "wchar_t const * const mini_installer::kMultiInstallTag" (?kMultiInstallTag@mini_installer@@3QB_WB) referenced in function "private: void __cdecl mini_installer::Configuration::SetChromeAppGuid(void)" (?SetChromeAppGuid@Configuration@mini_installer@@AEAAXXZ) configuration.obj : error LNK2019: unresolved external symbol "wchar_t const * const mini_installer::kApRegistryValue" (?kApRegistryValue@mini_installer@@3QB_WB) referenced in function "protected: virtual bool __cdecl mini_installer::Configuration::ReadClientStateRegistryValue(struct HKEY__ * const,wchar_t const *,long *,class mini_installer::StackString<128> &)" (?ReadClientStateRegistryValue@Configuration@mini_installer@@MEAA_NQEAUHKEY__@@PEB_WPEAJAEAV?$StackString@$0IA@@2@@Z) regkey.obj : error LNK2019: unresolved external symbol "wchar_t const * const mini_installer::kClientStateKeyBase" (?kClientStateKeyBase@mini_installer@@3QB_WB) referenced in function "bool __cdecl mini_installer::OpenClientStateKey(struct HKEY__ *,wchar_t const *,unsigned long,class mini_installer::RegKey *)" (?OpenClientStateKey@mini_installer@@YA_NPEAUHKEY__@@PEB_WKPEAVRegKey@1@@Z)
thakis@chromium.org changed reviewers: + thakis@chromium.org
Looks like this might also break setup_unittests in official builds: http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/... MiniInstallerConfigurationTest.ChromeAppGuid (run #1): [ RUN ] MiniInstallerConfigurationTest.ChromeAppGuid ..\..\chrome\installer\mini_installer\configuration_test.cc(102): error: Value of: std::wstring(google_update::kMultiInstallAppGuid) == TestConfiguration(L"spam.exe --multi-install --chrome") .chrome_app_guid() Actual: false Expected: true [ FAILED ] MiniInstallerConfigurationTest.ChromeAppGuid (0 ms)
On 2015/08/08 17:49:24, Nico (hiding) wrote: > Looks like this might also break setup_unittests in official builds: > http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/... > > MiniInstallerConfigurationTest.ChromeAppGuid (run #1): > [ RUN ] MiniInstallerConfigurationTest.ChromeAppGuid > ..\..\chrome\installer\mini_installer\configuration_test.cc(102): error: Value > of: std::wstring(google_update::kMultiInstallAppGuid) == > TestConfiguration(L"spam.exe --multi-install --chrome") .chrome_app_guid() > Actual: false > Expected: true > [ FAILED ] MiniInstallerConfigurationTest.ChromeAppGuid (0 ms) I don't get these failures locally and all the try-bots pass. However, I know that there is a secondary change that needs to be made in google3/ for the official builders but it was okay to submit this first. Is that what you're referring to?
On 2015/08/08 19:35:01, bcwhite wrote: > On 2015/08/08 17:49:24, Nico (hiding) wrote: > > Looks like this might also break setup_unittests in official builds: > > > http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/... > > > > MiniInstallerConfigurationTest.ChromeAppGuid (run #1): > > [ RUN ] MiniInstallerConfigurationTest.ChromeAppGuid > > ..\..\chrome\installer\mini_installer\configuration_test.cc(102): error: Value > > of: std::wstring(google_update::kMultiInstallAppGuid) == > > TestConfiguration(L"spam.exe --multi-install --chrome") .chrome_app_guid() > > Actual: false > > Expected: true > > [ FAILED ] MiniInstallerConfigurationTest.ChromeAppGuid (0 ms) > > I don't get these failures locally and all the try-bots pass. Are you doing a buildtype=Official branding=Chrome build locally? (We don't have trybots in that config that actually run tests, the trybots with that config only compile. So trybots wouldn't find this.) > However, I know > that there is a secondary change that needs to be made in google3/ for the > official builders but it was okay to submit this first. Is that what you're > referring to?
> Are you doing a buildtype=Official branding=Chrome build locally? (We don't have > trybots in that config that actually run tests, the trybots with that config > only compile. So trybots wouldn't find this.) Building that way, yes, and running setup tests off of Release (not Debug) build. I get some setup-test failures but not in the MiniInstaller category. The specific test you list as failing is definitely affected by new code and is affected by registry contents though a fake registry is used to deal with that. I'll take a second look through it.
> The specific test you list as failing is definitely affected by new code and is > affected by registry contents though a fake registry is used to deal with that. > I'll take a second look through it. Ah ha! I found that some test values were uninitialized in some tests.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org Link to the patchset: https://codereview.chromium.org/1247993002/#ps320001 (title: "untabify and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247993002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247993002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/4e19d36202c2d3efcded709c2ac67c48f5665334 Cr-Commit-Position: refs/heads/master@{#342683} |
