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=134062

[Aaron Boodman, 2012-03-21 21:00:47]

+yoz for his opinion on my idea.

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
chrome/browser/extensions/extension_service.cc:1941: if
(!file_util::PathExists(info->at(i)->extension_path)) {
You cannot use file_util::PathExists here (or indeed anything in file_util)
because it touches disk. In debug builds this will DCHECK.

You need to post a message to the file thread to collect a list of extension IDs
that are known there, and then compare it to the list that is known in
ExtensionService.

Stepping back slightly, I would like to pull GarbageCollection out into its own
little helper class (I hate how big ExtensionService is).

It would look a little similar to something like UnpackedInstaller. It would
start out on the UI thread, gather a list of known extension IDs, bounce to the
file thread, delete any unknown directories as well as collect a list of known
ids there, then bounce back to the UI thread where it would uninstall any
extensions with no directory.

Do you think this is something you could do?

[Devlin, 2012-03-21 21:48:38]

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
chrome/browser/extensions/extension_service.cc:1941: if
(!file_util::PathExists(info->at(i)->extension_path)) {
On 2012/03/21 21:00:47, Aaron Boodman wrote:
> You cannot use file_util::PathExists here (or indeed anything in file_util)
> because it touches disk. In debug builds this will DCHECK.
> 
> You need to post a message to the file thread to collect a list of extension
IDs
> that are known there, and then compare it to the list that is known in
> ExtensionService.
> 
> Stepping back slightly, I would like to pull GarbageCollection out into its
own
> little helper class (I hate how big ExtensionService is).
> 
> It would look a little similar to something like UnpackedInstaller. It would
> start out on the UI thread, gather a list of known extension IDs, bounce to
the
> file thread, delete any unknown directories as well as collect a list of known
> ids there, then bounce back to the UI thread where it would uninstall any
> extensions with no directory.
> 
> Do you think this is something you could do?

Not entirely sure why file_util works...but it does. (Is this something that
should be looked into separately?)
http://imgur.com/zJlhO

Otherwise, I see no problem with separating out GarbageCollection; I should be
able to do that.

[Yoyo Zhou, 2012-03-21 22:55:18]

On 2012/03/21 21:00:47, Aaron Boodman wrote:
> +yoz for his opinion on my idea.

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
> chrome/browser/extensions/extension_service.cc:1941: if
> (!file_util::PathExists(info->at(i)->extension_path)) {
> You cannot use file_util::PathExists here (or indeed anything in file_util)
> because it touches disk. In debug builds this will DCHECK.
> 
> You need to post a message to the file thread to collect a list of extension
IDs
> that are known there, and then compare it to the list that is known in
> ExtensionService.
> 
> Stepping back slightly, I would like to pull GarbageCollection out into its
own
> little helper class (I hate how big ExtensionService is).
> 
> It would look a little similar to something like UnpackedInstaller. It would
> start out on the UI thread, gather a list of known extension IDs, bounce to
the
> file thread, delete any unknown directories as well as collect a list of known
> ids there, then bounce back to the UI thread where it would uninstall any
> extensions with no directory.

This is a good idea, since we need to go back and forth between threads. I was
almost going to write the same thing.

> Do you think this is something you could do?

[Yoyo Zhou, 2012-03-21 22:55:37]

A few nits on the test.

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
chrome/browser/extensions/extension_service_unittest.cc:1159: // Test that
partially deleted extensions are cleaned up during startup.
This test seems to be testing that extensions who were deleted from the
preferences are cleaned up. Can you change this text to reflect that?

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
chrome/browser/extensions/extension_service_unittest.cc:1230:
ASSERT_TRUE(dictionary != NULL);
nit: usually written as just ASSERT_TRUE(dictionary)
also below

https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
chrome/browser/extensions/extension_service_unittest.cc:1234: // Test that
external extensions which are referenced in the preferences but
nit: write 'unpacked' instead of 'external'

[Aaron Boodman, 2012-03-21 23:26:18]

On 2012/03/21 21:48:38, D Cronin wrote:
>
https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9817018/diff/2001/chrome/browser/exten...
> chrome/browser/extensions/extension_service.cc:1941: if
> (!file_util::PathExists(info->at(i)->extension_path)) {
> On 2012/03/21 21:00:47, Aaron Boodman wrote:
> > You cannot use file_util::PathExists here (or indeed anything in file_util)
> > because it touches disk. In debug builds this will DCHECK.
> > 
> > You need to post a message to the file thread to collect a list of extension
> IDs
> > that are known there, and then compare it to the list that is known in
> > ExtensionService.
> > 
> > Stepping back slightly, I would like to pull GarbageCollection out into its
> own
> > little helper class (I hate how big ExtensionService is).
> > 
> > It would look a little similar to something like UnpackedInstaller. It would
> > start out on the UI thread, gather a list of known extension IDs, bounce to
> the
> > file thread, delete any unknown directories as well as collect a list of
known
> > ids there, then bounce back to the UI thread where it would uninstall any
> > extensions with no directory.
> > 
> > Do you think this is something you could do?
> 
> Not entirely sure why file_util works...but it does. (Is this something that
> should be looked into separately?)
> http://imgur.com/zJlhO

Somebody must be using a ScopedAllowIO lower on the stack that this code is
being called within :(. NBD.

> Otherwise, I see no problem with separating out GarbageCollection; I should be
> able to do that.

Cool.

[mihaip_google.com, 2012-03-22 19:25:57]

+Evan, for historical curiosity

On Wed, Mar 21, 2012 at 4:26 PM, <aa@chromium.org> wrote:

> Somebody must be using a ScopedAllowIO lower on the stack that this code is
> being called within :(. NBD.


I got curious about this. As far as I can tell, the restriction to disallow
IO on the UI thread is done near the end of BrowserMainLoop::CreateThreads :
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/browser_ma...

However, the ExtensionService::GarbageCollectExtensions call happens before
then; the callstack is:

ExtensionService::GarbageCollectExtensions()
ExtensionService::Init()
ProfileImpl::InitExtensions()
ProfileManager::DoFinalInitForServices()
ProfileManager::DoFinalInit()
ProfileManager::AddProfile()
ProfileManager::GetProfile()
ProfileManager::GetLastUsedProfile()
(anonymous namespace)::CreateProfile()
ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
ChromeBrowserMainParts::PreMainMessageLoopRun()
content::BrowserMainLoop::CreateThreads()

If I move the SetIOAllowed(false) call to be before
the PreMainMessageLoopRun, then the assert fires in other places, e.g.
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/browser_pro...

I'm guessing it's a significant undertaking to do this; so the GC should
either happen "later" (via PostTask) or a ScopedDisallowIO class could be
introduced to disallow IO in ExtensionService::Init.

Mihai

[mihaip_google.com, 2012-03-22 19:31:45]

On Thu, Mar 22, 2012 at 12:25 PM, Mihai Parparita <mihaip@google.com> wrote:

> The GC should either happen "later" (via PostTask)
>

And there's a TODO about exactly that:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

Mihai

[Aaron Boodman, 2012-03-22 19:44:30]

On Thu, Mar 22, 2012 at 12:25 PM, Mihai Parparita <mihaip@google.com> wrote:
> +Evan, for historical curiosity
>
> On Wed, Mar 21, 2012 at 4:26 PM, <aa@chromium.org> wrote:
>>
>> Somebody must be using a ScopedAllowIO lower on the stack that this code
>> is
>> being called within :(. NBD.
>
>
> I got curious about this. As far as I can tell, the restriction to disallow
> IO on the UI thread is done near the end of BrowserMainLoop::CreateThreads :
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/browser_ma...
>
> However, the ExtensionService::GarbageCollectExtensions call happens before
> then; the callstack is:
>
> ExtensionService::GarbageCollectExtensions()
> ExtensionService::Init()
> ProfileImpl::InitExtensions()
> ProfileManager::DoFinalInitForServices()
> ProfileManager::DoFinalInit()
> ProfileManager::AddProfile()
> ProfileManager::GetProfile()
> ProfileManager::GetLastUsedProfile()
> (anonymous namespace)::CreateProfile()
> ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
> ChromeBrowserMainParts::PreMainMessageLoopRun()
> content::BrowserMainLoop::CreateThreads()
>
> If I move the SetIOAllowed(false) call to be before
> the PreMainMessageLoopRun, then the assert fires in other places,
>
e.g. http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/browser_process_impl.cc&exact_package=chromium&q=PreMainMessageLoopRun&type=cs&l=754
>
> I'm guessing it's a significant undertaking to do this; so the GC should
> either happen "later" (via PostTask) or a ScopedDisallowIO class could be
> introduced to disallow IO in ExtensionService::Init.

I see. Thanks for the investigation.

Just moving GarbageCollectExtensions() is pretty weak. There are tons
of things in ExtensionService::Init() that would benefit from this
thread_restrictions. It would be better to start thread restrictions
earlier.

- a

[Aaron Boodman, 2012-03-22 20:16:52]

On Thu, Mar 22, 2012 at 12:25 PM, Mihai Parparita <mihaip@google.com> wrote:
> I'm guessing it's a significant undertaking to do this; so the GC should
> either happen "later" (via PostTask) or a ScopedDisallowIO class could be
> introduced to disallow IO in ExtensionService::Init.

I missed the ScopedDisallowIO suggestion last time I read this. That
is a good middle ground. In any case, it is a different bug than Matt
is working on.

- a

[Aaron Boodman, 2012-03-22 20:17:29]

On Thu, Mar 22, 2012 at 1:16 PM, Aaron Boodman <aa@chromium.org> wrote:
> On Thu, Mar 22, 2012 at 12:25 PM, Mihai Parparita <mihaip@google.com> wrote:
>> I'm guessing it's a significant undertaking to do this; so the GC should
>> either happen "later" (via PostTask) or a ScopedDisallowIO class could be
>> introduced to disallow IO in ExtensionService::Init.
>
> I missed the ScopedDisallowIO suggestion last time I read this. That
> is a good middle ground. In any case, it is a different bug than Matt
> is working on.

s/Matt/Devlin/

Sorry!

- a

[Devlin, 2012-03-31 15:49:44]

Refactor for pulling garbage collection into its own class completed.

[Aaron Boodman, 2012-03-31 23:33:33]

There's a race in here that was already there. I wonder if you could fix it
while you're working in here.

The race comes because in extension installation (see CrxInstaller), we write
the extensions to disk on the file thread first, then write it to the
preferences second.

So there can be cases like:

1. CrxInstaller writes extension to disk
2. GarbageCollector takes a snapshot of current extensions on UI thread
3. CrxInstaller writes extension to prefs on UI thread
4. GarbageCollector finds orphaned directory in filesystem and deletes it

We can address this by not doing any delete in the initial post to the file
thread, but instead only gathering information about the directories found there
and posting them back to the UI thread.

Back on the UI thread, compare the extensions from the prefs with the extensions
that were found on disk. The deletes from the preferences can happen directly as
you do now, and the deletes from the filesystem can happen by doing PostTask
with file_util::DeleteFile.

http://codereview.chromium.org/9817018/diff/9002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_garbage_collector.cc (right):

http://codereview.chromium.org/9817018/diff/9002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_garbage_collector.cc:47: DVLOG(1) <<
"Garbage collecting extensions...";
Add a CHECK that this is on the file thread.

http://codereview.chromium.org/9817018/diff/9002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_garbage_collector.cc:72: FilePath
install_directory = extension_service_->install_directory();
Add a CHECK that on the file thread.

http://codereview.chromium.org/9817018/diff/9002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_garbage_collector.cc:72: FilePath
install_directory = extension_service_->install_directory();
Not safe to access extension_service on a background thread. (in this case, it's
probably fine because this value doesn't change, but it's bad form) and might
confuse future maintainers.

http://codereview.chromium.org/9817018/diff/9002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_garbage_collector.cc:134: for
(std::vector<std::string>::const_iterator iter = bad_extensions.begin();
Add a CHECK for UI thread?

[Aaron Boodman, 2012-03-31 23:39:50]

http://codereview.chromium.org/9817018/diff/9002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_garbage_collector.cc (right):

http://codereview.chromium.org/9817018/diff/9002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_garbage_collector.cc:145:
extension_service_->UnloadExtension(
In that case, it seems like we wouldn't need to send this notification?

[Devlin, 2012-04-05 01:58:00]

All requested changes made, and the preexisting race condition should be
removed.

https://chromiumcodereview.appspot.com/9817018/diff/9002/chrome/browser/exten...
File chrome/browser/extensions/extension_garbage_collector.cc (right):

https://chromiumcodereview.appspot.com/9817018/diff/9002/chrome/browser/exten...
chrome/browser/extensions/extension_garbage_collector.cc:47: DVLOG(1) <<
"Garbage collecting extensions...";
On 2012/03/31 23:33:33, Aaron Boodman wrote:
> Add a CHECK that this is on the file thread.

Done.

https://chromiumcodereview.appspot.com/9817018/diff/9002/chrome/browser/exten...
chrome/browser/extensions/extension_garbage_collector.cc:72: FilePath
install_directory = extension_service_->install_directory();
On 2012/03/31 23:33:33, Aaron Boodman wrote:
> Add a CHECK that on the file thread.

Done.

https://chromiumcodereview.appspot.com/9817018/diff/9002/chrome/browser/exten...
chrome/browser/extensions/extension_garbage_collector.cc:72: FilePath
install_directory = extension_service_->install_directory();
On 2012/03/31 23:33:33, Aaron Boodman wrote:
> Not safe to access extension_service on a background thread. (in this case,
it's
> probably fine because this value doesn't change, but it's bad form) and might
> confuse future maintainers.

Done.

https://chromiumcodereview.appspot.com/9817018/diff/9002/chrome/browser/exten...
chrome/browser/extensions/extension_garbage_collector.cc:134: for
(std::vector<std::string>::const_iterator iter = bad_extensions.begin();
On 2012/03/31 23:33:33, Aaron Boodman wrote:
> Add a CHECK for UI thread?

Done.

https://chromiumcodereview.appspot.com/9817018/diff/9002/chrome/browser/exten...
chrome/browser/extensions/extension_garbage_collector.cc:145:
extension_service_->UnloadExtension(
On 2012/03/31 23:39:50, Aaron Boodman wrote:
> In that case, it seems like we wouldn't need to send this notification?

Done.

[Aaron Boodman, 2012-04-05 21:53:14]

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

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:30: if
(!content::BrowserThread::PostTask(
I think the current guidance to fire-and-forget PostTask. No need to check
return value.

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:31:
content::BrowserThread::FILE, FROM_HERE,
Add a comment saying something like:

// NOTE: It's important to use the file thread in particular
// because that is what the rest of the extension service 
// does and there are ordering dependencies between all the
// extension-file-management tasks.

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:55:
!extension_path.value().empty();
Can this part fit on the previous line?

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:114: extension_id =
UTF16ToASCII(basename.LossyDisplayName());
basename.MaybeAsASCII would simplify this a bit.

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:137:
file_util::FileEnumerator versions_enumerator(
You are on the UI thread here. No filesystem access on the UI thread (why isn't
this DCHECKing?). You could create a new helper method to do this part of the
work and PostTask to it here on the file thread.

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_garbage_collector.h (right):

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.h:33: void
CheckExtensionDirectories(const FilePath& installation_directory);
Can you add "OnBackgroundThread" to the methods that are called there? It makes
it easier to get a general gist of the class from the header file.

[Devlin, 2012-04-07 17:38:21]

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

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:30: if
(!content::BrowserThread::PostTask(
On 2012/04/05 21:53:14, Aaron Boodman wrote:
> I think the current guidance to fire-and-forget PostTask. No need to check
> return value.

There are a few instances where checking the value is used, such as
extension_service and crx_installer, but it looks like you're right - the vast
majority are fire-and-forget.  Changed to match.

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:31:
content::BrowserThread::FILE, FROM_HERE,
On 2012/04/05 21:53:14, Aaron Boodman wrote:
> Add a comment saying something like:
> 
> // NOTE: It's important to use the file thread in particular
> // because that is what the rest of the extension service 
> // does and there are ordering dependencies between all the
> // extension-file-management tasks.

Done.

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:55:
!extension_path.value().empty();
On 2012/04/05 21:53:14, Aaron Boodman wrote:
> Can this part fit on the previous line?

Done.

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:114: extension_id =
UTF16ToASCII(basename.LossyDisplayName());
On 2012/04/05 21:53:14, Aaron Boodman wrote:
> basename.MaybeAsASCII would simplify this a bit.

Done.

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:137:
file_util::FileEnumerator versions_enumerator(
On 2012/04/05 21:53:14, Aaron Boodman wrote:
> You are on the UI thread here. No filesystem access on the UI thread (why
isn't
> this DCHECKing?). You could create a new helper method to do this part of the
> work and PostTask to it here on the file thread.

Done.

[Aaron Boodman, 2012-04-07 18:01:03]

LGTM, don't need another review from me

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_garbage_collector.h (right):

http://codereview.chromium.org/9817018/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.h:33: void
CheckExtensionDirectories(const FilePath& installation_directory);
On 2012/04/05 21:53:14, Aaron Boodman wrote:
> Can you add "OnBackgroundThread" to the methods that are called there? It
makes
> it easier to get a general gist of the class from the header file.

You forgot this bit.

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

http://codereview.chromium.org/9817018/diff/22010/chrome/browser/extensions/e...
chrome/browser/extensions/extension_garbage_collector.cc:83: void
ExtensionGarbageCollector::CleanupOldVersions(
Nit: can you move the definition of this method down to the end of the file, it
is the last thing to happen.

http://codereview.chromium.org/9817018/diff/22010/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/9817018/diff/22010/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:1228:
PluginService::GetInstance()->Init();
I don't think this is needed unless you're using plugin functionality somehow.
Try removing it.

http://codereview.chromium.org/9817018/diff/22010/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:1233:
InitializeInstalledExtensionService(pref_path, source_dir);
ASSERT the number of extensions in the prefs you're starting with?

http://codereview.chromium.org/9817018/diff/22010/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:1253:
DictionaryPrefUpdate update(profile_->GetPrefs(), "extensions.settings");
Nit: If you're not actually doing any updating, you can just access the
Preferences object from GetPrefs() directly.

http://codereview.chromium.org/9817018/diff/22010/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:1261:
TEST_F(ExtensionServiceTest, GarbageCollectUnpackedExtensionsMissingContent) {
Can you look into moving all the garbage collection tests extension
extension_garbage_collector_unittest.cc?

[Yoyo Zhou, 2012-04-10 19:36:45]

Can you remove the garbage collection code from extension_file_util.cc now that
you've refactored it out?

[Devlin, 2012-04-14 18:13:03]

On 2012/04/10 19:36:45, Yoyo Zhou wrote:
> Can you remove the garbage collection code from extension_file_util.cc now
that
> you've refactored it out?

Dang, had that done and forgot to include in the file uploads. Sorry.

[Devlin, 2012-04-14 18:13:21]

https://chromiumcodereview.appspot.com/9817018/diff/13001/chrome/browser/exte...
File chrome/browser/extensions/extension_garbage_collector.h (right):

https://chromiumcodereview.appspot.com/9817018/diff/13001/chrome/browser/exte...
chrome/browser/extensions/extension_garbage_collector.h:33: void
CheckExtensionDirectories(const FilePath& installation_directory);
On 2012/04/07 18:01:03, Aaron Boodman wrote:
> On 2012/04/05 21:53:14, Aaron Boodman wrote:
> > Can you add "OnBackgroundThread" to the methods that are called there? It
> makes
> > it easier to get a general gist of the class from the header file.
> 
> You forgot this bit.

Whoops, done.

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

https://chromiumcodereview.appspot.com/9817018/diff/22010/chrome/browser/exte...
chrome/browser/extensions/extension_garbage_collector.cc:83: void
ExtensionGarbageCollector::CleanupOldVersions(
On 2012/04/07 18:01:03, Aaron Boodman wrote:
> Nit: can you move the definition of this method down to the end of the file,
it
> is the last thing to happen.

Done.

https://chromiumcodereview.appspot.com/9817018/diff/22010/chrome/browser/exte...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/9817018/diff/22010/chrome/browser/exte...
chrome/browser/extensions/extension_service_unittest.cc:1228:
PluginService::GetInstance()->Init();
On 2012/04/07 18:01:03, Aaron Boodman wrote:
> I don't think this is needed unless you're using plugin functionality somehow.
> Try removing it.

Tried removing; it broke.

https://chromiumcodereview.appspot.com/9817018/diff/22010/chrome/browser/exte...
chrome/browser/extensions/extension_service_unittest.cc:1233:
InitializeInstalledExtensionService(pref_path, source_dir);
On 2012/04/07 18:01:03, Aaron Boodman wrote:
> ASSERT the number of extensions in the prefs you're starting with?

Done.

https://chromiumcodereview.appspot.com/9817018/diff/22010/chrome/browser/exte...
chrome/browser/extensions/extension_service_unittest.cc:1253:
DictionaryPrefUpdate update(profile_->GetPrefs(), "extensions.settings");
On 2012/04/07 18:01:03, Aaron Boodman wrote:
> Nit: If you're not actually doing any updating, you can just access the
> Preferences object from GetPrefs() directly.

Done.

https://chromiumcodereview.appspot.com/9817018/diff/22010/chrome/browser/exte...
chrome/browser/extensions/extension_service_unittest.cc:1261:
TEST_F(ExtensionServiceTest, GarbageCollectUnpackedExtensionsMissingContent) {
On 2012/04/07 18:01:03, Aaron Boodman wrote:
> Can you look into moving all the garbage collection tests extension
> extension_garbage_collector_unittest.cc?

Unfortunately, these tests use a few things unique to ExtensionServiceTest
(versus ExtensionServiceTestBase, which is what's in the header file), e.g.
loaded_, ValidatePrefKeyCount(), etc. Moving the tests would mean moving the
definition of ExtensionServiceTest out of the cc file. Since other services use
this as a testing ground (e.g. installing, uninstalling, packing extensions), I
assumed it was alright for garbage collection to be here, too. If you want me to
refactor enough to move garbage collection out, lemme know and I'd be happy to
do it in a separate CL (this one's growing a little large, and it's a decent
size refactor).

[Aaron Boodman, 2012-04-15 23:24:10]

LGTM

[commit-bot: I haz the power, 2012-04-26 02:22:40]

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

[commit-bot: I haz the power, 2012-04-26 04:03:43]

Change committed as 134062

[Devlin, 2012-05-05 18:07:33]

On 2012/04/26 04:03:43, I haz the power (commit-bot) wrote:
> Change committed as 134062

Change was reverted since it broke sync integration tests (dang - didn't realize
these weren't done by the trybots).

The problem is that both the garbage collection and the installation of
extensions happen on the UI and FILE threads, which can cause a race condition. 
For instance:

1) Garbage collection looks for directories on the file system (FILE)
2) An extension is in the stages of install
3) The new extension is registered in the prefs
4) Garbage collection move to UI thread, checks the prefs, and sees an extension
seemingly without a corresponding file path (since the paths were gathered
before the extension was installed).
5) Garbage collection deletes the extension from the prefs.

I've verified this is the problem as best I can by tracing most of the flow of
control, and also by simply making garbage collection single-threaded and
watching it pass every time.  Of course, this is not a real solution.  Any
suggestions for how we can avoid this race condition?

[Aaron Boodman, 2012-05-07 22:10:31]

On Sat, May 5, 2012 at 11:07 AM,  <rdevlin.cronin@chromium.org> wrote:
> On 2012/04/26 04:03:43, I haz the power (commit-bot) wrote:
>>
>> Change committed as 134062
>
>
> Change was reverted since it broke sync integration tests (dang - didn't
> realize
> these weren't done by the trybots).
>
> The problem is that both the garbage collection and the installation of
> extensions happen on the UI and FILE threads, which can cause a race
> condition.
> For instance:
>
> 1) Garbage collection looks for directories on the file system (FILE)
> 2) An extension is in the stages of install
> 3) The new extension is registered in the prefs
> 4) Garbage collection move to UI thread, checks the prefs, and sees an
> extension
> seemingly without a corresponding file path (since the paths were gathered
> before the extension was installed).
> 5) Garbage collection deletes the extension from the prefs.

My understanding was that installation is always:

1) Write extension to disk on file thread
2) Write extension to preferences on UI thread

And that (new) GC is:

a) Read current extensions from disk on file thread
b) Read current extensions from preferences on UI thread
c) Delete any orphaned preferences on UI thread
d) Delete any orphaned directories on file thread

It sounds like you are saying the order [a, 1, 2, b, c, d] can occur.
But that doesn't make sense to me since both the file and UI thread
are serviced by shared event queues. It seems to me like the order
will always be either [a, b, ...] or [1, 2, ...]. What am I
misunderstanding? Perhaps in the case of sync, the order for
installation is not [1,2]?

> Of course, this is not a real solution.  Any
> suggestions for how we can avoid this race condition?

Threads are hard. The typical sane solution is to avoid them. To apply
that here, my idea has been to introduce the concept of an
ExtensionOperationQueue, which would be a queue of Task. Examples of
Tasks would be Install, Uninstall, CollectGarbage. A task would
encapsulate the file and UI thread bits. Only one task would operate
at a time.

That is a pretty big refactor though, so it would be good if we could
puzzle out what's going on with the sync tests.

- a