Make Chrome Frame registration and unregistration more robust.

chrome_frame/chrome_tab.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // chrome_tab.cc : Implementation of DLL Exports.
 
 // Include without path to make GYP build see it.
 #include "chrome_tab.h"  // NOLINT
 
 #include <atlsecurity.h>
 
 #include "base/at_exit.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/file_version_info.h"
 #include "base/logging.h"
 #include "base/logging_win.h"
+#include "base/memory/scoped_vector.h"
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #include "base/string_piece.h"
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
 #include "base/win/registry.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/installer/util/google_update_settings.h"
@@ skipping 455 matching lines @@
   ACTIVEX             = 0x0001,
   ACTIVEDOC           = 0x0002,
   GCF_PROTOCOL        = 0x0004,
   BHO_CLSID           = 0x0008,
   BHO_REGISTRATION    = 0x0010,
   TYPELIB             = 0x0020,
 
   ALL                 = 0xFFFF
 };
 
+class CustomRegistrationHelper {

[grt (UTC plus 2), 2011/09/02 19:01:35]

Looks like it wouldn't be hard to make this a pure interface rather than an
abstract base; remove reg_flags (see comments below) and change Do(reg) to
Do(is_system, reg).

[grt (UTC plus 2), 2011/09/02 19:01:35]

On second thought, I'd be sorely tempted to simplify this.  I think you could
whittle down each step down to just the (one- or two-line) Do() functions, then
make a constant table kinda like this:
typedef HRESULT (*RegistrationFn)(bool is_system, bool reg);
struct RegistrationStep {
  int step_id;
  uint32 step_condition;
  RegistrationFn registration_fn;
};
static const RegistrationStep registration_steps[] {
  { kStepSecuredMimeHandler, ACTIVEDOC, &RegisterSecuredMimeHandler },
...
};
And walk it with something like:
for (scan = ...; scan != end; ++scan) {
  if ((reg_flags & scan->step_condition) != 0) {
    HRESULT hr = scan->registration_fn(...);
    ...;
  }
}
It amounts to the same thing I suppose, but it might be less code overall.  I
won't be offended if you tell me to stuff it.

[robertshield, 2011/09/03 05:36:21]

Done.

[robertshield, 2011/09/03 05:36:21]

This is a much nicer approach, changed as you suggest.

+ public:
+  CustomRegistrationHelper(UINT reg_flags, bool is_system)

[grt (UTC plus 2), 2011/09/02 19:01:35]

i think uint32 is more stylistically appropriate.

[robertshield, 2011/09/03 05:36:21]

Done.

+      : reg_flags_(reg_flags), is_system_(is_system) {
+  }
+
+  HRESULT Run(bool reg) {
+    return Do(reg);
+  }
+
+  HRESULT Rollback(bool reg) {
+    return Do(!reg);
+  }
+
+ protected:
+  virtual HRESULT Do(bool reg) = 0;
+
+  UINT reg_flags_;
+  bool is_system_;
+};
+
+class CRHSecuredMimeHandler : public CustomRegistrationHelper {
+ public:
+  CRHSecuredMimeHandler(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {

[grt (UTC plus 2), 2011/09/02 19:01:35]

) OVERRIDE { here and in all other classes.

[robertshield, 2011/09/03 05:36:21]

No longer needed.

+    if (reg_flags_ & ACTIVEDOC) {
+      // Don't fail to unregister if we can't undo the secure mime

[grt (UTC plus 2), 2011/09/02 19:01:35]

This comment and code are stale; the decision of whether or not to continue/fail
unregistration is now handled by the calling code, no?  In other words, just
return S_OK or E_FAIL based on RegisterSecuredMimeHandler.  Better yet, follow
my advice on RSMH below and just return RSMH(...);

[robertshield, 2011/09/03 05:36:21]

Done.

+      // handler registration. This was observed getting hit during
+      // uninstallation.
+      if (!RegisterSecuredMimeHandler(reg ? true : false, is_system_) && reg)

[grt (UTC plus 2), 2011/09/02 19:01:35]

"reg ? true : false" -> "reg"

[robertshield, 2011/09/03 05:36:21]

Done.

+        return E_FAIL;
+      }
+    return S_OK;
+  }
+};
+

[grt (UTC plus 2), 2011/09/02 19:01:35]

This seems to have been lost:
class CRHActiveDoc : public CustomRegistrationHelper {
 ...
 private:
  HRESULT Do(bool reg) OVERRIDE {
    if (reg_flags_ & ACTIVEDOC)
      return ChromeActiveDocument::UpdateRegistry(reg);
  }
};
plus the corresponding addition to the vector down below.

[robertshield, 2011/09/03 05:36:21]

good catch

+class CRHActiveX : public CustomRegistrationHelper {
+ public:
+  CRHActiveX(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    if (reg_flags_ & ACTIVEX) {
+      // We have to call the static T::UpdateRegistry function instead of
+      // _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_ACTIVEX, reg)
+      // because there is specific OLEMISC replacement.
+      return ChromeFrameActivex::UpdateRegistry(reg);
+    }
+    return S_OK;
+  }
+};
+
+class CRHElevationPolicy : public CustomRegistrationHelper {
+ public:
+  CRHElevationPolicy(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    if ((reg_flags_ & (ACTIVEDOC | ACTIVEX)) && reg) {
+      // Register the elevation policy. We do this only for developer
+      // convenience as the installer is really responsible for doing this.
+      // Because of that, we do not unregister this policy and just leave that
+      // up to the installer.
+      _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_ELEVATION, reg);

[grt (UTC plus 2), 2011/09/02 19:01:35]

why no return this HRESULT?

[robertshield, 2011/09/03 05:36:21]

This is strictly a developer convenience thing. We don't gate registration on it
completing and indeed even the installer does this as a best-effort thing. As
such, I'm leery of changing the behaviour here.

I did make it only happen on Vista+ though, since it appears to always fail on
XP. 

+      RefreshElevationPolicy();

[grt (UTC plus 2), 2011/09/02 19:01:35]

why this not its own step with its own return HRESULT?

[robertshield, 2011/09/03 05:36:21]

Done.

+    }
+    return S_OK;
+  }
+};
+
+class CRHProtocol : public CustomRegistrationHelper {
+ public:
+  CRHProtocol(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    if (reg_flags_ & GCF_PROTOCOL) {
+      return _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEPROTOCOL, reg);
+    }
+    return S_OK;
+  }
+};
+
+class CRHBhoClsid : public CustomRegistrationHelper {
+ public:
+  CRHBhoClsid(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    if (reg_flags_ & BHO_CLSID) {
+      return Bho::UpdateRegistry(reg);
+    }
+    return S_OK;
+  }
+};
+
+class CRHBhoIERegistration : public CustomRegistrationHelper {
+ public:
+  CRHBhoIERegistration(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    if (reg_flags_ & BHO_REGISTRATION) {
+      if (is_system_) {
+        _AtlModule.UpdateRegistryFromResourceS(IDR_REGISTER_BHO, reg);

[grt (UTC plus 2), 2011/09/02 19:01:35]

why u no propagate error code?!?!!?

[robertshield, 2011/09/03 05:36:21]

Fixed up the bho registration to proagate error codes.

+      } else {
+        if (reg) {
+          // Setup the long running process:
+          SetupUserLevelHelper();

[grt (UTC plus 2), 2011/09/02 19:01:35]

or here

[robertshield, 2011/09/03 05:36:21]

Done.

+        } else {
+          // Unschedule the user-level helper. Note that we don't kill it here
+          // so that during updates we don't have a time window with no running
+          // helper. Uninstalls and updates will explicitly kill the helper from
+          // within the installer. Unregister existing run-at-startup entry.
+          base::win::RemoveCommandFromAutoRun(HKEY_CURRENT_USER, kRunKeyName);

[grt (UTC plus 2), 2011/09/02 19:01:35]

eh?

[robertshield, 2011/09/03 05:36:21]

Done.

+        }
+      }
+    }
+    return S_OK;
+  }
+};
+
+class CRHTypeLib : public CustomRegistrationHelper {
+ public:
+  CRHTypeLib(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    if (reg_flags_ & TYPELIB) {
+      if (reg && !is_system_) {
+        // Enables the RegisterTypeLib Function function to override default
+        // registry mappings under Windows Vista Service Pack 1 (SP1),
+        // Windows Server 2008, and later operating system versions
+        typedef void (WINAPI* OaEnablePerUserTypeLibReg)(void);
+        OaEnablePerUserTypeLibReg per_user_typelib_func =
+            reinterpret_cast<OaEnablePerUserTypeLibReg>(
+                GetProcAddress(GetModuleHandle(L"oleaut32.dll"),
+                               "OaEnablePerUserTLibRegistration"));
+        if (per_user_typelib_func) {
+          (*per_user_typelib_func)();
+        }
+      }
+      return reg ?
+          UtilRegisterTypeLib(_AtlComModule.m_hInstTypeLib,
+                              NULL, !is_system_) :
+          UtilUnRegisterTypeLib(_AtlComModule.m_hInstTypeLib,
+                                NULL, !is_system_);
+    }
+    return S_OK;
+  }
+};
+
+class CRHLegacyNPAPICleanup : public CustomRegistrationHelper {
+ public:
+  CRHLegacyNPAPICleanup(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    if (!reg) {
+      _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_NPAPI, reg);

[grt (UTC plus 2), 2011/09/02 19:01:35]

stylistically, i think this should return the error code just like all the
others; it's the job of the caller to ignore it if !reg.

[robertshield, 2011/09/03 05:36:21]

I tend to agree, but that means that I would need to add an extra field to the
RegistrationStep struct to indicate that an item is allowed to fail. Since this
is a one off, I'd rather handle it here with a loud comment.

[grt (UTC plus 2), 2011/09/06 14:56:55]

What makes this unregistration code special in that regard (this is in a !reg
block)?  All other handlers return their error code even if !reg, correct?  Is
the special thing that this is a best-case cleanup which should be allowed to
fail even if a policy change were to be made in the future to fail if any other
unregistration code were to fail?

+      UtilRemovePersistentNPAPIMarker();
+    }
+    // Ignore failures.

[grt (UTC plus 2), 2011/09/02 19:01:35]

bad comment

[robertshield, 2011/09/03 05:36:21]

the comment was correct, just unwanted ;)

