Observe adding external extensions via windows registry.

Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!

Committed: https://crrev.com/e8634172cb47984f8432daf97f62bd578311d8f1
Cr-Commit-Position: refs/heads/master@{#371911}

[lazyboy, 2016-01-20 05:47:59]

Description was changed from

==========
IN_PROGRESS: Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

There are lots of stuff that need to be fleshed out for this to land,
  but this provides a proof of concept.

Known issues:
  1. asargent@ suggested we might need to add a timeout before considering
  the registry as updated, because in my local test, depending on
  how I'm chaning the registry, I see more than one change callbacks
  being called.

  2. Any thread safety issues? I'm calling the registry watcher's
  observe function from FILE thread.

  3. Incremental update: each time we see any registry changes, we
  are calling SetPrefs() with the complete registry not the delta.
  Any good ways to compute the delta? Is it reasonable?

BUG=
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, I only have one unknown about WOW64 thing,
  the rest is pretty much similar to what plugin observer does
  to check for plugins through windows registry.

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[lazyboy, 2016-01-20 05:52:02]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, I only have one unknown about WOW64 thing,
  the rest is pretty much similar to what plugin observer does
  to check for plugins through windows registry.

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, there are couple of unknowns/questions:
  1) the WOW64 thing in external_registry_loader_win.cc and
  2) when I add an extension in the registry during test through
    a .reg file, I see two callbacks being fired. From MS
    documentation, it seems this is expected as it fires when
    a key has changed and again when the key is closed. I'm yet to
    verify this.
The rest is pretty much similar to what plugin observer does
  to check for plugins through windows registry.

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[lazyboy, 2016-01-20 05:54:59]

lazyboy@chromium.org changed reviewers:
+ asargent@chromium.org, rdevlin.cronin@chromium.org

[lazyboy, 2016-01-20 05:54:59]

Hi Anthony and Devlin, I'd like to get some feedback on the current iteration of
this CL. This is currently working when I manually test this on windows. Of
course, there are no tests yet (:, I'll start writing them once I have some
feedback. Thanks.

[lazyboy, 2016-01-20 19:28:12]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, there are couple of unknowns/questions:
  1) the WOW64 thing in external_registry_loader_win.cc and
  2) when I add an extension in the registry during test through
    a .reg file, I see two callbacks being fired. From MS
    documentation, it seems this is expected as it fires when
    a key has changed and again when the key is closed. I'm yet to
    verify this.
The rest is pretty much similar to what plugin observer does
  to check for plugins through windows registry.

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, there are couple of unknowns/questions:
  1) the WOW64 thing in external_registry_loader_win.cc and
  2) when I add an extension in the registry during test through
    a .reg file, I see two callbacks being fired. From MS
    documentation, it seems this is expected as it fires when
    a key has changed and again when the key is closed. I'm yet to
    verify this.
The rest is pretty much similar to what plugin observer does
  to check for plugins through windows registry
  (see plugin_service_imp.cc).

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[asargent_no_longer_on_chrome, 2016-01-20 22:49:00]

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:243:
updater_->CheckNow(empty_params);
It seems a little unfortunate to kick off an update check unconditionally (and
furthermore one that checks for all currently installed extensions), when
presumably we could know beforehand whether or not we have a new extension to
install via update url, and what its id is.

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
File chrome/browser/extensions/external_provider_impl.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
chrome/browser/extensions/external_provider_impl.h:103: void
NotifyExtensionsFromPrefs(bool is_initial_load);
nit: this could use an overview comment

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:209:
base::Unretained(this),
I know that reading from the registry must be done on the file thread because
the operations can possibly block - do you know whether the listener stuff can
be safely used on the UI thread without introducing long delays?

Unfortunately the comments in base/win/registry.h don't have much to say on the
matter, but it would be nice to find out from one of our windows experts like
scottmg (if you haven't already) and then update the comments there.

https://chromiumcodereview.appspot.com/1495403002/diff/140001/extensions/brow...
File extensions/browser/external_provider_interface.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/extensions/brow...
extensions/browser/external_provider_interface.h:73: const
std::set<std::string>& removed_extensions) = 0;
It seems a little asymmetric that this function gets called with a set of
removed extension ids, but not information about extensions to be added/updated
(I guess those get communicated via calls to
OnExternalExtensionFileFound/OnExternalExtensionUpdateUrlFound prior to this
being called?)

[Devlin, 2016-01-21 00:14:26]

