ProcessManagerFactory: fix dependencies in ApiResourceManager, LazyBgndTaskFactory

ProcessManagerFactory: fix dependencies in ApiResourceManager, LazyBgndTaskFactory

ProcessManagerFactory: add dependency on LazyBgndTaskFactory.
The absence of a factory is causing some crashes on shutdown
(see bug).

Add service dependencies from ApiResourceManager to
ExtensionRegistryFactory and ProcessManagerFactory.
ApiResourceManager is holding scoped observer objects on
those services. The observed objects must be alive when
the observers are unregistered, otherwise a use after free
condition may occur.

Add template struct-based dependency registration system,
necessary for supporting type inference using partial
specialization.


R=benwells@chromium.org
BUG=716348
Review-Url: https://codereview.chromium.org/2863213004
Cr-Commit-Position: refs/heads/master@{#472495}
Committed: https://chromium.googlesource.com/chromium/src/+/994885e87e81f4d513a81ff2730cb5a2a240793b

[benwells, 2017-05-06 05:01:57]

lgtm

[Kevin M, 2017-05-08 17:07:07]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-05-08 17:07:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 18:49:49]

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

[commit-bot: I haz the power, 2017-05-08 18:49:50]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin M, 2017-05-09 18:33:38]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-05-09 18:34:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 21:42:05]

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

[commit-bot: I haz the power, 2017-05-09 21:42:05]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin M, 2017-05-09 23:08:39]

Description was changed from

==========
ProcessManagerFactory: add dependency on LazyBgndTaskFactory

ProcessManagerFactory: add dependency on LazyBgndTaskFactory.
The absence of a factory is causing some crashes on shutdown
(see bug).

R=benwells@chromium.org
BUG=716348
==========

to

==========
ProcessManagerFactory: fix dependencies in ApiResourceManager,
LazyBgndTaskFactory

ProcessManagerFactory: add dependency on LazyBgndTaskFactory.
The absence of a factory is causing some crashes on shutdown
(see bug).

Add service dependencies from ApiResourceManager to
ExtensionRegistryFactory and ProcessManagerFactory.
ApiResourceManager is holding scoped observer objects on
those services. The observed objects must be alive when
the observers are unregistered, otherwise a use after free
condition may occur.


R=benwells@chromium.org
BUG=716348
==========

[Kevin M, 2017-05-09 23:10:04]

benwells@, please take another look. asan detected a deletion ordering issue in
ApiResourceManager relating to its observers. I addressed the issue by declaring
the appropriate service dependencies in the factory template.

[Kevin M, 2017-05-10 00:47:48]

The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 00:49:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 02:52:05]

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

[commit-bot: I haz the power, 2017-05-10 02:52:06]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[benwells, 2017-05-10 16:00:23]

If this is just for the ApiResourceManager, can you add these dependencies just
for that template of the BrowserContextApiFactory?

See
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/history/hi...
for an example.

https://codereview.chromium.org/2863213004/diff/20001/extensions/browser/proc...
File extensions/browser/process_manager_factory.cc (right):

https://codereview.chromium.org/2863213004/diff/20001/extensions/browser/proc...
extensions/browser/process_manager_factory.cc:5: #include
"extensions/browser/process_manager_factory.h"
Nit: blank link after this include.

[Kevin M, 2017-05-10 17:19:36]

Yup, just for the ApiResourceManager. It has some implicit dependencies that
need to be made explicit, thanks to these observers:

ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
extension_registry_observer_;

ScopedObserver<ProcessManager, ProcessManagerObserver>
process_manager_observer_;


My CL introduced dependencies which altered the teardown graph in such a way
that asan was able to detect the error as a use after free condition.

I wonder if the dependency manager could take a test parameter that could
shuffle the graph to produce alternate deletion orderings, to catch things like
this?

https://codereview.chromium.org/2863213004/diff/20001/extensions/browser/proc...
File extensions/browser/process_manager_factory.cc (right):

https://codereview.chromium.org/2863213004/diff/20001/extensions/browser/proc...
extensions/browser/process_manager_factory.cc:5: #include
"extensions/browser/process_manager_factory.h"
On 2017/05/10 16:00:23, benwells wrote:
> Nit: blank link after this include.

Done.

[Kevin M, 2017-05-10 23:30:08]

Don't quite get your suggestion, though - I already had uploaded a patch which
added the dependencies at the BrowserContextApiFactory template level?