[grt (UTC plus 2), 2011/09/06 14:56:55]

"Bad" in that it is in the wrong location.  Methinks it's more clear if the
comment saying "Ignore failures" immediately precedes the operation that might
fail.  In this way, the reader doesn't have to look down to know why the return
value is ignored.  On the other hand, I do see the logic in putting it down here
("Ignore failures from anywhere up there.")

+    return S_OK;
+  }
+};
+
+class CRHAppId : public CustomRegistrationHelper {
+ public:
+  CRHAppId(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    return _AtlModule.UpdateRegistryAppId(reg);
+  }
+};
+
+class CRHUserAgent : public CustomRegistrationHelper {
+ public:
+  CRHUserAgent(UINT reg_flags, bool is_system)
+      : CustomRegistrationHelper(reg_flags, is_system) {}
+
+ private:
+  HRESULT Do(bool reg) {
+    if (reg) {
+      return SetChromeFrameUA(is_system_, L"1");
+    } else {
+      return SetChromeFrameUA(is_system_, NULL);
+    }
+  }
+};
+
+// Mux the failure step into the hresult. We take only the first four bits
+// and stick those into the top four bits of the facility code. We also set the
+// Customer bit to be polite. Graphically, we write our error code to the
+// bits marked with ^:
+// |                    1                   2                   3  |
+// |0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1|

