Migrating to Profile Keyed Services

[Rachel Weinstein Petterson]

Hi, all,

If you own and/or contribute to any objects or services that are accessed via profile_->GetBlah(), please read!

As part of the Profiles effort, we need to cut down on the bloat which has occurred in the Profile class. Ideally the Profile class should be minimal rather than owning everything. The purpose is two fold:

1) Profile shutdown has too many dependencies to be done as needed without crashes or race conditions

2) Features need to be able to be compiled in or out for variants of Chrome (not to mention making it easier to add new features in the future)

In order to fix these issues, we need to move ownership of services away from the Profile and convert them to Profile Keyed Services. A Profile Keyed Service uses a ProfileKeyedServiceFactory to take care of most of the work of linking to the profile and hooking the service in to a dependency manager which handles the shutdown dependency issues for you.

We'd like to accomplish this by the end of Q1. Although the Profiles team is working diligently to convert as many of these over as possible, it's more than we can handle so we're asking for your help on any services you work on. Over the next few days, you may see a bug assigned to you for moving your service to a Profile Keyed Service. Please don't ignore it! Elliot put together a great instructive design doc with step-by-step instructions on how to convert a service to a Profile Keyed Service https://sites.google.com/a/chromium.org/dev/developers/design-documents/profile-architecture, including links to sample patches.

You're not alone! If you need assistance with converting your service, Miranda (mirandac@), Elliot (erg@) and I (rlp@) are here to help. Just ping us and we'll be happy to get you started and answer any questions along the way.

These problems will only get worse if more features are added to Chrome that need to hook into the Profile. In order to prevent that from happening, Elliot added a COMPILE_ASSERT to ensure that the size of Profile only decreases. This means you can no longer add more scoped_refptrs_ and corresponding GetBlah() functions to the Profile class.

Thanks in advance for your help,

Rachel

[Peter Kasting]

Just FYI, as someone who's busy converting a service to be profile-keyed, it's been really easy and there are lots of nice builtins like boolean oracle functions you can override to choose different behaviors like "my service is NULL in tests" or "I need to be started during startup rather than lazily".  So don't dread this too much.

PK

[Marja Hölttä]

Hi,

When using ProfileKeyedServiceFactory, I have had trouble trying to
make the Profile only hold a shared pointer to my service.
ProfileKeyedServiceFactory doesn't seem to like shared pointer as it
just deletes the service in
ProfileKeyedServiceFactory::ProfileDestroyed with delete.

To work around this, I've ended up creating a wrapper FooWrapper which
holds a shared pointer to my service Foo, and making FooWrapper a
ProfileKeyedService.

E.g.,
chrome/browser/content_settings/cookie_settings.h
contains such a wrapper (at the bottom).

This is clumsy. Am I doing it wrong? Or should
ProfileKeyedServiceFactory be adapted to allow this kind of usage,
too?

BR,
Marja

[Bernhard Bauer]

FWIW, I did that as well for PluginPrefs, and I didn't find it clumsy ;-)

Bernhard.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Torne (Richard Coles)]

On 12 January 2012 16:29, Marja Hölttä <ma...@chromium.org> wrote:
> When using ProfileKeyedServiceFactory, I have had trouble trying to
> make the Profile only hold a shared pointer to my service.
> ProfileKeyedServiceFactory doesn't seem to like shared pointer as it
> just deletes the service in
> ProfileKeyedServiceFactory::ProfileDestroyed with delete.

That's because generally this isn't safe :)

> To work around this, I've ended up creating a wrapper FooWrapper which
> holds a shared pointer to my service Foo, and making FooWrapper a
> ProfileKeyedService.
>
> E.g.,
> chrome/browser/content_settings/cookie_settings.h
> contains such a wrapper (at the bottom).
>
> This is clumsy. Am I doing it wrong? Or should
> ProfileKeyedServiceFactory be adapted to allow this kind of usage,
> too?

This looks unsafe to me (though I may be wrong) - your service has a
pointer to the profile's PrefService, no? This pointer will become
dangling once the profile is destroyed.

In other cases where profile services have been refcounted, we have
found that this behaviour was already unsafe (could theoretically
result in accessing a deleted Profile) and have reworked the services
not to be reference counted instead.

--
Torne (Richard Coles)
to...@google.com

[Elliot Glaysher (Chromium)]

On Thu, Jan 12, 2012 at 11:46 AM, Torne (Richard Coles)
<to...@google.com> wrote:
>> This is clumsy. Am I doing it wrong? Or should
>> ProfileKeyedServiceFactory be adapted to allow this kind of usage,
>> too?
>
> This looks unsafe to me (though I may be wrong) - your service has a
> pointer to the profile's PrefService, no? This pointer will become
> dangling once the profile is destroyed.
>
> In other cases where profile services have been refcounted, we have
> found that this behaviour was already unsafe (could theoretically
> result in accessing a deleted Profile) and have reworked the services
> not to be reference counted instead.

Agreed. In general, trying to share ownership of something tied to the
Profile has historically been error prone. We've had numerous shutdown
crashes because an object has tried to access a deleted
Profile/PrefService/Other thing tied to Profile that's already been
shut down.

After an admittedly quick skim of cookie_settings.{cc,h}, I can't
convince myself that what's going on here is safe.

-- Elliot

[Bernhard Bauer]

CookieSettingsWrapper::Shutdown calls
CookieSettings::ShutdownOnUIThread, which drops all references to the
Profile. CookieSettings in general can be accessed from any thread, so
it doesn't even directly use the Profile, it just listens to pref
change notifications.

