Allow Registry Iterator functions to use a specified WOW64 mode when iterating.

Allow Registry Iterator functions to use a specified WOW64 mode when iterating.

Also fix up any callers to access the correct hive (32 or 64) depending on
where the data is, or whether the hive is Redirected, Shared, or Reflected
as specified in MSDN

http://msdn.microsoft.com/en-us/library/windows/desktop/aa384253.aspx

BUG=338706, 408018
TEST=Follow steps in 408018 and make sure extension gets added on 64-bit Chrome
TEST=base_unittests --gtest_filter=Registry*

Committed: https://crrev.com/9353beb978ce832381ad4869dc4eb9822c9dc11c
Cr-Commit-Position: refs/heads/master@{#298758}

[Will Harris, 2014-10-01 00:59:21]

wfh@chromium.org changed reviewers:
+ cpu@chromium.org

[Will Harris, 2014-10-01 01:00:59]

cpu@ input requested on whether this is the best approach here.

This was originally to fix issue 408018 - first attempt at CL was
https://codereview.chromium.org/580493002 but this alone didn't work because
RegistryKeyIterator was not WOW64 aware.

[grt (UTC plus 2), 2014-10-01 17:14:30]

a comment from the peanut gallery

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
File chrome/installer/util/app_commands.cc (right):

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
chrome/installer/util/app_commands.cc:37: key.Handle(), kEmptyString,
KEY_WOW64_32KEY);
this is ugly. it so happens that it's correct, but it isn't clear from the
interface.

[Will Harris, 2014-10-01 17:20:21]

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
File chrome/installer/util/app_commands.cc (right):

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
chrome/installer/util/app_commands.cc:37: key.Handle(), kEmptyString,
KEY_WOW64_32KEY);
On 2014/10/01 17:14:30, grt wrote:
> this is ugly. it so happens that it's correct, but it isn't clear from the
> interface.

yes, and it's not by coincidence, I manually went through every call to check
what access to supply.

I agree it's ugly - I considered:

 - exposing an interface on RegKey to determine what wow64access it had
originally been opened with, then we could pass this into RegistryKeyIterator
 - adding a new constructor on RegistryKeyIterator to take a RegKey instead of a
handle (but I think this would be against chromium coding standard)

If you have any suggestions how this could be made more elegant they would be
appreciated.

[grt (UTC plus 2), 2014-10-01 17:33:50]

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
File chrome/installer/util/app_commands.cc (right):

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
chrome/installer/util/app_commands.cc:37: key.Handle(), kEmptyString,
KEY_WOW64_32KEY);
On 2014/10/01 17:20:21, Will Harris wrote:
> On 2014/10/01 17:14:30, grt wrote:
> > this is ugly. it so happens that it's correct, but it isn't clear from the
> > interface.
> 
> yes, and it's not by coincidence, I manually went through every call to check
> what access to supply.
> 
> I agree it's ugly - I considered:
> 
>  - exposing an interface on RegKey to determine what wow64access it had
> originally been opened with, then we could pass this into RegistryKeyIterator
>  - adding a new constructor on RegistryKeyIterator to take a RegKey instead of
a
> handle (but I think this would be against chromium coding standard)
> 
> If you have any suggestions how this could be made more elegant they would be
> appreciated.

I wish I did. The whys and wherefores for these magic bitness values are fairly
hard to understand/remember.

[cpu_(ooo_6.6-7.5), 2014-10-02 00:31:25]

at first blush this feels wrong, the callee should add the KEY_WOW64_32KEY

Can that be made to happen?

[Will Harris, 2014-10-02 17:35:16]

On 2014/10/02 00:31:25, cpu wrote:
> at first blush this feels wrong, the callee should add the KEY_WOW64_32KEY
> 
> Can that be made to happen?

can you be more specific here?  Are you talking about the entire CL, or just the
appcommand part that grt@ pointed out?  appcommand already uses KEY_WOW64_32KEY
because the keys are always in the 32-bit hive.  Other cases in the CL, the
registry keys are shared/reflected so there is no need to supply a WOW64 access
flag.  Are you suggesting adding the KEY_WOW64_32KEY flag in these other cases?

[Will Harris, 2014-10-06 16:19:48]

ping @cpu on above question. THanks!

[cpu_(ooo_6.6-7.5), 2014-10-06 19:48:57]