I didn't see anything much in addition to what Antony mentioned.

https://codereview.chromium.org/1495403002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/1495403002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/external_provider_impl.cc:126: void
ExternalProviderImpl::UpdatePrefs(base::DictionaryValue* prefs) {
This is a little hard to review - can you un-move these for this CL (or at least
early patch sets) so we can see the diff clearly?

https://codereview.chromium.org/1495403002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/1495403002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/external_registry_loader_win.cc:232:
key->StartWatching(
Save the reader the lookup into win/registry.h with a comment like:
// This is removed as an observer when the ChangeCallback is called, so we need
to re-register.

[lazyboy, 2016-01-21 21:02:11]

lazyboy@chromium.org changed reviewers:
+ scottmg@chromium.org

[lazyboy, 2016-01-21 21:02:12]

+scottmg for winregistry observing changes, specifically external_loader_win.cc

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:243:
updater_->CheckNow(empty_params);
On 2016/01/20 22:49:00, Antony Sargent wrote:
> It seems a little unfortunate to kick off an update check unconditionally (and
> furthermore one that checks for all currently installed extensions)

This should not kick off update check for all currently installed
extensions, only the ones pending_install_manager knows about, which
would be the new ones. b/c OnExternalExtensionUpdateUrlFound() skips
the extensions that are currently installed.

, when
> presumably we could know beforehand whether or not we have a new extension to
> install via update url, and what its id is.
It is possible to get those ids if Provider stores a copy of the registry and
calculates the "diff".
But that requires more code changes.

In that scenario:
When provider sees a registry change, compares it to the
current registry, calculate newly found ids and removed ids,
then pass them to this function here.

for each newly found items,
call OnExternal(ExtensionUpdateUrl|File)Found() with those ids.
Note that we probably need to store/retrieve the params of those functions
somehow in this
case, these include:
update_url,
ExternalProviderImpl::crx_location_,
creation_flags,
ExternalProviderImpl::auto_acknowledge_
ExternalProviderImpl::install_immediately_
install_parameter,
ExternalProviderImpl::download_location_,
ExternalProviderImpl::auto_acknowledge_
We can package these in a struct and send them as params instead of just ids.

In terms of actual work done, this is roughly equivalent to what we have in this
CL
though.

Thoughts?

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
chrome/browser/extensions/external_provider_impl.cc:126: void
ExternalProviderImpl::UpdatePrefs(base::DictionaryValue* prefs) {
On 2016/01/21 00:14:26, Devlin (Slow until 1-27) wrote:
> This is a little hard to review - can you un-move these for this CL (or at
least
> early patch sets) so we can see the diff clearly?

Done.

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
File chrome/browser/extensions/external_provider_impl.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
chrome/browser/extensions/external_provider_impl.h:103: void
NotifyExtensionsFromPrefs(bool is_initial_load);
On 2016/01/20 22:49:00, Antony Sargent wrote:
> nit: this could use an overview comment

Done.

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:209:
base::Unretained(this),
On 2016/01/20 22:49:00, Antony Sargent wrote:
> I know that reading from the registry must be done on the file thread because
> the operations can possibly block - do you know whether the listener stuff can
> be safely used on the UI thread without introducing long delays?
I've based my changes on plugin_service_impl.cc, which seems to be
doing very similar stuff. 
> 
> Unfortunately the comments in base/win/registry.h don't have much to say on
the
> matter, but it would be nice to find out from one of our windows experts like
> scottmg (if you haven't already) and then update the comments there. 
+scottmg.

https://chromiumcodereview.appspot.com/1495403002/diff/140001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:232:
key->StartWatching(
On 2016/01/21 00:14:26, Devlin (Slow until 1-27) wrote:
> Save the reader the lookup into win/registry.h with a comment like:
> // This is removed as an observer when the ChangeCallback is called, so we
need
> to re-register.

Done.

https://chromiumcodereview.appspot.com/1495403002/diff/140001/extensions/brow...
File extensions/browser/external_provider_interface.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/140001/extensions/brow...
extensions/browser/external_provider_interface.h:73: const
std::set<std::string>& removed_extensions) = 0;
On 2016/01/20 22:49:00, Antony Sargent wrote:
> It seems a little asymmetric that this function gets called with a set of
> removed extension ids, but not information about extensions to be
added/updated
> (I guess those get communicated via calls to
> OnExternalExtensionFileFound/OnExternalExtensionUpdateUrlFound prior to this
> being called?)

Yes.
See my reply in your other comment in ExtensionService.

[scottmg, 2016-01-21 21:16:16]

I'm afraid I've never actually used the registry watcher, but this looks fine to
me.

I think it's OK to watch on the UI thread -- in fact I think in general we jump
through too many hoops to read the registry off the main thread (because it's
always paged in), but here at least we're doing a decent amount of work in
loading prefs, so it's not completely pointless.

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:206: KEY_NOTIFY |
KEY_WOW64_32KEY) == ERROR_SUCCESS) {
How are these keys set? Is this because the installer is 32-bit only? Otherwise
I'd just expect the registry to want to retrieve it's natural bitness.

[lazyboy, 2016-01-21 21:24:48]

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:206: KEY_NOTIFY |
KEY_WOW64_32KEY) == ERROR_SUCCESS) {
On 2016/01/21 21:16:16, scottmg wrote:
> How are these keys set? Is this because the installer is 32-bit only?
Otherwise
> I'd just expect the registry to want to retrieve it's natural bitness.

I'm not sure about this one (as the TODO suggests):
Two clues I've used to specify this:
1) Line 73 above uses this while iterating through HKLM keys.
2) The documentation in
https://developer.chrome.com/extensions/external_extensions#registry
suggests adding registry entries under ..\WOW6432Node\

