Disable ProfileKeyedServices on import process by default.

Disable ProfileKeyedServices on import process by default.

Added a new ProfileKeyedBaseFactory::ServiceIsNULLOnImportProcess() API which
disables all services by default. Services which must run on the import
process can override this to allow creation.

BUG=174864
Closing this CL since I don't want to land it (gab@ is working on a better CL which delays the creation of the profile on the main browser process until after import is done)

[Andrew T Wilson (Slow), 2013-02-22 16:50:59]

erg: PTAL
pkasting: PTAL at web_data_service.cc (changed DCHECK to CHECK)
tapted: Is running the browser tests sufficient to test this change? I want to
make sure I don't break some part of import by not creating a service that might
be needed. Everything *seems* to work...

[Elliot Glaysher, 2013-02-22 19:01:38]

+rlp to see if she has an opinion on this.

[rpetterson, 2013-02-22 19:21:08]

On 2013/02/22 19:01:38, Elliot Glaysher wrote:
> +rlp to see if she has an opinion on this.

I don't see any issues with this assuming that once the import process is over,
we reload the profile to allow normal services to start up.

[Peter Kasting, 2013-02-22 21:34:55]

https://codereview.chromium.org/12330073/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/12330073/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service.cc:93:
CHECK(!ProfileManager::IsImportProcess(*CommandLine::ForCurrentProcess()));
CHECK means "Chrome has to crash immediately if this assertion fails, because we
are about to have a security exploit"; or "temporarily changed this to CHECK to
try and track down a bug".  Are those really what you mean?  If so, please add
an appropriate comment.  Otherwise, I don't think we should be using CHECK.

Also please see my comment on your last code review here.

[Andrew T Wilson (Slow), 2013-02-22 22:18:35]

Yes, I intend for it to crash the import process, for two reasons:

1) It's better that we crash the import process than cause the browser process
to fail to open the profile.
2) If this problem happens again, we want to have it show up in crash logs, not
linger as it has been.

If you feel strongly that this should be a DCHECK I can undo this change - no
problem. Just let me know. Otherwise I'll just add a comment.

[Peter Kasting, 2013-02-23 00:28:22]

On 2013/02/22 22:18:35, Andrew T Wilson wrote:
> Yes, I intend for it to crash the import process, for two reasons:
> 
> 1) It's better that we crash the import process than cause the browser process
> to fail to open the profile.
> 2) If this problem happens again, we want to have it show up in crash logs,
not
> linger as it has been.

:/  I see where you're coming from.  I just have a hard time feeling like this
falls outside the "don't handle DCHECK failure" rule in our guide.  I mean, the
DCHECK already is supposed to mean that we guaranteed there is no possible way
this can ever happen again.  If that's true, we don't need to "handle" the
potential DCHECK failure (and thus browser misbehavior, etc.).  If we're not
sure if we've actually guaranteed this can't happen -- and thus you want a
period of time to watch the relevant logs -- perhaps this shouldn't have been a
DCHECK at all, but rather a temporary CHECK (for debugging) or some kind of
logging statement?

[Andrew T Wilson (Slow), 2013-02-23 12:21:10]

On 2013/02/23 00:28:22, Peter Kasting wrote:
> On 2013/02/22 22:18:35, Andrew T Wilson wrote:
> > Yes, I intend for it to crash the import process, for two reasons:
> > 
> > 1) It's better that we crash the import process than cause the browser
process
> > to fail to open the profile.
> > 2) If this problem happens again, we want to have it show up in crash logs,
> not
> > linger as it has been.
> 
> :/  I see where you're coming from.  I just have a hard time feeling like this
> falls outside the "don't handle DCHECK failure" rule in our guide.  I mean,
the
> DCHECK already is supposed to mean that we guaranteed there is no possible way
> this can ever happen again.  If that's true, we don't need to "handle" the
> potential DCHECK failure (and thus browser misbehavior, etc.).  If we're not
> sure if we've actually guaranteed this can't happen -- and thus you want a
> period of time to watch the relevant logs -- perhaps this shouldn't have been
a
> DCHECK at all, but rather a temporary CHECK (for debugging) or some kind of
> logging statement?

Here's the problem with making this a DCHECK():

