Remove ExtensionService Garbage-Collecting methods.

Remove ExtensionService Garbage-Collecting methods.

Make a new class, ExtensionGarbageCollector, which performs:
- ExtensionService::GarbageCollectExtensions(),
- extension_file_util::GarbageCollectExtensions() (the file-thread impl of
  ExtensionService::GarbageCollectExtensions()),
- ExtensionService::GarbageCollectIsolatedStorage()

BUG=351891
TBR=rkc@chromium.org (for c/b/chromeos/kiosk_mode - no functional changes, but the TODO has moved from ExtensionService to ExtensionGarbageCollector).

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

[Yoyo Zhou, 2014-03-26 01:54:11]

LGTM with nits

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_garbage_collector.cc (right):

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_garbage_collector.cc:132: if
(disable_garbage_collection_)
This should be initialized to false.

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_garbage_collector.h (right):

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_garbage_collector.h:38: // Call
GarbageCollectExtensions(). Used only in tests.
nit: this comment is redundant. Maybe say "manually trigger a GarbageCollect..."

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_garbage_collector.h:51: // The FILE-thread
implementation of GarbageCollectEtxensions().
typo

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_garbage_collector.h:57: // active (i.e. is
not in ExtensionRegistry::ENABLED or does not have
I'm confused by the "does not have isolated storage" part here.

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_garbage_collector_unittest.cc (right):

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_garbage_collector_unittest.cc:39: // Note:
Was "ExtensionServiceTest.CleanupOnStartup" in a past life.
You don't need these historical footnotes.

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/common/ext...
File chrome/common/extensions/extension_file_util_unittest.cc (left):

https://chromiumcodereview.appspot.com/204983020/diff/40001/chrome/common/ext...
chrome/common/extensions/extension_file_util_unittest.cc:87:
std::multimap<std::string, base::FilePath> extension_paths;
Is this part of the code still tested?

[Devlin, 2014-03-26 23:56:31]

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

https://codereview.chromium.org/204983020/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_garbage_collector.cc:132: if
(disable_garbage_collection_)
On 2014/03/26 01:54:12, Yoyo Zhou wrote:
> This should be initialized to false.

Done.

https://codereview.chromium.org/204983020/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_garbage_collector.h (right):

https://codereview.chromium.org/204983020/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_garbage_collector.h:38: // Call
GarbageCollectExtensions(). Used only in tests.
On 2014/03/26 01:54:12, Yoyo Zhou wrote:
> nit: this comment is redundant. Maybe say "manually trigger a
GarbageCollect..."

Done.

https://codereview.chromium.org/204983020/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_garbage_collector.h:51: // The FILE-thread
implementation of GarbageCollectEtxensions().
On 2014/03/26 01:54:12, Yoyo Zhou wrote:
> typo

Done.

https://codereview.chromium.org/204983020/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_garbage_collector.h:57: // active (i.e. is
not in ExtensionRegistry::ENABLED or does not have
On 2014/03/26 01:54:12, Yoyo Zhou wrote:
> I'm confused by the "does not have isolated storage" part here.

Ah, yes, my parentheses were off.  Presumably, that whole part can just be
removed, because we shouldn't have to garbage collect isolated storage from apps
without isolated storage to begin with.

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

https://codereview.chromium.org/204983020/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_garbage_collector_unittest.cc:39: // Note:
Was "ExtensionServiceTest.CleanupOnStartup" in a past life.
On 2014/03/26 01:54:12, Yoyo Zhou wrote:
> You don't need these historical footnotes.

Done.

https://codereview.chromium.org/204983020/diff/40001/chrome/common/extensions...
File chrome/common/extensions/extension_file_util_unittest.cc (left):

https://codereview.chromium.org/204983020/diff/40001/chrome/common/extensions...
chrome/common/extensions/extension_file_util_unittest.cc:87:
std::multimap<std::string, base::FilePath> extension_paths;
On 2014/03/26 01:54:12, Yoyo Zhou wrote:
> Is this part of the code still tested?

Yes.  This was specifically testing the file-thread portion of the garbage
collection, which is tested in the other unittest.

[Devlin, 2014-03-26 23:57:11]

Jeffrey, wanna take a quick look at one_shot_event changes?  (If not, not needed
as Yoyo already lg'd.)

[Devlin, 2014-03-27 20:36:55]

Removed empty time delta per Jeffrey's (offline) request.

[Devlin, 2014-03-27 20:38:12]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-27 20:40:39]

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

[commit-bot: I haz the power, 2014-03-28 02:29:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 02:29:44]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Devlin, 2014-03-28 03:49:26]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-28 03:50:23]

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

[commit-bot: I haz the power, 2014-03-28 05:52:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 05:52:28]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[Devlin, 2014-03-28 05:55:04]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-28 05:55:53]

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

[commit-bot: I haz the power, 2014-03-28 10:53:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 10:53:20]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[Devlin, 2014-03-28 13:55:11]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-28 13:55:16]

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

[commit-bot: I haz the power, 2014-03-28 18:28:14]

Change committed as 260207

[DaleCurtis, 2014-03-28 20:35:58]

Hi I think this patch is causing browser crashes due to an incorrect threading
story:

Launch the browser and just wait a bit, eventually it crashes with:

[6912:6955:0328/131903:FATAL:weak_ptr.cc(26)] Check failed:
sequence_checker_.CalledOnValidSequencedThread(). WeakPtrs must be checked on
the same sequenced thread.

Commenting out that check then gets you to:

[12009:12077:0328/133048:FATAL:extension_garbage_collector.cc(232)] Check
failed: content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE). 

Reverting the patch fixes the issue.

[DaleCurtis, 2014-03-28 20:40:21]

A revert of this CL has been created in
https://codereview.chromium.org/217233004/ by dalecurtis@chromium.org.

The reason for reverting is: Launch the browser and just wait a bit, eventually
it crashes with:

[6912:6955:0328/131903:FATAL:weak_ptr.cc(26)] Check failed:
sequence_checker_.CalledOnValidSequencedThread(). WeakPtrs must be checked on
the same sequenced thread.

Commenting out that check then gets you to:

[12009:12077:0328/133048:FATAL:extension_garbage_collector.cc(232)] Check
failed: content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE). 

Reverting the patch fixes the issue..