Fix app_shell shutdown crash due to BrowserContextKeyedServices

Fix app_shell shutdown crash due to BrowserContextKeyedServices

* Create ExtensionSystem and register its dependencies from the ExtensionsBrowserClient interface.
* Create and tear down BrowserContextKeyedServices using the dependency manager like you're supposed to do it.
* Don't register AppLoaderServiceFactory - we don't use it anymore.
* Remove the "custom instance" code to ExtensionSystem::Get() - it was kind of hacky.

BUG=none
TEST=Launch app_shell then close the window. No crash.

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

[James Cook, 2013-12-21 00:10:45]

yoz, PTAL

[Yoyo Zhou, 2013-12-21 00:49:05]

This approach feels too hacky to me. It seems like we're solving the BCKS
dependency problem by making things not BCKS anymore.

I think I'd prefer if we #ifdefed out the offending BCKS dependencies for the
appshell build configuration. Of course, it would be ideal to figure out whether
we can break the dependencies.

[James Cook, 2013-12-21 05:05:21]

On 2013/12/21 00:49:05, Yoyo Zhou wrote:
> This approach feels too hacky to me. It seems like we're solving the BCKS
> dependency problem by making things not BCKS anymore.
> 
> I think I'd prefer if we #ifdefed out the offending BCKS dependencies for the
> appshell build configuration. Of course, it would be ideal to figure out
whether
> we can break the dependencies.

Yeah, I'll take another look. I'm a bit stuck because I need to use the
ExtensionSystem subclass from apps/shell, and ExtensionSystemFactory can't
create it. Also, I'm trying to avoid auditing the 750+ calls to
ExtensionSystem::Get() -- I really need that call to return my subclass. And, of
course, my ExtensionSystem isn't dependent on ExtensionSystemShared and its
dependencies.

I don't think you can add a build-time macro for app_shell that can be used
inside a Chrome .cc file. Doing so would require all of Chrome to be rebuilt
again for the app_shell target. I could do something tricky in the header file
-- some kind of inline GetDependency call that acts differently when the header
is included from the app_shell target. ExtensionService does this with
UNIT_TEST. But that feels even more hacky.

I could add some kind of static variable that said whether or not to add the
ExtensionSystemShared dependency. That's not pretty, but would be simple and
self-explanatory.

Thoughts?

[Yoyo Zhou, 2014-01-07 06:19:55]

On 2013/12/21 05:05:21, James Cook wrote:
> On 2013/12/21 00:49:05, Yoyo Zhou wrote:
> > This approach feels too hacky to me. It seems like we're solving the BCKS
> > dependency problem by making things not BCKS anymore.
> > 
> > I think I'd prefer if we #ifdefed out the offending BCKS dependencies for
the
> > appshell build configuration. Of course, it would be ideal to figure out
> whether
> > we can break the dependencies.
> 
> Yeah, I'll take another look. I'm a bit stuck because I need to use the
> ExtensionSystem subclass from apps/shell, and ExtensionSystemFactory can't
> create it. Also, I'm trying to avoid auditing the 750+ calls to
> ExtensionSystem::Get() -- I really need that call to return my subclass. And,
of
> course, my ExtensionSystem isn't dependent on ExtensionSystemShared and its
> dependencies.
> 
> I don't think you can add a build-time macro for app_shell that can be used
> inside a Chrome .cc file. Doing so would require all of Chrome to be rebuilt
> again for the app_shell target. I could do something tricky in the header file
> -- some kind of inline GetDependency call that acts differently when the
header
> is included from the app_shell target. ExtensionService does this with
> UNIT_TEST. But that feels even more hacky.
> 
> I could add some kind of static variable that said whether or not to add the
> ExtensionSystemShared dependency. That's not pretty, but would be simple and
> self-explanatory.
> 
> Thoughts?

I wonder if we could put the DependsOn calls in a ChromeExtensionsClient method.
Better to reuse that hack than invent a new one, I hope.

[James Cook, 2014-01-08 00:28:02]

yoz, please take another look

This removes the custom_instance_ hack in favor of moving most of
ExtensionSystem creation to the client interface. I can't just move the
DependsOn() calls as they are protected (and keeping them protected seems
appropriate to me).

[Yoyo Zhou, 2014-01-08 02:29:42]

LGTM

