Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11146003:
Remove the run verb on Windows 8. (Closed)
|
Created:
8 years, 2 months ago by gab Modified:
8 years, 2 months ago CC:
chromium-reviews, grt+watch_chromium.org, robertshield, chrome-win8-eng_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove the run verb on Windows 8.
R=grt@chromium.org
BUG=155640
TEST=Install old Chrome (user and system level), over-install new Chrome, make sure <root>\Software\Classes\Chrome<suffix>\.exe\shell\run is deleted.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162250
Patch Set 1 #
Total comments: 17
Patch Set 2 : add comment #Patch Set 3 : separate method #
Total comments: 9
Patch Set 4 : grt++ #Patch Set 5 : merge up to 162133 #Messages
Total messages: 15 (0 generated)
+ananta: You told me in chat this would be fine, but can I get your official lgtm here please :). Thanks, Gab
https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:742: // should be removed after a reasonable time, say 2012-08-01 (pushed back see what do you think about retiring the old "remove bad" code (which is on M22/stable and M23/beta, and is past its useful lifetime) now and update this to only do the new "remove bad" code? https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:753: // TODO (gab): This was fixed before the official release of Windows 8 and thus i think it's too late for this to be on stable before Windows 8 GA, so this comment is misleading. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:788: string16 run_verb_key(ShellUtil::kRegClasses); this code is nearly impenetrable. please add a comment saying that it removes the key ROOT\Software\Classes\Chrome<.suffix>\.exe\shell\run. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:796: InstallUtil::DeleteRegistryKey(root_key, run_verb_key); this could be in HKCU even for a system-level install, no? (see RegisterChromeBrowser)
See comments below. I would like to merge and thus am trying to minimize #lines changed. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:742: // should be removed after a reasonable time, say 2012-08-01 (pushed back see On 2012/10/13 00:34:26, grt wrote: > what do you think about retiring the old "remove bad" code (which is on > M22/stable and M23/beta, and is past its useful lifetime) now and update this to > only do the new "remove bad" code? Yea, I was tempted by that, but I want to merge this in M22 and M23 (see http://crbug.com/155640). We could remove it in a follow-up CL. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:753: // TODO (gab): This was fixed before the official release of Windows 8 and thus On 2012/10/13 00:34:26, grt wrote: > i think it's too late for this to be on stable before Windows 8 GA, so this > comment is misleading. As mentioned above, my intent is to merge it in both M22 and M23 so that we wouldn't have to deal with this verb ever being registered on a real Win8 Chrome install. If it doesn't get merged, I'll modify this comment. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:788: string16 run_verb_key(ShellUtil::kRegClasses); On 2012/10/13 00:34:26, grt wrote: > this code is nearly impenetrable. please add a comment saying that it removes > the key ROOT\Software\Classes\Chrome<.suffix>\.exe\shell\run. Done. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:796: InstallUtil::DeleteRegistryKey(root_key, run_verb_key); On 2012/10/13 00:34:26, grt wrote: > this could be in HKCU even for a system-level install, no? (see > RegisterChromeBrowser) No, on Win8 all user level stuff is in HKCU and system-level in HKLM. Technically it's possible for user-level stuff to be in HKLM if the user upgraded a Win7 install to a Win8 install, but we can't clean that up on upgrade anyways so no need to bother...
If your motivation for merging this is to try to prevent stable instals on Win8 with the run verb, then I don't think that's possible. There will be machines in that state since every combination of possible path of Chrome and OS versions exists in the wild. What's the harm in just putting a proper fix in M24? https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:742: // should be removed after a reasonable time, say 2012-08-01 (pushed back see On 2012/10/13 03:33:12, gab wrote: > On 2012/10/13 00:34:26, grt wrote: > > what do you think about retiring the old "remove bad" code (which is on > > M22/stable and M23/beta, and is past its useful lifetime) now and update this > to > > only do the new "remove bad" code? > > Yea, I was tempted by that, but I want to merge this in M22 and M23 (see > http://crbug.com/155640). Okay > We could remove it in a follow-up CL. I think it's a good idea. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:753: // TODO (gab): This was fixed before the official release of Windows 8 and thus On 2012/10/13 03:33:12, gab wrote: > On 2012/10/13 00:34:26, grt wrote: > > i think it's too late for this to be on stable before Windows 8 GA, so this > > comment is misleading. > > As mentioned above, my intent is to merge it in both M22 and M23 so that we > wouldn't have to deal with this verb ever being registered on a real Win8 Chrome > install. Merging it will not guarantee that. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:796: InstallUtil::DeleteRegistryKey(root_key, run_verb_key); On 2012/10/13 03:33:12, gab wrote: > On 2012/10/13 00:34:26, grt wrote: > > this could be in HKCU even for a system-level install, no? (see > > RegisterChromeBrowser) > > No, on Win8 all user level stuff is in HKCU and system-level in HKLM. > > Technically it's possible for user-level stuff to be in HKLM if the user > upgraded a Win7 install to a Win8 install, but we can't clean that up on upgrade > anyways so no need to bother... I expect there to be a great number of machines upgraded from Win7, so I think your statement should read "it's probable" rather than "it's possible".
> If your motivation for merging this is to try to prevent stable instals on Win8 > with the run verb, then I don't think that's possible. There will be machines > in that state since every combination of possible path of Chrome and OS versions > exists in the wild. Well is not right to say that machines to ship with the upcoming Win8 release will have Chrome 22 or higher (as it will be impossible to install an older version of Chrome then... at least not through the download page)? Remember the "run" verb is registered under the appid which is only registered on Win8 8370+. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:796: InstallUtil::DeleteRegistryKey(root_key, run_verb_key); On 2012/10/13 13:21:37, grt wrote: > On 2012/10/13 03:33:12, gab wrote: > > On 2012/10/13 00:34:26, grt wrote: > > > this could be in HKCU even for a system-level install, no? (see > > > RegisterChromeBrowser) > > > > No, on Win8 all user level stuff is in HKCU and system-level in HKLM. > > > > Technically it's possible for user-level stuff to be in HKLM if the user > > upgraded a Win7 install to a Win8 install, but we can't clean that up on > upgrade > > anyways so no need to bother... > > I expect there to be a great number of machines upgraded from Win7, so I think > your statement should read "it's probable" rather than "it's possible". Well technically if this is merged in M22 and M23 now, people upgrading their Win7 to Win8 come Win8 Release will never even have the run verb registered. In fact, there is no way for us to register it in HKLM as we don't have admin rights on OSupgrade detection (so change "it's possible" to "it's not possible"). I think the current flow is to re-register everything in HKCU on WIn8.
Can you explain why this would need to be merged on to M23 and M22? The bug doesn't make it clear why the presence of the run verb registration is harmful. Thanks. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:753: // TODO (gab): This was fixed before the official release of Windows 8 and thus On 2012/10/13 13:21:37, grt wrote: > On 2012/10/13 03:33:12, gab wrote: > > On 2012/10/13 00:34:26, grt wrote: > > > i think it's too late for this to be on stable before Windows 8 GA, so this > > > comment is misleading. > > > > As mentioned above, my intent is to merge it in both M22 and M23 so that we > > wouldn't have to deal with this verb ever being registered on a real Win8 > Chrome > > install. > > Merging it will not guarantee that. To elaborate, there will be a population of machines with M23 (which contains the OS upgrade handler) that will be updated to Win8 before your change reaches them. Updates are never rolled out to 100% of users. To simplify the merge, I suggest moving this new code into a new function with its own detailed comment that is invoked immediately before or after RemoveBadWindows8RegistrationIfNeeded (and revert all changes to this function). If I'm not mistaken, the logic to remove the run verb key can be simply "if Windows 8". https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:796: InstallUtil::DeleteRegistryKey(root_key, run_verb_key); On 2012/10/13 15:17:50, gab wrote: > On 2012/10/13 13:21:37, grt wrote: > > On 2012/10/13 03:33:12, gab wrote: > > > On 2012/10/13 00:34:26, grt wrote: > > > > this could be in HKCU even for a system-level install, no? (see > > > > RegisterChromeBrowser) > > > > > > No, on Win8 all user level stuff is in HKCU and system-level in HKLM. > > > > > > Technically it's possible for user-level stuff to be in HKLM if the user > > > upgraded a Win7 install to a Win8 install, but we can't clean that up on > > upgrade > > > anyways so no need to bother... > > > > I expect there to be a great number of machines upgraded from Win7, so I think > > your statement should read "it's probable" rather than "it's possible". > > Well technically if this is merged in M22 and M23 now, people upgrading their > Win7 to Win8 come Win8 Release will never even have the run verb registered. > > In fact, there is no way for us to register it in HKLM as we don't have admin > rights on OSupgrade detection (so change "it's possible" to "it's not > possible"). I think the current flow is to re-register everything in HKCU on > WIn8. Ah, I just realized that the Win7 -> Win8 path is only relevant if the version of Chrome present at the time of OS upgrade has the OS migration feature (i.e., M23) since the Run verb registration isn't done prior to Win8.
Comments below and on http://crbug.com/155640 https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:753: // TODO (gab): This was fixed before the official release of Windows 8 and thus On 2012/10/13 17:50:29, grt wrote: > On 2012/10/13 13:21:37, grt wrote: > > On 2012/10/13 03:33:12, gab wrote: > > > On 2012/10/13 00:34:26, grt wrote: > > > > i think it's too late for this to be on stable before Windows 8 GA, so > this > > > > comment is misleading. > > > > > > As mentioned above, my intent is to merge it in both M22 and M23 so that we > > > wouldn't have to deal with this verb ever being registered on a real Win8 > > Chrome > > > install. > > > > Merging it will not guarantee that. > > To elaborate, there will be a population of machines with M23 (which contains > the OS upgrade handler) that will be updated to Win8 before your change reaches > them. Updates are never rolled out to 100% of users. Right, but see my reply/clarification on the bug. > > To simplify the merge, I suggest moving this new code into a new function with > its own detailed comment that is invoked immediately before or after > RemoveBadWindows8RegistrationIfNeeded (and revert all changes to this function). > If I'm not mistaken, the logic to remove the run verb key can be simply "if > Windows 8". Will do. https://chromiumcodereview.appspot.com/11146003/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:796: InstallUtil::DeleteRegistryKey(root_key, run_verb_key); On 2012/10/13 17:50:29, grt wrote: > On 2012/10/13 15:17:50, gab wrote: > > On 2012/10/13 13:21:37, grt wrote: > > > On 2012/10/13 03:33:12, gab wrote: > > > > On 2012/10/13 00:34:26, grt wrote: > > > > > this could be in HKCU even for a system-level install, no? (see > > > > > RegisterChromeBrowser) > > > > > > > > No, on Win8 all user level stuff is in HKCU and system-level in HKLM. > > > > > > > > Technically it's possible for user-level stuff to be in HKLM if the user > > > > upgraded a Win7 install to a Win8 install, but we can't clean that up on > > > upgrade > > > > anyways so no need to bother... > > > > > > I expect there to be a great number of machines upgraded from Win7, so I > think > > > your statement should read "it's probable" rather than "it's possible". > > > > Well technically if this is merged in M22 and M23 now, people upgrading their > > Win7 to Win8 come Win8 Release will never even have the run verb registered. > > > > In fact, there is no way for us to register it in HKLM as we don't have admin > > rights on OSupgrade detection (so change "it's possible" to "it's not > > possible"). I think the current flow is to re-register everything in HKCU on > > WIn8. > > Ah, I just realized that the Win7 -> Win8 path is only relevant if the version > of Chrome present at the time of OS upgrade has the OS migration feature (i.e., > M23) since the Run verb registration isn't done prior to Win8. Well not quite, the migration can also happen on natural update, but given this goes in, a natural update will remove the run verb anyways...
Done (separate method). I'll remove the old one in a follow-up CL and will adjust the comment on the new one then if the merges are declined. Tested again, still works. Cheers, Gab http://codereview.chromium.org/11146003/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/11146003/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:753: // TODO (gab): This was fixed before the official release of Windows 8 and thus On 2012/10/13 19:02:09, gab wrote: > On 2012/10/13 17:50:29, grt wrote: > > On 2012/10/13 13:21:37, grt wrote: > > > On 2012/10/13 03:33:12, gab wrote: > > > > On 2012/10/13 00:34:26, grt wrote: > > > > > i think it's too late for this to be on stable before Windows 8 GA, so > > this > > > > > comment is misleading. > > > > > > > > As mentioned above, my intent is to merge it in both M22 and M23 so that > we > > > > wouldn't have to deal with this verb ever being registered on a real Win8 > > > Chrome > > > > install. > > > > > > Merging it will not guarantee that. > > > > To elaborate, there will be a population of machines with M23 (which contains > > the OS upgrade handler) that will be updated to Win8 before your change > reaches > > them. Updates are never rolled out to 100% of users. > > Right, but see my reply/clarification on the bug. > > > > > To simplify the merge, I suggest moving this new code into a new function with > > its own detailed comment that is invoked immediately before or after > > RemoveBadWindows8RegistrationIfNeeded (and revert all changes to this > function). > > If I'm not mistaken, the logic to remove the run verb key can be simply "if > > Windows 8". > > Will do. Done.
http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1019: // TODO (gab): This was fixed before the official release of Windows 8 and thus "official release" -> "general availability" http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1024: HKEY root_key = DetermineShellIntegrationRoot( wrap this function in: if (IsChromeMetroSupported()) { ... }? http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1024: HKEY root_key = DetermineShellIntegrationRoot( is root_key always HKCU? http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1025: InstallUtil::IsPerUserInstall(chrome_exe.c_str())); store the value of IsPerUserInstall() and re-use on line 1031.
Comments addressed PTAL. http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1019: // TODO (gab): This was fixed before the official release of Windows 8 and thus On 2012/10/16 01:43:03, grt wrote: > "official release" -> "general availability" Done. http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1024: HKEY root_key = DetermineShellIntegrationRoot( On 2012/10/16 01:43:03, grt wrote: > wrap this function in: > if (IsChromeMetroSupported()) { > ... > }? Done. http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1024: HKEY root_key = DetermineShellIntegrationRoot( On 2012/10/16 01:43:03, grt wrote: > is root_key always HKCU? HKCU on user-level installs, HKLM on system-level installs. Just figured I'd re-use this method instead of hardcoding this fact. http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1025: InstallUtil::IsPerUserInstall(chrome_exe.c_str())); On 2012/10/16 01:43:03, grt wrote: > store the value of IsPerUserInstall() and re-use on line 1031. Indeed! Done.
lgtm http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/11146003/diff/4004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1024: HKEY root_key = DetermineShellIntegrationRoot( On 2012/10/16 14:59:50, gab wrote: > On 2012/10/16 01:43:03, grt wrote: > > is root_key always HKCU? > > HKCU on user-level installs, HKLM on system-level installs. Just figured I'd > re-use this method instead of hardcoding this fact. Oh yeah. Good call.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11146003/12001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11146003/21001
Change committed as 162250 |