https://codereview.chromium.org/616173003/diff/1/base/win/registry.h
File base/win/registry.h (right):

https://codereview.chromium.org/616173003/diff/1/base/win/registry.h#newcode163
base/win/registry.h:163: 
yeah definitely two different ctors make sense here.

Secondly in the one that takes wow64access needs a bit of explanation in this
header (aka add a comment explaning what is that).

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
File chrome/installer/util/app_commands.cc (right):

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
chrome/installer/util/app_commands.cc:37: key.Handle(), kEmptyString,
KEY_WOW64_32KEY);
On 2014/10/01 17:20:21, Will Harris wrote:
> On 2014/10/01 17:14:30, grt wrote:
> > this is ugly. it so happens that it's correct, but it isn't clear from the
> > interface.
> 
> yes, and it's not by coincidence, I manually went through every call to check
> what access to supply.
> 
> I agree it's ugly - I considered:
> 
>  - exposing an interface on RegKey to determine what wow64access it had
> originally been opened with, then we could pass this into RegistryKeyIterator
>  - adding a new constructor on RegistryKeyIterator to take a RegKey instead of
a
> handle (but I think this would be against chromium coding standard)
> 
> If you have any suggestions how this could be made more elegant they would be
> appreciated.

you can have as many constructors as you need, in particular peppering the
codebase with (.., 0) is bad form.

[Will Harris, 2014-10-07 02:03:16]

Added new constructor, it's much neater.  Also pulled the wow64access out of the
AppCommands::Initialize function, which is only called in one place anyway.

PTAL.

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
File chrome/installer/util/app_commands.cc (right):

https://codereview.chromium.org/616173003/diff/1/chrome/installer/util/app_co...
chrome/installer/util/app_commands.cc:37: key.Handle(), kEmptyString,
KEY_WOW64_32KEY);
On 2014/10/06 19:48:57, cpu wrote:
> On 2014/10/01 17:20:21, Will Harris wrote:
> > On 2014/10/01 17:14:30, grt wrote:
> > > this is ugly. it so happens that it's correct, but it isn't clear from the
> > > interface.
> > 
> > yes, and it's not by coincidence, I manually went through every call to
check
> > what access to supply.
> > 
> > I agree it's ugly - I considered:
> > 
> >  - exposing an interface on RegKey to determine what wow64access it had
> > originally been opened with, then we could pass this into
RegistryKeyIterator
> >  - adding a new constructor on RegistryKeyIterator to take a RegKey instead
of
> a
> > handle (but I think this would be against chromium coding standard)
> > 
> > If you have any suggestions how this could be made more elegant they would
be
> > appreciated.
> 
> you can have as many constructors as you need, in particular peppering the
> codebase with (.., 0) is bad form.

Done.

https://codereview.chromium.org/616173003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/616173003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_registry_loader_win.cc:79: 0);
This could use the two parameter constructor but I think it makes it more
explicit that we're setting no WOW64 flags for the HKCU access.

[grt (UTC plus 2), 2014-10-07 15:47:09]

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

[grt (UTC plus 2), 2014-10-07 15:47:10]

https://codereview.chromium.org/616173003/diff/20001/base/win/registry.cc
File base/win/registry.cc (right):

https://codereview.chromium.org/616173003/diff/20001/base/win/registry.cc#new...
base/win/registry.cc:507: DCHECK(!wow64access || wow64access &
kWow64AccessMask);
is DCHECK_EQ((wow64Access & ~kWow64AccessMask), static_cast<REGSAM>(0)) not
better since it will fail if bits outside of the mask are set? (same for line
646.)

https://codereview.chromium.org/616173003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/616173003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_registry_loader_win.cc:79: 0);
On 2014/10/07 02:03:16, Will Harris wrote:
> This could use the two parameter constructor but I think it makes it more
> explicit that we're setting no WOW64 flags for the HKCU access.

Why here and not everywhere that HKCU is used?

