Don't register or use the DelegateExecute verb handler if it isn't present.

Don't register or use the DelegateExecute verb handler if it isn't present.

BUG=124666, 123994
TEST=install canary on a win8 box and note that the DelegateExecute verb handler isn't registered in HKCR and that registration for shell verbs don't refer to it.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133995

[grt (UTC plus 2), 2012-04-24 21:56:09]

This should make the COM registration for the DelegateExecute verb handler
conditional on the handler being present.  If it isn't present, stale
registrations should be removed.  I haven't actually tested this yet.  Let me
know if it doesn't work.  ;-)

[gab, 2012-04-24 22:36:19]

Arg... I can't upload as I don't "own" this issue...

I g2g for now, but here is the hacked shell_util code:

    // Before registering the delegate execute, make sure its class has been
    // registered. (If it hasn't been, don't register as this means there
    // was no delegate execute present in the installer archive)
    if (set_delegate_execute) {
      string16 delegate_execute_clsid(ShellUtil::kRegClasses);
      delegate_execute_clsid.push_back(FilePath::kSeparators[0]);
      delegate_execute_clsid.append(L"Wow6432Node\\CLSID\\");
      delegate_execute_clsid.append(delegate_guid);
      RegistryEntry delegate_execute_regkey = new RegistryEntry(
          delegate_execute_clsid, L"CommandExecuteImpl Class");
      set_delegate_execute = delegate_execute_regkey.ExistsInRegistry();
    }


This goes here in shell_util.cc::GetProgIdEntries:
...
    bool set_delegate_execute =
        base::win::GetVersion() >= base::win::VERSION_WIN8 &&
        dist->GetDelegateExecuteHandlerData(&delegate_guid, NULL, NULL, NULL);

-------------> HERE

    // DelegateExecute ProgId. Needed for Chrome Metro in Windows 8.
    if (set_delegate_execute) {
...



This will NOT remove the bad entries yet. I'll hack more on it tomorrow.

[grt (UTC plus 2), 2012-04-25 03:30:28]

Here's a CL that inhibits DelegateExecute handler registration and use when the
actual binary isn't present.

On the installer side (install_worker.cc), the COM registration is done when the
binary is in the to-be-moved archive.  There's no need to use a conditional work
item list since the presence of the file can be determined when the work item
list is being constructed.  If the file isn't present, any stale COM
registration is removed.

On the Chrome side (shell_util.cc), the association of the DelegateExecute
handler with the shell verbs is only done if the binary is present in the
installation.

I don't think we need to bend over backwards to remove stale registration since
I'm almost positive that ordinary canary installs of Chrome on Windows 8 don't
contain the shell verb association (at least, I couldn't make it happen with the
two versions I tried).  I believe that the reporter had installed the canary
build as plain-old-chrome (no --chrome-sxs on the command-line).  I've asked
her/him to clarify.

Gab: I decided to make the check in shell_util.cc go to the filesystem rather
than the registry to avoid the case where an older installer had put the COM
registration in place and it wasn't removed.  Seems safer this way.  Let me know
what you think.

[grt (UTC plus 2), 2012-04-25 03:39:59]

On 2012/04/25 03:30:28, grt wrote:
> I don't think we need to bend over backwards to remove stale registration
since
> I'm almost positive that ordinary canary installs of Chrome on Windows 8 don't
> contain the shell verb association (at least, I couldn't make it happen with
the
> two versions I tried).

Of note: this has hit the dev channel, so dev Chrome on Win8 is somewhat busted.

[gab, 2012-04-25 05:18:29]

See CR comments inline below.

I'm also wondering whether the dual-mode properties set on the shortcut are
going to be a problem if no delegate_execute handler is registered.

---------------------------
Break details and potential actions
---------------------------

This only breaks Windows 8 users who:
- Installed Canary by running the installer by hand (i.e. without
--chrome-sxs)).
or
- Have chrome dev-channel.

We definitely want to fix the issue, the question is do we also want to put code
to undo the bad registrations for the few installs out there having it.