[benwells, 2017-05-11 06:53:25]

On 2017/05/10 23:30:08, Kevin M wrote:
> Don't quite get your suggestion, though - I already had uploaded a patch which
> added the dependencies at the BrowserContextApiFactory template level?

Sorry I wasn't clear enough. You've added it to the template definition, which
means it will be applied to all specializations of the template. If you add it
to an explicit template specialization, it will only affect the one
specialization it needs to. (not sure if I'm using those terms completely
correctly :)

To do this you write something like:
template <>
void
BrowserContextKeyedAPIFactory<ApiResourceManager>::DeclareFactoryDependencies()
{
  // dependencies here.
}

and put it in api_resource_manager.cc or wherever that class lives.

See this for an actual example:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/history/hi...

Maybe that won't work for some reason, but if it does it looks like a better
approach.

[Kevin M, 2017-05-11 20:38:13]

kmarshall@chromium.org changed reviewers:
+ dcheng@chromium.org, rdevlin.cronin@chromium.org
- benwells@chromium.org

[Kevin M, 2017-05-11 20:38:14]

+devlin

Hey Devlin,

The explicit specialization pattern recommended in
browser_context_keyed_api_factory.h doesn't seem to work for templated service
types like ApiResourceManager<T>. 

For code like this:

template <typename T>
template <>
void
BrowserContextKeyedAPIFactory<ApiResourceManager<T>>::DeclareFactoryDependencies()
{}

I get this error:

