Force restoration of Chrome's shortcuts when Active Setup kicks in in response to a major OS upgrad…

Force restoration of Chrome's shortcuts when Active Setup kicks in in response to a major OS upgrade.

BUG=502363
TEST=setup_unittests.exe --gtest_filter=SetupUtilTest.UpdateLastOSUpgradeHandledByActiveSetup

Committed: https://crrev.com/7c874dddfb6568a30b0b59b072488b56dbc961f8
Cr-Commit-Position: refs/heads/master@{#338572}

[gab, 2015-07-09 20:54:44]

gab@chromium.org changed reviewers:
+ grt@chromium.org

[gab, 2015-07-09 20:54:44]

Sir Greg PTAL, another merge candidate for M44 for Win10.

This CL goes on top of https://codereview.chromium.org/1223953003/ and
technically also needs https://codereview.chromium.org/1223933005/ to be any
useful (though it compiles without it).

Thanks!
Gab

https://codereview.chromium.org/1231973002/diff/1/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/1231973002/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:170: bool
InstallUtil::UpdateLastOSUpgradeHandledByActiveSetup(
@grt: This duplicates a bit of logic from a few places, but I couldn't quite see
where to put it to justify re-use, I think in this special case a little bit of
duplication saves a lot of complexity.

Duplication is pretty much:
 - Usage of GetStateMediumKey() and GetUserSidString() a la
google_update_settings.cc and beacons.cc
 - Knowledge of the "Version" entry of Active Setup a la
update_active_setup_version_work_item.cc

Let me know if you have a better idea.

[grt (UTC plus 2), 2015-07-10 18:15:24]

looks pretty good. i have no great suggestion for the duplicated code at the
moment.

https://codereview.chromium.org/1231973002/diff/1/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/1231973002/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:201: DWORD latest_os_upgrade = 0;
does this method always return false without updating the registry if
latest_os_upgrade == 0? if so, it looks like you could get rid of a potential
failure mode by doing this read first, and then Create last_upgrade_key for what
is currently part one (but would become part two) with KEY_WOW64_32KEY |
KEY_QUERY_VALUE | KEY_SET_VALUE. this way you never fail down in part four as a
result of not being able to create the key with write perms.

[gab, 2015-07-13 17:02:08]

Patchset #3 (id:40001) has been deleted

[gab, 2015-07-13 17:44:38]

Patchset #3 (id:60001) has been deleted

[gab, 2015-07-13 18:19:01]

PTAL, I did the discussed change to ensure the method fails on failed write, but
see thoughts below as I'm second guessing whether this is the right thing to do.

https://codereview.chromium.org/1231973002/diff/1/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/1231973002/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:201: DWORD latest_os_upgrade = 0;
On 2015/07/10 18:15:24, grt wrote:
> does this method always return false without updating the registry if
> latest_os_upgrade == 0? if so, it looks like you could get rid of a potential
> failure mode by doing this read first, and then Create last_upgrade_key for
what
> is currently part one (but would become part two) with KEY_WOW64_32KEY |
> KEY_QUERY_VALUE | KEY_SET_VALUE. this way you never fail down in part four as
a
> result of not being able to create the key with write perms.

Tweaked the order and made sure it only ever returns true if it successfully
saved its state (i.e. make the writes an explicit failure rather than a best
effort). In case of failure this will make sure the OS upgrade is never handled
more than once, though it may still be handled at a much later time, when it
eventually succeeds in an infrequent and potentially unrelated follow-up Active
Setup run.

Doing it as a best effort had the slight advantage of doing it when its right on
top of doing it again later when it will appear random (we can't avoid doing it
later if the initial write fails; unless we write a timestamp on-os-upgrade and
never do it X days after the OS upgrade, but this is more complexity for a
theoretical use case).

Since doing it later will look like a bug regardless and doing it initially is
always right, shall we go back to best-effort write?

[gab, 2015-07-13 18:23:08]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-13 18:23:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231973002/160001

[commit-bot: I haz the power, 2015-07-13 18:30:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-13 18:30:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

(exceeded global retry quota)

[gab, 2015-07-13 19:05:00]

Patchset #8 (id:180001) has been deleted

[gab, 2015-07-13 19:05:34]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-13 19:08:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231973002/160001

[commit-bot: I haz the power, 2015-07-13 19:12:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-13 19:12:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

(exceeded global retry quota)

[grt (UTC plus 2), 2015-07-13 20:05:08]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-07-13 20:05:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231973002/160001

[commit-bot: I haz the power, 2015-07-13 20:05:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-13 20:05:51]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[grt (UTC plus 2), 2015-07-13 20:13:54]

lgtm

[grt (UTC plus 2), 2015-07-13 20:14:00]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-07-13 20:14:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231973002/160001

[commit-bot: I haz the power, 2015-07-13 21:58:48]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-07-13 21:59:52]

Patchset 7 (id:??) landed as
https://crrev.com/7c874dddfb6568a30b0b59b072488b56dbc961f8
Cr-Commit-Position: refs/heads/master@{#338572}