[lazyboy, 2016-01-22 07:52:41]

@asargent, I've made the changes we've discussed offline, please take a look,
thanks.

https://chromiumcodereview.appspot.com/1495403002/diff/180001/chrome/browser/...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/180001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:240: if
(!update_url_extensions.empty() || !file_extensions.empty()) {
@asargent: I've added this guard we talked about offline.

https://chromiumcodereview.appspot.com/1495403002/diff/180001/chrome/browser/...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/180001/chrome/browser/...
chrome/browser/extensions/extension_service.h:240: void
OnExternalProviderUpdateComplete(
This function now includes all the info the provider has found, and the
provider doesn't call ExtensionService functions to update each one as it used
to before.

https://chromiumcodereview.appspot.com/1495403002/diff/180001/extensions/brow...
File extensions/browser/external_provider_interface.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/180001/extensions/brow...
extensions/browser/external_provider_interface.h:29: struct
ExternalExtensionInfo {
These structs basically hold the information from previous
OnExternalExtension(UpdateUrl/File)Found's arguments.

[asargent_no_longer_on_chrome, 2016-01-25 18:50:52]

I like the changes. Just a couple of small nits/suggestions

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:206: KEY_NOTIFY |
KEY_WOW64_32KEY) == ERROR_SUCCESS) {
On 2016/01/21 21:24:47, lazyboy wrote:
> On 2016/01/21 21:16:16, scottmg wrote:
> > How are these keys set? Is this because the installer is 32-bit only?
> Otherwise
> > I'd just expect the registry to want to retrieve it's natural bitness.
> 
> I'm not sure about this one (as the TODO suggests):
> Two clues I've used to specify this:
> 1) Line 73 above uses this while iterating through HKLM keys.
> 2) The documentation in
> https://developer.chrome.com/extensions/external_extensions#registry
> suggests adding registry entries under ..\WOW6432Node\

These registry entries are written by third party programs (eg antivirus or
other programs that have a companion chrome extension they want to offer to the
user), so maybe it could be the case that their installer is 32 bit even if
chrome is 64 bit. Also it's possible we used to only tell developers "write to
HKLM\Software\Google\Chrome\Extensions" and later added the advice about the
Wow6432Node registry path, so some 64 bit installers might have used the 32 bit
path.

