Create a ProfileDependencyManager to order ProfileKeyedService destruction.

Create a ProfileDependencyManager to order ProfileKeyedService destruction.

Since ProfileKeyedServices need to be able to specify complex graphs
representing their dependencies on other ProfileKeyedServices, add a simple
::DependsOn() method to explicitly state these dependencies and a singleton
manager in the background that does a topological sort to get a safe
destruction ordering.

This changes ThemeServiceFactory to use this new system, which also revealed a bunch of unsafe casting of Source<> pointers and NotificationCenter misuse between incompatible types. (Source<> uses void* underneath and therefore can't increment/decrement the pointer during casting.)

BUG=77155
TEST=compiles, new ProfileDependencyManagerUnittests.*

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

[Elliot Glaysher, 2011-03-29 00:59:55]

Unit test failures repro locally.

[Miranda Callahan, 2011-03-29 14:06:21]

On 2011/03/29 00:59:55, Elliot Glaysher wrote:
> Unit test failures repro locally.

Should I wait for you to fix unit tests to review?

[Elliot Glaysher, 2011-03-29 17:22:28]

Feel free to review now. The tests look like it'll be a one liner to fix.

On Tue, Mar 29, 2011 at 7:06 AM,  <mirandac@chromium.org> wrote:
> On 2011/03/29 00:59:55, Elliot Glaysher wrote:
>>
>> Unit test failures repro locally.
>
> Should I wait for you to fix unit tests to review?
>
> http://codereview.chromium.org/6766004/
>

[Miranda Callahan, 2011-03-29 17:30:01]

On 2011/03/29 17:22:28, Elliot Glaysher wrote:
> Feel free to review now. The tests look like it'll be a one liner to fix.
> 
> On Tue, Mar 29, 2011 at 7:06 AM,  <mailto:mirandac@chromium.org> wrote:
> > On 2011/03/29 00:59:55, Elliot Glaysher wrote:
> >>
> >> Unit test failures repro locally.
> >
> > Should I wait for you to fix unit tests to review?
> >
> > http://codereview.chromium.org/6766004/
> >

Great, I'll go ahead.

[Miranda Callahan, 2011-03-29 19:07:04]

Hmm -- I've spent the last hour going through this code, drawing pictures of the
interrelationships between all the factories and services, and reading about the
Kahn algorithm on Wikipedia, and I can't find anything at all to complain about
except little nits. I'm going to go walk the dog and think about it some more
and give it one more go-over when I get back, but I suspect it's going to be an
LGTM.

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_dependency_manager.cc:119: const
NotificationDetails& details) {
Should probably make sure that the NotificationType is PROFILE_DESTROYED here.

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_dependency_manager.cc:126:
destruction_order_.begin(); it != destruction_order_.end(); ++it) {
nit: indent

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_dependency_manager.cc:131:
destruction_order_.begin(); it != destruction_order_.end(); ++it) {
nit: indent

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_keyed_service.h (right):

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_keyed_service.h:14: // ever system has been
given a chance to drop references do we start deleting
s/ever/every

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_keyed_service_factory.cc (right):

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_keyed_service_factory.cc:15: #include
"content/common/notification_registrar.h"
alphabetter

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_keyed_service_factory.h:15: // on a one-to-one
mapping. Each factory that dervices from this class *must*
dervices should be herp-derpvices or something more reasonable

[Paweł Hajdan Jr., 2011-03-29 19:10:25]

Drive-by with a tiny testing nit. No need to wait for me, and if you really want
it to be NOTREACHED() I'm fine with that.

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_dependency_manager_unittest.cc (right):

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_dependency_manager_unittest.cc:47: NOTREACHED()
<< "This isn't part of the tests!";
nit: NOTREACHED() will fire only for the Debug builds, and will crash the entire
test binary. Of course if the NULL returned below is dereferenced it will also
crash, but could you just switch to ADD_FAILURE above to consistently apply the
rule "avoid NOTREACHED() in unit tests"?

[Elliot Glaysher, 2011-03-29 20:33:59]

Update. I can reproduce the unit test failures with:

  ./out/Debug/unit_tests
--gtest_filter=ProfileSyncServiceSessionTest.*:ThemeUtilTest.*

but not with:

  ./out/Debug/unit_tests --gtest_filter=ThemeUtilTest.*

[Elliot Glaysher, 2011-03-29 22:18:29]

I've removed the dependency on NotificationSerivce to fix the unit tests (the
NotificationService can change which is what was breaking the unit tests).

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_dependency_manager.cc:119: const
NotificationDetails& details) {
On 2011/03/29 19:07:04, Miranda Callahan wrote:
> Should probably make sure that the NotificationType is PROFILE_DESTROYED here.

No longer applicable.

http://codereview.chromium.org/6766004/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_dependency_manager.cc:126:
destruction_order_.begin(); it != destruction_order_.end(); ++it) {
On 2011/03/29 19:07:04, Miranda Callahan wrote:
> nit: indent

Indentation is correct? Breaking a statement is previous indentation plus 4,
right?

[Elliot Glaysher, 2011-03-30 00:55:48]

Aaaannnd...the mac failures look legitimate and repeatable. :(

[Elliot Glaysher, 2011-03-31 21:17:00]

On 2011/03/30 00:55:48, Elliot Glaysher wrote:
> Aaaannnd...the mac failures look legitimate and repeatable. :(

Miranda, can I get a final LGTM? The trybots have passed the previous mac
failures.

[Miranda Callahan, 2011-03-31 21:18:55]

On 2011/03/31 21:17:00, Elliot Glaysher wrote:
> On 2011/03/30 00:55:48, Elliot Glaysher wrote:
> > Aaaannnd...the mac failures look legitimate and repeatable. :(
> 
> Miranda, can I get a final LGTM? The trybots have passed the previous mac
> failures.

yes indeed -- LGTM!