Fix in-use updates for Chrome Frame.

Fix in-use updates for Chrome Frame.

On in-use updates, make a copy of the old chrome launcher's IE low rights elevation policy prior to registering the new npchrome_frame.dll so that running instances of IE can still launch Chrome.

In so doing, I also removed elevation policy addition/removal code from the installer so that npchrome_frame.dll's {un,}registration code is the one and only place where this is done.

BUG=95810
TEST=Install a previous version of GCF, run IE and visit some page that activates GCF, update to a version of GCF containing this fix, then try to visit another page that will activate GCF.  If all goes well, you won't see an IE security prompt.

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

[grt (UTC plus 2), 2011-09-22 19:28:35]

http://codereview.chromium.org/7976045/diff/10/chrome/chrome_installer.gypi
File chrome/chrome_installer.gypi (right):

http://codereview.chromium.org/7976045/diff/10/chrome/chrome_installer.gypi#n...
chrome/chrome_installer.gypi:345:
'installer/util/installation_validation_helper.cc',
i already have a pending change that moves these down where they belong.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/setup_m...
File chrome/installer/setup/setup_main.cc (left):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/setup_m...
chrome/installer/setup/setup_main.cc:1263: const Products& products =
installer_state.products();
note: i pulled this out into its own function since it was getting big.

[robertshield, 2011-09-22 20:34:10]

Looking good, but I abhor MaybeFoo() methods.. 

do they do Foo()? do they do something else instead? nobody
knows!</facetiousness> :)

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:988: // rather than do something like
StringFromGUID2(__uuidof(ChromeFrame), ...).
If there is nothing that constrains the GUID from matching the Chrome Frame one,
why not keep it as it was, and reuse the GUID? 

