Do not load extension system in the Profile import process.

Do not load extension system in the Profile import process.
Also, defer loading component extensions with background pages until
the import process is complete, so that the renderer host does not
compete for the database lock.

This also adds a regression test for http://crbug.com/163925.

BUG=163925, 107636
TEST=Test first-run flows and profile import after uninstalling chrome and removing AppData/Local/Google/Chrome directory

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

[Yoyo Zhou, 2012-12-15 02:00:22]

Extensions code LGTM, but you should find a more qualified reviewer for the
profile and first run code.

https://codereview.chromium.org/11572036/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11572036/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:415:
CHECK(!ProfileManager::IsImportProcess(*command_line));
FYI, if this check fails (and the import process crashes) you'll get the
"profile could not be opened correctly" error.

https://codereview.chromium.org/11572036/diff/11001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11572036/diff/11001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:127: static bool
DidPerformProfileImport();
This looks like it's only used in tests; I would add a note about that.

[tapted, 2012-12-17 05:40:39]

I've done some additional end-to-end testing with official/release builds and
the installer. Everything seems happy.

PTAL

sky, for
 * chrome/test
 * chrome/browser{,first_run}
 * content/public/test

mirandac, for
 * chrome/browser/profiles/profile_manager.*

joi, for OWNERS
 * content/public/common/content_switches.*

(or other comments always welcome!)

The regression test was a bit tricky. Lots of dead ends because --no-first-run
is added to args on all tests quite late in the test harness, and the first-run
import triggers are too early in the browser process to slip in observers. This
seemed like the least messy way, but I'm open to trying other approaches.


On 2012/12/15 02:00:22, Yoyo Zhou wrote:
> Extensions code LGTM, but you should find a more qualified reviewer for the
> profile and first run code.
> 
>
https://codereview.chromium.org/11572036/diff/11001/chrome/browser/extensions...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/11572036/diff/11001/chrome/browser/extensions...
> chrome/browser/extensions/extension_service.cc:415:
> CHECK(!ProfileManager::IsImportProcess(*command_line));
> FYI, if this check fails (and the import process crashes) you'll get the
> "profile could not be opened correctly" error.

https://codereview.chromium.org/11572036/diff/11001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11572036/diff/11001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:127: static bool
DidPerformProfileImport();
On 2012/12/15 02:00:23, Yoyo Zhou wrote:
> This looks like it's only used in tests; I would add a note about that.

Done.

[Jói, 2012-12-17 13:46:15]

https://codereview.chromium.org/11572036/diff/13001/content/public/common/con...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/11572036/diff/13001/content/public/common/con...
content/public/common/content_switches.cc:468: const char kLaunchAsBrowser[]    
          = "as-browser";
It feels a bit weird to promote this to a publicly-available switch since it is
only checked by test code AFAIU.  Is there no way to add the flag on the Chrome
side only in test code (which is already allowed to use content/public/test,
where this flag was originally defined)?

[sky, 2012-12-17 17:19:53]

LGTM

https://codereview.chromium.org/11572036/diff/13001/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/11572036/diff/13001/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:55: };
private: DISALLOW_..

[Miranda Callahan, 2012-12-17 19:04:34]

profile changes LGTM with nit.