../../extensions/browser/api/api_resource_manager.h:370:60: error: nested name
specifier 'BrowserContextKeyedAPIFactory<ApiResourceManager<T> >::' for
declaration does not refer into a class, class template or class template
partial specialization
void
BrowserContextKeyedAPIFactory<ApiResourceManager<T>>::DeclareFactoryDependencies()
{
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^


I've tried a variety of alternative approaches but nothing builds. I think the
issue has to do with the fact that this is an example of partial specialization,
and methods don't support partial specialization.

Do you have any suggestions?

[Devlin, 2017-05-12 19:26:58]

On 2017/05/11 20:38:14, Kevin M wrote:
> +devlin
> 
> Hey Devlin,
> 
> The explicit specialization pattern recommended in
> browser_context_keyed_api_factory.h doesn't seem to work for templated service
> types like ApiResourceManager<T>. 
> 
> For code like this:
> 
> template <typename T>
> template <>
> void
>
BrowserContextKeyedAPIFactory<ApiResourceManager<T>>::DeclareFactoryDependencies()
> {}
> 
> I get this error:
> 
> ../../extensions/browser/api/api_resource_manager.h:370:60: error: nested name
> specifier 'BrowserContextKeyedAPIFactory<ApiResourceManager<T> >::' for
> declaration does not refer into a class, class template or class template
> partial specialization
> void
>
BrowserContextKeyedAPIFactory<ApiResourceManager<T>>::DeclareFactoryDependencies()
> {
>      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
> 
> 
> I've tried a variety of alternative approaches but nothing builds. I think the
> issue has to do with the fact that this is an example of partial
specialization,
> and methods don't support partial specialization.
> 
> Do you have any suggestions?

Fun!

C++ doesn't allow partial specialization of functions, but it does allow partial
specialization of classes/structs.  One workaround would be something like this:

// browser_context_keyed_api_factory.h

// Base:
template<typename T>
struct BrowserContextFactoryDependencies {
  void AddFactoryDependencies(BrowserContextKeyedAPIFactory<T>* factory) {
   
factory->DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
  }
}

template<typename T>
class BrowserContextKeyedAPIFactory : ... {
 ...

  void DeclareFactoryDependencies() {
    BrowserContextFactoryDependencies<T>::AddFactoryDependencies(this);
  }

 private:
  friend struct BrowserContextFactoryDependencies;  // DependsOn() is protected.
};

// api_resource_manager.h

template<class T>
struct BrowserContextFactoryDependencies<ApiResourceManager<T>> {
  static void
AddFactoryDependencies(BrowserContextKeyedAPIFactory<ApiResourceManager<T>>*
factory) {
   
factory->DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
    factory->DependsOn(<other stuff>);
  }
};


I *think* this would work.  It's a little weird, since now there's two ways to
specialize dependencies - by fully specializing
BrowserContextKeyedAPIFactory::DeclareFactoryDependencies (e.g.,
BrowserContextKeyedAPIFactory<SomeClass>::DeclareFactoryDependencies() { ... })
and by specializing the helper struct.  Ideally, we'd probably limit that to
one.

Can you check that, and see if it works?

[Kevin M, 2017-05-12 21:54:44]

Yep, that worked. Smart!!

I defined it as a parallel path to the existing factory declaration method, so
that the fix CL doesn't get bogged down with a massive, multi-OWNER code cleanup
effort.

I can file a bug for the migration tasks.

[Kevin M, 2017-05-12 21:56:53]

Description was changed from

==========
ProcessManagerFactory: fix dependencies in ApiResourceManager,
LazyBgndTaskFactory

ProcessManagerFactory: add dependency on LazyBgndTaskFactory.
The absence of a factory is causing some crashes on shutdown
(see bug).

Add service dependencies from ApiResourceManager to
ExtensionRegistryFactory and ProcessManagerFactory.
ApiResourceManager is holding scoped observer objects on
those services. The observed objects must be alive when
the observers are unregistered, otherwise a use after free
condition may occur.


R=benwells@chromium.org
BUG=716348
==========

to

==========
ProcessManagerFactory: fix dependencies in ApiResourceManager,
LazyBgndTaskFactory

ProcessManagerFactory: add dependency on LazyBgndTaskFactory.
The absence of a factory is causing some crashes on shutdown
(see bug).

Add service dependencies from ApiResourceManager to
ExtensionRegistryFactory and ProcessManagerFactory.
ApiResourceManager is holding scoped observer objects on
those services. The observed objects must be alive when
the observers are unregistered, otherwise a use after free
condition may occur.

Add template struct-based dependency registration system,
necessary for supporting type inference using partial
specialization.


R=benwells@chromium.org
BUG=716348
==========

[Kevin M, 2017-05-12 21:56:58]

The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 21:58:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-05-12 22:54:00]

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/api/...
File extensions/browser/api/api_resource_manager.h (right):

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/api/...
extensions/browser/api/api_resource_manager.h:373:
BrowserContextKeyedAPIFactory<ApiResourceManager<T>>* factory) {
Have you done an audit to ensure that all ApiResourceManager implementations
don't implement BrowserContextKeyedAPIFactory::DeclareFactoryDependencies? 
(Otherwise, this wouldn't be called)

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/brow...
File extensions/browser/browser_context_keyed_api_factory.h (right):

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/brow...
extensions/browser/browser_context_keyed_api_factory.h:120:
BrowserContextFactoryDependencies<T>::DeclareFactoryDependencies(this);
Hmm... wouldn't doing this result in ExtensionSystemFactory getting added
multiple times for all current overrides?  Once in DeclareFactoryDepenencies
specialization, and once in the BrowserContextFactoryDependencies helper?

[commit-bot: I haz the power, 2017-05-12 23:18:03]

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

[commit-bot: I haz the power, 2017-05-12 23:18:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin M, 2017-05-15 18:09:04]

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/api/...
File extensions/browser/api/api_resource_manager.h (right):

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/api/...
extensions/browser/api/api_resource_manager.h:373:
BrowserContextKeyedAPIFactory<ApiResourceManager<T>>* factory) {
On 2017/05/12 22:53:59, Devlin (catching up) wrote:
> Have you done an audit to ensure that all ApiResourceManager implementations
> don't implement BrowserContextKeyedAPIFactory::DeclareFactoryDependencies? 
> (Otherwise, this wouldn't be called)

Yes I have. All ApiResourceManagers are produced by
BrowserContextKeyedAPIFactory singletons, or are directly instantiated, as in
the case of webcam_private.

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/brow...
File extensions/browser/browser_context_keyed_api_factory.h (right):

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/brow...
extensions/browser/browser_context_keyed_api_factory.h:120:
BrowserContextFactoryDependencies<T>::DeclareFactoryDependencies(this);
On 2017/05/12 22:53:59, Devlin (catching up) wrote:
> Hmm... wouldn't doing this result in ExtensionSystemFactory getting added
> multiple times for all current overrides?  Once in DeclareFactoryDepenencies
> specialization, and once in the BrowserContextFactoryDependencies helper?

Hm, yes. Thanks. How's this for backwards compatibility?

A) If subclasses implement DeclareFactoryDependencies: only
DeclareFactoryDependencies() is called.

B) If subclasses implement BrowserContextFactoryDependencies<>: only
BrowserContextFactoryDependencies<> is called.

C) If subclasses implement A and B: only DeclareFactoryDependencies() is called.

