Revise user-data-dir switch handling; avoid crashes.

Revise user-data-dir switch handling; avoid crashes.

Chrome needs a pre-existing or creatable user-data-dir to run.
ChromeMainDelegate::PreSandboxStartup was CHECK'ing for this.
(crashing on startup with --user-data-dir="M:\invalid")
(crashing on startup with --user-data-dir="C:\windows")

Supposedly, we had a dialog to let users choose another dir.
(I've never gotten this to work on M28-M32 stable versions)

Revise the user-data-dir code to warn on invalid paths.
(see crbug.com/318999#c18 for the warning screenshot)
Then attempt to use the default directory as a backup.
(no alternate dir selection UI, users can try again)

This should avoid many of the numerous related crashes.

BUG=318999
TEST=Supplying an invalid --user-data-dir=<path> will warn users and attempt to use the default directory instead of silently crashing.
R=sky@chromium.org

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

[msw, 2014-02-13 00:50:46]

Hey Scott, please take a look; thanks!

[sky, 2014-02-13 01:35:06]

LGTM

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

https://codereview.chromium.org/123693003/diff/270001/chrome/browser/user_dat...
chrome/browser/user_data_dir_extractor.cc:57:
ResourceBundle::CleanupSharedInstance();
I'm not sure we've ever done this outside of tests. Doing this in production
makes me slightly nervous.

[msw, 2014-02-13 01:49:33]

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

https://codereview.chromium.org/123693003/diff/270001/chrome/browser/user_dat...
chrome/browser/user_data_dir_extractor.cc:57:
ResourceBundle::CleanupSharedInstance();
On 2014/02/13 01:35:07, sky wrote:
> I'm not sure we've ever done this outside of tests. Doing this in production
> makes me slightly nervous.

Skipping this causes a DCHECK and memory leak in
ResourceBundle::InitSharedInstance when
ChromeBrowserMainParts::PreCreateThreadsImpl re-initializes the bundle. Would
you prefer this call or the DCHECK + leak (+ maybe leak suppressions?)?

https://codereview.chromium.org/123693003/diff/270001/chrome/browser/user_dat...
chrome/browser/user_data_dir_extractor.cc:57:
ResourceBundle::CleanupSharedInstance();
On 2014/02/13 01:35:07, sky wrote:
> I'm not sure we've ever done this outside of tests. Doing this in production
> makes me slightly nervous.

Seems like it's called in ChromeMainDelegate::ProcessExiting, and
remoting::UnloadResources but those are on shutdown and this is on startup, so
it might have unforeseen consequences...

Skipping this cleanup causes a DCHECK and memory leak in
ResourceBundle::InitSharedInstance when
ChromeBrowserMainParts::PreCreateThreadsImpl re-initializes the bundle. Leaving
it in doesn't seem to have any adverse effects from my local testing. Would you
prefer this call or the DCHECK + leak (+ maybe leak suppressions?)?

[msw, 2014-02-13 02:20:39]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:22:15]

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

[commit-bot: I haz the power, 2014-02-13 04:19:43]

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

[commit-bot: I haz the power, 2014-02-13 04:19:44]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[msw, 2014-02-13 15:54:42]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-02-13 15:55:01]

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

[sky, 2014-02-13 18:01:42]

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

https://codereview.chromium.org/123693003/diff/270001/chrome/browser/user_dat...
chrome/browser/user_data_dir_extractor.cc:57:
ResourceBundle::CleanupSharedInstance();
On 2014/02/13 01:49:33, msw wrote:
> On 2014/02/13 01:35:07, sky wrote:
> > I'm not sure we've ever done this outside of tests. Doing this in production
> > makes me slightly nervous.
> 
> Seems like it's called in ChromeMainDelegate::ProcessExiting, and
> remoting::UnloadResources but those are on shutdown and this is on startup, so
> it might have unforeseen consequences...
> 
> Skipping this cleanup causes a DCHECK and memory leak in
> ResourceBundle::InitSharedInstance when
> ChromeBrowserMainParts::PreCreateThreadsImpl re-initializes the bundle.
Leaving
> it in doesn't seem to have any adverse effects from my local testing. Would
you
> prefer this call or the DCHECK + leak (+ maybe leak suppressions?)?