https://chromiumcodereview.appspot.com/118043003/diff/190001/apps/shell/shell...
File apps/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/118043003/diff/190001/apps/shell/shell...
apps/shell/shell_browser_main_parts.cc:41:
extensions::ExtensionPrefsFactory::GetInstance();
Is there some reason this can't be declared as a DependsOn instead?

[James Cook, 2014-01-08 17:37:02]

https://codereview.chromium.org/118043003/diff/190001/apps/shell/shell_browse...
File apps/shell/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/118043003/diff/190001/apps/shell/shell_browse...
apps/shell/shell_browser_main_parts.cc:41:
extensions::ExtensionPrefsFactory::GetInstance();
On 2014/01/08 02:29:42, Yoyo Zhou wrote:
> Is there some reason this can't be declared as a DependsOn instead?

Yeah, it should be done there. Chrome does it in both places, but in this case
DependsOn() is sufficient. Good catch.

[commit-bot: I haz the power, 2014-01-08 17:38:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/118043003/280001

[commit-bot: I haz the power, 2014-01-08 19:30:44]

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

[James Cook, 2014-01-09 01:23:35]

tbarzic, PTAL at chrome/browser/chromeos/*

Deferring the factory instance creation seems to have fixed these tests.

[tbarzic, 2014-01-09 01:58:24]

c/b/cros lgtm

https://codereview.chromium.org/118043003/diff/580001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/drive_integration_service.h (right):

https://codereview.chromium.org/118043003/diff/580001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/drive_integration_service.h:224: // (and all its
dependent factories) until the profile is created.
nit: What do you think about slightly more context here:

These are static so they can be set without instantiating the factory. This
makes it possible to delay factory creation until it normally happens (on
profile creation) rather than when tests are set up.
DriveIntegrationServiceFactory transitively depends on ExtensionSystemFactory
which crashes if created too soon (i.e. before browser process exists).

https://codereview.chromium.org/118043003/diff/580001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(right):

https://codereview.chromium.org/118043003/diff/580001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:300: 
you could also (optionally) do something like:

this->drive_integration_factory_callback_ = 
base::Bind(&DriveFileSystemExtensionApiTest::CreateDriveIntegrationService,
           this);

DriveIntegrationServiceFactory::SetFactoryForTest(&drive_integration_factory_callback_);

That way you'd avoid the static cast bellow.

[James Cook, 2014-01-09 17:32:19]

xiyuan, can I get OWNERS for c/b/chromeos ?

https://codereview.chromium.org/118043003/diff/580001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/drive_integration_service.h (right):

https://codereview.chromium.org/118043003/diff/580001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/drive_integration_service.h:224: // (and all its
dependent factories) until the profile is created.
On 2014/01/09 01:58:25, tbarzic wrote:
> nit: What do you think about slightly more context here:
> 
> These are static so they can be set without instantiating the factory. This
> makes it possible to delay factory creation until it normally happens (on
> profile creation) rather than when tests are set up.
> DriveIntegrationServiceFactory transitively depends on ExtensionSystemFactory
> which crashes if created too soon (i.e. before browser process exists).

That's a much better comment. Done.

https://codereview.chromium.org/118043003/diff/580001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(right):

https://codereview.chromium.org/118043003/diff/580001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:300: 
On 2014/01/09 01:58:25, tbarzic wrote:
> you could also (optionally) do something like:
> 
> this->drive_integration_factory_callback_ = 
> base::Bind(&DriveFileSystemExtensionApiTest::CreateDriveIntegrationService,
>            this);
> 
>
DriveIntegrationServiceFactory::SetFactoryForTest(&drive_integration_factory_callback_);
> 
> That way you'd avoid the static cast bellow.

Nice idea. Done.

[xiyuan, 2014-01-09 17:53:21]

c/b/chromeos LGTM

https://codereview.chromium.org/118043003/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/drive_integration_service.h (right):

https://codereview.chromium.org/118043003/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/drive_integration_service.h:213: static void
SetFactoryForTest(FactoryCallback* factory_for_test);
nit: How about making this private and use a
ScopedDriveIntegrationServiceFactoryForTest class to access it and automatically
clears it on destruction?

[commit-bot: I haz the power, 2014-01-09 18:39:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/118043003/840001

[commit-bot: I haz the power, 2014-01-09 21:04:57]

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

[commit-bot: I haz the power, 2014-01-09 21:35:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/118043003/840001

[commit-bot: I haz the power, 2014-01-09 23:18:37]

Change committed as 244013