There are some maintenance advantages since code in chrome_tab.cc still refers
to the elevation policy as well.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:989: const wchar_t
kIELowRightsPolicyGuid[] =
Sometimes we call things ElevationPolicy and others LowRightsPolicy, I think
this CL should standardize on ElevationPolicy, so kIELowRightsPolicyGuid ->
kIEElevationPolicyGuid, etc.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:997: void
GetIELowRightsElevationPolicyKeyPath(ElevationPolicyId policy,
Drop the "LowRights" bit of the name.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:999: DCHECK(policy ==
CURRENT_ELEVATION_POLICY || policy == OLD_ELEVATION_POLICY);
Do we commonly dcheck that enum values are one of the defined values?

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1013: bool
InstallerOwnsOldLowRightsPolicies(const InstallerState& installer_state) {
InstallerOwnsOldLowRightsPolicies -> InstallerOwnsOldElevationPolicies

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1037: void
MaybeAddCopyIELowRightsPolicyWorkItems(
what about instead of having these MaybeFoo methods, we dropped the "Maybe" part
and called them from AddChromeFrameWorkItems above? That would let us get rid of
the InstallerOwnsOldLowRightsPolicies function.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
File chrome/installer/setup/install_worker.h (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.h:142: // Unregisters the "opv" version of
ChromeLauncher from IE's low rights
Unregisters -> conditionally

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.h:144: void
MaybeAddDeleteOldIELowRightsPolicyWorkItems(
Replace the LowRights bit of the name with "Elevation"?

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.h:150: void
MaybeAddCopyIELowRightsPolicyWorkItems(
ditto

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/setup_m...
File chrome/installer/setup/setup_main.cc (left):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/setup_m...
chrome/installer/setup/setup_main.cc:1263: const Products& products =
installer_state.products();
On 2011/09/22 19:28:35, grt wrote:
> note: i pulled this out into its own function since it was getting big.

bene

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/setup_m...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/setup_m...
chrome/installer/setup/setup_main.cc:209: // exist.
Drop the Maybe, wrap this in an "if we're doing CF stuff" check?

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/uninsta...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/uninsta...
chrome/installer/setup/uninstall.cc:803:
ProcessIELowRightsPolicyWorkItems(installer_state);
Feels like it would be okay to wrap this in an is_chrome_frame check as per
other checks above (related to comment about removing the "Maybe" from the EP
methods.

[grt (UTC plus 2), 2011-09-23 04:16:49]

Thanks for the review.  Let the bikeshedding begin!

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:988: // rather than do something like
StringFromGUID2(__uuidof(ChromeFrame), ...).
On 2011/09/22 20:34:10, robertshield wrote:
> If there is nothing that constrains the GUID from matching the Chrome Frame
one,
> why not keep it as it was, and reuse the GUID?

Because the reader could be confused into thinking that the "old" one won't work
since it doesn't match the class GUID, and because I have to make a function
call to get it.  Maybe those aren't very good reasons, though.

> There are some maintenance advantages since code in chrome_tab.cc still refers
> to the elevation policy as well.

I'm not so sure that the maintenance advantages you wish for are actually there.
 :-)

The references in chrome_tab.cc did not refer to the GUID in any way;
chrome_frame_elevation.rgs hard-coded the GUID.  Had the ChromeFrame class GUID
changed for some reason, DLL registration would have used the old value while
the previous code here that did the StringFromGUID2 thing would have used the
new value.  This would be bad, as they'd be two trains passing in the night.

IMO, having two baked-in constants is a moderate step up from that.  I agree,
however, that that sucks, too.  Thanks for calling me on it.

I've changed it so that both places now derive a string from a constant out of
the .idl file.  Let me know what you think.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:989: const wchar_t
kIELowRightsPolicyGuid[] =
On 2011/09/22 20:34:10, robertshield wrote:
> Sometimes we call things ElevationPolicy and others LowRightsPolicy, I think
> this CL should standardize on ElevationPolicy, so kIELowRightsPolicyGuid ->
> kIEElevationPolicyGuid, etc.

GCF requires some entries in two subkeys of IE's "Low Rights" key.  One of those
is called "ElevationPolicy", and the other is called "DragDrop".  I thought that
referring to these two things as "ElevationPolicy" wasn't ideal, so I started
collectively referring to them as "Low Rights policies".  For the in-use update
case, we only need to copy the ElevationPolicy since the DragDrop entry points
to (unversioned) chrome.

My preference is for the GUIDs to be labeled at the level of "Low Rights"
thingamaboobs.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:997: void
GetIELowRightsElevationPolicyKeyPath(ElevationPolicyId policy,
On 2011/09/22 20:34:10, robertshield wrote:
> Drop the "LowRights" bit of the name.

This function returns the path to the "ElevationPolicy" subkey of GCF's two "Low
Rights" keys.  I prefer to leave it as-is.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:999: DCHECK(policy ==
CURRENT_ELEVATION_POLICY || policy == OLD_ELEVATION_POLICY);
On 2011/09/22 20:34:10, robertshield wrote:
> Do we commonly dcheck that enum values are one of the defined values?

I'm not sure.  I put this here so that if a third one appears in the future (you
know, sent from the past by one of these on a mission to kill us all), the func
will blow up if it hasn't been adjusted accordingly.  Do you think this is being
overly paranoid?

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1037: void
MaybeAddCopyIELowRightsPolicyWorkItems(
On 2011/09/22 20:34:10, robertshield wrote:
> what about instead of having these MaybeFoo methods, we dropped the "Maybe"
part
> and called them from AddChromeFrameWorkItems above? That would let us get rid
of
> the InstallerOwnsOldLowRightsPolicies function.

The Copy is only called for an in-use update, while the delete is called during
a non-in-use update (to clean up a stale value).  I think moving them into
AddCFWI would mean duplicating a bunch of code to add the conditional work item
lists corresponding to the two cases there.  Do you see a better way?

[grt (UTC plus 2), 2011-09-23 04:18:42]

http://codereview.chromium.org/7976045/diff/9001/base/threading/thread_restri...
File base/threading/thread_restrictions.cc (right):

http://codereview.chromium.org/7976045/diff/9001/base/threading/thread_restri...
base/threading/thread_restrictions.cc:36: LOG(ERROR) <<
uh, obviously this wasn't supposed to be included.  i'm still lering git.  i
made this change in a branch between this one and my master.  i guess the cl
upload stuff includes everything between this branch and the master.  i wonder
if i can control that?  i'll remove this tomorrow.

[robertshield, 2011-09-23 14:22:18]

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:988: // rather than do something like
StringFromGUID2(__uuidof(ChromeFrame), ...).
On 2011/09/23 04:16:49, grt wrote:
> On 2011/09/22 20:34:10, robertshield wrote:
> > If there is nothing that constrains the GUID from matching the Chrome Frame
> one,
> > why not keep it as it was, and reuse the GUID?
> 
> Because the reader could be confused into thinking that the "old" one won't
work
> since it doesn't match the class GUID, and because I have to make a function
> call to get it.  Maybe those aren't very good reasons, though.
> 
> > There are some maintenance advantages since code in chrome_tab.cc still
refers
> > to the elevation policy as well.
> 
> I'm not so sure that the maintenance advantages you wish for are actually
there.
>  :-)
> 
> The references in chrome_tab.cc did not refer to the GUID in any way;
> chrome_frame_elevation.rgs hard-coded the GUID.  Had the ChromeFrame class
GUID
> changed for some reason, DLL registration would have used the old value while
> the previous code here that did the StringFromGUID2 thing would have used the
> new value.  This would be bad, as they'd be two trains passing in the night.
> 
> IMO, having two baked-in constants is a moderate step up from that.  I agree,
> however, that that sucks, too.  Thanks for calling me on it.
> 
> I've changed it so that both places now derive a string from a constant out of
> the .idl file.  Let me know what you think.

SGTM.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:989: const wchar_t
kIELowRightsPolicyGuid[] =
On 2011/09/23 04:16:49, grt wrote:
> On 2011/09/22 20:34:10, robertshield wrote:
> > Sometimes we call things ElevationPolicy and others LowRightsPolicy, I think
> > this CL should standardize on ElevationPolicy, so kIELowRightsPolicyGuid ->
> > kIEElevationPolicyGuid, etc.
> 
> GCF requires some entries in two subkeys of IE's "Low Rights" key.  One of
those
> is called "ElevationPolicy", and the other is called "DragDrop".  I thought
that
> referring to these two things as "ElevationPolicy" wasn't ideal, so I started
> collectively referring to them as "Low Rights policies".  For the in-use
update
> case, we only need to copy the ElevationPolicy since the DragDrop entry points
> to (unversioned) chrome.
> 
> My preference is for the GUIDs to be labeled at the level of "Low Rights"
> thingamaboobs.

Ok, I don't feel strongly, as long as you admit that your bikeshed is a lesser
shade of burgundy than mine.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:997: void
GetIELowRightsElevationPolicyKeyPath(ElevationPolicyId policy,
On 2011/09/23 04:16:49, grt wrote:
> On 2011/09/22 20:34:10, robertshield wrote:
> > Drop the "LowRights" bit of the name.
> 
> This function returns the path to the "ElevationPolicy" subkey of GCF's two
"Low
> Rights" keys.  I prefer to leave it as-is.

As you will.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:999: DCHECK(policy ==
CURRENT_ELEVATION_POLICY || policy == OLD_ELEVATION_POLICY);
On 2011/09/23 04:16:49, grt wrote:
> On 2011/09/22 20:34:10, robertshield wrote:
> > Do we commonly dcheck that enum values are one of the defined values?
> 
> I'm not sure.  I put this here so that if a third one appears in the future
(you
> know, sent from the past by one of these on a mission to kill us all), the
func
> will blow up if it hasn't been adjusted accordingly.  Do you think this is
being
> overly paranoid?

Yes, but just because you're paranoid doesn't mean they aren't out to get you. I
wouldn't say that in general using an enum value as a parameter implies that a
function has to handle ALL values of that enum, but the DCHECK here is fine.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1006: } else {
perhaps change this to 

} else if (policy == OLD_ELEVATION_POLICY) {

to guard against future malicious enum changes.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1013: bool
InstallerOwnsOldLowRightsPolicies(const InstallerState& installer_state) {
On 2011/09/22 20:34:10, robertshield wrote:
> InstallerOwnsOldLowRightsPolicies -> InstallerOwnsOldElevationPolicies

I take it that you disagree? Fine :-)

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1037: void
MaybeAddCopyIELowRightsPolicyWorkItems(
On 2011/09/23 04:16:49, grt wrote:
> On 2011/09/22 20:34:10, robertshield wrote:
> > what about instead of having these MaybeFoo methods, we dropped the "Maybe"
> part
> > and called them from AddChromeFrameWorkItems above? That would let us get
rid
> of
> > the InstallerOwnsOldLowRightsPolicies function.
> 
> The Copy is only called for an in-use update, while the delete is called
during
> a non-in-use update (to clean up a stale value).  I think moving them into
> AddCFWI would mean duplicating a bunch of code to add the conditional work
item
> lists corresponding to the two cases there.  Do you see a better way?

How about just wrapping the existing call sites in 
if (installer_state.chrome_frame()) ?

It keeps policy decisions about whether to have these methods add workitems in
one place which makes the code easier to read imo.

http://codereview.chromium.org/7976045/diff/9001/base/threading/thread_restri...
File base/threading/thread_restrictions.cc (right):

http://codereview.chromium.org/7976045/diff/9001/base/threading/thread_restri...
base/threading/thread_restrictions.cc:36: LOG(ERROR) <<
On 2011/09/23 04:18:42, grt wrote:
> uh, obviously this wasn't supposed to be included.  i'm still lering git.  i
> made this change in a branch between this one and my master.  i guess the cl
> upload stuff includes everything between this branch and the master.  i wonder
> if i can control that?  i'll remove this tomorrow.

Ok, please remove, I suspect that this would make a few people unhappy :-)

[grt (UTC plus 2), 2011-09-23 18:21:07]

thanks for the thoughtful review.  PTAL.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1006: } else {
On 2011/09/23 14:22:18, robertshield wrote:
> perhaps change this to 
> 
> } else if (policy == OLD_ELEVATION_POLICY) {
> 
> to guard against future malicious enum changes.

My hope was that the DCHECK is sufficient.  How strongly do you feel?

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1037: void
MaybeAddCopyIELowRightsPolicyWorkItems(
On 2011/09/23 14:22:18, robertshield wrote:
> On 2011/09/23 04:16:49, grt wrote:
> > On 2011/09/22 20:34:10, robertshield wrote:
> > > what about instead of having these MaybeFoo methods, we dropped the
"Maybe"
> > part
> > > and called them from AddChromeFrameWorkItems above? That would let us get
> rid
> > of
> > > the InstallerOwnsOldLowRightsPolicies function.
> > 
> > The Copy is only called for an in-use update, while the delete is called
> during
> > a non-in-use update (to clean up a stale value).  I think moving them into
> > AddCFWI would mean duplicating a bunch of code to add the conditional work
> item
> > lists corresponding to the two cases there.  Do you see a better way?
> 
> How about just wrapping the existing call sites in 
> if (installer_state.chrome_frame()) ?
> 
> It keeps policy decisions about whether to have these methods add workitems in
> one place which makes the code easier to read imo. 

Done.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/setup_m...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/setup_m...
chrome/installer/setup/setup_main.cc:209: // exist.
On 2011/09/22 20:34:10, robertshield wrote:
> Drop the Maybe, wrap this in an "if we're doing CF stuff" check?

Done.

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/uninsta...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/uninsta...
chrome/installer/setup/uninstall.cc:803:
ProcessIELowRightsPolicyWorkItems(installer_state);
On 2011/09/22 20:34:10, robertshield wrote:
> Feels like it would be okay to wrap this in an is_chrome_frame check as per
> other checks above (related to comment about removing the "Maybe" from the EP
> methods.

Done.

[robertshield, 2011-09-23 18:34:24]

LGTM

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/7976045/diff/10/chrome/installer/setup/install...
chrome/installer/setup/install_worker.cc:1006: } else {
On 2011/09/23 18:21:07, grt wrote:
> On 2011/09/23 14:22:18, robertshield wrote:
> > perhaps change this to 
> > 
> > } else if (policy == OLD_ELEVATION_POLICY) {
> > 
> > to guard against future malicious enum changes.
> 
> My hope was that the DCHECK is sufficient.  How strongly do you feel?

lg.

http://codereview.chromium.org/7976045/diff/21001/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/7976045/diff/21001/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:125:
WorkItem::CreateNoRollbackWorkItemList());
^ fits on one line?

[grt (UTC plus 2), 2011-09-23 18:40:15]

Thanks!

http://codereview.chromium.org/7976045/diff/21001/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/7976045/diff/21001/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:125:
WorkItem::CreateNoRollbackWorkItemList());
On 2011/09/23 18:34:24, robertshield wrote:
> ^ fits on one line?

Done.

[commit-bot: I haz the power, 2011-09-23 19:06:50]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/7976045/24001

[commit-bot: I haz the power, 2011-09-23 19:07:00]

Presubmit check for 7976045-24001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
chrome/installer/util/copy_reg_key_work_item.h, line 34: new code should not use
wstrings.  If you are calling an API that accepts a wstring, fix the API.

chrome/installer/util/work_item.cc, line 29: new code should not use wstrings. 
If you are calling an API that accepts a wstring, fix the API.

chrome/installer/util/work_item.h, line 66: new code should not use wstrings. 
If you are calling an API that accepts a wstring, fix the API.

chrome/installer/util/work_item_list.cc, line 79: new code should not use
wstrings.  If you are calling an API that accepts a wstring, fix the API.

chrome/installer/util/copy_reg_key_work_item.cc, line 19: new code should not
use wstrings.  If you are calling an API that accepts a wstring, fix the API.

chrome/installer/util/work_item_list.h, line 43: new code should not use
wstrings.  If you are calling an API that accepts a wstring, fix the API.

chrome/installer/setup/install_worker.cc, line 1000: new code should not use
wstrings.  If you are calling an API that accepts a wstring, fix the API.

chrome/installer/setup/install_worker.cc, line 1023: new code should not use
wstrings.  If you are calling an API that accepts a wstring, fix the API.

chrome/installer/setup/install_worker.cc, line 1037: new code should not use
wstrings.  If you are calling an API that accepts a wstring, fix the API.

chrome/installer/setup/install_worker.cc, line 1038: new code should not use
wstrings.  If you are calling an API that accepts a wstring, fix the API.

Presubmit checks took 3.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!