Cleaning Up Extensions When Local Content Removed

Cleaning Up Extensions When Local Content Removed

This fixes an issue where removing an extension's local content, e.g. deleting
the user-data-dir/Default/Extensions/<id> directory, would result in a broken
extension. Chrome will now uninstall/remove any extensions where the path to
the extension cannot be resolved.

BUG=31910
TEST=ExtensionServiceTest.CleanupInternalExtensionsMissingLocalContent;
ExtensionServiceTest.CleanupUnpackedExtensionsMissingLocalContent;
test by hand by loading an extension, deleting the local directory, and
restarting chrome


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

[Aaron Boodman, 2012-06-14 19:35:18]

Delegating to yoz

[Yoyo Zhou, 2012-06-14 20:37:22]

Can you comment on what's changed since the previous version of this CL?

[Devlin, 2012-06-14 21:07:00]

On 2012/06/14 20:37:22, Yoyo Zhou wrote:
> Can you comment on what's changed since the previous version of this CL?

Sorry, commented on the differences in a message to aa@ in IRC - shoulda put it
here.

The only significant change is to sync_extension_helper; the previous cl broke
these tests. GarbageCollector behaved properly and cleaned up the extensions
that were created in an order contrary to the actual installation process. Now
sync_extension_helper uses threading to avoid the race condition. Passed it
through the trybots for regular tests and sync_integration_tests.

[Yoyo Zhou, 2012-06-14 22:34:09]

LGTM; I didn't review the garbage_collector code as carefully since I assume it
hasn't changed much.

http://codereview.chromium.org/10541126/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_garbage_collector.cc (right):

http://codereview.chromium.org/10541126/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:35: // ExtensionService
uses, and there are ordering dependencies between all of
nit: ExtensionService uses for what?

http://codereview.chromium.org/10541126/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/10541126/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:489:
extensions::ExtensionGarbageCollector* extension_garbage_collector() {
You don't need an accessor if it's not used outside of ExtensionService.

[Devlin, 2012-06-14 22:51:34]

Changes done; thanks for the review :)

https://chromiumcodereview.appspot.com/10541126/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/extension_garbage_collector.cc (right):

https://chromiumcodereview.appspot.com/10541126/diff/5001/chrome/browser/exte...
chrome/browser/extensions/extension_garbage_collector.cc:35: // ExtensionService
uses, and there are ordering dependencies between all of
On 2012/06/14 22:34:09, Yoyo Zhou wrote:
> nit: ExtensionService uses for what?

Done.

https://chromiumcodereview.appspot.com/10541126/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/10541126/diff/5001/chrome/browser/exte...
chrome/browser/extensions/extension_service.h:489:
extensions::ExtensionGarbageCollector* extension_garbage_collector() {
On 2012/06/14 22:34:09, Yoyo Zhou wrote:
> You don't need an accessor if it's not used outside of ExtensionService.

Done.

[commit-bot: I haz the power, 2012-06-14 22:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10541126/6016

[commit-bot: I haz the power, 2012-06-14 22:51:50]

Presubmit check for 10541126-6016 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/sync

Presubmit checks took 1.4s to calculate.

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

[Andrew T Wilson (Slow), 2012-06-15 16:45:44]

browser/sync LGTM

[commit-bot: I haz the power, 2012-06-15 16:58:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10541126/1...

[commit-bot: I haz the power, 2012-06-15 18:09:43]

Change committed as 142427

[Evan Stade, 2012-06-15 21:06:04]

this patch seems to have broken --load-extension. Reverting it locally makes the
flag start working again.

[Devlin, 2012-06-15 21:16:15]

On 2012/06/15 21:06:04, Evan Stade wrote:
> this patch seems to have broken --load-extension. Reverting it locally makes
the
> flag start working again.

Dang, you're probably right. GarbageCollector doesn't play nice with
synchronously installed extensions, because it needs to jump around the threads.
 Ironically, the TODO here
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
would probably fix it...

I'll see what I can do; thanks for the heads-up.

[Devlin, 2012-06-15 21:38:42]

On 2012/06/15 21:06:04, Evan Stade wrote:
> this patch seems to have broken --load-extension. Reverting it locally makes
the
> flag start working again.

On second thought...this might not be me.  I have a revision from 142392, which
has the same issue.  Also, when it breaks (at least for me), I get the "Manifest
file is missing or can't be read" from here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/e...
I've checked, the manifest file is there...

[Devlin, 2012-06-15 21:44:29]

On 2012/06/15 21:38:42, D Cronin wrote:
> On 2012/06/15 21:06:04, Evan Stade wrote:
> > this patch seems to have broken --load-extension. Reverting it locally makes
> the
> > flag start working again.
> 
> On second thought...this might not be me.  I have a revision from 142392,
which
> has the same issue.  Also, when it breaks (at least for me), I get the
"Manifest
> file is missing or can't be read" from here:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/e...
> I've checked, the manifest file is there...

Another update, --load-extension doesn't seem to be working on 142414 either...