[Devlin, 2017-05-15 18:24:52]

Patch Set naming nit: Dana's not on this CL. :)

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/brow...
File extensions/browser/browser_context_keyed_api_factory.h (right):

https://codereview.chromium.org/2863213004/diff/60001/extensions/browser/brow...
extensions/browser/browser_context_keyed_api_factory.h:120:
BrowserContextFactoryDependencies<T>::DeclareFactoryDependencies(this);
On 2017/05/15 18:09:04, Kevin M wrote:
> On 2017/05/12 22:53:59, Devlin (catching up) wrote:
> > Hmm... wouldn't doing this result in ExtensionSystemFactory getting added
> > multiple times for all current overrides?  Once in DeclareFactoryDepenencies
> > specialization, and once in the BrowserContextFactoryDependencies helper?
> 
> Hm, yes. Thanks. How's this for backwards compatibility?
> 
> A) If subclasses implement DeclareFactoryDependencies: only
> DeclareFactoryDependencies() is called.
> 
> B) If subclasses implement BrowserContextFactoryDependencies<>: only
> BrowserContextFactoryDependencies<> is called.
> 
> C) If subclasses implement A and B: only DeclareFactoryDependencies() is
called.

SGTM; that should work!

https://codereview.chromium.org/2863213004/diff/80001/extensions/browser/brow...
File extensions/browser/browser_context_keyed_api_factory.h (right):

https://codereview.chromium.org/2863213004/diff/80001/extensions/browser/brow...
extensions/browser/browser_context_keyed_api_factory.h:74: // be placed in your
.cc file.
For background, maybe we should include a comment about why we took this
approach rather than just allowing the override of DeclareFactoryDependencies? 
(Specifically, this allows definitions for partial specializations, such as
those in ApiResourceManager<T>)

[Devlin, 2017-05-15 18:26:53]

On 2017/05/15 18:24:52, Devlin (catching up) wrote:
> Patch Set naming nit: Dana's not on this CL. :)

I swear PS4 and PS5 said 'danakj feedback'... either I'm crazy (totally
possible!), or it was already fixed when I submitted comments.  Either way,
ignore, and sorry for the noise.

[Kevin M, 2017-05-15 18:34:39]

I fixed the patch titles at the same time you were writing that reply. You are
not going crazy. ;)

https://codereview.chromium.org/2863213004/diff/80001/extensions/browser/brow...
File extensions/browser/browser_context_keyed_api_factory.h (right):

https://codereview.chromium.org/2863213004/diff/80001/extensions/browser/brow...
extensions/browser/browser_context_keyed_api_factory.h:74: // be placed in your
.cc file.
On 2017/05/15 18:24:52, Devlin (catching up) wrote:
> For background, maybe we should include a comment about why we took this
> approach rather than just allowing the override of DeclareFactoryDependencies?

> (Specifically, this allows definitions for partial specializations, such as
> those in ApiResourceManager<T>)

Done - added the explanation to the deprecated DeclareBrowserDependencies()
method. IMO once everything's migrated the explanation will become trivia noise.

[Devlin, 2017-05-15 18:37:47]

https://codereview.chromium.org/2863213004/diff/80001/extensions/browser/brow...
File extensions/browser/browser_context_keyed_api_factory.h (right):

https://codereview.chromium.org/2863213004/diff/80001/extensions/browser/brow...
extensions/browser/browser_context_keyed_api_factory.h:74: // be placed in your
.cc file.
On 2017/05/15 18:34:39, Kevin M wrote:
> On 2017/05/15 18:24:52, Devlin (catching up) wrote:
> > For background, maybe we should include a comment about why we took this
> > approach rather than just allowing the override of
DeclareFactoryDependencies?
> 
> > (Specifically, this allows definitions for partial specializations, such as
> > those in ApiResourceManager<T>)
> 
> Done - added the explanation to the deprecated DeclareBrowserDependencies()
> method. IMO once everything's migrated the explanation will become trivia
noise.