We could add temporary fixing code that we remove after an iteration or two of
dev-channel (I don't think it needs to stick around forever given these users
are most likely very active and, very soon, no one will be left with the bad
update). 

This is probably not mandatory though if it turns out to be hairy given people
on Windows 8 with chrome-dev are advanced users well able to uninstall and
reinstall Chrome (we should put a note in the next dev-channel release though if
we go this way imo).

It is technically not impossible that someone installed Win8 and chrome-dev
today, shuts down that computer and only-reopens it to install release Win8 on
it in a couple months (i.e. updating Win8, leaving the bad Chrome on). But by
then hopefully our auto-update would kick in and actually place the currently
missing binaries in place in which case the currently bad registrations would
become correct.

--------------------------

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
chrome/installer/setup/install_worker.cc:1098: // TODO(grt): remove the extra
check for the .exe when it's ever-present.
Actually, will all distributions have the .exe? If not this check probably needs
to stay (maybe AddDelegateExecuteWorkItems is only called for Chrome now, but in
shell_util any distribution can call GetProgIdEntries for sure).

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:705: const FilePath& setup_path,
|setup_path| is now unused.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:708:
AddDelegateExecuteWorkItems(installer_state, FilePath(), Version(), product,
I don't like having to create empty FilePath and Version objects here, but I
realize there is no such thing as passing a null reference in C++. What about
making the interface use pointers instead of references? I guess that puts the
burden in AddDelegateExecuteWorkItems now having to check for null-ptrs..?

Just looking for C++ coding insights :)

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
File chrome/installer/util/install_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/install_util.cc:347: if (!version.get()) {
I think having the main logic first feels cleaner:
if (version.get()) {
  (main logic)
} else {
  LOG(ERROR) ...
} 

Alternatively I also like:
if (!version.get()) {
  LOG(ERROR) ...
  return false;
}
(main logic)

Your call :).

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:92: bool set_delegate_execute =
Do we want to also check && dist->CanSetAsDefault()? Given there is no point
setting up a delegate execute if we can't use Metro (but it probably doesn't
hurt doing so either and it would help having the registrations ready if the
Win8 rules magically change to allow non-default browsers in Metro).

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:94:
InstallUtil::HasDelegateExecuteHandler(dist, chrome_exe);
This is somewhat cleaner than the registry lookup however what I don't like is
that it depends on two independent parts of the code checking the same thing
(i.e. someone can change one and not the other).

Where as if its sequential (i.e. by checking registry entry here), we have:
delegate_execute present --> register its class
delegate_execute class registered --> register it as a handler for chrome

So that we can never do the latter if the former has not been done (which, if it
happened, would result in class not registered errors again (as we were getting
before doing the automatic registration if forgetting to call
delegate_execute.exe /regserver).

I suggest keeping the InstallUtil::HasDelegateExecuteHandler function as the
check is cleaner in this function than the inline thing I hacked together before
leaving, but changing the function to lookup in the registry. The way I had it
earlier it would look at use ExistsInRegistry which also confirms the value. I
feel we should add RegistryEntry.NameExistsInRegistry() as such:

  bool NameExistsInRegistry() {
    return (StatusInRegistryUnderRoot(HKEY_CURRENT_USER) != DOES_NOT_EXISTS ||
               StatusInRegistryUnderRoot(HKEY_LOCAL_MACHINE) !=
DOES_NOT_EXISTS));
  }

since we don't care about the value for this lookup imo.

Actually since ::RegistryEntry is not exposed outside of shell_util.cc we
probably need to move the HasDelegateExecuteHandler function here as well.

[grt (UTC plus 2), 2012-04-25 13:05:11]

Thanks for the comments; PTAL.

My gut tells me that we should do what we can to automatically fix Win8 dev
channel users on the next update since we have no way of contacting them to give
them specific instructions.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
chrome/installer/setup/install_worker.cc:1098: // TODO(grt): remove the extra
check for the .exe when it's ever-present.
On 2012/04/25 05:18:29, gab wrote:
> Actually, will all distributions have the .exe? If not this check probably
needs
> to stay (maybe AddDelegateExecuteWorkItems is only called for Chrome now, but
in
> shell_util any distribution can call GetProgIdEntries for sure).