[grt (UTC plus 2), 2011/09/02 19:01:35]

Interesting bit number choice.  I think it'd be more clear to make the least
significant bit #0.

[robertshield, 2011/09/03 05:36:21]

I had copied this from the msdn link below, but sure, I can invert it.

[grt (UTC plus 2), 2011/09/06 14:56:55]

Funny: winerror.h has it the other way. :-)

+// |S|R|C|N|X|Facility             |Code                           |
+//      ^     ^ ^ ^ ^
+// See http://msdn.microsoft.com/en-us/library/cc231198(PROT.10).aspx for
+// more details on HRESULTS.
+//
+// The resulting error can be extracted by:
+// error_code = (fiddled_hr & 0x07800000) >> 23
+HRESULT MuxErrorIntoHRESULT(HRESULT hr, uint16 error_code) {
+  DCHECK((error_code & 15) == error_code);

[grt (UTC plus 2), 2011/09/02 19:01:35]

nit: i find 0x0F more obvious than 15.  I think error_code should be an int,
though, so:
DCHECK_GE(error_code, 0);
DCHECK LE(error_code, 15);
would be even better.

[robertshield, 2011/09/03 05:36:21]

Done.

+
+  // Check that our four desired bits are clear.
+  // 0xF87FFFFF == 11111000011111111111111111111111
+  DCHECK(static_cast<HRESULT>(hr & 0xF87FFFFF) == hr) << hr;

[grt (UTC plus 2), 2011/09/02 19:01:35]

DCHECK_EQ(static_cast<HRESULT>(hr & 0xF87FFFFF), hr); ?

[robertshield, 2011/09/03 05:36:21]

Done.

+
+  HRESULT fiddled_hr = ((error_code & 15) << 23) | hr;

[grt (UTC plus 2), 2011/09/02 19:01:35]

15 -> 0x0F and you may need to stick a cast in there if error_code is an int.

[robertshield, 2011/09/03 05:36:21]

Done.

+  fiddled_hr |= 1 << 29;  // Set the customer bit.
+
+  return fiddled_hr;
+}
+
 STDAPI CustomRegistration(UINT reg_flags, BOOL reg, bool is_system) {

[grt (UTC plus 2), 2011/09/02 19:01:35]

Why is this STDAPI?  Change to just returning an HRESULT unless I'm missing
something.

[robertshield, 2011/09/03 05:36:21]

I think once upon a time this was intended to be an export. No more though, so
changed.

   UINT flags = reg_flags;
 
   if (reg && (flags & (ACTIVEDOC | ACTIVEX)))
     flags |= (TYPELIB | GCF_PROTOCOL);
 
   HRESULT hr = S_OK;
 
   // Set the flag that gets checked in AddCommonRGSReplacements before doing
   // registration work.
   _AtlModule.do_system_registration_ = is_system;
 
-  if ((hr == S_OK) && (flags & ACTIVEDOC)) {
-    // Don't fail to unregister if we can't undo the secure mime
-    // handler registration. This was observed getting hit during
-    // uninstallation.
-    if (!RegisterSecuredMimeHandler(reg ? true : false, is_system) && reg)
-      return E_FAIL;
-    hr = ChromeActiveDocument::UpdateRegistry(reg);
-  }
-
-  if ((hr == S_OK) && (flags & ACTIVEX)) {
-    // We have to call the static T::UpdateRegistry function instead of
-    // _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_ACTIVEX, reg)
-    // because there is specific OLEMISC replacement.
-    hr = ChromeFrameActivex::UpdateRegistry(reg);
-  }
-
-  // Register the elevation policy.  We do this only for developer convenience
-  // as the installer is really responsible for doing this.
-  // Because of that, we do not unregister this policy and just leave that up
-  // to the installer.
-  if (hr == S_OK && (flags & (ACTIVEDOC | ACTIVEX)) && reg) {
-    _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_ELEVATION, reg);
-    RefreshElevationPolicy();
-  }
-
-  if ((hr == S_OK) && (flags & GCF_PROTOCOL)) {
-    hr = _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEPROTOCOL, reg);
-  }
-
-  if ((hr == S_OK) && (flags & BHO_CLSID)) {
-    hr = Bho::UpdateRegistry(reg);
-  }
-
-  if ((hr == S_OK) && (flags & BHO_REGISTRATION)) {
-    if (is_system) {
-      _AtlModule.UpdateRegistryFromResourceS(IDR_REGISTER_BHO, reg);
-    } else {
-      if (reg) {
-        // Setup the long running process:
-        SetupUserLevelHelper();
-      } else {
-        // Unschedule the user-level helper. Note that we don't kill it here so
-        // that during updates we don't have a time window with no running
-        // helper. Uninstalls and updates will explicitly kill the helper from
-        // within the installer. Unregister existing run-at-startup entry.
-        base::win::RemoveCommandFromAutoRun(HKEY_CURRENT_USER, kRunKeyName);
+  ScopedVector<CustomRegistrationHelper> registration_helpers;

[grt (UTC plus 2), 2011/09/02 19:01:35]

If you stick with the classes and vector approach, I think it's easier to follow
what's going on if the helpers are added to the vector only if dictated by
|flags|.  See below for how I think the failure step should be handled.

[robertshield, 2011/09/03 05:36:21]

Approach changed.

+  registration_helpers.push_back(
+      new CRHSecuredMimeHandler(reg_flags, is_system));  // 0

[grt (UTC plus 2), 2011/09/02 19:01:35]

|flags| rather than |reg_flags| should be used here and everywhere below.

[robertshield, 2011/09/03 05:36:21]

Done.

+  registration_helpers.push_back(
+      new CRHActiveX(reg_flags, is_system));             // 1
+  registration_helpers.push_back(
+      new CRHElevationPolicy(reg_flags, is_system));     // 2
+  registration_helpers.push_back(
+      new CRHProtocol(reg_flags, is_system));            // 3
+  registration_helpers.push_back(
+      new CRHBhoClsid(reg_flags, is_system));            // 4
+  registration_helpers.push_back(
+      new CRHBhoIERegistration(reg_flags, is_system));   // 5
+  registration_helpers.push_back(
+      new CRHTypeLib(reg_flags, is_system));             // 6
+  registration_helpers.push_back(
+      new CRHLegacyNPAPICleanup(reg_flags, is_system));  // 7
+  registration_helpers.push_back(
+      new CRHAppId(reg_flags, is_system));               // 8
+  registration_helpers.push_back(
+      new CRHUserAgent(reg_flags, is_system));           // 9
+
+  ScopedVector<CustomRegistrationHelper>::iterator reg_iter(
+      registration_helpers->begin());
+
+  bool do_register = (reg == TRUE);

[grt (UTC plus 2), 2011/09/02 19:01:35]

reg != FALSE, unless you take my request to change BOOL to bool.

[robertshield, 2011/09/03 05:36:21]

Done.

+  bool rollback = false;
+
+  // This value will be muxed into the hresult in case of failure. It will
+  // correspond to the index of the failing step.

[grt (UTC plus 2), 2011/09/02 19:01:35]

Using the index of the step introduces a requirement that the current set of
steps never change.  I think this is fragile.  How about adding an int
GetStepIdentifier() = 0; to the base interface so that their numbering is
defined explicitly (an enum, integer constants, whatever)?

[robertshield, 2011/09/03 05:36:21]

Thought about that but figured it would add even more code bloat. Since I
switched to using an array of RegistrationSteps, I feel an int index is
appropriate. What say you?

[grt (UTC plus 2), 2011/09/06 14:56:55]

It just means that you have to refer to the exact same version of the source to
figure out on which step a failure occurred.  I find this fragile.  A
non-verbose installer log will have the stage + HRESULT but I don't think it
will have the version #.  It seems to me that this introduces an opportunity for
someone to unknowingly dig up the wrong version of the .cc file to map a stage #
back to its meaning.  Head scratching may ensue.  Looks like the latest edition
of the change uses an enum, so I'm satisfied.

+  uint16 registration_failure_step = 0;

[grt (UTC plus 2), 2011/09/02 19:01:35]

The arbiters of style say to use "int" for this.

[robertshield, 2011/09/03 05:36:21]

Done.

+
+  for (; reg_iter != registration_helpers->end(); ++reg_iter) {
+    if (!SUCCEEDED(hr = (*reg_iter)->Run(do_register))) {

[grt (UTC plus 2), 2011/09/02 19:01:35]

I don't know that this is banned by the style guide, but assignment within macro
invocation within conditional seems a bit dense.  I think
hr = ...;
if (!SUCCEEDED(hr) && do_register) { ... }
is more readable

[robertshield, 2011/09/03 05:36:21]

Done.

+      if (do_register) {
+        // Only rollback during registration. During unregistration keep going.
+        rollback = true;
+        break;
       }
     }
-  }
-
-  if ((hr == S_OK) && (flags & TYPELIB)) {
-    if (reg && !is_system) {
-      // Enables the RegisterTypeLib Function function to override default
-      // registry mappings under Windows Vista Service Pack 1 (SP1),
-      // Windows Server 2008, and later operating system versions
-      typedef void (WINAPI* OaEnablePerUserTypeLibReg)(void);
-      OaEnablePerUserTypeLibReg per_user_typelib_func =
-          reinterpret_cast<OaEnablePerUserTypeLibReg>(
-              GetProcAddress(GetModuleHandle(L"oleaut32.dll"),
-                             "OaEnablePerUserTLibRegistration"));
-      if (per_user_typelib_func) {
-        (*per_user_typelib_func)();
-      }
-    }
-    hr = (reg)?
-        UtilRegisterTypeLib(_AtlComModule.m_hInstTypeLib, NULL, !is_system) :
-        UtilUnRegisterTypeLib(_AtlComModule.m_hInstTypeLib, NULL, !is_system);
-  }
-
-  // Unconditionally remove NPAPI registration when unregistering any component.
-  if ((hr == S_OK) && !reg) {
-    // Ignore failures.
-    _AtlModule.UpdateRegistryFromResourceS(IDR_CHROMEFRAME_NPAPI, reg);
-    UtilRemovePersistentNPAPIMarker();
-  }
-
-  if (hr == S_OK) {
-    hr = _AtlModule.UpdateRegistryAppId(reg);
-  }
-
-  if (hr == S_OK) {
-    if (reg) {
-      hr = SetChromeFrameUA(is_system, L"1");
-    } else {
-      hr = SetChromeFrameUA(is_system, NULL);
-    }
-  }
-  return hr;
+    registration_failure_step++;
+  }
+
+  if (rollback) {
+    // Rollback all actions that preceded reg_iter's current position. Note that
+    // this does not rollback the failing action.

[grt (UTC plus 2), 2011/09/02 19:01:35]

Consider rolling back the failing action.  Otherwise an action that does three
things has to manage its own internal rollback mechanism.

[robertshield, 2011/09/03 05:36:21]

Done.

+    ScopedVector<CustomRegistrationHelper>::reverse_iterator rollback_iter(

[grt (UTC plus 2), 2011/09/02 19:01:35]

I don't see an advantage to making a new object for this rather than just
walking reg_iter backwards to the beginning.  I think it's more obvious to just
--reg_iter.

[robertshield, 2011/09/03 05:36:21]

Changed to use an index.

+        reg_iter);
+    for (; rollback_iter != registration_helpers->rend(); ++rollback_iter) {
+      (*rollback_iter)->Rollback(do_register);
+    }
+  }
+
+  return MuxErrorIntoHRESULT(hr, registration_failure_step);

[grt (UTC plus 2), 2011/09/02 19:01:35]

Consider changing SelfRegWorkItem::RegisterDll as follows:
- change PLOG(ERROR) to LOG(ERROR) since the former logs the last error code,
which we don't care about
- check for the customer bit in the HRESULT and unmux the step number when
logging.
I think it'd be more useful if the log contains the real HRESULT and the step
number since N weeks down the road (where N is some small number) I, at least,
will have forgotten about this muxing and will have a heck of a time trying to
figure out what the undefined HRESULT means.

[robertshield, 2011/09/03 05:36:21]

Good idea, so changed.

 }
 
-
-
 // DllRegisterServer - Adds entries to the system registry
 STDAPI DllRegisterServer() {
   UINT flags =  ACTIVEX | ACTIVEDOC | TYPELIB | GCF_PROTOCOL |
                 BHO_CLSID | BHO_REGISTRATION;
 
   HRESULT hr = CustomRegistration(flags, TRUE, true);

[grt (UTC plus 2), 2011/09/02 19:01:35]

TRUE, true?  Knowing that you didn't commit this sin in this CL, please help me
by changing the BOOL to bool or 'splain to me why this is correct.

[robertshield, 2011/09/03 05:36:21]

Done.

   if (SUCCEEDED(hr)) {
     SetupRunOnce();
   }
 
   return hr;
 }
 
 // DllUnregisterServer - Removes entries from the system registry
 STDAPI DllUnregisterServer() {
   HRESULT hr = CustomRegistration(ALL, FALSE, true);
   return hr;
 }
 
 // DllRegisterServer - Adds entries to the HKCU hive in the registry

[grt (UTC plus 2), 2011/09/02 19:01:35]

Please fix comment while you're here.

[robertshield, 2011/09/03 05:36:21]

Done.

 STDAPI DllRegisterUserServer() {
   UINT flags =  ACTIVEX | ACTIVEDOC | TYPELIB | GCF_PROTOCOL |
                 BHO_CLSID | BHO_REGISTRATION;
 
   HRESULT hr = CustomRegistration(flags, TRUE, false);
   if (SUCCEEDED(hr)) {
     SetupRunOnce();
   }
 
   return hr;
 }
 
 // DllRegisterServer - Removes entries from the HKCU hive in the registry.

[grt (UTC plus 2), 2011/09/02 19:01:35]

Please fix comment while you're here.

[robertshield, 2011/09/03 05:36:21]

Done.

 STDAPI DllUnregisterUserServer() {
   HRESULT hr = CustomRegistration(ALL, FALSE, false);
   return hr;
 }
 
 class SecurityDescBackup {
  public:
   explicit SecurityDescBackup(const std::wstring& backup_key)
       : backup_key_name_(backup_key) {}
   ~SecurityDescBackup() {}
@@ skipping 99 matching lines @@
     return user_;
   }
 
  private:
   CAccessToken token_;
   CTokenPrivileges take_ownership_;
   CTokenPrivileges restore_;
   CSid user_;
 };
 
 static bool SetOrDeleteMimeHandlerKey(bool set, HKEY root_key) {

[grt (UTC plus 2), 2011/09/02 19:01:35]

change this to return an HRESULT.

[robertshield, 2011/09/03 05:36:21]

Done.

   std::wstring key_name = kInternetSettings;
   key_name.append(L"\\Secure Mime Handlers");
   RegKey key(root_key, key_name.c_str(), KEY_READ | KEY_WRITE);
   if (!key.Valid())
     return false;
 
   LONG result1 = ERROR_SUCCESS;
   LONG result2 = ERROR_SUCCESS;
   if (set) {
     result1 = key.WriteValue(L"ChromeTab.ChromeActiveDocument", 1);
     result2 = key.WriteValue(L"ChromeTab.ChromeActiveDocument.1", 1);
   } else {
     result1 = key.DeleteValue(L"ChromeTab.ChromeActiveDocument");
     result2 = key.DeleteValue(L"ChromeTab.ChromeActiveDocument.1");
   }
 
   return (result2 == ERROR_SUCCESS) && (result2 == ERROR_SUCCESS);

[grt (UTC plus 2), 2011/09/02 19:01:35]

Boy, this is funny.  I'm sure I reviewed this and gave an LGTM.  sigh.  Change
this to:
return result1 != ERROR_SUCCESS ? HRESULT_FROM_WIN32(result1) :
HRESULT_FROM_WIN32(result2);

[robertshield, 2011/09/03 05:36:21]

Yes, this is awesome. Fixed.

 }
 
 bool RegisterSecuredMimeHandler(bool enable, bool is_system) {

[grt (UTC plus 2), 2011/09/02 19:01:35]

change this to return an HRESULT

[robertshield, 2011/09/03 05:36:21]

Done.

   if (!is_system) {
     return SetOrDeleteMimeHandlerKey(enable, HKEY_CURRENT_USER);
   } else if (base::win::GetVersion() < base::win::VERSION_VISTA) {
     return SetOrDeleteMimeHandlerKey(enable, HKEY_LOCAL_MACHINE);
   }
 
   std::wstring mime_key = kInternetSettings;
   mime_key.append(L"\\Secure Mime Handlers");
   std::wstring backup_key = kInternetSettings;
   backup_key.append(L"\\__backup_SMH__");
   std::wstring object_name = L"MACHINE\\";
   object_name.append(mime_key);
 
   TokenWithPrivileges token_;
   if (!token_.EnablePrivileges())
     return false;

[grt (UTC plus 2), 2011/09/02 19:01:35]

return E_FAIL or HRESULT_FROM_WIN32(GetLastError()) or somefin'.

[robertshield, 2011/09/03 05:36:21]

Done.

 
   // If there is a backup key - something bad happened; try to restore
   // security on "Secure Mime Handlers" from the backup.
   SecurityDescBackup backup(backup_key);
   backup.RestoreSecurity(object_name.c_str());
 
   // Read old security descriptor of the Mime key first.
   CSecurityDesc sd;
   if (!AtlGetSecurityDescriptor(object_name.c_str(), SE_REGISTRY_KEY, &sd)) {
     return false;
   }
 
   backup.SaveSecurity(sd);
   bool result = false;
   // set new owner
   if (AtlSetOwnerSid(object_name.c_str(), SE_REGISTRY_KEY, token_.GetUser())) {
     // set new dacl
     CDacl new_dacl;
     sd.GetDacl(&new_dacl);
     new_dacl.AddAllowedAce(token_.GetUser(), GENERIC_WRITE | GENERIC_READ);
     if (AtlSetDacl(object_name.c_str(), SE_REGISTRY_KEY, new_dacl)) {
       result = SetOrDeleteMimeHandlerKey(enable, HKEY_LOCAL_MACHINE);
     }
   }
 
   backup.RestoreSecurity(object_name.c_str());
   return result;
 }