I dunno - I think it might help avoid a well-meaning developer trying to
simplify the code down the road (only to realize it won't compile). :)  For the
one line of comment code it adds, I'd leave it in.

[Kevin M, 2017-05-15 18:43:01]

https://codereview.chromium.org/2863213004/diff/80001/extensions/browser/brow...
File extensions/browser/browser_context_keyed_api_factory.h (right):

https://codereview.chromium.org/2863213004/diff/80001/extensions/browser/brow...
extensions/browser/browser_context_keyed_api_factory.h:74: // be placed in your
.cc file.
On 2017/05/15 18:37:47, Devlin (catching up) wrote:
> On 2017/05/15 18:34:39, Kevin M wrote:
> > On 2017/05/15 18:24:52, Devlin (catching up) wrote:
> > > For background, maybe we should include a comment about why we took this
> > > approach rather than just allowing the override of
> DeclareFactoryDependencies?
> > 
> > > (Specifically, this allows definitions for partial specializations, such
as
> > > those in ApiResourceManager<T>)
> > 
> > Done - added the explanation to the deprecated DeclareBrowserDependencies()
> > method. IMO once everything's migrated the explanation will become trivia
> noise.
> 
> I dunno - I think it might help avoid a well-meaning developer trying to
> simplify the code down the road (only to realize it won't compile). :)  For
the
> one line of comment code it adds, I'd leave it in.

Done.

[Devlin, 2017-05-15 20:24:27]

LGTM!

[Kevin M, 2017-05-15 20:27:18]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2017-05-15 20:27:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2863213004/#ps120001
(title: "more explanation")

[commit-bot: I haz the power, 2017-05-15 20:28:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 21:15:17]

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

[commit-bot: I haz the power, 2017-05-15 21:15:18]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[benwells, 2017-05-16 06:42:38]

On 2017/05/15 21:15:18, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_clang on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

Sorry to go silent on this one, I was travelling and took a few days off. Thanks
Devlin for getting it sorted out, the solution looks great.

[Kevin M, 2017-05-16 18:24:57]

I can't reproduce the compile error on my Windows machine, so perhaps it's a
transient Goma issue. Retrying CQ.

GOMA:clang-cl
../../chrome/browser/extensions/browser_context_keyed_service_factories.cc:*ERROR*:
compiler_proxy:1764ms: reached max number of active fail fallbacks

[Kevin M, 2017-05-16 18:25:05]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-05-16 18:26:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 21:41:16]

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

[commit-bot: I haz the power, 2017-05-16 21:41:17]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Kevin M, 2017-05-17 16:52:59]

kmarshall@chromium.org changed reviewers:
+ benwells@chromium.org
- dcheng@chromium.org

[Kevin M, 2017-05-17 16:53:10]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-05-17 16:54:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 18:10:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495039990952390,
"parent_rev": "9cf3fd03d8f4d885b6e1c88cf3ae6ecc8d5542b0", "commit_rev":
"994885e87e81f4d513a81ff2730cb5a2a240793b"}

[commit-bot: I haz the power, 2017-05-17 18:10:24]

Description was changed from

==========
ProcessManagerFactory: fix dependencies in ApiResourceManager,
LazyBgndTaskFactory

ProcessManagerFactory: add dependency on LazyBgndTaskFactory.
The absence of a factory is causing some crashes on shutdown
(see bug).

Add service dependencies from ApiResourceManager to
ExtensionRegistryFactory and ProcessManagerFactory.
ApiResourceManager is holding scoped observer objects on
those services. The observed objects must be alive when
the observers are unregistered, otherwise a use after free
condition may occur.

Add template struct-based dependency registration system,
necessary for supporting type inference using partial
specialization.


R=benwells@chromium.org
BUG=716348
==========

to

==========
ProcessManagerFactory: fix dependencies in ApiResourceManager,
LazyBgndTaskFactory

ProcessManagerFactory: add dependency on LazyBgndTaskFactory.
The absence of a factory is causing some crashes on shutdown
(see bug).

Add service dependencies from ApiResourceManager to
ExtensionRegistryFactory and ProcessManagerFactory.
ApiResourceManager is holding scoped observer objects on
those services. The observed objects must be alive when
the observers are unregistered, otherwise a use after free
condition may occur.

Add template struct-based dependency registration system,
necessary for supporting type inference using partial
specialization.


R=benwells@chromium.org
BUG=716348

Review-Url: https://codereview.chromium.org/2863213004
Cr-Commit-Position: refs/heads/master@{#472495}
Committed:
https://chromium.googlesource.com/chromium/src/+/994885e87e81f4d513a81ff2730c...
==========

[commit-bot: I haz the power, 2017-05-17 18:10:26]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/994885e87e81f4d513a81ff2730c...