The check on line 1075 ensures that this is only hit on Win8 for distros that
support the handler, so I think we're safe to remove the check once the handler
is fully deployed for Chromium and Google Chrome.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:705: const FilePath& setup_path,
On 2012/04/25 05:18:29, gab wrote:
> |setup_path| is now unused.

Good catch.  Done.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:708:
AddDelegateExecuteWorkItems(installer_state, FilePath(), Version(), product,
On 2012/04/25 05:18:29, gab wrote:
> I don't like having to create empty FilePath and Version objects here, but I
> realize there is no such thing as passing a null reference in C++. What about
> making the interface use pointers instead of references?

For the most part, pointers are used for out/inout params in Chromium.  Although
the style guide permits const Foo*, it discourages its use.  I think it
generally reduces readability to do so since it's so rare.

An alternative would be to create an AddRemoveDelegateExecuteWorkItems function,
or somesuch, that only takes the params required to remove the registration. 
I'm hesitant to do this, though, since the remove code is hit during installs
and because it separates the implementation of the two.  Also, there's precedent
in the installer for funcs to do double-duty like this.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
File chrome/installer/util/install_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/install_util.cc:347: if (!version.get()) {
On 2012/04/25 05:18:29, gab wrote:
> I think having the main logic first feels cleaner:

I often do, too.  When the else{} clause is a one or two line error case,
though, I tend to put it first.  As the main logic gets bigger (as it often
does), it pushes the error handling further and further away from its raison
d'etre, making it harder to review.  By putting it first, it's easy to see both
sides of the if/else regardless of how big the main logic gets.  Personal
preference...

> Alternatively I also like:
> if (!version.get()) {
>   LOG(ERROR) ...
>   return false;

Although I'm not fanatical about it, I prefer one exit to functions.  If your
curious, Tommi can give you some arguments about why multiple exits can
potentially add overhead.  :-)

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:92: bool set_delegate_execute =
On 2012/04/25 05:18:29, gab wrote:
> Do we want to also check && dist->CanSetAsDefault()?

This function is never called when !dist->CanSetAsDefault().  We could put in a
DCHECK if you think it's necessary, but I think it's part of the contract: this
is only for setting up the default browser.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:94:
InstallUtil::HasDelegateExecuteHandler(dist, chrome_exe);
On 2012/04/25 05:18:29, gab wrote:
> This is somewhat cleaner than the registry lookup however what I don't like is
> that it depends on two independent parts of the code checking the same thing
> (i.e. someone can change one and not the other).



> 
> Where as if its sequential (i.e. by checking registry entry here), we have:
> delegate_execute present --> register its class
> delegate_execute class registered --> register it as a handler for chrome
> 
> So that we can never do the latter if the former has not been done (which, if
it
> happened, would result in class not registered errors again (as we were
getting
> before doing the automatic registration if forgetting to call
> delegate_execute.exe /regserver).
> 
> I suggest keeping the InstallUtil::HasDelegateExecuteHandler function as the
> check is cleaner in this function than the inline thing I hacked together
before
> leaving, but changing the function to lookup in the registry. The way I had it
> earlier it would look at use ExistsInRegistry which also confirms the value. I
> feel we should add RegistryEntry.NameExistsInRegistry() as such:
> 
>   bool NameExistsInRegistry() {
>     return (StatusInRegistryUnderRoot(HKEY_CURRENT_USER) != DOES_NOT_EXISTS ||
>                StatusInRegistryUnderRoot(HKEY_LOCAL_MACHINE) !=
> DOES_NOT_EXISTS));
>   }
> 
> since we don't care about the value for this lookup imo.
> 
> Actually since ::RegistryEntry is not exposed outside of shell_util.cc we
> probably need to move the HasDelegateExecuteHandler function here as well.

I think it's safer for all logic to key off the same conditions.  With "if A
then B" and "if B then C", you leave the possibility that you end up doing C
because B happens to be satisfied when it shouldn't.  We've learned this lesson
the hard way in other parts of the installer where we have two pieces of state
that have to stay in sync.  Even though you think you've covered all your bases,
life happens and, for example, the power goes out at just the wrong time, so one
of the bits of state is left behind.

To make things more concrete, I think checking the registry is especially
un-safe in this case because all installs out there right now have the COM
registration in place.  If we somehow miss cleaning it up (entirely possible),
then this code would go ahead and put the DelegateExecute=GUID code in place.

Are you comfortable with having comments in the two checks referring to each
other so that they're less likely to drift?  Also, this check should be removed
eventually just like the other, so this is really a short-term change, is it
not?

[gab, 2012-04-25 13:44:02]

More comments inline.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/se...
chrome/installer/setup/install_worker.cc:1098: // TODO(grt): remove the extra
check for the .exe when it's ever-present.
On 2012/04/25 13:05:11, grt wrote:
> On 2012/04/25 05:18:29, gab wrote:
> > Actually, will all distributions have the .exe? If not this check probably
> needs
> > to stay (maybe AddDelegateExecuteWorkItems is only called for Chrome now,
but
> in
> > shell_util any distribution can call GetProgIdEntries for sure).
> 
> The check on line 1075 ensures that this is only hit on Win8 for distros that
> support the handler

Ah ok, perfect.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
File chrome/installer/util/install_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/install_util.cc:347: if (!version.get()) {
On 2012/04/25 13:05:11, grt wrote:
> On 2012/04/25 05:18:29, gab wrote:
> > I think having the main logic first feels cleaner:
> 
> I often do, too.  When the else{} clause is a one or two line error case,
> though, I tend to put it first.  As the main logic gets bigger (as it often
> does), it pushes the error handling further and further away from its raison
> d'etre, making it harder to review.  By putting it first, it's easy to see
both
> sides of the if/else regardless of how big the main logic gets.  Personal
> preference...
> 
> > Alternatively I also like:
> > if (!version.get()) {
> >   LOG(ERROR) ...
> >   return false;
> 
> Although I'm not fanatical about it, I prefer one exit to functions.  If your
> curious, Tommi can give you some arguments about why multiple exits can
> potentially add overhead.  :-)

Reading overhead or actual overhead in the binary?
I disagree with reading overhead given at least by seeing the return I know this
case is handled and done; where as without a return I have to skim over the
entire main logic to see if there is something else happening after the else
block (but before the global return).

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:92: bool set_delegate_execute =
On 2012/04/25 13:05:11, grt wrote:
> On 2012/04/25 05:18:29, gab wrote:
> > Do we want to also check && dist->CanSetAsDefault()?
> 
> This function is never called when !dist->CanSetAsDefault().  We could put in
a
> DCHECK if you think it's necessary, but I think it's part of the contract:
this
> is only for setting up the default browser.

Ah ok right. A DCHECK wouldn't hurt, but to be consistent it would need to be in
every similar calls... I don't think it's necessary to add this right now.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:94:
InstallUtil::HasDelegateExecuteHandler(dist, chrome_exe);
On 2012/04/25 13:05:11, grt wrote:
> On 2012/04/25 05:18:29, gab wrote:
> > This is somewhat cleaner than the registry lookup however what I don't like
is
> > that it depends on two independent parts of the code checking the same thing
> > (i.e. someone can change one and not the other).
> 
> 
> 
> > 
> > Where as if its sequential (i.e. by checking registry entry here), we have:
> > delegate_execute present --> register its class
> > delegate_execute class registered --> register it as a handler for chrome
> > 
> > So that we can never do the latter if the former has not been done (which,
if
> it
> > happened, would result in class not registered errors again (as we were
> getting
> > before doing the automatic registration if forgetting to call
> > delegate_execute.exe /regserver).
> > 
> > I suggest keeping the InstallUtil::HasDelegateExecuteHandler function as the
> > check is cleaner in this function than the inline thing I hacked together
> before
> > leaving, but changing the function to lookup in the registry. The way I had
it
> > earlier it would look at use ExistsInRegistry which also confirms the value.
I
> > feel we should add RegistryEntry.NameExistsInRegistry() as such:
> > 
> >   bool NameExistsInRegistry() {
> >     return (StatusInRegistryUnderRoot(HKEY_CURRENT_USER) != DOES_NOT_EXISTS
||
> >                StatusInRegistryUnderRoot(HKEY_LOCAL_MACHINE) !=
> > DOES_NOT_EXISTS));
> >   }
> > 
> > since we don't care about the value for this lookup imo.
> > 
> > Actually since ::RegistryEntry is not exposed outside of shell_util.cc we
> > probably need to move the HasDelegateExecuteHandler function here as well.
> 
> I think it's safer for all logic to key off the same conditions.  With "if A
> then B" and "if B then C", you leave the possibility that you end up doing C
> because B happens to be satisfied when it shouldn't.  We've learned this
lesson
> the hard way in other parts of the installer where we have two pieces of state
> that have to stay in sync.  Even though you think you've covered all your
bases,
> life happens and, for example, the power goes out at just the wrong time, so
one
> of the bits of state is left behind.

But in this case your code checking for A deletes entries B if they are not
needed, so that I don't see how we can get a B present when undesired.

> 
> To make things more concrete, I think checking the registry is especially
> un-safe in this case because all installs out there right now have the COM
> registration in place.  If we somehow miss cleaning it up (entirely possible),
> then this code would go ahead and put the DelegateExecute=GUID code in place.

The only case I see where we can't remove the COM registration in A is if it is
in HKLM and this is a user-level install, but the last patch also made sure that
all progids are registered in HKCU only for user-level installs.

I agree your method is safer code as is, but it's more programmer error-prone in
the future.

> 
> Are you comfortable with having comments in the two checks referring to each
> other so that they're less likely to drift?  Also, this check should be
removed
> eventually just like the other, so this is really a short-term change, is it
> not?

We can only remove this check in the future if we are confident that everyone
will keep dist->GetDelegateExecuteHandlerData() returning false for
distributions without the .exe ( I think this is safe to assume, but just
pointing it out ).

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:97: if (set_delegate_execute) {
FYI, this code still will not remove
\\Software\Classes\Chrome
and
\\Software\Classes\ChromeHTML\open\DelegateExecute
if !set_delegate_execute

This will be needed if we want to fix current bad installs on update.

https://chromiumcodereview.appspot.com/10213010/diff/16001/chrome/installer/s...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/16001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:1099: // cf. shell_util.cc's
GetProgIdEntries.
Not sure if "cf." is common use in Chromium, but I had to look it up :), so
thinking about readability, maybe a full word is better here.

https://chromiumcodereview.appspot.com/10213010/diff/16001/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/16001/chrome/installer/u...
chrome/installer/util/shell_util.cc:93: // cf. install_worker.cc's
AddDelegateExecuteWorkItems.
Same comment about "cf."

[gab, 2012-04-25 13:46:33]

> I'm also wondering whether the dual-mode properties set on the shortcut are
> going to be a problem if no delegate_execute handler is registered.

Looks like the shortcut properties are not a problem given users are reporting
simply deleting the bad registry keys manually fixes it.

http://crbug.com/123994/

[grt (UTC plus 2), 2012-04-25 14:33:49]

PTAL.  Now with egregious removal code!

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
File chrome/installer/util/install_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/install_util.cc:347: if (!version.get()) {
On 2012/04/25 13:44:02, gab wrote:
> On 2012/04/25 13:05:11, grt wrote:
> > Although I'm not fanatical about it, I prefer one exit to functions.  If
your
> > curious, Tommi can give you some arguments about why multiple exits can
> > potentially add overhead.  :-)
> 
> Reading overhead or actual overhead in the binary?

In the binary.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:92: bool set_delegate_execute =
On 2012/04/25 13:44:02, gab wrote:
> On 2012/04/25 13:05:11, grt wrote:
> > On 2012/04/25 05:18:29, gab wrote:
> > > Do we want to also check && dist->CanSetAsDefault()?
> > 
> > This function is never called when !dist->CanSetAsDefault().  We could put
in
> a
> > DCHECK if you think it's necessary, but I think it's part of the contract:
> this
> > is only for setting up the default browser.
> 
> Ah ok right. A DCHECK wouldn't hurt, but to be consistent it would need to be
in
> every similar calls... I don't think it's necessary to add this right now.

Groovy.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:94:
InstallUtil::HasDelegateExecuteHandler(dist, chrome_exe);
On 2012/04/25 13:44:02, gab wrote:
> On 2012/04/25 13:05:11, grt wrote:
> > On 2012/04/25 05:18:29, gab wrote:
> > > This is somewhat cleaner than the registry lookup however what I don't
like
> is
> > > that it depends on two independent parts of the code checking the same
thing
> > > (i.e. someone can change one and not the other).
> > 
> > 
> > 
> > > 
> > > Where as if its sequential (i.e. by checking registry entry here), we
have:
> > > delegate_execute present --> register its class
> > > delegate_execute class registered --> register it as a handler for chrome
> > > 
> > > So that we can never do the latter if the former has not been done (which,
> if
> > it
> > > happened, would result in class not registered errors again (as we were
> > getting
> > > before doing the automatic registration if forgetting to call
> > > delegate_execute.exe /regserver).
> > > 
> > > I suggest keeping the InstallUtil::HasDelegateExecuteHandler function as
the
> > > check is cleaner in this function than the inline thing I hacked together
> > before
> > > leaving, but changing the function to lookup in the registry. The way I
had
> it
> > > earlier it would look at use ExistsInRegistry which also confirms the
value.
> I
> > > feel we should add RegistryEntry.NameExistsInRegistry() as such:
> > > 
> > >   bool NameExistsInRegistry() {
> > >     return (StatusInRegistryUnderRoot(HKEY_CURRENT_USER) !=
DOES_NOT_EXISTS
> ||
> > >                StatusInRegistryUnderRoot(HKEY_LOCAL_MACHINE) !=
> > > DOES_NOT_EXISTS));
> > >   }
> > > 
> > > since we don't care about the value for this lookup imo.
> > > 
> > > Actually since ::RegistryEntry is not exposed outside of shell_util.cc we
> > > probably need to move the HasDelegateExecuteHandler function here as well.
> > 
> > I think it's safer for all logic to key off the same conditions.  With "if A
> > then B" and "if B then C", you leave the possibility that you end up doing C
> > because B happens to be satisfied when it shouldn't.  We've learned this
> lesson
> > the hard way in other parts of the installer where we have two pieces of
state
> > that have to stay in sync.  Even though you think you've covered all your
> bases,
> > life happens and, for example, the power goes out at just the wrong time, so
> one
> > of the bits of state is left behind.
> 
> But in this case your code checking for A deletes entries B if they are not
> needed,

Except when it doesn't.  Rather than mathematically prove that the
install_worker.cc codepath will be hit and do the right thing 100% of the time,
I think it's safer to make this code tolerate the cases when it isn't.

> so that I don't see how we can get a B present when undesired.
> 
> > 
> > To make things more concrete, I think checking the registry is especially
> > un-safe in this case because all installs out there right now have the COM
> > registration in place.  If we somehow miss cleaning it up (entirely
possible),
> > then this code would go ahead and put the DelegateExecute=GUID code in
place.
> 
> The only case I see where we can't remove the COM registration in A is if it
is
> in HKLM and this is a user-level install, but the last patch also made sure
that
> all progids are registered in HKCU only for user-level installs.
> 
> I agree your method is safer code as is, but it's more programmer error-prone
in
> the future.

I think this is a six-of-one, half-dozen-of-the-other case.  I hope this code
doesn't live long enough into the future to be an issue.

> 
> > 
> > Are you comfortable with having comments in the two checks referring to each
> > other so that they're less likely to drift?  Also, this check should be
> removed
> > eventually just like the other, so this is really a short-term change, is it
> > not?
> 
> We can only remove this check in the future if we are confident that everyone
> will keep dist->GetDelegateExecuteHandlerData() returning false for
> distributions without the .exe ( I think this is safe to assume, but just
> pointing it out ).

That's the intent for BrowserDistribution::GetDelegateExecuteHandlerData.  I've
added a clarifying comment to browser_distribution.h.

https://chromiumcodereview.appspot.com/10213010/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:97: if (set_delegate_execute) {
On 2012/04/25 13:44:02, gab wrote:
> FYI, this code still will not remove
> \\Software\Classes\Chrome
> and
> \\Software\Classes\ChromeHTML\open\DelegateExecute
> if !set_delegate_execute
> 
> This will be needed if we want to fix current bad installs on update.

Indeed.  I've added this to a more recent patchset.

https://chromiumcodereview.appspot.com/10213010/diff/16001/chrome/installer/s...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/16001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:1099: // cf. shell_util.cc's
GetProgIdEntries.
On 2012/04/25 13:44:03, gab wrote:
> Not sure if "cf." is common use in Chromium, but I had to look it up :), so
> thinking about readability, maybe a full word is better here.

Done.  :-)

https://chromiumcodereview.appspot.com/10213010/diff/16001/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/16001/chrome/installer/u...
chrome/installer/util/shell_util.cc:93: // cf. install_worker.cc's
AddDelegateExecuteWorkItems.
On 2012/04/25 13:44:03, gab wrote:
> Same comment about "cf."

Done.

[gab, 2012-04-25 14:59:05]

LGTM, one last comment about logging below.

https://chromiumcodereview.appspot.com/10213010/diff/10005/chrome/installer/s...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/10005/chrome/installer/s...
chrome/installer/setup/install_worker.cc:1180: VLOG(1) << "Adding
DelegateExecute verb handler COM unregistration items.";
Don't we want a slightly different log message for install/uninstall behavior
(otherwise we can't tell which path was hit by looking at the log...)?

[grt (UTC plus 2), 2012-04-25 15:09:21]

Done, thanks.

The removal code works for me when updating a dev channel install on a Win8 box,
so I think this'll fix it.  We should test with the next official build to
double-check.

https://chromiumcodereview.appspot.com/10213010/diff/10005/chrome/installer/s...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/10213010/diff/10005/chrome/installer/s...
chrome/installer/setup/install_worker.cc:1180: VLOG(1) << "Adding
DelegateExecute verb handler COM unregistration items.";
On 2012/04/25 14:59:05, gab wrote:
> Don't we want a slightly different log message for install/uninstall behavior
> (otherwise we can't tell which path was hit by looking at the log...)?

Moved "{,un}registration" closer to the front of the message.

[gab, 2012-04-25 15:16:35]

On 2012/04/25 15:09:21, grt wrote:
> Done, thanks.
> 
> The removal code works for me when updating a dev channel install on a Win8
box,
> so I think this'll fix it.  We should test with the next official build to
> double-check.
> 

Can we ask for an earlier next dev release when this goes in or is it not worth
it? I don't how many people have chrome-dev on Win8 atm.

[gab, 2012-04-25 18:57:29]

I heard you say you committed this right? However it's not in the CQ nor on the
waterfall if you dcommitted...? Did I hear the wrong thing?!

[grt (UTC plus 2), 2012-04-25 19:52:44]

Erik,

Here's that CL I mentioned.  Thanks for the review on short notice.

[erikwright (departed), 2012-04-25 20:49:31]

LGTM.

It doesn't seem that it would be hard to write a test that covers some parts of
this. I see a few things that could be covered:

1) Redirect the registry, manually create the registration, verify that
RemoveBadWindows8RegistrationIfNeeded removes it when appropriate.
1) Redirect the registry, manually create the registration, verify that
RemoveBadWindows8RegistrationIfNeeded doesn't touch it when appropriate.
2) Redirect the registry, manually create the registration, verify that
AddDelegateExecuteWorkItems creates items that remove it  (when appropriate).
3) Don't write the registration, verify that AddDelegateExecuteWorkItems doesn't
create it (when appropriate)

Tests that verify that AddDelegateExecuteWorkItems does create the registration
when it should were presumably already warranted by the previous implementation.
If they don't exist, it wouldn't hurt to create them too.

[commit-bot: I haz the power, 2012-04-25 20:50:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10213010/16003

[commit-bot: I haz the power, 2012-04-25 22:16:07]

Change committed as 133995