Bernhard.

> -- Elliot

[Elliot Glaysher (Chromium)]

On Thu, Jan 12, 2012 at 1:01 PM, Bernhard Bauer <bau...@google.com> wrote:
> CookieSettingsWrapper::Shutdown calls
> CookieSettings::ShutdownOnUIThread, which drops all references to the
> Profile. CookieSettings in general can be accessed from any thread, so
> it doesn't even directly use the Profile, it just listens to pref
> change notifications.

I'd be more confident of this if PrefChangeRegistrar::RemoveAll (or
probably a new method for this task) also NULLed out the PrefService*
pointer. (I'm being pedantic now; I'm just a bit worried about the
possibility of re Add()ing new preferences.)

-- Elliot

[Drew Wilson]

Speaking of preferences, it looks like when using TestingProfile, ProfileDependencyManager doesn't call RegisterUserPrefs() on the various services, leaving us with a profile without all of our prefs registered.

Is it expected that users of TestingProfile have to figure out all services/prefs that are used by code invoked in their test, and now manually call RegisterUserPrefs() on the related service factories? That seems somewhat fragile/difficult to maintain - can we revisit this decision? Especially since this policy seems to be applied unevenly - RegisterUserPrefs() *is* invoked if SetTestingFactory() is used to provide a custom factory...

-atw

-- Elliot

[Miranda Callahan]

(once more, with feeling)

On Fri, Jan 13, 2012 at 10:44, Miranda Callahan <mira...@google.com> wrote:

> On Fri, Jan 13, 2012 at 01:18, Drew Wilson <atwi...@chromium.org> wrote:
>> Speaking of preferences, it looks like when using TestingProfile,
>> ProfileDependencyManager doesn't call RegisterUserPrefs() on the various
>> services, leaving us with a profile without all of our prefs registered.

I think that this decision was made mainly to allow the TestingProfile
to insert its own PrefService.

Maybe instead of just outright not registering user prefs for any
testing profile, we can pass an argument when creating the
TestingProfile to determine whether or not it's going to be using a
separate PrefService? Then we can just ask the TestingProfile whether
it wants to handle its own user prefs registration or not in the
ProfileDependencyManager.

[Drew Wilson]

On Fri, Jan 13, 2012 at 7:45 AM, Miranda Callahan <mira...@chromium.org> wrote:

(once more, with feeling)

On Fri, Jan 13, 2012 at 10:44, Miranda Callahan <mira...@google.com> wrote:
> On Fri, Jan 13, 2012 at 01:18, Drew Wilson <atwi...@chromium.org> wrote:
>> Speaking of preferences, it looks like when using TestingProfile,
>> ProfileDependencyManager doesn't call RegisterUserPrefs() on the various
>> services, leaving us with a profile without all of our prefs registered.

I think that this decision was made mainly to allow the TestingProfile
to insert its own PrefService.

From what I can tell from my work on this last night, it seems like this is the rarer case, and the common case just uses the built-in PrefService that TestingProfile provides.

 

Maybe instead of just outright not registering user prefs for any
testing profile, we can pass an argument when creating the
TestingProfile to determine whether or not it's going to be using a
separate PrefService? Then we can just ask the TestingProfile whether
it wants to handle its own user prefs registration or not in the
ProfileDependencyManager.

I think it's not bad to just do the RegisterUserPrefs always - if we then throw out that PrefService because we're swapping in a new one, then the caller can just register them again with the new PrefService.

After talking with Miranda about this, I'm going to move forward with a patch that registers prefs from TestingProfile also - I did this last night and it seems to be working (mostly :). I'll be sure to include both Miranda and Elliot on the review.

[Elliot Glaysher (Chromium)]

On Fri, Jan 13, 2012 at 9:36 AM, Drew Wilson <atwi...@chromium.org> wrote:
>
>
> On Fri, Jan 13, 2012 at 7:45 AM, Miranda Callahan <mira...@chromium.org>
> wrote:
>>
>> (once more, with feeling)
>>
>> On Fri, Jan 13, 2012 at 10:44, Miranda Callahan <mira...@google.com>
>> wrote:
>> > On Fri, Jan 13, 2012 at 01:18, Drew Wilson <atwi...@chromium.org>
>> > wrote:
>> >> Speaking of preferences, it looks like when using TestingProfile,
>> >> ProfileDependencyManager doesn't call RegisterUserPrefs() on the
>> >> various
>> >> services, leaving us with a profile without all of our prefs
>> >> registered.
>>
>> I think that this decision was made mainly to allow the TestingProfile
>> to insert its own PrefService.
>
> From what I can tell from my work on this last night, it seems like this is
> the rarer case, and the common case just uses the built-in PrefService that
> TestingProfile provides.

IIRC, I thought the opposite, though I guess it's likely that the set
of services that I was converting weren't representative of chrome as
a whole.

> I think it's not bad to just do the RegisterUserPrefs always - if we then
> throw out that PrefService because we're swapping in a new one, then the
> caller can just register them again with the new PrefService.
>
> After talking with Miranda about this, I'm going to move forward with a
> patch that registers prefs from TestingProfile also - I did this last night
> and it seems to be working (mostly :). I'll be sure to include both Miranda
> and Elliot on the review.

My memory is fuzzy, but I think I put all my knowledge about what I
used to know about this in the comment in
ProfileKeyedServiceFactory::RegisterUserPrefsOnProfile(). Good luck
doing this.

-- Elliot