https://chromiumcodereview.appspot.com/1495403002/diff/180001/chrome/browser/...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/180001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:240: if
(!update_url_extensions.empty() || !file_extensions.empty()) {
On 2016/01/22 07:52:41, lazyboy wrote:
> @asargent: I've added this guard we talked about offline.

Acknowledged.

https://chromiumcodereview.appspot.com/1495403002/diff/180001/chrome/browser/...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/180001/chrome/browser/...
chrome/browser/extensions/extension_service.h:240: void
OnExternalProviderUpdateComplete(
On 2016/01/22 07:52:41, lazyboy wrote:
> This function now includes all the info the provider has found, and the
> provider doesn't call ExtensionService functions to update each one as it used
> to before.

Acknowledged.

https://chromiumcodereview.appspot.com/1495403002/diff/180001/extensions/brow...
File extensions/browser/external_provider_interface.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/180001/extensions/brow...
extensions/browser/external_provider_interface.h:29: struct
ExternalExtensionInfo {
On 2016/01/22 07:52:41, lazyboy wrote:
> These structs basically hold the information from previous
> OnExternalExtension(UpdateUrl/File)Found's arguments.

Acknowledged.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:248: if (updater_) {
Does the extension updater need to run if we only had entries in
|file_extensions| but not in |update_url_extensions| ? Off the top of my head I
would have guessed not.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:256: CheckExternalUninstall(id);
Can we move the handling of uninstalls a few lines up to be before the run of
the extension updater? It would be nice to avoid the edge case of beginning to
download an update for an extension that we are going to uninstall anyway.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/extension_service_unittest.cc:444: ADD_FAILURE() <<
"Not tested path yet.";
You should add a test for this =)

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:211: // TODO(lazyboy):
Error handling.
Did you have any particular ideas in mind? I don't know that there's a lot more
you'd want to do in this case. Perhaps more details are available about the
error that you could add to the below log statement?

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
File extensions/browser/external_extension_install_info.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
extensions/browser/external_extension_install_info.h:16: struct
ExternalExtensionInstallInfo {
suggestion: consider removing "Extension" from this struct name to make it
shorter, since its connection with extensions can be inferred from it being in
the extensions namespace


(and same suggestion for ExternalExtensionInstallInfoFile and
ExternalExtensionInstallInfoUpdateUrl below)

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
File extensions/browser/external_provider_interface.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
extensions/browser/external_provider_interface.h:38:
ExternalExtensionInstallInfoFile* info) = 0;
nit: If ownership of this data is passed, this should probably use
scoped_ptr<ExternalExtensionInstallInfoFile> to make that clear. If ownership
isn't passed, it should use const ExternalExtensionInstallInfoFile& (I assume it
wouldn't be useful to be able to pass nullptr)

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
extensions/browser/external_provider_interface.h:44:
ExternalExtensionInstallInfoUpdateUrl* info,
same comment re: ownership

[lazyboy, 2016-01-26 05:20:05]

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:206: KEY_NOTIFY |
KEY_WOW64_32KEY) == ERROR_SUCCESS) {
On 2016/01/25 18:50:52, Antony Sargent wrote:
> On 2016/01/21 21:24:47, lazyboy wrote:
> > On 2016/01/21 21:16:16, scottmg wrote:
> > > How are these keys set? Is this because the installer is 32-bit only?
> > Otherwise
> > > I'd just expect the registry to want to retrieve it's natural bitness.
> > 
> > I'm not sure about this one (as the TODO suggests):
> > Two clues I've used to specify this:
> > 1) Line 73 above uses this while iterating through HKLM keys.
> > 2) The documentation in
> > https://developer.chrome.com/extensions/external_extensions#registry
> > suggests adding registry entries under ..\WOW6432Node\
> 
> These registry entries are written by third party programs (eg antivirus or
> other programs that have a companion chrome extension they want to offer to
the
> user), so maybe it could be the case that their installer is 32 bit even if
> chrome is 64 bit. Also it's possible we used to only tell developers "write to
> HKLM\Software\Google\Chrome\Extensions" and later added the advice about the
> Wow6432Node registry path, so some 64 bit installers might have used the 32
bit
> path.

I'll drop by to talk to you person about this one and fix.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:248: if (updater_) {
On 2016/01/25 18:50:52, Antony Sargent wrote:
> Does the extension updater need to run if we only had entries in
> |file_extensions| but not in |update_url_extensions| ? Off the top of my head
I
> would have guessed not.

Ah right, the CRX download is handled differently.
Fixed.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:256: CheckExternalUninstall(id);
On 2016/01/25 18:50:52, Antony Sargent wrote:
> Can we move the handling of uninstalls a few lines up to be before the run of
> the extension updater?
Moved.
It would be nice to avoid the edge case of beginning to
> download an update for an extension that we are going to uninstall anyway.
From a single provider, this shouldn't happen though, ids from
|removed_extensions| shouldn't be specified in either |update_url_extensions| or
|file_extensions|. I've added a DCHECK for that.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/extension_service_unittest.cc:444: ADD_FAILURE() <<
"Not tested path yet.";
On 2016/01/25 18:50:52, Antony Sargent wrote:
> You should add a test for this =)
>  