1) This bug *can* happen again. It's as easy as somebody inadvertently
introducing a dependency on WebDataService in some import-process code . I can't
think of a better way to protect against the re-introduction of this bug except
from some form of assertion (CHECK or DCHECK).
2) Our test coverage of import-related code is not great. It needs to be
improved, definitely - see the ongoing thread on this topic. But the end result
is that it's not at all clear to me that automated tests would catch the
re-introduction of this WebDataService dependency. And tragically we seem to be
moving more and more in the direction of using release-mode tests rather than
debug tests anyway.
3) Nobody runs manual tests of the import code using the DEBUG version. So a
DCHECK isn't going to help us catch this problem with our manual testing.

So, I guess I don't understand your point about "handling DCHECK failure",
because I'm not handling DCHECK failure. Instead, I'm turning a debug assertion
into a hard, real-time assertion, because IMO it's the best way to keep this
problem from recurring in the future. And I'd rather have crashes on the canary
version than weird, unreproducible race conditions in our release versions.

I hope I've explained my thinking around this CHECK in some detail. You're the
owner of WebDataService, so it's your call - just let me know what you want me
to do (add a comment? revert my change? whatever you like). I saw your comment
on the previous review and I'll address that in this CL before landing.

[Andrew T Wilson (Slow), 2013-02-23 12:22:16]

On 2013/02/22 19:21:08, rpetterson wrote:
> I don't see any issues with this assuming that once the import process is
over,
> we reload the profile to allow normal services to start up.

We don't have to. The services start up just fine on the main browser process.
We're just preventing them from starting up on the import process, which is OK
because the import process just does some import tasks and exits - we don't need
those services to run on that process.

[Peter Kasting, 2013-02-23 18:59:08]

Really the solution to all this stuff seems like "fix bug 22142".  Until that
happens, I guess I'm not opposed strongly enough to you making this a CHECK to
override you.

LGTM

[tapted, 2013-02-24 22:33:53]

On 2013/02/22 16:50:59, Andrew T Wilson wrote:
> tapted: Is running the browser tests sufficient to test this change? I want to
> make sure I don't break some part of import by not creating a service that
might
> be needed. Everything *seems* to work...

On import process errors, the win (typically debug) bot would tend to print a
stackdump, but still report the test as passing. This is due to
http://crbug.com/174866 . I'll get a patch up for that today. Until then, we can
try inspecting the output of the win bot (although I think stray output from
passing tests is harder to find with the new trybot output).

[rpetterson, 2013-02-25 19:00:40]

On 2013/02/23 12:22:16, Andrew T Wilson wrote:
> On 2013/02/22 19:21:08, rpetterson wrote:
> > I don't see any issues with this assuming that once the import process is
> over,
> > we reload the profile to allow normal services to start up.
> 
> We don't have to. The services start up just fine on the main browser process.
> We're just preventing them from starting up on the import process, which is OK
> because the import process just does some import tasks and exits - we don't
need
> those services to run on that process.

LGTM from me as long as erg@ doesn't have any objections.

[Elliot Glaysher, 2013-02-25 19:06:39]

I really dislike this since we're now baking in details just for the import
process into profile startup. I'm tempted to say "give up and block for import"
like Scott emailed out earlier, and would urge you to at least think about that.

But barring that...I guess this lgtm.

[tapted, 2013-02-26 11:00:35]

Nothing stands out to me btw - lgtm. But you might want to wait for
http://crrev.com/12314093/ to land first (I need to ping cpu@..). This might get
some extra reassurance from trybots. (alternatively patch these CLs in together
somewhere and throw them at trybots).

[gab, 2013-03-07 15:46:57]

See below (+cc macourteau).

Cheers,
Gab

https://codereview.chromium.org/12330073/diff/1011/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/12330073/diff/1011/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:93:
CHECK(!ProfileManager::IsImportProcess(*CommandLine::ForCurrentProcess()));
FWIW, we (me and macourteau) were hitting the DCHECK below when using
import_search_engine in master_preferences (http://crbug.com/180306); we are
investigating the root cause, but making this a CHECK will cause the import
process to crash for everyone using that preference...

[Andrew T Wilson (Slow), 2013-03-07 15:54:14]

I would say that's a good thing - we should fail to import rather than
generating errors starting chrome because the import process is blocking access
to Web Data.

[gab, 2013-03-07 15:57:22]

On 2013/03/07 15:54:14, Andrew T Wilson wrote:
> I would say that's a good thing - we should fail to import rather than
> generating errors starting chrome because the import process is blocking
access
> to Web Data.

Agreed, just thought I'd point out this fact.

[gab, 2013-03-07 15:58:23]

On 2013/03/07 15:57:22, gab wrote:
> On 2013/03/07 15:54:14, Andrew T Wilson wrote:
> > I would say that's a good thing - we should fail to import rather than
> > generating errors starting chrome because the import process is blocking
> access
> > to Web Data.
> 
> Agreed, just thought I'd point out this fact.

Oh also, will the CHECK fire an error message box in a Release build or simply
silently kill the process? The former is not desirable for sure.

[Andrew T Wilson (Slow), 2013-03-07 16:06:08]

Agreed, and I don't know the behavior since I don't have a release build built.
I can try making one before landing.

[macourteau, 2013-03-07 16:36:37]

FWIW, the issue related to the DCHECK being hit is this one:
https://code.google.com/p/chromium/issues/detail?id=180306

[Andrew T Wilson (Slow), 2013-03-07 17:57:36]

erg, gab, PTAL since I've updated the patch to allow more services to run on the
import process (due to explicit dependencies from ProfileImpl -> the services).

[gab, 2013-03-07 19:54:22]

I'm not an expert in ProfileKeyedServices (caitkp CC'ed might have a better
opinion).

