Auto-install/uninstall shared module dependencies for extensions.

Auto-install/uninstall shared module dependencies for extensions.

BUG=237251
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207744

[elijahtaylor1, 2013-05-16 18:52:16]

Early prototype, no tests yet, looking for feedback on how viable this solution
is.

As discussed with asargent, this works the following way:
1) shared modules don't sync automatically (already true)
2) on install of dependent extension, imports are checked and added to pending
extension manager, dependent extension is disabled initially
3) when shared module is installed, re-check all dependent extensions and enable
them as necessary
4) on uninstall of a dependent extension, check all shared modules it imports
and uninstall ones that are no longer referenced by any other extensions.

Also checks all extensions on startup to catch any shared modules that were in
progress when the browser was shutdown.

This interacts poorly with sync right now: when a second client syncs the
dependent extension and goes to fetch the shared module, it disables the
dependent extension globally.  It really should use some local state to
determine that it's "not ready" instead of using the extension/extension_pref
disabled state.  Ideas?

[Matt Perry, 2013-05-16 19:17:55]

https://codereview.chromium.org/14973007/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14973007/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:2472: if
(ShouldDelayExtensionUpdate(id, wait_for_idle)) {
We have this concept of delaying an extension update until some conditions are
met. Perhaps we can generalize that to "delay extension install". The dependent
extension can be put in this limbo state until the dependencies are installed.

[asargent_no_longer_on_chrome, 2013-05-17 17:13:49]

Overall direction seems fine.

https://codereview.chromium.org/14973007/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14973007/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:2306: if
(pending_extension_manager()->AddFromExtensionImport(
in the implementation of PendingExtensionManager::AddFromExtensionImport, I
notice you log an error if the dependent module is already installed. But you
get into this code path even if the dependent module *is* installed but just not
a new enough version. Maybe you want to skip calling
PendingExtensionManager::AddFromExtensionImport if it's already installed, and
just pull the call to CheckForUpdatesSoon out of the if statement body here?

https://codereview.chromium.org/14973007/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:2472: if
(ShouldDelayExtensionUpdate(id, wait_for_idle)) {
I endorse Matt's suggestion here.

[Marijn Kruisselbrink, 2013-05-20 18:28:23]

I'm sure you've thought of it, but if you use the enabled/disabled state to keep
track of which apps/extensions have missing dependencies, you'll probably also
have to modify ExtensionEnableFlow to prevent a user from manually re-enabling
an app/extension that doesn't have all its dependencies installed? Especially
since disabled apps can still be launched, and will just silently get re-enabled
unless they were disabled because of increased permissions (at least I think
that is still the behavior).

[elijahtaylor1, 2013-05-21 18:30:30]

FYI since I'm putting this on hold to work on other things this week for a
fixit: I've been trying an implemention of Matt's suggestion to use delayed
install to handle the install of these.  There are two concepts of delayed
installs (global based on GC being active, and delayed updates for non-idle
extensions), but I was looking to unify these concepts.

One tricky part I've encountered is persisting delayed installs (not updates)
across browser restart.  These aren't handled properly because only the
delayed_info extension prefs is filled out and the current delayed update logic
requires other information filled out.  It should be possible with a slight
refactor of the delayed update code.


On 2013/05/20 18:28:23, Marijn Kruisselbrink wrote:
> I'm sure you've thought of it, but if you use the enabled/disabled state to
keep
> track of which apps/extensions have missing dependencies, you'll probably also
> have to modify ExtensionEnableFlow to prevent a user from manually re-enabling
> an app/extension that doesn't have all its dependencies installed? Especially
> since disabled apps can still be launched, and will just silently get
re-enabled
> unless they were disabled because of increased permissions (at least I think
> that is still the behavior).

[elijahtaylor1, 2013-06-12 00:21:47]

Hi folks.  PTAL.

I unified all delayed installs into a single ExtensionSet.  No tests still,
wanted preliminary feedback before committing to them.

One thing that is unfortunate now is that after the original webstore download,
the extension is in limbo and not installed, but the focus goes to the new tab
page as though it's supposed to display the newly installed app.  Delayed
installed extensions probably deserve some UI treatment, but I wasn't familiar
with the NTP or chrome://extensions page to do this.  Pointers/contacts are
welcome there, but I would prefer to submit this CL before worrying about UI
treatment.

[Matt Perry, 2013-06-12 00:58:29]

Good stuff. I really like how clean this ended up being.

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.cc:1454:
pending_install_dict->Remove(kDelayedInstallReason, NULL);
you can call Remove without calling GetInteger first.

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2273:
GetSharedModuleImporters(imported_module);
Seems like all you really need is HasSharedModuleImporters.

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2383: } else if
(!CheckImports(extension)) {
If this fails because the imported module is not a shared module or if extension
is not from the webstore, it will forever be stuck in a delayed-install state.
We should probably call it an installation failure and alert the user, instead.

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:69: } // namespace extensions
nit: 2 spaces before //. But the comment is unnecessary in this context, IMO.

[elijahtaylor1, 2013-06-12 23:53:23]

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.cc:1454:
pending_install_dict->Remove(kDelayedInstallReason, NULL);
On 2013/06/12 00:58:29, Matt Perry wrote:
> you can call Remove without calling GetInteger first.

Done.

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2273:
GetSharedModuleImporters(imported_module);
On 2013/06/12 00:58:29, Matt Perry wrote:
> Seems like all you really need is HasSharedModuleImporters.

I originally had that functionality only, but then started to refactor because I
was going to add a check to IsExtensionIdle to check importers to see if all of
them are idle before updating.  But then I felt that deserved a separate CL so I
didn't follow through, but wanted to keep it this way so it will be easy to
implement later.

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2383: } else if
(!CheckImports(extension)) {
On 2013/06/12 00:58:29, Matt Perry wrote:
> If this fails because the imported module is not a shared module or if
extension
> is not from the webstore, it will forever be stuck in a delayed-install state.
> We should probably call it an installation failure and alert the user,
instead.

As discussed offline, I've added a TODO for messaging the user, but I've also
added logic here and in MaybeFinishDelayedInstallation to check if this is an
unrecoverable error and if so, don't add the extension to (or remove it from)
the delayed list and extension prefs.

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/14973007/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:69: } // namespace extensions
On 2013/06/12 00:58:29, Matt Perry wrote:
> nit: 2 spaces before //. But the comment is unnecessary in this context, IMO.

I did this in response to a presubmit check.  It only flagged this one, I
assumed it was a "number of lines" threshold for a namespace that was recently
added.

[Matt Perry, 2013-06-12 23:57:07]

lgtm

[asargent_no_longer_on_chrome, 2013-06-13 05:47:16]

a few small nits/suggestions/questions

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:398: enum DelayReason {
Since the literal values of these are stored in users Preferences files, you
should add a warning comment here that existing values in this enum should not
be changed/removed.

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:400: DELAY_REASON_GLOBAL = 1,
nit: it's not immediately obvious what GLOBAL means - maybe add a comment (or
come up with a more descriptive name?)

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2215: // unrecoverable.
Maybe add "CHECK(unrecoverable != NULL);" here?

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2389: bool
unrecoverable_import_error;
consider initializing (otherwise you might get a compile or clang plugin warning
about passing address of uninitialized variable below, right?)

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:420: bool* unrecoverable_error);
Huh, it's pretty unusual to see a function returning two bools, one as the
normal return value and one as an out param. If I'm understanding your comment
right, it seems like this one function might be serving 3 different purposes:

1) checking whether imports are already satisfied
2) checking if imports cannot ever be satisfied (e.g. it depends on another
extension, but isn't served from the webstore and that other extension isn't
already installed)
3) beginning the process of fetching dependencies

