Don't block shared_module extensions by the policy blacklist.

Don't block shared_module extensions by the policy blacklist.

Extensions may import resources from other extensions. Extensions that
export resources to other extensions are known as shared_modules.
Chrome is currently applying the blacklist to all kinds of extensions;
this means that whitelisting (or force-installing) an extension that
requires shared_modules will fail unless the modules are whitelisted
too.

This CL bypasses the policy blacklist checks for shared_modules.

Also fixed a bug in the CrxInstaller, which prevented extensions
loaded via external providers (such as policy) from getting their
dependencies installed.

BUG=424377
Committed: https://crrev.com/30c8be95aea94a91aa5772e6b070fb9243eaf10d
Cr-Commit-Position: refs/heads/master@{#300984}

[Joao da Silva, 2014-10-22 12:28:08]

joaodasilva@chromium.org changed reviewers:
+ asargent@chromium.org, elijahtaylor@chromium.org, kalman@chromium.org

[Joao da Silva, 2014-10-22 12:28:08]

I'm not sure about the CrxInstaller changes, but without some fix there then
extensions installed via external providers won't get their dependencies
installed.

This problem doesn't happen for manual installations via the CWS because the
WebstoreInstaller has different logic to handle dependencies.

IIUC, the CrxInstaller relies on the logic in
ExtensionService::OnExtensionInstalled to put the extension in a pending state
until its dependencies are installed too.

Please advise. Thanks!

[not at google - send to devlin, 2014-10-22 15:29:15]

kalman@chromium.org changed reviewers:
- kalman@chromium.org

[not at google - send to devlin, 2014-10-22 15:29:15]

Removing myself as reviewer, Antony is more appropriate.

[elijahtaylor1, 2014-10-22 16:40:02]

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.cc:489: if (imported_module &&
As I stated in the other CL, this is probably ok.  Since WebstoreInstaller
installs the shared modules first, there should be no way for this to fail from
there.  The only downside is that developers side-loading CRXs that forget to
install shared modules first will not get any error warning and the extension
gets into a weird state (delayed I guess), but we are phasing out that workflow

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/policy/po...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:1615: const char kSharedModuleId[] =
"nfgclafboonjbiafbllihiailjlhelpm";
be careful if you are using a new ID that is not whitelisted to be a shared
module.  "export" is whitelisted for all extensions on dev, but only a handful
on stable.  I've gotten bitten by the windows official builder because it would
report "stable" or "beta" channel.  You can override the channel via a scoped
member variable for the class, but I forget the exact class name.

[elijahtaylor1, 2014-10-22 16:45:17]

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.cc (left):

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.cc:501: } else if (imported_module &&
(version_required.IsValid() &&
one rare-ish edge-case here to consider: It is possible for an app which imports
a shared module to require a newer version of the shared module than what is
installed (possibly because another app that depends on the module has been long
running and is keeping the update from happening).  

For the webstore: for new extensions you try to install, it gives you an error
message, which is kind of actionable.  For updated extensions which require a
newer SM, the update just silently fails and it will retry next update cycle.

If there is a policy change which requires a new App to be installed which
requires a newer SM, is that handled at least as gracefully as web store
installs (retry later)?

[Joao da Silva, 2014-10-22 18:31:05]

Thanks for the hints. I'll update this CL tomorrow; just a couple more questions
inline.

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.cc (left):

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.cc:501: } else if (imported_module &&
(version_required.IsValid() &&
On 2014/10/22 16:45:17, elijahtaylor1 wrote:
> one rare-ish edge-case here to consider: It is possible for an app which
imports
> a shared module to require a newer version of the shared module than what is
> installed (possibly because another app that depends on the module has been
long
> running and is keeping the update from happening).  
> 
> For the webstore: for new extensions you try to install, it gives you an error
> message, which is kind of actionable.  For updated extensions which require a
> newer SM, the update just silently fails and it will retry next update cycle.
> 
> If there is a policy change which requires a new App to be installed which
> requires a newer SM, is that handled at least as gracefully as web store
> installs (retry later)?

Yes. Basically requests to install an extension via policy will stay in the
pending install list, and every extension update check will try to install them.
This check is also always done a couple of minutes after every restart.

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.cc:489: if (imported_module &&
On 2014/10/22 16:40:01, elijahtaylor1 wrote:
> As I stated in the other CL, this is probably ok.  Since WebstoreInstaller
> installs the shared modules first, there should be no way for this to fail
from
> there.  The only downside is that developers side-loading CRXs that forget to
> install shared modules first will not get any error warning and the extension
> gets into a weird state (delayed I guess), but we are phasing out that
workflow

If you agree then it would be great to do this fix for now.

Another possibility is to move the logic that installs the shared_modules out of
the WebstoreInstaller and reuse it for installations via external_loaders too;
only developers loading unpacked extensions should bypass that.

WDYT?

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/policy/po...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:1615: const char kSharedModuleId[] =
"nfgclafboonjbiafbllihiailjlhelpm";
On 2014/10/22 16:40:01, elijahtaylor1 wrote:
> be careful if you are using a new ID that is not whitelisted to be a shared
> module.  "export" is whitelisted for all extensions on dev, but only a handful
> on stable.  I've gotten bitten by the windows official builder because it
would
> report "stable" or "beta" channel.  You can override the channel via a scoped
> member variable for the class, but I forget the exact class name.

Thanks for the tip. I'll fake the channel.

[elijahtaylor1, 2014-10-22 19:57:49]

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.cc:489: if (imported_module &&
On 2014/10/22 18:31:05, Joao da Silva wrote:
> On 2014/10/22 16:40:01, elijahtaylor1 wrote:
> > As I stated in the other CL, this is probably ok.  Since WebstoreInstaller
> > installs the shared modules first, there should be no way for this to fail
> from
> > there.  The only downside is that developers side-loading CRXs that forget
to
> > install shared modules first will not get any error warning and the
extension
> > gets into a weird state (delayed I guess), but we are phasing out that
> workflow
> 
> If you agree then it would be great to do this fix for now.
> 
> Another possibility is to move the logic that installs the shared_modules out
of
> the WebstoreInstaller and reuse it for installations via external_loaders too;
> only developers loading unpacked extensions should bypass that.
> 
> WDYT?

I don't know much about external_loaders, but if we fixed it this way:  Does
this mean that if you drag-n-drop a CRX that depends on a shared module that is
not installed, that you would download it from the webstore?  I've shied away
from downloading shared modules for users unless the depending extension is also
from the webstore, but that may be unreasonable.

At the very least we can leave this as-is for this CL and file a bug to track
moving the webstoreinstaller code as you described.  Please elaborate on the
external loaders stuff in the bug, as I'm unfamiliar.

[Joao da Silva, 2014-10-23 09:15:02]

Ready to land. Please have a look, thanks!

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/666223005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.cc:489: if (imported_module &&
On 2014/10/22 19:57:49, elijahtaylor1 wrote:
> On 2014/10/22 18:31:05, Joao da Silva wrote:
> > On 2014/10/22 16:40:01, elijahtaylor1 wrote:
> > > As I stated in the other CL, this is probably ok.  Since WebstoreInstaller
> > > installs the shared modules first, there should be no way for this to fail
> > from
> > > there.  The only downside is that developers side-loading CRXs that forget
> to
> > > install shared modules first will not get any error warning and the
> extension
> > > gets into a weird state (delayed I guess), but we are phasing out that
> > workflow
> > 
> > If you agree then it would be great to do this fix for now.
> > 
> > Another possibility is to move the logic that installs the shared_modules
out
> of
> > the WebstoreInstaller and reuse it for installations via external_loaders
too;
> > only developers loading unpacked extensions should bypass that.
> > 
> > WDYT?
> 
> I don't know much about external_loaders, but if we fixed it this way:  Does
> this mean that if you drag-n-drop a CRX that depends on a shared module that
is
> not installed, that you would download it from the webstore?  I've shied away
> from downloading shared modules for users unless the depending extension is
also
> from the webstore, but that may be unreasonable.
> 
> At the very least we can leave this as-is for this CL and file a bug to track
> moving the webstoreinstaller code as you described.  Please elaborate on the
> external loaders stuff in the bug, as I'm unfamiliar.

I think the drag-and-drop flow is the same as the flow for external providers,
so yes, this would enable the installation of dependencies for CRXes installed
by drap-and-drop.

I've filed a bug to fix this at
https://code.google.com/p/chromium/issues/detail?id=426356.

IMO, extensions installed via external providers should get any dependencies
installed; this includes policy configuration, child accounts, retail-mode
preinstalled extensions, etc.

[asargent_no_longer_on_chrome, 2014-10-23 20:56:47]

lgtm, apologies for delay

[Joao da Silva, 2014-10-23 20:59:12]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-10-23 20:59:45]

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

[commit-bot: I haz the power, 2014-10-23 23:40:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-23 23:41:30]

Patchset 5 (id:??) landed as
https://crrev.com/30c8be95aea94a91aa5772e6b070fb9243eaf10d
Cr-Commit-Position: refs/heads/master@{#300984}