It seems like a lot of services are set to be started on the import process
anyways (i.e. they override ServiceIsNULLOnImportProcess() to return false).

Could you add each of these services in the CL description and explain why each
one of them is required on the import process?

Thanks,
Gab

https://codereview.chromium.org/12330073/diff/26001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/12330073/diff/26001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:475:
!ProfileManager::IsImportProcess(*CommandLine::ForCurrentProcess());
While you're probably right this doesn't need to be initialized; I'm not a fan
of this paradigm of checking IsImportProcess() all over the place; seems like a
hack to a bigger problem.

[Elliot Glaysher, 2013-03-07 20:45:48]

https://codereview.chromium.org/12330073/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/extension_system_factory.cc (right):

https://codereview.chromium.org/12330073/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/extension_system_factory.cc:90: return false;
The extension system depends on a lot of stuff, and IIUC, this will implicitly
create those services.

[Cait (Slow), 2013-03-07 21:00:26]

I'm by no means a PKS expert either, but I'll weigh in:

I agree with Elliot that there's the issue of a service implicitly creating
several other services (or failing to, if those services are disabled on the
import process). 

Also, it seems that the services which override this method (esp. those that
deal with writing anything to a db, which is a fairly common import task) are
precisely the most dangerous ones to have running on the import and browser
process simultaneously (as there is only one db).

On 2013/03/07 20:45:48, Elliot Glaysher wrote:
>
https://codereview.chromium.org/12330073/diff/26001/chrome/browser/extensions...
> File chrome/browser/extensions/extension_system_factory.cc (right):
> 
>
https://codereview.chromium.org/12330073/diff/26001/chrome/browser/extensions...
> chrome/browser/extensions/extension_system_factory.cc:90: return false;
> The extension system depends on a lot of stuff, and IIUC, this will implicitly
> create those services.

[Andrew T Wilson (Slow), 2013-03-07 22:35:52]

On 2013/03/07 21:00:26, caitkp wrote:
> I'm by no means a PKS expert either, but I'll weigh in:
> 
> I agree with Elliot that there's the issue of a service implicitly creating
> several other services (or failing to, if those services are disabled on the
> import process). 

Not quite true - those services *cannot* be created on the import process if
they don't return false to ServiceIsNULLOnImportProcess(). The creation code
will always return NULL for any other services. So in this particular case, the
ExtensionService has just enough of its dependencies enabled to allow the
Profile to finish initialization.

> 
> Also, it seems that the services which override this method (esp. those that
> deal with writing anything to a db, which is a fairly common import task) are
> precisely the most dangerous ones to have running on the import and browser
> process simultaneously (as there is only one db).

Indeed. And yet as far as I can tell this is the minimum set of services that
*must* be created in order to let import proceed, without doing surgery on
ProfileImpl and ProfileIOData to only partially initialize those objects when
running on the Import process, which seemed even more fragile.

I think the right long-term fix is to refactor the firstrun code so that we
don't create the profile on the browser process until after import is complete -
that seems like a difficult task to try to squeeze into M26 though (and I'm the
wrong person to own that), hence this short-term solution.

Let me know what you guys would like me to do here - if there's a different
solution to the M26 blocking bugs that you guys prefer, I'm happy to discard
this CL. I was originally not planning to land it, and only whipped it into
shape because @gab asked me to in the comments for crbug.com/180459.

