Do not register Chrome's DelegateExecute on Win10+.

Do not register Chrome's DelegateExecute on Win10+.

And delete active registrations for installs that made it
to Win10 with such registrations (in place OS upgrade or
installs that were on Win10 already before this fix).

The deletions do not perform a pristine cleanup, but a
sufficient in place cleanup to disable delegate execution
while not being too intrusive on the codebase (no duplication
of knowledge of each single registry key).

Uninstall will take care of cleaning up the dead leaves.

BUG=516349
Committed: https://crrev.com/635098e6af574e60e3ebdc7e2ef38b23f9f3ab24
Cr-Commit-Position: refs/heads/master@{#346321}

[gab, 2015-08-20 19:42:58]

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

[gab, 2015-08-20 19:42:58]

Greg PTAL, I think this is the cleanest way to do it, but I'm open to
suggestions. I'm trying to avoid having mostly mirror logic in two places, e.g.:
 1) First place: if(metro) install registry x,y,z
 2) Second place: if(!metro) uninstall registry x,y,z

This is pretty much the first time we cleanup a value that is still being
installed (i.e. typically we deprecate something and add a cleanup path, but now
we need both install and cleanup paths at once), so had to mold shell_util.cc a
bit to make that possible.

Cheers,
Gab

[grt (UTC plus 2), 2015-08-24 15:08:20]

