Initialize chrome::DIR_USER_DATA early on for service processes, etc.

Initialize chrome::DIR_USER_DATA early on for service processes, etc.

http://crrev.com/251126 broke service/helper processes.
(--user-data-dir command-line args weren't respected)

Restore the early PathService DIR_USER_DATA init/override.
(done as before in ChromeMainDelegate::PreSandboxStartup)

Move init code to a file-local InitializeUserDataDir helper.
Append the fallback to the commandline for other processes.
(otherwise child/service processes re-use the bad dir)

Simplify ChromeBrowserMainParts::PreCreateThreadsImpl.
(just get DIR_USER_DATA, don't re-attempt init/override)
Show a warning messagebox here if user-data-dir was invalid.
Move warning UI helper to c/b/ui/startup/bad_flags_prompt.h

Add [Get|Set]InvalidSpecifiedUserDataDir for warning UI.
(add dynamic_annotations dependency for LazyInstance usage)

Remove the ChromeMainUserDataDirTest.GetUserDataDir test.
(now ineffective, it can't hook before PreSandboxStartup)

BUG=345025, 345582, 318999
TEST=The --user-data-dir command-line argument works as expected for browser, child, and service processes (cloud print connector, chrome logging, etc.). Users are still warned with a dialog when they supply invalid or restricted directory paths for the browser.
R=sky@chromium.org,vitalybuka@chromium.org

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

[msw, 2014-02-20 19:35:43]

Hey Scott and Vitaly, please take a look; thanks!
Sorry for the regression, hopefully this fixes everything.
(Andrew CC'ed for the TRACE call removal, seems okay)

[Vitaly Buka (NO REVIEWS), 2014-02-20 19:58:16]

lgtm

https://codereview.chromium.org/174253002/diff/1/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/174253002/diff/1/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:584:
chrome::GetUserDataDir(content::MainFunctionParams(command_line));
Should we fail silently for service or any other child processes?

[msw, 2014-02-20 20:58:47]

https://codereview.chromium.org/174253002/diff/1/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/174253002/diff/1/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:584:
chrome::GetUserDataDir(content::MainFunctionParams(command_line));
On 2014/02/20 19:58:17, Vitaly Buka wrote:
> Should we fail silently for service or any other child processes?

Hmm, the dialog does show up a second time when I launch the Chrome Connector
process from Chromium with --user-data-dir="Fake:\Garbage", and worse, it
doesn't create the expected "Service State" file in the actual fallback
DIR_USER_DATA ("C:\Users\msw\AppData\Local\Chromium\User Data\"). Note that
child tab/extension/gpu processes for the browser itself don't fire the dialog
and seem to work okay.

In the next patch set, I fix that issue by appending a --user-data-dir switch
onto the command-line with the fallback directory as needed, so child/service
processes use the expected fallback, rather than re-attempting to use the bad
directory. Thoughts?

[Vitaly Buka (NO REVIEWS), 2014-02-20 21:25:24]

On 2014/02/20 20:58:47, msw wrote:
>
https://codereview.chromium.org/174253002/diff/1/chrome/app/chrome_main_deleg...
> File chrome/app/chrome_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/174253002/diff/1/chrome/app/chrome_main_deleg...
> chrome/app/chrome_main_delegate.cc:584:
> chrome::GetUserDataDir(content::MainFunctionParams(command_line));
> On 2014/02/20 19:58:17, Vitaly Buka wrote:
> > Should we fail silently for service or any other child processes?
> 
> Hmm, the dialog does show up a second time when I launch the Chrome Connector
> process from Chromium with --user-data-dir="Fake:\Garbage", and worse, it
> doesn't create the expected "Service State" file in the actual fallback
> DIR_USER_DATA ("C:\Users\msw\AppData\Local\Chromium\User Data\"). Note that
> child tab/extension/gpu processes for the browser itself don't fire the dialog
> and seem to work okay.
> 
> In the next patch set, I fix that issue by appending a --user-data-dir switch
> onto the command-line with the fallback directory as needed, so child/service
> processes use the expected fallback, rather than re-attempting to use the bad
> directory. Thoughts?

runs by different code then other children. Look at
ServiceProcessControl::Launch 
It's just copy flag --user-data-dir.
Also I see a lot of code directly read user-data-dir flag.
Should we just reset value of user-data-dir for current process?


Also service process is not always started by browser. It maybe be started from
registry or as windows service. In later case it message box is undesirable.

[Vitaly Buka (NO REVIEWS), 2014-02-20 21:26:11]

On 2014/02/20 21:25:24, Vitaly Buka wrote:
> On 2014/02/20 20:58:47, msw wrote:
> >
>
https://codereview.chromium.org/174253002/diff/1/chrome/app/chrome_main_deleg...
> > File chrome/app/chrome_main_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/174253002/diff/1/chrome/app/chrome_main_deleg...
> > chrome/app/chrome_main_delegate.cc:584:
> > chrome::GetUserDataDir(content::MainFunctionParams(command_line));
> > On 2014/02/20 19:58:17, Vitaly Buka wrote:
> > > Should we fail silently for service or any other child processes?
> > 
> > Hmm, the dialog does show up a second time when I launch the Chrome
Connector
> > process from Chromium with --user-data-dir="Fake:\Garbage", and worse, it
> > doesn't create the expected "Service State" file in the actual fallback
> > DIR_USER_DATA ("C:\Users\msw\AppData\Local\Chromium\User Data\"). Note that
> > child tab/extension/gpu processes for the browser itself don't fire the
dialog
> > and seem to work okay.
> > 
> > In the next patch set, I fix that issue by appending a --user-data-dir
switch
> > onto the command-line with the fallback directory as needed, so
child/service
> > processes use the expected fallback, rather than re-attempting to use the
bad
> > directory. Thoughts?
> 
> runs by different code then other children. Look at
> ServiceProcessControl::Launch 
> It's just copy flag --user-data-dir.
> Also I see a lot of code directly read user-data-dir flag.
> Should we just reset value of user-data-dir for current process?
> 
> 
> Also service process is not always started by browser. It maybe be started
from
> registry or as windows service. In later case it message box is undesirable.

Service process runs by different code then other children. Look at
ServiceProcessControl::Launch 
It's just copy flag --user-data-dir.
Also I see a lot of code directly read user-data-dir flag.
Should we just reset value of user-data-dir for current process?


Also service process is not always started by browser. It maybe be started from

[Vitaly Buka (NO REVIEWS), 2014-02-20 21:28:04]

lgtm

https://codereview.chromium.org/174253002/diff/100001/chrome/browser/user_dat...
File chrome/browser/user_data_dir_extractor.cc (right):

https://codereview.chromium.org/174253002/diff/100001/chrome/browser/user_dat...
chrome/browser/user_data_dir_extractor.cc:112:
CommandLine::ForCurrentProcess()->AppendSwitchPath(switches::kUserDataDir,
actually you just did that

[Vitaly Buka (NO REVIEWS), 2014-02-20 21:31:32]

On 2014/02/20 21:28:04, Vitaly Buka wrote:
> lgtm
> 
>
https://codereview.chromium.org/174253002/diff/100001/chrome/browser/user_dat...
> File chrome/browser/user_data_dir_extractor.cc (right):
> 
>
https://codereview.chromium.org/174253002/diff/100001/chrome/browser/user_dat...
> chrome/browser/user_data_dir_extractor.cc:112:
> CommandLine::ForCurrentProcess()->AppendSwitchPath(switches::kUserDataDir,
> actually you just did that

We still have the issue windows service runs: chrome --type=service
--user-data-dir=INVALID
It's not a big issue, because our windows service should make sure that value is
valid.

[msw, 2014-02-20 21:48:33]

On 2014/02/20 21:31:32, Vitaly Buka wrote:
> We still have the issue windows service runs: chrome --type=service
> --user-data-dir=INVALID
> It's not a big issue, because our windows service should make sure that value
is
> valid.

Can you file a followup issue for service owners to fix that?

[Vitaly Buka (NO REVIEWS), 2014-02-20 21:56:53]

On 2014/02/20 21:48:33, msw wrote:
> On 2014/02/20 21:31:32, Vitaly Buka wrote:
> > We still have the issue windows service runs: chrome --type=service
> > --user-data-dir=INVALID
> > It's not a big issue, because our windows service should make sure that
value
> is
> > valid.
> 
> Can you file a followup issue for service owners to fix that?

Sure, it could be handled as separate issue. I am maintainer of that service.

[sky, 2014-02-20 22:38:58]

LGTM

[msw, 2014-02-21 22:54:30]

Please take another look; I revised the init/warn approach.

[Vitaly Buka (NO REVIEWS), 2014-02-21 23:11:33]

lgtm

lgtm with

https://codereview.chromium.org/174253002/diff/460001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/174253002/diff/460001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:583: switches::InitializeUserDataDir();
Maybe note about logging too?

https://codereview.chromium.org/174253002/diff/460001/chrome/common/switch_ut...
File chrome/common/switch_utils.cc (right):

https://codereview.chromium.org/174253002/diff/460001/chrome/common/switch_ut...
chrome/common/switch_utils.cc:26: static
base::LazyInstance<base::FilePath>::Leaky
FilePath is simple structure.
I guess it's OK to have non ::Leaky.

https://codereview.chromium.org/174253002/diff/460001/chrome/common/switch_ut...
File chrome/common/switch_utils.h (right):

https://codereview.chromium.org/174253002/diff/460001/chrome/common/switch_ut...
chrome/common/switch_utils.h:24: void InitializeUserDataDir();
How about adding bool IsUserDataDirInitialized() initialized

and DCHECK(IsUserDataDirInitialized)in 

chrome_paths.cc:158

switch (key) {
    case chrome::DIR_USER_DATA:
      DCHECK(IsUserDataDirInitialized());
      if (!GetDefaultUserDataDirectory(&cur)) {

[msw, 2014-02-21 23:38:30]

Shoot, I'm getting a check_deps failure:
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/2316...

ERROR in src/chrome/common/switch_utils.cc
  Illegal include: "chrome/browser/policy/policy_path_parser.h"
    Because of "-chrome" from chrome's include_rules.

I might have to move around some of the policy_path_parser files, maybe just add
policy_path_parser as a chrome/common dependency, or maybe move the init
functions out of chrome/common??? Advice appreciated.

https://codereview.chromium.org/174253002/diff/460001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/174253002/diff/460001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:583: switches::InitializeUserDataDir();
On 2014/02/21 23:11:33, Vitaly Buka wrote:
> Maybe note about logging too?

Done.

https://codereview.chromium.org/174253002/diff/460001/chrome/common/switch_ut...
File chrome/common/switch_utils.cc (right):

https://codereview.chromium.org/174253002/diff/460001/chrome/common/switch_ut...
chrome/common/switch_utils.cc:26: static
base::LazyInstance<base::FilePath>::Leaky
On 2014/02/21 23:11:33, Vitaly Buka wrote:
> FilePath is simple structure.
> I guess it's OK to have non ::Leaky.

Done.

https://codereview.chromium.org/174253002/diff/460001/chrome/common/switch_ut...
File chrome/common/switch_utils.h (right):

https://codereview.chromium.org/174253002/diff/460001/chrome/common/switch_ut...
chrome/common/switch_utils.h:24: void InitializeUserDataDir();
On 2014/02/21 23:11:33, Vitaly Buka wrote:
> How about adding bool IsUserDataDirInitialized() initialized
> 
> and DCHECK(IsUserDataDirInitialized)in 
> 
> chrome_paths.cc:158
> 
> switch (key) {
>     case chrome::DIR_USER_DATA:
>       DCHECK(IsUserDataDirInitialized());
>       if (!GetDefaultUserDataDirectory(&cur)) {

Sadly, I intended this functionality to live in chrome_paths.cc altogether, but
that's part of the common_constants target, which this code depends upon. So
this code can't live there or be called from there without creating a circular
dependency. Your idea could be implemented by setting a flag in chrome_paths.cc
from InitializeUserDataDir, so I've done that, please take a look.

[msw, 2014-02-22 00:15:03]

Please take a look; the latest patch set should work.

[Vitaly Buka (NO REVIEWS), 2014-02-22 00:27:08]

lgtm with CHECK->DCHECK

https://codereview.chromium.org/174253002/diff/660001/chrome/common/chrome_pa...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/174253002/diff/660001/chrome/common/chrome_pa...
chrome/common/chrome_paths.cc:164: CHECK(g_user_data_dir_initialized);
DCHECK is safer here. We need to merge this patch into beta. We are going to
have problems if we have old code that works fine now.

I don't mind to set this as CHECK in separate CL without merging into beta.

[Vitaly Buka (NO REVIEWS), 2014-02-22 00:28:00]

On 2014/02/22 00:27:08, Vitaly Buka wrote:
> lgtm with CHECK->DCHECK
> 
>
https://codereview.chromium.org/174253002/diff/660001/chrome/common/chrome_pa...
> File chrome/common/chrome_paths.cc (right):
> 
>
https://codereview.chromium.org/174253002/diff/660001/chrome/common/chrome_pa...
> chrome/common/chrome_paths.cc:164: CHECK(g_user_data_dir_initialized);
> DCHECK is safer here. We need to merge this patch into beta. We are going to
> have problems if we have old code that works fine now.

if we have old code that works fine now, but actually calls this before
initialization

> 
> I don't mind to set this as CHECK in separate CL without merging into beta.

[msw, 2014-02-22 00:42:07]

https://codereview.chromium.org/174253002/diff/660001/chrome/common/chrome_pa...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/174253002/diff/660001/chrome/common/chrome_pa...
chrome/common/chrome_paths.cc:164: CHECK(g_user_data_dir_initialized);
On 2014/02/22 00:27:09, Vitaly Buka wrote:
> DCHECK is safer here. We need to merge this patch into beta. We are going to
> have problems if we have old code that works fine now.
> 
> I don't mind to set this as CHECK in separate CL without merging into beta.

Done.

[sky, 2014-02-24 14:26:06]

LGTM

[msw, 2014-02-24 15:14:07]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-24 15:14:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/930001

[commit-bot: I haz the power, 2014-02-24 20:19:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/930001

[msw, 2014-02-24 21:44:56]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-24 21:46:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 00:10:24]

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

[commit-bot: I haz the power, 2014-02-25 00:10:25]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[msw, 2014-02-25 00:10:51]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-25 00:15:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 01:46:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 05:06:35]

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

[commit-bot: I haz the power, 2014-02-25 05:06:35]

Try jobs failed on following builders: mac_chromium_rel

[Vitaly Buka (NO REVIEWS), 2014-02-25 05:22:23]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-02-25 05:26:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 05:29:59]

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

[commit-bot: I haz the power, 2014-02-25 05:29:59]

Try jobs failed on following builders: mac_chromium_rel

[msw, 2014-02-25 06:15:58]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-25 06:46:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 06:54:46]

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

[commit-bot: I haz the power, 2014-02-25 06:54:47]

Try jobs failed on following builders: mac_chromium_rel

[msw, 2014-02-25 16:09:40]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-25 16:10:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 16:11:22]

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

[commit-bot: I haz the power, 2014-02-25 16:11:23]

Try jobs failed on following builders: mac_chromium_rel

[Vitaly Buka (NO REVIEWS), 2014-02-25 19:29:37]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-02-25 19:29:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 19:32:43]

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

[commit-bot: I haz the power, 2014-02-25 19:32:44]

Try jobs failed on following builders: mac_chromium_rel

[Vitaly Buka (NO REVIEWS), 2014-02-25 19:34:06]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-02-25 19:35:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 19:37:26]

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

[commit-bot: I haz the power, 2014-02-25 19:37:27]

Try jobs failed on following builders: mac_chromium_rel

[Vitaly Buka (NO REVIEWS), 2014-02-25 22:46:58]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-02-25 22:57:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[commit-bot: I haz the power, 2014-02-25 23:53:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160001

[Vitaly Buka (NO REVIEWS), 2014-02-26 00:09:55]

On 2014/02/25 23:53:30, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/msw%40chromium.org/174253002/1160001

Because of CQ issues, I'll set NOTRY and commit this. It should be safe. All
executed try-jobs are good.

[Vitaly Buka (NO REVIEWS), 2014-02-26 00:10:15]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-02-26 00:10:23]

The CQ bit was checked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-02-26 01:45:25]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-02-26 01:50:23]

there is a real linking error

https://codereview.chromium.org/174253002/diff/1160001/chrome/common/chrome_p...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/174253002/diff/1160001/chrome/common/chrome_p...
chrome/common/chrome_paths.cc:98: static base::LazyInstance<base::FilePath>
Needs 
'../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',

[msw, 2014-02-26 20:34:27]

https://codereview.chromium.org/174253002/diff/1160001/chrome/common/chrome_p...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/174253002/diff/1160001/chrome/common/chrome_p...
chrome/common/chrome_paths.cc:98: static base::LazyInstance<base::FilePath>
On 2014/02/26 01:50:24, Vitaly Buka wrote:
> Needs 
>
'../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',

src/base/base.gyp already lists 'dynamic_annotations' as a dependency for the
'base' target.:
https://code.google.com/p/chromium/codesearch#chromium/src/base/base.gyp&l=28

I suppose the base dependency isn't transitively applied for the
chrome/common_constants.gyp 'common_constants' target, which does list
'../base/base.gyp:base' as a dependency. I've added 'dynamic_annotations' as an
explicit dependency there, hopefully this'll actually work now!

[msw, 2014-02-26 20:34:33]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-26 20:37:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1180001

[commit-bot: I haz the power, 2014-02-26 22:56:42]

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

[commit-bot: I haz the power, 2014-02-26 22:56:43]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[msw, 2014-02-26 23:16:44]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-26 23:19:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1160002

[commit-bot: I haz the power, 2014-02-27 02:34:25]

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

[commit-bot: I haz the power, 2014-02-27 02:34:26]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[msw, 2014-02-27 02:41:35]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-27 02:44:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1210001

[commit-bot: I haz the power, 2014-02-27 08:26:42]

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

[commit-bot: I haz the power, 2014-02-27 08:26:43]

Try jobs failed on following builders: mac_chromium_rel

[Vitaly Buka (NO REVIEWS), 2014-02-27 08:27:47]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-02-27 08:28:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/174253002/1210001

[commit-bot: I haz the power, 2014-02-27 14:20:00]

Change committed as 253803