Move all callers of GetHomeDir() to PathService::Get(base::DIR_HOME).

Move all callers of GetHomeDir() to PathService::Get(base::DIR_HOME).

* Fixes GetHomeDir() for multi-profiles case on Chrome OS.
* Once user signs in on Chrome OS base::DIR_HOME is overridden with primary user homedir.
* Added content switch --homedir to pass that information to ppapi plugins since they
run in a separate process and previous base::DIR_HOME override does not apply there.

This fix doesn't require checking for --multi-profiles switch
since user_id hash is known even without it.

Note:
download_prefs.cc still uses GetHomeDir() in its DownloadPathIsDangerous() check.
// Consider downloads 'dangerous' if they go to the home directory on Linux and
// to the desktop on any platform.
In this context correct behavior is to use "real" base::GetHomeDir() and not "virtual one" base::DIR_HOME.
Since latter is remapped to some test dir in tests, in some subfolders in Chrome OS etc.

BUG=331530
TBR=vitalybuka@chromium.org

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

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

[Dmitry Polukhin, 2014-03-14 16:35:25]

https://codereview.chromium.org/200473002/diff/20001/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/200473002/diff/20001/base/file_util.h#newcode235
base/file_util.h:235: BASE_EXPORT void SetPrimaryUserIdHash(std::string
user_id_hash);
I think we can use empty user_id_hash as sign that there is no logged in user.
IMHO, SetUserLoggedIn doesn't add much value. But I would ask base OWNERS.

[Nikita (slow), 2014-03-14 16:39:52]

https://codereview.chromium.org/200473002/diff/20001/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/200473002/diff/20001/base/file_util.h#newcode235
base/file_util.h:235: BASE_EXPORT void SetPrimaryUserIdHash(std::string
user_id_hash);
On 2014/03/14 16:35:25, Dmitry Polukhin wrote:
> I think we can use empty user_id_hash as sign that there is no logged in user.
> IMHO, SetUserLoggedIn doesn't add much value. But I would ask base OWNERS.

Done.

[Nikita (slow), 2014-03-14 16:42:11]