https://codereview.chromium.org/11572036/diff/13001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11572036/diff/13001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1184: ||
parsed_command_line().HasSwitch(switches::kFirstRunForceImport)) {
style for this file is to put || at the end of the line.

[tapted, 2012-12-18 02:25:58]

sky please take another look for changes:
 * chrome/browser/first_run
 * chrome/browser/ui/views (/location_bar -- see below)

sky could you please take another look at the changes to
chrome/browser/first_run [patchset 6]. This introduces a generic interface so
that "--as-browser" isn't promoted into content/public, via

 100 // Allow a test to specify additional arguments for the profile import
process.
 101 void SetExtraArgumentsForImportProcess(const CommandLine& arguments);
 102 
 103 // Get any extra arguments set with SetExtraArgumentsForImportProcess.
 104 const CommandLine& GetExtraArgumentsForImportProcess();

I've also managed to run into a possible debug-only flake while running this
test in a loop, due to a DCHECK in chrome::FindBrowserWithWebContents.  sky
(sorry!), could you please also take a look at the change [patchset 7] to 
https://codereview.chromium.org/11572036/diff/23004/chrome/browser/ui/views/l...
 -- LocationBarView has checks for a null WebContents during shutdown in other
places, but in ShowFirstRunBubbleInternal it can hit that DCHECK. I say
debug-only because in release builds it looks like the FirstRunBubble can handle
being constructed with a null Browser.

(the style changes for earlier comments are in patchset 5)

Thanks.

https://codereview.chromium.org/11572036/diff/13001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11572036/diff/13001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1184: ||
parsed_command_line().HasSwitch(switches::kFirstRunForceImport)) {
On 2012/12/17 19:04:34, Miranda Callahan wrote:
> style for this file is to put || at the end of the line.

Done.

https://codereview.chromium.org/11572036/diff/13001/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/11572036/diff/13001/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:55: };
On 2012/12/17 17:19:53, sky wrote:
> private: DISALLOW_..

Done.

https://codereview.chromium.org/11572036/diff/13001/content/public/common/con...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/11572036/diff/13001/content/public/common/con...
content/public/common/content_switches.cc:468: const char kLaunchAsBrowser[]    
          = "as-browser";
On 2012/12/17 13:46:16, Jói wrote:
> It feels a bit weird to promote this to a publicly-available switch since it
is
> only checked by test code AFAIU.  Is there no way to add the flag on the
Chrome
> side only in test code (which is already allowed to use content/public/test,
> where this flag was originally defined)?

I've put up an alternative that removes the changes in content/public by adding
new, generic interfaces to first_run. Let me know what you think.

[Jói, 2012-12-18 10:00:11]

LGTM, much preferred, and great to have the added test to verify the behavior.

[sky, 2012-12-18 16:24:02]

https://codereview.chromium.org/11572036/diff/23004/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_win.cc (right):

https://codereview.chromium.org/11572036/diff/23004/chrome/browser/first_run/...
chrome/browser/first_run/first_run_win.cc:336:
import_cmd.AppendArguments(first_run::GetExtraArgumentsForImportProcess(),
Instead of this, can you mutate the command line after the browser test launches
but before import?

[tapted, 2012-12-19 00:07:53]

https://codereview.chromium.org/11572036/diff/23004/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_win.cc (right):

https://codereview.chromium.org/11572036/diff/23004/chrome/browser/first_run/...
chrome/browser/first_run/first_run_win.cc:336:
import_cmd.AppendArguments(first_run::GetExtraArgumentsForImportProcess(),
On 2012/12/18 16:24:02, sky wrote:
> Instead of this, can you mutate the command line after the browser test
launches
> but before import?

I haven't yet found a clean way to do that.. The problem was that if
"--as-browser" is not passed at this point for a test process, then
test_launcher.cc:LaunchTests() tries to restart all the tests, because
--gtest_filter is not included in kSwitchNames just above this line (and I don't
think I can get away with including it ;-). (also, with --gtest_filter, it
crosses the test sharding code paths, which I'm not sure it would like). So
currently only --as-browser is ever appended here (but there's scope for more if
further integration testing is done for the first-run import).

I looked at content::ChildProcessHost::GetChildPath(..CHILD_NORMAL) instead of
cmdline.GetProgram() [line 326 here], but it boils down the same thing, so also
tries to relaunch ./browser_tests from scratch and run all the tests.

Another option would be to include "--import" (and "--import-from-file") in the
list of flags in ShouldRunContentMain(), at
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/test/test_l...
But I didn't see a current dependency in test_launcher.cc on
chrome/common/content_switches.h (nor c/b/profiles to get the IsImportProcess
helper I'm introducing).

Creating a duplicate kLaunchAsBrowser[] (.. or "--as-browser" literal) in
first_run_win.cc would also achieve the goal, but might be naughty (and I didn't
think #including test_launcher.h in first_run_win.cc would pass review/DEPS
either). But maybe that's the lesser evil..

Then, I've been poking around more in the test_launcher.cc and browser_tests
code paths, but I'm not seeing a way to hook back into a specific test in order
to mutate the command line, without passing --gtest_filter in the relaunch
command.

[tapted, 2012-12-19 13:19:13]

yoz - PTAL for changes (extensions/component_loader*, extension_service)

Trying out those experiments let me pick up something I'd missed earlier. While
the earlier patches addressed the "guaranteed" conflict in the import process
when it tries to load extensions (and fixed the "your profile could not be
opened" error/popup from http://crbug.com/163925), there is another potential
conflict in the browser process.

The repro case for this part is: "--first-run" AND remove profile folder
(otherwise the background pages stay lazy) AND it's sometimes racy. In this
case, it's possible for the background pages load to be attempted while the
import process is still running. The symptoms are the LOG(ERROR) lines from the
browser process (and -possibly- incorrectly registered lazy background page
events). There are some tryjobs to isolate this in patchset 8 -- it happened in
the "win_rel" job, but not "win" (which is red for a different reason).

i.e. 
[104356:104580:1219/011450:ERROR:error_delegate_util.h(27)] sqlite error 5,
errno 33: database is locked

So, this CL now allows ExtensionService to load component extensions in two
stages on a first_run; with extensions with background pages able to be deferred
until after the profile import process is complete. I've updated the CL
description accordingly. Yoyo, can you please take a look?

I've added a unit_test for the new component_loader logic. However, the
FirstRunIntegration browser test is currently only checking for regressions in
the import process (e.g. if it dies), not the behaviour around the LOG(ERROR)s
in the browser process.

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
chrome/browser/extensions/component_loader.h:118: void Load(const
ComponentExtensionInfo& info,
(note: return type changed to void - nothing used it, and it raises fewer
questions this way)

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:604:
extensions::InstalledLoader(this).LoadAllExtensions();
a question raised from this line might be, "what about user-installed extensions
with background pages?" There shouldn't (yet) be any installed extensions in a
"real" first-run/auto-import. So this line here should typically do nothing.
However, extensions (with background pages) installed via master_preferences
might be an exception.

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:57:
extensions::ComponentLoader::EnableBackgroundExtensionsForTesting();
added line - to check for conflicts in the browser process (as well as the
import process)

[sky, 2012-12-19 16:14:00]

https://codereview.chromium.org/11572036/diff/23004/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_win.cc (right):

https://codereview.chromium.org/11572036/diff/23004/chrome/browser/first_run/...
chrome/browser/first_run/first_run_win.cc:336:
import_cmd.AppendArguments(first_run::GetExtraArgumentsForImportProcess(),
On 2012/12/19 00:07:53, tapted wrote:
> On 2012/12/18 16:24:02, sky wrote:
> > Instead of this, can you mutate the command line after the browser test
> launches
> > but before import?
> 
> I haven't yet found a clean way to do that.. The problem was that if
> "--as-browser" is not passed at this point for a test process, then
> test_launcher.cc:LaunchTests() tries to restart all the tests, because
> --gtest_filter is not included in kSwitchNames just above this line (and I
don't
> think I can get away with including it ;-). (also, with --gtest_filter, it
> crosses the test sharding code paths, which I'm not sure it would like). So
> currently only --as-browser is ever appended here (but there's scope for more
if
> further integration testing is done for the first-run import).
> 
> I looked at content::ChildProcessHost::GetChildPath(..CHILD_NORMAL) instead of
> cmdline.GetProgram() [line 326 here], but it boils down the same thing, so
also
> tries to relaunch ./browser_tests from scratch and run all the tests.
> 
> Another option would be to include "--import" (and "--import-from-file") in
the
> list of flags in ShouldRunContentMain(), at
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/test/test_l...
> But I didn't see a current dependency in test_launcher.cc on
> chrome/common/content_switches.h (nor c/b/profiles to get the IsImportProcess
> helper I'm introducing).
> 
> Creating a duplicate kLaunchAsBrowser[] (.. or "--as-browser" literal) in
> first_run_win.cc would also achieve the goal, but might be naughty (and I
didn't
> think #including test_launcher.h in first_run_win.cc would pass review/DEPS
> either). But maybe that's the lesser evil..
> 
> Then, I've been poking around more in the test_launcher.cc and browser_tests
> code paths, but I'm not seeing a way to hook back into a specific test in
order
> to mutate the command line, without passing --gtest_filter in the relaunch
> command.

What happens if you add --as-browser to your tests SetUpCommandLine?

[Yoyo Zhou, 2012-12-19 21:50:38]

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
chrome/browser/extensions/component_loader.h:36: // will be deferred until
LoadDeferred if the argument is true.
LoadAllDeferred

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
chrome/browser/extensions/component_loader.h:37: void LoadAll(bool
defer_extensions_with_background_pages);
Reading LoadAll(false) in the code isn't very self-descriptive. I'd prefer this
to be 2 functions, like LoadAllNow and LoadAllDeferringBackgroundPages (this
name isn't any good so feel free to improve it).

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:604:
extensions::InstalledLoader(this).LoadAllExtensions();
On 2012/12/19 13:19:14, tapted wrote:
> a question raised from this line might be, "what about user-installed
extensions
> with background pages?" There shouldn't (yet) be any installed extensions in a
> "real" first-run/auto-import. So this line here should typically do nothing.
> However, extensions (with background pages) installed via master_preferences
> might be an exception.

If non-component extensions are not being deferred, then the comment should read
"defer component extensions".

[tapted, 2012-12-19 21:53:01]

On 2012/12/19 16:14:00, sky wrote:
> What happens if you add --as-browser to your tests SetUpCommandLine?

Unfortunately the code in first_run_win.cc will not copy it to the new process.
It has a whitelist of 3 flags that it copies here:
https://codereview.chromium.org/11572036/diff/45019/chrome/browser/first_run/...

  const char* kSwitchNames[] = {
    switches::kUserDataDir,
    switches::kChromeFrame,
    switches::kCountry,
  };
  import_cmd.CopySwitchesFrom(cmdline, kSwitchNames, arraysize(kSwitchNames));

I've put up a CL here -
https://codereview.chromium.org/11647018/diff2/1:17/chrome/browser/first_run/...
- to make sure I'm doing it right ;-). There's a job there (still running) for
-bwin_rel:browser_tests:FirstRunIntegrationBrowserTest.WaitForImport, but the
result on my workstation was for it to start running all browser_tests from
scratch in the import process.


The import process might be unaffected if the code in first_run_win.cc is
changed to always copy the entire command line.. I can look into this if you
like.

[sky, 2012-12-19 22:24:47]

I like the white list, but is it possible to add --as-browser to the white
list?

  -Scott

On Wed, Dec 19, 2012 at 1:53 PM,  <tapted@chromium.org> wrote:
> On 2012/12/19 16:14:00, sky wrote:
>>
>> What happens if you add --as-browser to your tests SetUpCommandLine?
>
>
> Unfortunately the code in first_run_win.cc will not copy it to the new
> process.
> It has a whitelist of 3 flags that it copies here:
>
https://codereview.chromium.org/11572036/diff/45019/chrome/browser/first_run/...
>
>   const char* kSwitchNames[] = {
>     switches::kUserDataDir,
>     switches::kChromeFrame,
>     switches::kCountry,
>   };
>   import_cmd.CopySwitchesFrom(cmdline, kSwitchNames,
> arraysize(kSwitchNames));
>
> I've put up a CL here -
>
https://codereview.chromium.org/11647018/diff2/1:17/chrome/browser/first_run/...
> - to make sure I'm doing it right ;-). There's a job there (still running)
> for
> -bwin_rel:browser_tests:FirstRunIntegrationBrowserTest.WaitForImport, but
> the
> result on my workstation was for it to start running all browser_tests from
> scratch in the import process.
>
>
> The import process might be unaffected if the code in first_run_win.cc is
> changed to always copy the entire command line.. I can look into this if you
> like.
>
> https://codereview.chromium.org/11572036/

[tapted, 2012-12-19 22:31:38]

On 2012/12/19 22:24:47, sky wrote:
> I like the white list, but is it possible to add --as-browser to the white
> list?

The constant is currently in public/test/test_launcher.h. I arrived at the
{Get,Set}ExtraArgumentsForImportProcess approach to avoid adding that #include
to first_run_win.cc (deps won't let me..), and to avoid promoting --as-browser
from test_launcher.h to content/public/common/content_switches.h [as in patchset
5].

Of course.... I can add it as a new literal, if you approve.

[tapted, 2012-12-19 22:58:24]

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
chrome/browser/extensions/component_loader.h:36: // will be deferred until
LoadDeferred if the argument is true.
On 2012/12/19 21:50:38, Yoyo Zhou wrote:
> LoadAllDeferred

Done.

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
chrome/browser/extensions/component_loader.h:37: void LoadAll(bool
defer_extensions_with_background_pages);
On 2012/12/19 21:50:38, Yoyo Zhou wrote:
> Reading LoadAll(false) in the code isn't very self-descriptive. I'd prefer
this
> to be 2 functions, like LoadAllNow and LoadAllDeferringBackgroundPages (this
> name isn't any good so feel free to improve it).

Done. I went with
- BulkLoadAll
- BulkLoadDeferBackgroundPages
- BulkLoadDeferred

(I also toyed with an enum in an earlier iteration, since true/false was giving
me an itch too).

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11572036/diff/45019/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:604:
extensions::InstalledLoader(this).LoadAllExtensions();
On 2012/12/19 21:50:38, Yoyo Zhou wrote:
> On 2012/12/19 13:19:14, tapted wrote:
> > a question raised from this line might be, "what about user-installed
> extensions
> > with background pages?" There shouldn't (yet) be any installed extensions in
a
> > "real" first-run/auto-import. So this line here should typically do nothing.
> > However, extensions (with background pages) installed via master_preferences
> > might be an exception.
> 
> If non-component extensions are not being deferred, then the comment should
read
> "defer component extensions".

Done.

[Yoyo Zhou, 2012-12-19 22:59:41]

LGTM for extensions

[tapted, 2012-12-20 00:08:34]

On 2012/12/19 22:31:38, tapted wrote:
> On 2012/12/19 22:24:47, sky wrote:
> > I like the white list, but is it possible to add --as-browser to the white
> > list?

Alternative (with a new literal) is in patchset 10, so you can see it in
context.

https://codereview.chromium.org/11572036/diff2/48032:53015/chrome/browser/fir...

[sky, 2012-12-20 00:19:54]

John knows more about this flag than I do. I'm passing it to him. John, look at
the changes to chrome/browser/first_run/first_run_win.cc .

[sky, 2012-12-20 00:20:19]

Sorry, forgot to add John.

John knows more about this flag than I do. I'm passing it to him. John, look at
the changes to chrome/browser/first_run/first_run_win.cc .

[jam, 2012-12-20 00:42:29]

On 2012/12/20 00:20:19, sky wrote:
> Sorry, forgot to add John.
> 
> John knows more about this flag than I do. I'm passing it to him. John, look
at
> the changes to chrome/browser/first_run/first_run_win.cc .

first, apologies for all the different opinions reviewers keep bouncing at you
:)

I looked through most (but not all) of these comments. I like patchset 10, i.e.
with Joi's suggestion of having an interface that a test can add extra command
line parameters.  hardcoding "as-browser" in production code is the worst of
both worlds (i.e. this is a test-only thing, and now a constant is duplicated).
why did you switch to patchset 11?

[tapted, 2012-12-20 00:57:28]

On 2012/12/20 00:42:29, John Abd-El-Malek wrote:
> I looked through most (but not all) of these comments. I like patchset 10,
i.e.
> with Joi's suggestion of having an interface that a test can add extra command
> line parameters.  hardcoding "as-browser" in production code is the worst of
> both worlds (i.e. this is a test-only thing, and now a constant is
duplicated).
> why did you switch to patchset 11?

Oh - sorry - I just realised in comment 18 I meant to say patchset 11 (not 10).
I put it up just as a comparison - I enthusiastically agree with you!

I've reverted to patchset 10 (as a new, patchset 12).

[jam, 2012-12-20 02:34:18]

On 2012/12/20 00:57:28, tapted wrote:
> On 2012/12/20 00:42:29, John Abd-El-Malek wrote:
> > I looked through most (but not all) of these comments. I like patchset 10,
> i.e.
> > with Joi's suggestion of having an interface that a test can add extra
command
> > line parameters.  hardcoding "as-browser" in production code is the worst of
> > both worlds (i.e. this is a test-only thing, and now a constant is
> duplicated).
> > why did you switch to patchset 11?
> 
> Oh - sorry - I just realised in comment 18 I meant to say patchset 11 (not
10).
> I put it up just as a comparison - I enthusiastically agree with you!
> 
> I've reverted to patchset 10 (as a new, patchset 12).

lgtm, thanks

[commit-bot: I haz the power, 2012-12-20 22:28:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/11572036/45024

[commit-bot: I haz the power, 2012-12-21 01:16:27]

Change committed as 174302