Would it make sense to break this into 3 separate functions for these 3 uses?

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:423: scoped_ptr<const
ExtensionSet> GetSharedModuleImporters(
optional naming suggestion: GetDependents / GetDependentExtensions  might be
slightly better

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:428: // Uninstalls shared modules
that are no longer referenced.
nit: The first sentence in this comment is sort of confusing. Consider removing
it, and just making the second sentence something like "Uninstalls shared
modules that are only referenced by |extension|."

[Matt Perry, 2013-06-13 17:42:58]

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:398: enum DelayReason {
Also, move this to the top of the class.

[elijahtaylor1, 2013-06-13 23:32:09]

working on some unit tests now...

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:398: enum DelayReason {
On 2013/06/13 17:42:59, Matt Perry wrote:
> Also, move this to the top of the class.

Done.

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:398: enum DelayReason {
On 2013/06/13 05:47:16, Antony Sargent wrote:
> Since the literal values of these are stored in users Preferences files, you
> should add a warning comment here that existing values in this enum should not
> be changed/removed. 

Done.

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:400: DELAY_REASON_GLOBAL = 1,
On 2013/06/13 05:47:16, Antony Sargent wrote:
> nit: it's not immediately obvious what GLOBAL means - maybe add a comment (or
> come up with a more descriptive name?)

Done.

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2215: // unrecoverable.
On 2013/06/13 05:47:16, Antony Sargent wrote:
> Maybe add "CHECK(unrecoverable != NULL);" here?

removed

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2389: bool
unrecoverable_import_error;
On 2013/06/13 05:47:16, Antony Sargent wrote:
> consider initializing (otherwise you might get a compile or clang plugin
warning
> about passing address of uninitialized variable below, right?)

removed

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:420: bool* unrecoverable_error);
On 2013/06/13 05:47:16, Antony Sargent wrote:
> Huh, it's pretty unusual to see a function returning two bools, one as the
> normal return value and one as an out param. If I'm understanding your comment
> right, it seems like this one function might be serving 3 different purposes:
> 
> 1) checking whether imports are already satisfied
> 2) checking if imports cannot ever be satisfied (e.g. it depends on another
> extension, but isn't served from the webstore and that other extension isn't
> already installed)
> 3) beginning the process of fetching dependencies
> 
> Would it make sense to break this into 3 separate functions for these 3 uses?