Done.
I've added a different provider since this one would quite give me what I'm
looking for.
Let me know if you have ideas to improve the test.
Thanks.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:211: // TODO(lazyboy):
Error handling.
On 2016/01/25 18:50:52, Antony Sargent wrote:
> Did you have any particular ideas in mind? I don't know that there's a lot
more
> you'd want to do in this case.
No I don't, maybe a LOG(WARNING) is enough?
Perhaps more details are available about the
> error that you could add to the below log statement?

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
File extensions/browser/external_extension_install_info.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
extensions/browser/external_extension_install_info.h:16: struct
ExternalExtensionInstallInfo {
On 2016/01/25 18:50:52, Antony Sargent wrote:
> suggestion: consider removing "Extension" from this struct name to make it
> shorter, since its connection with extensions can be inferred from it being in
> the extensions namespace
> 
> 
> (and same suggestion for ExternalExtensionInstallInfoFile and
> ExternalExtensionInstallInfoUpdateUrl below)

Thanks.
Done.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
File extensions/browser/external_provider_interface.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
extensions/browser/external_provider_interface.h:38:
ExternalExtensionInstallInfoFile* info) = 0;
On 2016/01/25 18:50:52, Antony Sargent wrote:
> nit: If ownership of this data is passed, this should probably use
> scoped_ptr<ExternalExtensionInstallInfoFile> to make that clear. If ownership
> isn't passed, it should use const ExternalExtensionInstallInfoFile& (I assume
it
> wouldn't be useful to be able to pass nullptr)

Done.

https://chromiumcodereview.appspot.com/1495403002/diff/240001/extensions/brow...
extensions/browser/external_provider_interface.h:44:
ExternalExtensionInstallInfoUpdateUrl* info,
On 2016/01/25 18:50:52, Antony Sargent wrote:
> same comment re: ownership

Done.

[asargent_no_longer_on_chrome, 2016-01-26 17:48:07]

lgtm

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/extension_service.cc:256: CheckExternalUninstall(id);
On 2016/01/26 05:20:04, lazyboy wrote:
> On 2016/01/25 18:50:52, Antony Sargent wrote:
> > Can we move the handling of uninstalls a few lines up to be before the run
of
> > the extension updater?
> Moved.
> It would be nice to avoid the edge case of beginning to
> > download an update for an extension that we are going to uninstall anyway.
> From a single provider, this shouldn't happen though, ids from
> |removed_extensions| shouldn't be specified in either |update_url_extensions|
or
> |file_extensions|. I've added a DCHECK for that.

You're right, from a single provider this should not happen. I was thinking of
the case where the item was already installed for some other reason, such as
manually by the user, or from some other external installer. (When you kick off
the extension updater, usually it asks for updates for all items.)

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/240001/chrome/browser/...
chrome/browser/extensions/extension_service_unittest.cc:444: ADD_FAILURE() <<
"Not tested path yet.";
On 2016/01/26 05:20:04, lazyboy wrote:
> On 2016/01/25 18:50:52, Antony Sargent wrote:
> > You should add a test for this =)
> >  
> 
> Done.
> I've added a different provider since this one would quite give me what I'm
> looking for.
> Let me know if you have ideas to improve the test.
> Thanks.
I think what you've added is fine

[lazyboy, 2016-01-26 23:44:43]

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://chromiumcodereview.appspot.com/1495403002/diff/160001/chrome/browser/...
chrome/browser/extensions/external_registry_loader_win.cc:206: KEY_NOTIFY |
KEY_WOW64_32KEY) == ERROR_SUCCESS) {
On 2016/01/26 05:20:04, lazyboy wrote:
> On 2016/01/25 18:50:52, Antony Sargent wrote:
> > On 2016/01/21 21:24:47, lazyboy wrote:
> > > On 2016/01/21 21:16:16, scottmg wrote:
> > > > How are these keys set? Is this because the installer is 32-bit only?
> > > Otherwise
> > > > I'd just expect the registry to want to retrieve it's natural bitness.
> > > 
> > > I'm not sure about this one (as the TODO suggests):
> > > Two clues I've used to specify this:
> > > 1) Line 73 above uses this while iterating through HKLM keys.
> > > 2) The documentation in
> > > https://developer.chrome.com/extensions/external_extensions#registry
> > > suggests adding registry entries under ..\WOW6432Node\
> > 
> > These registry entries are written by third party programs (eg antivirus or
> > other programs that have a companion chrome extension they want to offer to
> the
> > user), so maybe it could be the case that their installer is 32 bit even if
> > chrome is 64 bit. Also it's possible we used to only tell developers "write
to
> > HKLM\Software\Google\Chrome\Extensions" and later added the advice about the
> > Wow6432Node registry path, so some 64 bit installers might have used the 32
> bit
> > path.
> 
> I'll drop by to talk to you person about this one and fix.