https://codereview.chromium.org/616173003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_registry_loader_win.cc:92: if
(key.Open(HKEY_LOCAL_MACHINE,
nit:
    if (key.Open(...,
                 ...) != ERROR SUCCESS &&
        key.Open(...,
                 ...) != ERROR_SUCCESS) {
      LOG(ERROR) << ...;
      continue;
    }

[Will Harris, 2014-10-07 17:16:14]

PTAL

https://codereview.chromium.org/616173003/diff/20001/base/win/registry.cc
File base/win/registry.cc (right):

https://codereview.chromium.org/616173003/diff/20001/base/win/registry.cc#new...
base/win/registry.cc:507: DCHECK(!wow64access || wow64access &
kWow64AccessMask);
On 2014/10/07 15:47:10, grt wrote:
> is DCHECK_EQ((wow64Access & ~kWow64AccessMask), static_cast<REGSAM>(0)) not
> better since it will fail if bits outside of the mask are set? (same for line
> 646.)

Done.

https://codereview.chromium.org/616173003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/616173003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_registry_loader_win.cc:79: 0);
On 2014/10/07 15:47:10, grt wrote:
> On 2014/10/07 02:03:16, Will Harris wrote:
> > This could use the two parameter constructor but I think it makes it more
> > explicit that we're setting no WOW64 flags for the HKCU access.
> 
> Why here and not everywhere that HKCU is used?

True, I'll change it back.

https://codereview.chromium.org/616173003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/external_registry_loader_win.cc:92: if
(key.Open(HKEY_LOCAL_MACHINE,
On 2014/10/07 15:47:10, grt wrote:
> nit:
>     if (key.Open(...,
>                  ...) != ERROR SUCCESS &&
>         key.Open(...,
>                  ...) != ERROR_SUCCESS) {
>       LOG(ERROR) << ...;
>       continue;
>     }

Done.

[cpu_(ooo_6.6-7.5), 2014-10-07 18:11:13]

lgtm on my side.

[grt (UTC plus 2), 2014-10-07 20:03:20]

lgtm

[Will Harris, 2014-10-07 20:34:40]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2014-10-07 20:36:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616173003/40001

[commit-bot: I haz the power, 2014-10-07 20:46:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-07 20:46:57]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Will Harris, 2014-10-07 20:52:51]

wfh@chromium.org changed reviewers:
+ ananta@chromium.org, miket@chromium.org

[Will Harris, 2014-10-07 20:56:51]

adding some OWNERS

miket@ - Please review external_registry_loader_win.cc.  This CL is the complete
fix for crbug.com/408018 and supercedes
https://codereview.chromium.org/580493002
ananta@ - Change to plugin_list_win.cc to pull candidate list from both 64-bit
and 32-bit - only the correct plugins are loaded, this just expands the search
path. See https://codereview.chromium.org/327983002

[miket_OOO, 2014-10-07 23:00:01]

> miket@ - Please review external_registry_loader_win.cc.  This CL is the
complete

This file LGTM

[ananta, 2014-10-08 20:36:09]

https://codereview.chromium.org/616173003/diff/40001/content/common/plugin_li...
File content/common/plugin_list_win.cc (right):

https://codereview.chromium.org/616173003/diff/40001/content/common/plugin_li...
content/common/plugin_list_win.cc:369:
GetPluginsInRegistryDirectory(HKEY_LOCAL_MACHINE,
Getting 32 bit plugins from a 64 bit process and vice versa. Is this needed?

[Will Harris, 2014-10-08 20:43:23]

https://codereview.chromium.org/616173003/diff/40001/content/common/plugin_li...
File content/common/plugin_list_win.cc (right):

https://codereview.chromium.org/616173003/diff/40001/content/common/plugin_li...
content/common/plugin_list_win.cc:369:
GetPluginsInRegistryDirectory(HKEY_LOCAL_MACHINE,
On 2014/10/08 20:36:09, ananta wrote:
> Getting 32 bit plugins from a 64 bit process and vice versa. Is this needed?

it potentially overpopulates the list at this stage but they get filtered out by
the PE header check later on.

This is realy just to cater for some NPAPI plugins that register in the wrong
place e.g. I think quicktime does this.  If we don't care too much about this we
can search in fewer places but I thought it would better to cover all the bases.

[ananta, 2014-10-08 21:39:48]

lgtm

[Will Harris, 2014-10-08 23:55:29]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2014-10-08 23:59:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616173003/40001

[commit-bot: I haz the power, 2014-10-09 04:31:53]

Committed patchset #3 (id:40001) as 7adaec1cb743e292b55b4157484032053be5df00

[commit-bot: I haz the power, 2014-10-09 04:32:46]

Patchset 3 (id:??) landed as
https://crrev.com/9353beb978ce832381ad4869dc4eb9822c9dc11c
Cr-Commit-Position: refs/heads/master@{#298758}