Leave what you have in.

[commit-bot: I haz the power, 2014-02-13 19:46:27]

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

[commit-bot: I haz the power, 2014-02-13 21:28:00]

Change committed as 251126

[Vitaly Buka (NO REVIEWS), 2014-02-20 11:24:07]

https://codereview.chromium.org/123693003/diff/270001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (left):

https://codereview.chromium.org/123693003/diff/270001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:608: user_data_dir,
We need to revert this part of change or to find work around.

Service process also keeps config in user-data-dir
Non sandboxed child processes may write chrome_debug.log.

Myabe we can keep this code without CHECK.
and do CHECK() in browser process?

[Vitaly Buka (NO REVIEWS), 2014-02-20 11:40:33]

https://codereview.chromium.org/123693003/diff/270001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (left):

https://codereview.chromium.org/123693003/diff/270001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:630: logging::InitChromeLogging(command_line,
file_state);
Actually logging is brocken for all processes including browser.
We need user-data-dir here

[Vitaly Buka (NO REVIEWS), 2014-02-20 11:52:43]

On 2014/02/20 11:40:33, Vitaly Buka wrote:
>
https://codereview.chromium.org/123693003/diff/270001/chrome/app/chrome_main_...
> File chrome/app/chrome_main_delegate.cc (left):
> 
>
https://codereview.chromium.org/123693003/diff/270001/chrome/app/chrome_main_...
> chrome/app/chrome_main_delegate.cc:630:
logging::InitChromeLogging(command_line,
> file_state);
> Actually logging is brocken for all processes including browser.
> We need user-data-dir here

Looks we have to either keep initialization in chrome_main_delegate.cc or remove
all dependent stuff from chrome/common/chrome_paths.h.
Now that enum has no use for anything but browser.

[msw, 2014-02-20 18:13:49]

On 2014/02/20 11:40:33, Vitaly Buka wrote:
>Looks we have to either keep initialization in chrome_main_delegate.cc or
remove all dependent stuff from chrome/common/chrome_paths.h.
> Now that enum has no use for anything but browser.

So is the issue that the PathService doesn't initialize the value for
chrome::DIR_USER_DATA early enough for service processes and logging? If so, I
think calling chrome::GetUserDataDir from chrome_main_delegate.cc should do the
trick. If we do that, we should replace the call in
ChromeBrowserMainParts::PreCreateThreadsImpl with something that simply
retrieves the PathService value for chrome::DIR_USER_DATA, rather than
attempting to parse and override the value from the command-line again. If you
link me to the bugs with repro steps I can probably make a fix.

[msw, 2014-02-20 19:40:00]

On 2014/02/20 18:13:49, msw wrote:
> On 2014/02/20 11:40:33, Vitaly Buka wrote:
> >Looks we have to either keep initialization in chrome_main_delegate.cc or
> remove all dependent stuff from chrome/common/chrome_paths.h.
> > Now that enum has no use for anything but browser.
> 
> So is the issue that the PathService doesn't initialize the value for
> chrome::DIR_USER_DATA early enough for service processes and logging? If so, I
> think calling chrome::GetUserDataDir from chrome_main_delegate.cc should do
the
> trick. If we do that, we should replace the call in
> ChromeBrowserMainParts::PreCreateThreadsImpl with something that simply
> retrieves the PathService value for chrome::DIR_USER_DATA, rather than
> attempting to parse and override the value from the command-line again. If you
> link me to the bugs with repro steps I can probably make a fix.

Seems so; see a WIP fix at https://codereview.chromium.org/174253002/