+brettw@ for base/*

[Dmitry Polukhin, 2014-03-14 16:44:40]

lgtm

[brettw, 2014-03-14 18:22:27]

This whole situation seems really regrettable. I don't think we should be doing
this kind of wallpaper patch.

Also, how do you ensure that nobody calls GetHomeDir before you do this setting
and getting the wrong value? Even worse, PathService::Get(base::DIR_HOME) will
call through to this function and will cache the result. So even if this works
today, if somebody adds a new call to get the home dir earlier, not only will it
give an incorrect result, but will ensure that all subsequent callers get the
same incorrect result.

I didn't like hardcoding it before, and it seems that this makes the whole thing
worse. Are there any other ideas for addressing the core problem?

[Nikita (slow), 2014-03-14 18:58:27]

Brett, I would be interested in getting your suggestions.

Thing is that user home directory is defined on Chrome OS only after sign
in so this information has to be announced only after user signs in.

As for the calls before user log in we could as well return /tmp as most of
the Chrome services are not running at that point.

As for caching value via PathService it seems that we'll need to reset that
cached value after sign in.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2014-03-14 19:51:38]

It looks like PathService::Override does what you want (sets the value
to something you want, clears the cache). So maybe the best solution
is to remove base::GetHomeDir and convert the existing callers of that
to use the PathService instead.

Brett

On Fri, Mar 14, 2014 at 11:58 AM, Nikita Kostylev
<nkostylev@chromium.org> wrote:
> Brett, I would be interested in getting your suggestions.
>
> Thing is that user home directory is defined on Chrome OS only after sign in
> so this information has to be announced only after user signs in.
>
> As for the calls before user log in we could as well return /tmp as most of
> the Chrome services are not running at that point.
>
> As for caching value via PathService it seems that we'll need to reset that
> cached value after sign in.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nikita (slow), 2014-03-17 18:05:30]

Brett, please take a look.

[Dmitry Polukhin, 2014-03-17 18:28:29]

https://codereview.chromium.org/200473002/diff/110001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

https://codereview.chromium.org/200473002/diff/110001/base/file_util_posix.cc...
base/file_util_posix.cc:491: return FilePath("/tmp");
I think it is bad idea to use /tmp attacker could control /tmp. I think we
should use some readonly path. For example even root '/' should be better.

[Nikita (slow), 2014-03-18 14:15:24]

+Will, please see latest comment from Dmitry.

Do you agree that using root / as a default homedir till user is logged in is
acceptable in GetHomeDir() on Chrome OS?

[Nikita (slow), 2014-03-18 14:51:02]

Made additional changes in the way how ppapi plugin gets information about
homedir (passed as a cmd line switch).

Brett, please take a look at this approach and once its fine by you I'll add
additional OWNER reviewers.

[brettw, 2014-03-18 19:30:25]

Wait, why do we need this command line switch for the PPAPI process? Can we
install the content path service override for this process type too so we don't
have to handle the command line switch for every case?

[Nikita (slow), 2014-03-18 21:04:08]

When this ppapi process starts since it is not the usual main Chrome
browser process it doesn't have information about the user (user_id hash)
so the only way to provide this information to this process is through
command line.

Information about the user hash is only known to main browser process, it
is asynchronously fetched from cryptohome on user session start. And the
PathService override that is defined doesn't automatically propagate to
other processes.

Please see ppapi main function which processes this switch and installs
this PathService override in ppapi plugin process.
18 марта 2014 г. 23:30 пользователь <brettw@chromium.org> написал:

> Wait, why do we need this command line switch for the PPAPI process? Can we
> install the content path service override for this process type too so we
> don't
> have to handle the command line switch for every case?
>
> https://codereview.chromium.org/200473002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2014-03-18 21:06:56]

On 2014/03/18 21:04:08, Nikita Kostylev wrote:
> When this ppapi process starts since it is not the usual main Chrome
> browser process it doesn't have information about the user (user_id hash)
> so the only way to provide this information to this process is through
> command line.
> 
> Information about the user hash is only known to main browser process, it
> is asynchronously fetched from cryptohome on user session start. And the
> PathService override that is defined doesn't automatically propagate to
> other processes.
> 
> Please see ppapi main function which processes this switch and installs
> this PathService override in ppapi plugin process.

Sorry, I don't follow which code this sentence is referring to. The closest
thing I can find is the ppapi_plugin_main.cc changes and that doesn't install
any PathService overrides.

[Nikita (slow), 2014-03-19 04:08:22]

On 2014/03/18 21:06:56, brettw wrote:
> On 2014/03/18 21:04:08, Nikita Kostylev wrote:
> > When this ppapi process starts since it is not the usual main Chrome
> > browser process it doesn't have information about the user (user_id hash)
> > so the only way to provide this information to this process is through
> > command line.
> > 
> > Information about the user hash is only known to main browser process, it
> > is asynchronously fetched from cryptohome on user session start. And the
> > PathService override that is defined doesn't automatically propagate to
> > other processes.
> > 
> > Please see ppapi main function which processes this switch and installs
> > this PathService override in ppapi plugin process.
> 
> Sorry, I don't follow which code this sentence is referring to. The closest
> thing I can find is the ppapi_plugin_main.cc changes and that doesn't install
> any PathService overrides.

Right, I've fixed that. Now it also calls PathService::Override().

[brettw, 2014-03-19 21:03:46]

It seems suspicious to me that the subprocesses should all be in charge of doing
this. Can we promote the home dir overriding to content_main_runner.cc so all
subprocesses get it?

[Nikita (slow), 2014-03-20 12:20:32]

On 2014/03/19 21:03:46, brettw wrote:
> It seems suspicious to me that the subprocesses should all be in charge of
doing
> this. Can we promote the home dir overriding to content_main_runner.cc so all
> subprocesses get it?

So I've did add code into the ContentMainRunnerImpl::Initialize()
but it doesn't quite work for ppapi plugin as PpapiPluginMain still reported
that it uses default homedir, not the one that got overriden.

I had to set override in RunZygote() method but that didn't work either as
PathService::OverrideAndCreateIfNeeded() fails on

  // For some locations this will fail if called from inside the sandbox there-
  // fore we protect this call with a flag.
  if (create) {
    // Make sure the directory exists. We need to do this before we translate
    // this to the absolute path because on POSIX, MakeAbsoluteFilePath fails
    // if called on a non-existent path.
    if (!base::PathExists(file_path) &&
        !base::CreateDirectory(file_path)) {
      LOG(ERROR) << ">>> Override NOT done 1";
      return false;
    }
  }

I've leaving PathService::Override() in both ContentMainRunnerImpl::Initialize()
and PpapiPluginMain

[Nikita (slow), 2014-03-20 12:37:32]

New code is only in content/app/content_main_runner.cc

The rest is rebase.

[Nikita (slow), 2014-03-24 06:47:31]

Brett, please take a look.

See also my previous comments on why I need code in both places:
content_main_runner.cc and ppapi_plugin_main.cc

[brettw, 2014-03-25 19:40:49]

On 2014/03/20 12:20:32, Nikita Kostylev wrote:
> On 2014/03/19 21:03:46, brettw wrote:
> > It seems suspicious to me that the subprocesses should all be in charge of
> doing
> > this. Can we promote the home dir overriding to content_main_runner.cc so
all
> > subprocesses get it?
> 
> So I've did add code into the ContentMainRunnerImpl::Initialize()
> but it doesn't quite work for ppapi plugin as PpapiPluginMain still reported
> that it uses default homedir, not the one that got overriden.
> 
> I had to set override in RunZygote() method but that didn't work either as
> PathService::OverrideAndCreateIfNeeded() fails on
> 
>   // For some locations this will fail if called from inside the sandbox
there-
>   // fore we protect this call with a flag.
>   if (create) {
>     // Make sure the directory exists. We need to do this before we translate
>     // this to the absolute path because on POSIX, MakeAbsoluteFilePath fails
>     // if called on a non-existent path.
>     if (!base::PathExists(file_path) &&
>         !base::CreateDirectory(file_path)) {
>       LOG(ERROR) << ">>> Override NOT done 1";
>       return false;
>     }
>   }
> 
> I've leaving PathService::Override() in both
ContentMainRunnerImpl::Initialize()
> and PpapiPluginMain

It looks like you can just OverrideAndCreateIfNeeded with create = false to
avoid that problem. I'm assuming its failing because the child process is
running in the sandbox and operations on the home directory will be prohibited.
So it doesn't seem alarming that this fails.

[Nikita (slow), 2014-03-26 09:54:08]

On 2014/03/25 19:40:49, brettw wrote:
> On 2014/03/20 12:20:32, Nikita Kostylev wrote:
> > On 2014/03/19 21:03:46, brettw wrote:
> > > It seems suspicious to me that the subprocesses should all be in charge of
> > doing
> > > this. Can we promote the home dir overriding to content_main_runner.cc so
> all
> > > subprocesses get it?
> > 
> > So I've did add code into the ContentMainRunnerImpl::Initialize()
> > but it doesn't quite work for ppapi plugin as PpapiPluginMain still reported
> > that it uses default homedir, not the one that got overriden.
> > 
> > I had to set override in RunZygote() method but that didn't work either as
> > PathService::OverrideAndCreateIfNeeded() fails on
> > 
> >   // For some locations this will fail if called from inside the sandbox
> there-
> >   // fore we protect this call with a flag.
> >   if (create) {
> >     // Make sure the directory exists. We need to do this before we
translate
> >     // this to the absolute path because on POSIX, MakeAbsoluteFilePath
fails
> >     // if called on a non-existent path.
> >     if (!base::PathExists(file_path) &&
> >         !base::CreateDirectory(file_path)) {
> >       LOG(ERROR) << ">>> Override NOT done 1";
> >       return false;
> >     }
> >   }
> > 
> > I've leaving PathService::Override() in both
> ContentMainRunnerImpl::Initialize()
> > and PpapiPluginMain
> 
> It looks like you can just OverrideAndCreateIfNeeded with create = false to
> avoid that problem. I'm assuming its failing because the child process is
> running in the sandbox and operations on the home directory will be
prohibited.
> So it doesn't seem alarming that this fails.

I've tried calling OverrideAndCreateIfNeeded with create = false and that
doesn't quite work because it hits following code snippet:

  // We need to have an absolute path.
  file_path = MakeAbsoluteFilePath(file_path);
  if (file_path.empty())
    return false;

Which means that override is not configured and default value "/" is used.

The change the way it is right now works i.e. override is correctly setup for
pepper plugin.

[brettw, 2014-03-26 17:12:27]

It seems like this should be a supported use-case for the path service, as
anything trying to override a path in the sandbox will fail in the same way. It
would be nice to find a way to make it work.

You always have an absolute path already for your case, right? What if we wrap
the MakeAbsolute call in an
  if (!file_path.IsAbsolute()) {
(with a comment explaining why). Does that make it useful for your case?

[Nikita (slow), 2014-03-26 17:38:51]

On 2014/03/26 17:12:27, brettw wrote:
> It seems like this should be a supported use-case for the path service, as
> anything trying to override a path in the sandbox will fail in the same way.
It
> would be nice to find a way to make it work.
> 
> You always have an absolute path already for your case, right? What if we wrap
> the MakeAbsolute call in an
>   if (!file_path.IsAbsolute()) {
> (with a comment explaining why). Does that make it useful for your case?

Yes, this should work.

[Nikita (slow), 2014-04-03 10:55:53]

On 2014/03/26 17:12:27, brettw wrote:
> It seems like this should be a supported use-case for the path service, as
> anything trying to override a path in the sandbox will fail in the same way.
It
> would be nice to find a way to make it work.
> 
> You always have an absolute path already for your case, right? What if we wrap
> the MakeAbsolute call in an
>   if (!file_path.IsAbsolute()) {
> (with a comment explaining why). Does that make it useful for your case?

Brett, I've made this change. See diff between patchsets 18..19.

Please take a look.

[Nikita (slow), 2014-04-08 14:29:22]

On 2014/04/03 10:55:53, Nikita Kostylev wrote:
> On 2014/03/26 17:12:27, brettw wrote:
> > It seems like this should be a supported use-case for the path service, as
> > anything trying to override a path in the sandbox will fail in the same way.
> It
> > would be nice to find a way to make it work.
> > 
> > You always have an absolute path already for your case, right? What if we
wrap
> > the MakeAbsolute call in an
> >   if (!file_path.IsAbsolute()) {
> > (with a comment explaining why). Does that make it useful for your case?
> 
> Brett, I've made this change. See diff between patchsets 18..19.
> 
> Please take a look.

Brett, ping.

[Nikita (slow), 2014-04-08 15:19:26]

Dmitry, can you please take another look as well?

[Will Drewry, 2014-04-08 15:55:04]

A couple of questions that likely just show my misunderstanding and one comment
on the default failure-path.

Thanks!

https://codereview.chromium.org/200473002/diff/110001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

https://codereview.chromium.org/200473002/diff/110001/base/file_util_posix.cc...
base/file_util_posix.cc:491: return FilePath("/tmp");
On 2014/03/17 18:28:30, Dmitry Polukhin wrote:
> I think it is bad idea to use /tmp attacker could control /tmp. I think we
> should use some readonly path. For example even root '/' should be better.

'/' is definitely a better choice since it isn't writable.  I'm trying to think
if there's a better alternative, but / is certainly good.  If we had "/empty"
that'd be even better, but it doesn't exist :)

https://codereview.chromium.org/200473002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/200473002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:1516:
PathService::Override(base::DIR_HOME, homedir);
Does this mean the home dir will always be the first user/primary user?  So
things like the RLZ check will only work if the "primary" account has it on,
etc?

https://codereview.chromium.org/200473002/diff/140001/content/browser/ppapi_p...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/200473002/diff/140001/content/browser/ppapi_p...
content/browser/ppapi_plugin_process_host.cc:312: // On Chrome OS need to pass
primary user homedir.
What does PPAPI use the home dir for?  What sort of side effects can it have on
the primary user data?

[Nikita (slow), 2014-04-08 16:40:08]

https://codereview.chromium.org/200473002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/200473002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:1516:
PathService::Override(base::DIR_HOME, homedir);
On 2014/04/08 15:55:04, Will Drewry wrote:
> Does this mean the home dir will always be the first user/primary user?  So
> things like the RLZ check will only work if the "primary" account has it on,
> etc? 

Correct. This is the way how it works today and has been discussed in more
details here
https://code.google.com/p/chromium/issues/detail?id=331530

https://codereview.chromium.org/200473002/diff/140001/content/browser/ppapi_p...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/200473002/diff/140001/content/browser/ppapi_p...
content/browser/ppapi_plugin_process_host.cc:312: // On Chrome OS need to pass
primary user homedir.
On 2014/04/08 15:55:04, Will Drewry wrote:
> What does PPAPI use the home dir for?  What sort of side effects can it have
on
> the primary user data?

This doesn't change how it is implemented right now since now it is using
GetHomeDir() which returns /home/chronos/user which is the primary user home
dir.

I don't know what they used it for, here's the snippet
https://code.google.com/p/chromium/codesearch#chromium/src/content/ppapi_plug...

// Specifies $HOME explicitly because some plugins rely on $HOME but
// no other part of Chrome OS uses that.  See crbug.com/335290.

https://code.google.com/p/chromium/issues/detail?id=335290 mentions
ScreenshotTaker.

[Dmitry Polukhin, 2014-04-08 21:11:19]

LGTM

[brettw, 2014-04-10 20:15:21]

LGTM for base.

I was kind of hoping GetHomeDir as a standalone function could be deleted and
inlined into the various PathService implementations. That way we wouldn't have
to worry about GetHomeDir getting called "too early" on ChromeOS.

I've taken too long on this review. If you think it's possible to delete
GetHomeDir, can we do that in a follow-up patch?

[Nikita (slow), 2014-04-11 13:03:07]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-04-11 13:03:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/430001

[Nikita (slow), 2014-04-11 13:06:34]

On 2014/04/10 20:15:21, brettw wrote:
> I've taken too long on this review. If you think it's possible to delete
> GetHomeDir, can we do that in a follow-up patch?

I'll try to investigate that later. At this point I was focusing on making sure
that Chrome OS use case gets addressed and
no other platform is affected.

However I see few FileSystemApiTest.* are failing in Linux/Mac. Those that use
"graylisted" paths.

[commit-bot: I haz the power, 2014-04-11 13:46:49]

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

[commit-bot: I haz the power, 2014-04-11 13:46:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nikita (slow), 2014-04-11 14:50:28]

OWNERS review:

cpu@ for chrome/browser/diagnostics/*

rdsmith@ for chrome/browser/download/*

jam@ for chrome/common/* and chrome/common/importer/*

wtc@ for crypto/*

vitalybuka@ for printing/*

sergeyu@ for remoting/*

rogerta@ for rlz/*

[Roger Tawa OOO till Jul 10th, 2014-04-11 15:55:54]

rlz lgtm

[cpu_(ooo_6.6-7.5), 2014-04-11 17:03:33]

lgtm

[Randy Smith (Not in Mondays), 2014-04-11 17:18:05]

lgtm

[Sergey Ulanov, 2014-04-11 17:35:50]

remoting LGTM

[jam, 2014-04-11 17:44:57]

it seems like a layering violation that content is setting something for
chromeos to support multi-profile. why can't chrome layer set this up itself,
say in ChromeContentClient?

[Nikita (slow), 2014-04-18 16:09:41]

On 2014/04/11 17:44:57, jam wrote:
> it seems like a layering violation that content is setting something for
> chromeos to support multi-profile. why can't chrome layer set this up itself,
> say in ChromeContentClient?

Can you please suggest a proper way to do this?

Looking at ChromeContentClient
(http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/chrome_content_...)
I don't quite see a place where such code may be added.

What we need on Chrome OS is override PathService::Get(base::DIR_HOME) in all
processess as well.
That's why cmd line switch is passed to content in
content/browser/ppapi_plugin_process_host.cc

Other changes in content:

content/app/content_main_runner.cc - processing this cmd line switch and
applying override.

content/ppapi_plugin/ppapi_plugin_main.cc - migrate GetHomeDir() call to
PathService::Get(base::DIR_HOME).
This change is done across all chromium codebase in this CL. Brett suggested
dropping GetHomeDir() but this is not happening yet.

content/public/common/content_switches.* - new switch

[Nikita (slow), 2014-04-18 16:12:37]

TBR'ing:

wtc@ for crypto/*
vitalybuka@ for printing/*

[jam, 2014-04-21 07:27:13]

On 2014/04/18 16:09:41, Nikita Kostylev wrote:
> On 2014/04/11 17:44:57, jam wrote:
> > it seems like a layering violation that content is setting something for
> > chromeos to support multi-profile. why can't chrome layer set this up
itself,
> > say in ChromeContentClient?
> 
> Can you please suggest a proper way to do this?
> 
> Looking at ChromeContentClient
>
(http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/chrome_content_...)
> I don't quite see a place where such code may be added.

I was thinking in the constructor of ChromeContentClient. Now that I think about
it more, another place that will work is ChromeMainDelegate.

> 
> What we need on Chrome OS is override PathService::Get(base::DIR_HOME) in all
> processess as well.

content calls ChromeMainDelegate in all processes.

> That's why cmd line switch is passed to content in
> content/browser/ppapi_plugin_process_host.cc

See ChromeContentBrowserClient::AppendExtraCommandLineSwitches where chrome
layer can add its own switches to the command line for child processes.

> 
> Other changes in content:
> 
> content/app/content_main_runner.cc - processing this cmd line switch and
> applying override.

content_main_runner.cc calls ChromeMainDelegate::BasicStartupComplete a little
above this. perhaps it can be done there? (I had mentioned ChromeContentClient
because it's constructed right before where the new code got added to
content_main_runner.cc).

> 
> content/ppapi_plugin/ppapi_plugin_main.cc - migrate GetHomeDir() call to
> PathService::Get(base::DIR_HOME).

To figure out where chrome layer can do this, we can work backwards.
PpapiPluginMain is called in ContentMainRunnerImpl::Run.
ContentMainRunnerImpl::Initialize is called before that, and it calls various
ContentMainDelegate methods. So any one of these it'll work to add the new code.

> This change is done across all chromium codebase in this CL. Brett suggested
> dropping GetHomeDir() but this is not happening yet.
> 
> content/public/common/content_switches.* - new switch

[Nikita (slow), 2014-04-21 17:18:23]

On 2014/04/21 07:27:13, jam wrote:
> I was thinking in the constructor of ChromeContentClient. Now that I think
about
> it more, another place that will work is ChromeMainDelegate.
> 
> See ChromeContentBrowserClient::AppendExtraCommandLineSwitches where chrome
> layer can add its own switches to the command line for child processes.
> 
> content_main_runner.cc calls ChromeMainDelegate::BasicStartupComplete a little
> above this. perhaps it can be done there? (I had mentioned ChromeContentClient
> because it's constructed right before where the new code got added to
> content_main_runner.cc).

Thanks, these suggestions were really helpful!

Modified code so that now 2 only changes on content are using base::DIR_HOME
instead of GetHomeDir().
See diff 25..26

Since on Chrome OS boot we now default to "/" for GetHomeDir() till user signs
in we now pass this homedir for those processes that
start early.

/opt/google/chrome/chrome-sandbox /opt/google/chrome/chrome --type=zygote
--user-data-dir=/home/chronos --homedir=/
/opt/google/chrome/chrome --type=zygote --user-data-dir=/home/chronos
--homedir=/
/opt/google/chrome/chrome --type=zygote --user-data-dir=/home/chronos
--homedir=/
/opt/google/chrome/chrome --type=gpu-process --user-data-dir=/home/chronos
--homedir=/

So they previously used /home/chronos/user which is empty till user logs in and
maps to primary user homedir.

However /home/chronos/user path constant is deprecated (essentially this CL) so
these processes will not be instructed to use "/".

The main difference is that / has root access, /home/chronos/user has chronos
user access (Chrome can r/w).
But I don't think that these processes (sandbox/gpu/zygote) would ever need to
write something there or read from there.

[jam, 2014-04-22 15:21:19]

lgtm

[Nikita (slow), 2014-04-22 15:27:24]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-04-22 15:28:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/500001

[commit-bot: I haz the power, 2014-04-22 16:47:31]

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

[commit-bot: I haz the power, 2014-04-22 16:47:31]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[wtc, 2014-04-22 22:26:54]

Patch set 26 LGTM.

https://codereview.chromium.org/200473002/diff/500001/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/200473002/diff/500001/base/file_util.h#newcod...
base/file_util.h:219: // Path service may as well override DIR_HOME.

Nit: change "as well" to "also", or put "as well" at the end.

https://codereview.chromium.org/200473002/diff/500001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/200473002/diff/500001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:30: #include "base/file_util.h"

We don't need to include "base/file_util.h" now, right?

[Nikita (slow), 2014-04-23 04:48:07]

https://codereview.chromium.org/200473002/diff/500001/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/200473002/diff/500001/base/file_util.h#newcod...
base/file_util.h:219: // Path service may as well override DIR_HOME.
On 2014/04/22 22:26:55, wtc wrote:
> 
> Nit: change "as well" to "also", or put "as well" at the end.

Done.

https://codereview.chromium.org/200473002/diff/500001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/200473002/diff/500001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:30: #include "base/file_util.h"
On 2014/04/22 22:26:55, wtc wrote:
> 
> We don't need to include "base/file_util.h" now, right?

Still uses some of the functions from it like

#92 base::CreateDirectory()

[Nikita (slow), 2014-04-29 11:16:23]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-04-29 11:16:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/540001

[commit-bot: I haz the power, 2014-04-29 11:53:10]

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

[commit-bot: I haz the power, 2014-04-29 11:53:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Nikita (slow), 2014-04-29 12:13:05]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:13:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/540001

[commit-bot: I haz the power, 2014-04-29 12:19:39]

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

[commit-bot: I haz the power, 2014-04-29 12:19:40]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[Nikita (slow), 2014-05-13 14:27:13]

+sammc@ for changes in
chrome/test/data/extensions/api_test/file_system/open_directory/test.js

Comment in test pretty much summarizes it.

My change eliminates all calls to GetHomeDir() and replaces them with
PathService::Get(base::DIR_HOME).

FileSystemApiTest.FileSystemApiOpenDirectoryOnGraylistAndAllowTest overrides
base::DIR_HOME:

PathService::OverrideAndCreateIfNeeded(kGraylistedPath, test_directory, false,
false);

and then lauches platform app test which also results in initializing crash
reports directory which in turn creates .config directory in base::DIR_HOME.

So now on POSIX systems this test receives two entries for its openDirectory
call:
.config
open_existing.txt

I think ignoring .config directory is a safe and correct way to handle this in
test.

[Nikita (slow), 2014-05-13 14:27:42]

See
https://docs.google.com/a/google.com/document/d/1WIWU3JPgTtuT5VGFwrTECUr3uRb4...
which has stack trace and some debug info.

[Sam McNally, 2014-05-13 16:53:32]

chrome/test/data/extensions/api_test/file_system/open_directory/test.js lgtm

[Nikita (slow), 2014-05-15 15:35:52]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-05-15 15:38:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/770001

[commit-bot: I haz the power, 2014-05-15 19:46:27]

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

[commit-bot: I haz the power, 2014-05-15 19:46:28]

Failed to apply patch for chromeos/chromeos_switches.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chromeos/chromeos_switches.h
  Hunk #1 FAILED at 57.
  1 out of 1 hunk FAILED -- saving rejects to file
chromeos/chromeos_switches.h.rej

Patch:       chromeos/chromeos_switches.h
Index: chromeos/chromeos_switches.h
diff --git a/chromeos/chromeos_switches.h b/chromeos/chromeos_switches.h
index
0290c2bb452a7b40cfc03da75a198e03d13557ba..626696fee9d47d024632a2a4e6039aa9d37009b3
100644
--- a/chromeos/chromeos_switches.h
+++ b/chromeos/chromeos_switches.h
@@ -57,6 +57,7 @@ CHROMEOS_EXPORT extern const char kForceLoginManagerInTests[];
 CHROMEOS_EXPORT extern const char kGuestSession[];
 CHROMEOS_EXPORT extern const char kHasChromeOSDiamondKey[];
 CHROMEOS_EXPORT extern const char kHasChromeOSKeyboard[];
+CHROMEOS_EXPORT extern const char kHomedir[];
 CHROMEOS_EXPORT extern const char kKioskModeScreensaverPath[];
 CHROMEOS_EXPORT extern const char kLoginManager[];
 CHROMEOS_EXPORT extern const char kLoginPassword[];

[Nikita (slow), 2014-05-15 20:01:55]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-05-15 20:03:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/790001

[commit-bot: I haz the power, 2014-05-16 00:20:38]

Change committed as 270872

[Nikita (slow), 2014-05-26 10:12:05]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-05-26 10:12:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/950001

[commit-bot: I haz the power, 2014-05-26 12:12:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[commit-bot: I haz the power, 2014-05-26 12:19:26]

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

[commit-bot: I haz the power, 2014-05-26 12:19:27]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[Nikita (slow), 2014-05-26 12:20:20]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-05-26 12:20:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/950001

[commit-bot: I haz the power, 2014-05-26 12:46:12]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[Nikita (slow), 2014-05-26 19:17:36]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-05-26 19:18:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/950001

[Nikita (slow), 2014-05-26 19:18:36]

The CQ bit was unchecked by nkostylev@chromium.org

[Nikita (slow), 2014-05-26 19:19:05]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-05-26 19:19:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/200473002/950001

[commit-bot: I haz the power, 2014-05-27 00:12:36]

Change committed as 272898