After chatting /w asargent@, I've removed the TODO because it seems we need
WOW64_32KEY.
Chrome extension's external provider mechanism always reads the 32-bit registry:
https://developer.chrome.com/extensions/external_extensions#registry
Also the code at line #73 agrees with that, i.e. the initial registry reading
also reads the key from 32-bit.

https://chromiumcodereview.appspot.com/1495403002/diff/340001/chrome/browser/...
File chrome/browser/extensions/external_loader.h (right):

https://chromiumcodereview.appspot.com/1495403002/diff/340001/chrome/browser/...
chrome/browser/extensions/external_loader.h:62: virtual void
OnUpdated(scoped_ptr<base::DictionaryValue> updated_prefs);
@asargent, as we discussed offline, I've made the change to
pass around the pref value while we are updating. Please take
a look at the diff between patch set #17 and #18

[asargent_no_longer_on_chrome, 2016-01-27 00:49:32]

new version lgtm

[lazyboy, 2016-01-27 18:11:07]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, there are couple of unknowns/questions:
  1) the WOW64 thing in external_registry_loader_win.cc and
  2) when I add an extension in the registry during test through
    a .reg file, I see two callbacks being fired. From MS
    documentation, it seems this is expected as it fires when
    a key has changed and again when the key is closed. I'm yet to
    verify this.
The rest is pretty much similar to what plugin observer does
  to check for plugins through windows registry
  (see plugin_service_imp.cc).

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[lazyboy, 2016-01-27 18:21:01]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[lazyboy, 2016-01-27 18:23:31]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up.

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.
  

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[lazyboy, 2016-01-27 22:04:18]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.
  

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.
  

TBR=achuith@chromium.org for customization_document_unittest.cc
BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[lazyboy, 2016-01-27 22:06:02]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.
  

TBR=achuith@chromium.org for customization_document_unittest.cc
BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.
  

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[lazyboy, 2016-01-27 22:06:47]

lazyboy@chromium.org changed reviewers:
+ achuith@chromium.org

[lazyboy, 2016-01-27 22:06:48]

+achuith@chromiu.org for
chrome/browser/chromeos/customization/customization_document_unittest.cc

[achuithb, 2016-01-27 22:22:00]

On 2016/01/27 22:06:48, lazyboy wrote:
> mailto:+achuith@chromiu.org for
> chrome/browser/chromeos/customization/customization_document_unittest.cc

lgtm for c/b/cros

[lazyboy, 2016-01-27 23:26:30]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-01-27 23:26:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1495403002/#ps360001 (title: "sync
@tott")

[commit-bot: I haz the power, 2016-01-27 23:28:49]

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

[commit-bot: I haz the power, 2016-01-28 00:10:55]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.
  

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.
  

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

[commit-bot: I haz the power, 2016-01-28 00:10:56]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-01-28 00:11:52]

Description was changed from

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.
  

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!
==========

to

==========
Observe adding external extensions via windows registry.
  Install the extension right away (no chrome restart).

For registry changes, we only look at 32-bit registry values
for extensions, see
https://developer.chrome.com/extensions/external_extensions#registry

A lot of the changes in this CL is to make ExternalProvider-s be
  able to discover new external extensions at times other than
  chrome start up. The added method is
  OnExternalProviderUpdateComplete(), which provides a list of
  a. Extensions that were added via update_url,
  b. Extensions that were added via crx,
  c. Extensions that were removed.

BUG=581756
Test=While chrome is running, add a registry entry for an extension
under HKLM or HKCU as described in
https://developer.chrome.com/extensions/external_extensions#registry

Now observe that external install would appear for that extension
in chrome shortly.

If you had some extension installed this way, removing the registry
entry would cause the extension to be uninstalled too!

Committed: https://crrev.com/e8634172cb47984f8432daf97f62bd578311d8f1
Cr-Commit-Position: refs/heads/master@{#371911}
==========

[commit-bot: I haz the power, 2016-01-28 00:11:58]

Patchset 19 (id:??) landed as
https://crrev.com/e8634172cb47984f8432daf97f62bd578311d8f1
Cr-Commit-Position: refs/heads/master@{#371911}