Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1351453006:
Handle upgrade case from previous MSI install. (Closed)
|
Created:
5 years, 3 months ago by bcwhite Modified:
5 years, 2 months ago CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org, robertshield Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle upgrade case from previous MSI install.
An MSI install leaves behind a master_preferences file. Use that
information to update version numbers when auto-updates are installed.
BUG=67348
Committed: https://crrev.com/fe3fb71878ba25c963b6a3259ba948bdc828e44a
Cr-Commit-Position: refs/heads/master@{#353459}
Patch Set 1 #
Total comments: 4
Patch Set 2 : remove check for existing master_preferences file #Patch Set 3 : rebased #
Total comments: 3
Patch Set 4 : extract MSI product ID from existing ClientState #
Total comments: 6
Patch Set 5 : addressed review comments by grt #Messages
Total messages: 23 (6 generated)
bcwhite@chromium.org changed reviewers: + grt@chromium.org
How's this for handling the auto-updates to MSI installs?
On 2015/09/17 19:04:57, bcwhite wrote: > How's this for handling the auto-updates to MSI installs? Ping?
+Robert for his thoughts. https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:1672: } else if (base::PathExists(master_preferences_file)) { I don't think it's safe to rely on the value being in master_preferences. There is documentation on the internets saying that admins can do what they wish with this file to control Chrome. These don't say "be sure not to touch the msi product id". I think it's more reliable to get the value from the EnterpriseProduct value name in the ClientState key.
robertshield@chromium.org changed reviewers: + robertshield@chromium.org
https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:1672: } else if (base::PathExists(master_preferences_file)) { On 2015/09/25 15:01:27, grt wrote: > I don't think it's safe to rely on the value being in master_preferences. There > is documentation on the internets saying that admins can do what they wish with > this file to control Chrome. These don't say "be sure not to touch the msi > product id". I think it's more reliable to get the value from the > EnterpriseProduct value name in the ClientState key. I tend to agree, relying on master_preferences to be un-molested seems a little fragile. I can't think of any reason not to use the value in the ClientState key off the top of my head and I agree that seems more reliable.
https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:1672: } else if (base::PathExists(master_preferences_file)) { On 2015/09/25 15:01:27, grt wrote: > I don't think it's safe to rely on the value being in master_preferences. There > is documentation on the internets saying that admins can do what they wish with > this file to control Chrome. These don't say "be sure not to touch the msi > product id". I think it's more reliable to get the value from the > EnterpriseProduct value name in the ClientState key. I'll look into the ClientState key. Is it okay to do this check first, before looking there or skip this completely?
https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:1672: } else if (base::PathExists(master_preferences_file)) { On 2015/10/01 13:11:53, bcwhite wrote: > On 2015/09/25 15:01:27, grt wrote: > > I don't think it's safe to rely on the value being in master_preferences. > There > > is documentation on the internets saying that admins can do what they wish > with > > this file to control Chrome. These don't say "be sure not to touch the msi > > product id". I think it's more reliable to get the value from the > > EnterpriseProduct value name in the ClientState key. > > I'll look into the ClientState key. Is it okay to do this check first, before > looking there or skip this completely? My inclination is to only check the one authoritative source of data. https://codereview.chromium.org/749133003/ has Robert's code for extracting the product id from ClientState.
On 2015/10/01 14:37:59, grt wrote: > https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... > File chrome/installer/setup/setup_main.cc (right): > > https://codereview.chromium.org/1351453006/diff/1/chrome/installer/setup/setu... > chrome/installer/setup/setup_main.cc:1672: } else if > (base::PathExists(master_preferences_file)) { > On 2015/10/01 13:11:53, bcwhite wrote: > > On 2015/09/25 15:01:27, grt wrote: > > > I don't think it's safe to rely on the value being in master_preferences. > > There > > > is documentation on the internets saying that admins can do what they wish > > with > > > this file to control Chrome. These don't say "be sure not to touch the msi > > > product id". I think it's more reliable to get the value from the > > > EnterpriseProduct value name in the ClientState key. > > > > I'll look into the ClientState key. Is it okay to do this check first, before > > looking there or skip this completely? > > My inclination is to only check the one authoritative source of data. > https://codereview.chromium.org/749133003/ has Robert's code for extracting the > product id from ClientState. Check for master_preferences file removed. Using FindProduct(CHROME_BROWSER) GUID instead.
https://codereview.chromium.org/1351453006/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1680: base::string16 app_guid = chrome->distribution() Chrome's appguid for registration with Google Update isn't the same as the MSI product id. The product ID needs to be pulled from the registry à la ExtractMSIProductId in https://codereview.chromium.org/749133003/diff/80001/chrome/installer/setup/i...
https://codereview.chromium.org/1351453006/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1680: base::string16 app_guid = chrome->distribution() On 2015/10/02 17:55:47, grt (very slow until Oct 12) wrote: > Chrome's appguid for registration with Google Update isn't the same as the MSI > product id. The product ID needs to be pulled from the registry à la > ExtractMSIProductId in > https://codereview.chromium.org/749133003/diff/80001/chrome/installer/setup/i... That's odd. I checked the value and it matched what I was expecting. I checked the registry on an install and couldn't find the data/value that code was searching for. I'll check again.
https://codereview.chromium.org/1351453006/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1680: base::string16 app_guid = chrome->distribution() On 2015/10/02 17:58:20, bcwhite wrote: > On 2015/10/02 17:55:47, grt (very slow until Oct 12) wrote: > > Chrome's appguid for registration with Google Update isn't the same as the MSI > > product id. The product ID needs to be pulled from the registry à la > > ExtractMSIProductId in > > > https://codereview.chromium.org/749133003/diff/80001/chrome/installer/setup/i... > > That's odd. I checked the value and it matched what I was expecting. I checked > the registry on an install and couldn't find the data/value that code was > searching for. > > I'll check again. GetAppGuid() will always return {8A69D345-D564-463c-AFF1-A69D9E530F96} for Chrome. The MSI product ID is dynamic, changing for every build. Chrome's ClientState key will have a value named something like "EnterpriseProduct0FA69F39-D0BF-3023-BAD8-9AA90EA79ED3" where the stuff after "EnterpriseProduct" (IIRC) is the product ID at the time of install.
> GetAppGuid() will always return {8A69D345-D564-463c-AFF1-A69D9E530F96} for
> Chrome. The MSI product ID is dynamic, changing for every build. Chrome's
> ClientState key will have a value named something like
> "EnterpriseProduct0FA69F39-D0BF-3023-BAD8-9AA90EA79ED3" where the stuff after
> "EnterpriseProduct" (IIRC) is the product ID at the time of install.
Okay, I see what I was missing. Fixed.
lgtm w/ comment nits https://codereview.chromium.org/1351453006/diff/60001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/60001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:213: base::string16 FindMsiProductId(const InstallerState& installer_state, please include robert's excellent doc comment for this function: // Returns the MSI product ID from the ClientState key that is populated for MSI // installs. This property is encoded in a value name whose format is // "EnterpriseId<GUID>" where <GUID> is the MSI product id. <GUID> is in the // format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX. The id will be returned if found // otherwise this method will return an empty string. // // This format is strange and its provenance is shrouded in mystery but it has // the data we need, so use it. https://codereview.chromium.org/1351453006/diff/60001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:225: return value_name.substr(arraysize(kMsiProductIdPrefix) - 1); nit: braces around this https://codereview.chromium.org/1351453006/diff/60001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1697: // Get the app's GUID from an entry in ClientState nit: // Get the app's MSI product ID from EnterpriseProduct in ClientState.
The CQ bit was checked by bcwhite@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/1351453006/#ps80001 (title: "addressed review comments by grt")
https://codereview.chromium.org/1351453006/diff/60001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1351453006/diff/60001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:213: base::string16 FindMsiProductId(const InstallerState& installer_state, On 2015/10/09 13:16:39, grt (very slow until Oct 12) wrote: > please include robert's excellent doc comment for this function: > > // Returns the MSI product ID from the ClientState key that is populated for MSI > // installs. This property is encoded in a value name whose format is > // "EnterpriseId<GUID>" where <GUID> is the MSI product id. <GUID> is in the > // format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX. The id will be returned if found > // otherwise this method will return an empty string. > // > // This format is strange and its provenance is shrouded in mystery but it has > // the data we need, so use it. Done. https://codereview.chromium.org/1351453006/diff/60001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:225: return value_name.substr(arraysize(kMsiProductIdPrefix) - 1); On 2015/10/09 13:16:39, grt (very slow until Oct 12) wrote: > nit: braces around this Done. https://codereview.chromium.org/1351453006/diff/60001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1697: // Get the app's GUID from an entry in ClientState On 2015/10/09 13:16:39, grt (very slow until Oct 12) wrote: > nit: > // Get the app's MSI product ID from EnterpriseProduct in ClientState. Done.
The CQ bit was unchecked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1351453006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351453006/80001
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/1351453006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351453006/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fe3fb71878ba25c963b6a3259ba948bdc828e44a Cr-Commit-Position: refs/heads/master@{#353459} |