> 
> On 2013/03/07 20:45:48, Elliot Glaysher wrote:
> >
>
https://codereview.chromium.org/12330073/diff/26001/chrome/browser/extensions...
> > File chrome/browser/extensions/extension_system_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/12330073/diff/26001/chrome/browser/extensions...
> > chrome/browser/extensions/extension_system_factory.cc:90: return false;
> > The extension system depends on a lot of stuff, and IIUC, this will
implicitly
> > create those services.

[gab, 2013-03-07 22:49:41]

What bothers me is that the import process probably needs more services if other
import preferences are present (i.e. import_bookmarks,
import_bookmarks_from_file, import_history, import_home_page, and
import_search_engine).

In fact we should probably add a FirstRunBrowserTest to test the different
import preferences (currently the only tests are (1) default prefs and (2) all
prefs == false).

Gab


On 2013/03/07 22:35:52, Andrew T Wilson wrote:
> On 2013/03/07 21:00:26, caitkp wrote:
> > I'm by no means a PKS expert either, but I'll weigh in:
> > 
> > I agree with Elliot that there's the issue of a service implicitly creating
> > several other services (or failing to, if those services are disabled on the
> > import process). 
> 
> Not quite true - those services *cannot* be created on the import process if
> they don't return false to ServiceIsNULLOnImportProcess(). The creation code
> will always return NULL for any other services. So in this particular case,
the
> ExtensionService has just enough of its dependencies enabled to allow the
> Profile to finish initialization.
> 
> > 
> > Also, it seems that the services which override this method (esp. those that
> > deal with writing anything to a db, which is a fairly common import task)
are
> > precisely the most dangerous ones to have running on the import and browser
> > process simultaneously (as there is only one db).
> 
> Indeed. And yet as far as I can tell this is the minimum set of services that
> *must* be created in order to let import proceed, without doing surgery on
> ProfileImpl and ProfileIOData to only partially initialize those objects when
> running on the Import process, which seemed even more fragile.
> 
> I think the right long-term fix is to refactor the firstrun code so that we
> don't create the profile on the browser process until after import is complete
-
> that seems like a difficult task to try to squeeze into M26 though (and I'm
the
> wrong person to own that), hence this short-term solution.
> 
> Let me know what you guys would like me to do here - if there's a different
> solution to the M26 blocking bugs that you guys prefer, I'm happy to discard
> this CL. I was originally not planning to land it, and only whipped it into
> shape because @gab asked me to in the comments for crbug.com/180459.
> 
> > 
> > On 2013/03/07 20:45:48, Elliot Glaysher wrote:
> > >
> >
>
https://codereview.chromium.org/12330073/diff/26001/chrome/browser/extensions...
> > > File chrome/browser/extensions/extension_system_factory.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12330073/diff/26001/chrome/browser/extensions...
> > > chrome/browser/extensions/extension_system_factory.cc:90: return false;
> > > The extension system depends on a lot of stuff, and IIUC, this will
> implicitly
> > > create those services.

[gab, 2013-03-07 22:57:18]

On 2013/03/07 22:49:41, gab wrote:
> What bothers me is that the import process probably needs more services if
other
> import preferences are present (i.e. import_bookmarks,
> import_bookmarks_from_file, import_history, import_home_page, and
> import_search_engine).
> 
> In fact we should probably add a FirstRunBrowserTest to test the different
> import preferences (currently the only tests are (1) default prefs and (2) all
> prefs == false).

Actually, tapted has a TODO to do just that I think
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fir...),
would be nice to get those tests up before landing this (especially if we want
to merge this in M26...).

Trent could you look into that please?

Thanks!
Gab

[Andrew T Wilson (Slow), 2013-03-07 23:20:19]

Agreed, we don't have test coverage for the rest of the import prefs, so it'd be
good to get some tests running for some of those other options. It's possible
that those settings need more services than I have turned on here.

[tapted, 2013-03-07 23:24:57]

On 2013/03/07 22:57:18, gab wrote:
> On 2013/03/07 22:49:41, gab wrote:
> > What bothers me is that the import process probably needs more services if
> other
> > import preferences are present (i.e. import_bookmarks,
> > import_bookmarks_from_file, import_history, import_home_page, and
> > import_search_engine).
> > 
> > In fact we should probably add a FirstRunBrowserTest to test the different
> > import preferences (currently the only tests are (1) default prefs and (2)
all
> > prefs == false).
> 
> Actually, tapted has a TODO to do just that I think
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fir...),
> would be nice to get those tests up before landing this (especially if we want
> to merge this in M26...).
> 
> Trent could you look into that please?

Yep - I'll check it out. That TODO has been an itch I've been meaning to scratch
for a while..