Seems a bit complex in a sense, but I can see how it avoids duplication
otherwise. Thanks for taking care of this!

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:178: // Identifies the type of removal this
RegistryEntry is flagged for if any.
"...for, if any."

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:180: // Default case, no flag.
// Default case: install the key/value.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:281: for (size_t i = 0; i <
arraysize(verbs); ++i) {
nit: for (const auto verb_and_id& : verbs) {

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:343: if (!app_info.delegate_clsid.empty()) {
    if (dist->GetCommandExecuteImplClsid(&app_info.delegate_clsid)) {
(and remove line 331)

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:348: // Need to get rid of the
DelegateExecute keys (verbs, etc.)
nit: move the comment out of the loop body so the loop can be just:
        // Remove the keys (not only their values) so that Windows will continue
        // to launch Chrome without a pesky association error.
        for (RegistryEntry* entry : delegate_execute_entries)
          entry->set_removal_flag(RemovalFlag::KEY);

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:391:
delegate_execute_entry->set_removal_flag(RemovalFlag::VALUE);
do away with the delegate_execute_entry local var and change this to:
        entries->back().set_removal_flag(RemovalFlag::VALUE);

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:681: // Generate work_item tasks required to
create (or potentially delete based on
nit: Generates...

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:687: name_);
this should be best-effort, no?
                                       name_)->set_ignore_failure(true);

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:689: items->AddDeleteRegKeyWorkItem(root,
key_path_, WorkItem::kWow64Default);
and this?

[gab, 2015-08-27 20:51:08]

Comments addressed, PTAL.

Thanks,
Gab

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:178: // Identifies the type of removal this
RegistryEntry is flagged for if any.
On 2015/08/24 15:08:20, grt wrote:
> "...for, if any."

Done.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:180: // Default case, no flag.
On 2015/08/24 15:08:19, grt wrote:
> // Default case: install the key/value.

Done.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:281: for (size_t i = 0; i <
arraysize(verbs); ++i) {
On 2015/08/24 15:08:19, grt wrote:
> nit: for (const auto verb_and_id& : verbs) {

Woohoo C++11 in shell_util :-)!

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:343: if (!app_info.delegate_clsid.empty()) {
On 2015/08/24 15:08:19, grt wrote:
>     if (dist->GetCommandExecuteImplClsid(&app_info.delegate_clsid)) {
> (and remove line 331)

Done.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:348: // Need to get rid of the
DelegateExecute keys (verbs, etc.)
On 2015/08/24 15:08:19, grt wrote:
> nit: move the comment out of the loop body so the loop can be just:
>         // Remove the keys (not only their values) so that Windows will
continue
>         // to launch Chrome without a pesky association error.
>         for (RegistryEntry* entry : delegate_execute_entries)
>           entry->set_removal_flag(RemovalFlag::KEY);

Done.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:391:
delegate_execute_entry->set_removal_flag(RemovalFlag::VALUE);
On 2015/08/24 15:08:20, grt wrote:
> do away with the delegate_execute_entry local var and change this to:
>         entries->back().set_removal_flag(RemovalFlag::VALUE);

Done.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:681: // Generate work_item tasks required to
create (or potentially delete based on
On 2015/08/24 15:08:19, grt wrote:
> nit: Generates...

Done.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:687: name_);
On 2015/08/24 15:08:20, grt wrote:
> this should be best-effort, no?
>                                        name_)->set_ignore_failure(true);

I don't think so, when I was figuring out what should be removed I could easily
get into states where Chrome fails to launch with an error dialog when only some
of the stuff was removed, so we should either make sure it's all gone or all
still there, in betweens are bad.

(if you were talking about the failing if the entry is already not present, this
is already the behavior of the delete work items)

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:689: items->AddDeleteRegKeyWorkItem(root,
key_path_, WorkItem::kWow64Default);
On 2015/08/24 15:08:19, grt wrote:
> and this?

Same thing.

[gab, 2015-08-27 21:44:06]

Just realized while testing this that I also need to fix IsChromeRegistered() to
return false when keys to be removed are present or updates will no-op and not
fix this in place.

[gab, 2015-08-27 21:48:53]

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:343: if (!app_info.delegate_clsid.empty()) {
On 2015/08/27 20:51:07, gab (OOO friday) wrote:
> On 2015/08/24 15:08:19, grt wrote:
> >     if (dist->GetCommandExecuteImplClsid(&app_info.delegate_clsid)) {
> > (and remove line 331)
> 
> Done.

Actually, no, GetProgIdEntries() needs app_info.delegate_clsid.

Which actually makes it more consistent since everybody checks for .empty() --
making the bool return on this GetCommandExecuteImplClsid() unused I think
(shall we clean it up in a follow-up?)

[gab, 2015-08-27 22:10:25]

On 2015/08/27 21:44:06, gab (OOO friday) wrote:
> Just realized while testing this that I also need to fix IsChromeRegistered()
to
> return false when keys to be removed are present or updates will no-op and not
> fix this in place.

Ok fixed this, in place removal on update now works, PTAL

Cheers!
Gab

[grt (UTC plus 2), 2015-08-29 01:38:41]

Nice catch on the IsChromeRegistered() fix!

LGTM provided that you add test coverage as requested below.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:343: if (!app_info.delegate_clsid.empty()) {
On 2015/08/27 21:48:53, gab (OOO friday) wrote:
> On 2015/08/27 20:51:07, gab (OOO friday) wrote:
> > On 2015/08/24 15:08:19, grt wrote:
> > >     if (dist->GetCommandExecuteImplClsid(&app_info.delegate_clsid)) {
> > > (and remove line 331)
> > 
> > Done.
> 
> Actually, no, GetProgIdEntries() needs app_info.delegate_clsid.

Ah! Subtle!

> Which actually makes it more consistent since everybody checks for .empty() --
> making the bool return on this GetCommandExecuteImplClsid() unused I think
> (shall we clean it up in a follow-up?)

Yeah. The bool-as-return-value is vestigial. The method once had several out
params. Now, however, it should just return the one string directly.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:687: name_);
On 2015/08/27 20:51:07, gab (OOO friday) wrote:
> On 2015/08/24 15:08:20, grt wrote:
> > this should be best-effort, no?
> >                                        name_)->set_ignore_failure(true);
> 
> I don't think so, when I was figuring out what should be removed I could
easily
> get into states where Chrome fails to launch with an error dialog when only
some
> of the stuff was removed, so we should either make sure it's all gone or all
> still there, in betweens are bad.
> 
> (if you were talking about the failing if the entry is already not present,
this
> is already the behavior of the delete work items)

It looks like DeleteRegValueWorkItemTest.DeleteNonExistentValue tests that the
work item succeeds if the value is not present, so that's good. Could you add a
test that the work item succeeds if the containing key is not present? The exact
last-error-code for RegOpenKeyEx when the key doesn't exist isn't documented. If
memory serves, it's ERROR_FILE_NOT_FOUND on some OS versions, and
ERROR_PATH_NOT_FOUND on others.

[gab, 2015-08-29 14:14:25]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2015-08-29 14:14:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294863006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294863006/60001

[gab, 2015-08-29 14:39:30]

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:343: if (!app_info.delegate_clsid.empty()) {
On 2015/08/29 01:38:41, grt wrote:
> On 2015/08/27 21:48:53, gab (OOO friday) wrote:
> > On 2015/08/27 20:51:07, gab (OOO friday) wrote:
> > > On 2015/08/24 15:08:19, grt wrote:
> > > >     if (dist->GetCommandExecuteImplClsid(&app_info.delegate_clsid)) {
> > > > (and remove line 331)
> > > 
> > > Done.
> > 
> > Actually, no, GetProgIdEntries() needs app_info.delegate_clsid.
> 
> Ah! Subtle!
> 
> > Which actually makes it more consistent since everybody checks for .empty()
--
> > making the bool return on this GetCommandExecuteImplClsid() unused I think
> > (shall we clean it up in a follow-up?)
> 
> Yeah. The bool-as-return-value is vestigial. The method once had several out
> params. Now, however, it should just return the one string directly.

Ok will do.

https://codereview.chromium.org/1294863006/diff/1/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:687: name_);
On 2015/08/29 01:38:41, grt wrote:
> On 2015/08/27 20:51:07, gab (OOO friday) wrote:
> > On 2015/08/24 15:08:20, grt wrote:
> > > this should be best-effort, no?
> > >                                        name_)->set_ignore_failure(true);
> > 
> > I don't think so, when I was figuring out what should be removed I could
> easily
> > get into states where Chrome fails to launch with an error dialog when only
> some
> > of the stuff was removed, so we should either make sure it's all gone or all
> > still there, in betweens are bad.
> > 
> > (if you were talking about the failing if the entry is already not present,
> this
> > is already the behavior of the delete work items)
> 
> It looks like DeleteRegValueWorkItemTest.DeleteNonExistentValue tests that the
> work item succeeds if the value is not present, so that's good. Could you add
a
> test that the work item succeeds if the containing key is not present? The
exact
> last-error-code for RegOpenKeyEx when the key doesn't exist isn't documented.
If
> memory serves, it's ERROR_FILE_NOT_FOUND on some OS versions, and
> ERROR_PATH_NOT_FOUND on others.

Done in follow-up CL @ https://codereview.chromium.org/1309873005/ (preferred
follow-up CL because I also modernized those tests as part of touching them).

ERROR_FILE_NOT_FOUND is what's used in other tests IIRC, pretty sure it passes
on all versions we support, but we can adjust if not.

[commit-bot: I haz the power, 2015-08-29 16:17:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-29 16:18:16]

Patchset 4 (id:??) landed as
https://crrev.com/635098e6af574e60e3ebdc7e2ef38b23f9f3ab24
Cr-Commit-Position: refs/heads/master@{#346321}