Ok you caught me writing bad code for sure.  I would really say this function
does two things: your numbers 1+2 combined, and 3.  I've changed the return
value to an enum to capture state, and restructured the code in CheckImports to
more clearly separate 1+2 and 3 functionality.  I think it makes sense to keep
it all together rather than duplicate code or factor out the code to get the
pending list of extensions.  PTAL and let me know if this is ok with you.

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:423: scoped_ptr<const
ExtensionSet> GetSharedModuleImporters(
On 2013/06/13 05:47:16, Antony Sargent wrote:
> optional naming suggestion: GetDependents / GetDependentExtensions  might be
> slightly better

Done.

https://codereview.chromium.org/14973007/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:428: // Uninstalls shared modules
that are no longer referenced.
On 2013/06/13 05:47:16, Antony Sargent wrote:
> nit: The first sentence in this comment is sort of confusing. Consider
removing
> it, and just making the second sentence something like "Uninstalls shared
> modules that are only referenced by |extension|."

Done.

[asargent_no_longer_on_chrome, 2013-06-13 23:43:55]

lgtm

https://codereview.chromium.org/14973007/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/14973007/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:422: ImportStatus
CheckImports(const extensions::Extension* extension);
optional naming suggestion: would "SatisfyImports" or "EnsureImports" or
something along those lines be a better name? For some reason "check" implies to
me that it would check state but not take action to initiate an install. But
this isn't that big a deal so feel free to leave as-is if you can't come up with
anything better.

[elijahtaylor1, 2013-06-19 00:26:58]

rebased and updated with tests.  Waiting for trys before submitting since I've
not run any yet.  Also incorporates https://codereview.chromium.org/17296003 so
waiting until that lands to rebase again and submit.

https://codereview.chromium.org/14973007/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/14973007/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:422: ImportStatus
CheckImports(const extensions::Extension* extension);
On 2013/06/13 23:43:55, Antony Sargent wrote:
> optional naming suggestion: would "SatisfyImports" or "EnsureImports" or
> something along those lines be a better name? For some reason "check" implies
to
> me that it would check state but not take action to initiate an install. But
> this isn't that big a deal so feel free to leave as-is if you can't come up
with
> anything better. 

Done.

[commit-bot: I haz the power, 2013-06-20 20:05:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/14973007/52001

[commit-bot: I haz the power, 2013-06-21 07:11:21]

Change committed as 207744