Show user data dialog earlier on Windows.

Show user data dialog earlier on Windows.

On Windows if the user data directory does not exist, we must show the user
data dir picker dialog prior to creating the local state to avoid a CHECK
failure. After obtaining the new dir we will restart chrome with the switch
--user-data-dir appended to the command line.

BUG=196301
TEST=CQ


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

[Mattias Nissler (ping if slow), 2013-03-22 17:48:24]

I think the idea is the right one, but I'd move the check. Also, the try jobs
didn't seem to like the change.

https://codereview.chromium.org/12662033/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12662033/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.cc:817:
PathService::Get(chrome::DIR_USER_DATA, &user_data_dir_);
I'd move the check here actually.

[hshi1, 2013-03-22 17:59:03]

How about patch set #3? Thanks!

https://codereview.chromium.org/12662033/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12662033/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.cc:817:
PathService::Get(chrome::DIR_USER_DATA, &user_data_dir_);
On 2013/03/22 17:48:24, Mattias Nissler wrote:
> I'd move the check here actually.

Done.

[Mattias Nissler (ping if slow), 2013-03-22 18:19:57]

LGTM from my side, but you'll need OWNERS approval as well.

[hshi1, 2013-03-22 18:25:19]

Sky/Ben - can one of you review this as owner? Thanks.

[sky, 2013-03-22 19:34:53]

Code like this is fragile. Can you add a test to make sure its executed at the
right time?

[hshi1, 2013-03-22 20:10:06]

Technically we can unit-test this (for windows only) by creating a mock
chrome::ShowUserDataDirDialog then call PreCreateThreadsImpl with a bogus
user_data_dir_ and verify that the mock ShowUserDataDirDialog... but aside from
the work required, it is not clear to me how useful is such a test, which
basically duplicates the behavior that this CL added.

I think this really should be manually tested on Windows with an inaccessible
default user data directory to ensure that the dialog shows up and the crash
does not occur.

On 2013/03/22 19:34:53, sky wrote:
> Code like this is fragile. Can you add a test to make sure its executed at the
> right time?

[sky, 2013-03-22 20:12:03]

manual=its going to break again and no one will know.


On Fri, Mar 22, 2013 at 1:10 PM, <hshi@chromium.org> wrote:

> Technically we can unit-test this (for windows only) by creating a mock
> chrome::ShowUserDataDirDialog then call PreCreateThreadsImpl with a bogus
> user_data_dir_ and verify that the mock ShowUserDataDirDialog... but aside
> from
> the work required, it is not clear to me how useful is such a test, which
> basically duplicates the behavior that this CL added.
>
> I think this really should be manually tested on Windows with an
> inaccessible
> default user data directory to ensure that the dialog shows up and the
> crash
> does not occur.
>
>
> On 2013/03/22 19:34:53, sky wrote:
>
>> Code like this is fragile. Can you add a test to make sure its executed
>> at the
>> right time?
>>
>
>
>
https://codereview.chromium.**org/12662033/<https://codereview.chromium.org/1...
>

[hshi1, 2013-03-22 20:23:23]

On 2013/03/22 20:12:03, sky wrote:
> manual=its going to break again and no one will know.

Ok, but before I start this adventure I want to make sure you agree on the
technical difficulties involved in writing this test just to verify that we
display the dialog when PreCreateThreadsImpl is invoked with no valid user data
dir.

Problems: (1) ChromeBrowserMainPart::PreCreateThreadsImpl() is a ~300-line long
function that has numerous dependencies on stuff we're not interested in for
this test. (2) The chrome::ShowUserDataDirDialog is a global function and at its
current shape is not mockable.

What I think would be needed is to (1) break up the logic that determines user
data dir out of ChromeBrowserMainPart::PreCreateThreadsImpl() (2) re-write
chrome::ShowUserDataDirDialog into a virtual function of an interface and then
create a mock for that interface (3) write a unit test that calls the separate
logic created in (1) to ensure that the ShowUserDataDirDialog is called in (2)

But this test is still not useful at all because (1) it just exactly duplicates
the behavior in this CL, and (2) someone can still break it by simply reordering
the code in chrome_browser_main.cc such that the logic that determines user data
dir is moved to be later than CreateLocalState. How can a test verify that
timing?

[hshi1, 2013-03-22 22:29:20]

Please review Patch Set #6 then. It is rather ugly but it is what seems to be
necessary to test the fix.

[sky, 2013-03-22 23:14:27]

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

https://codereview.chromium.org/12662033/diff/25001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:358: base::FilePath GetUserDataDir(const
content::MainFunctionParams& parameters) {
Move all of this into it its own file, maybe named user_data_dir_extractor.
Create two implementations, one for windows that has what you have here and the
other for the non-windows that is just the CHECK below. Then move the
user_data_dialog code into user_data_dir_extractor_win. Additionally create
user_data_dir_extractor_win.h that has the setter for the function.

https://codereview.chromium.org/12662033/diff/25001/chrome/browser/ui/views/u...
File chrome/browser/ui/views/user_data_dir_dialog_view.cc (right):

https://codereview.chromium.org/12662033/diff/25001/chrome/browser/ui/views/u...
chrome/browser/ui/views/user_data_dir_dialog_view.cc:106: base::Closure
g_custom_callback_for_test;
Constants like this can not be objects.

[hshi1, 2013-03-22 23:51:04]

CL updated, please review again. Thanks!

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

https://codereview.chromium.org/12662033/diff/25001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:358: base::FilePath GetUserDataDir(const
content::MainFunctionParams& parameters) {
On 2013/03/22 23:14:27, sky wrote:
> Move all of this into it its own file, maybe named user_data_dir_extractor.
> Create two implementations, one for windows that has what you have here and
the
> other for the non-windows that is just the CHECK below. Then move the
> user_data_dialog code into user_data_dir_extractor_win. Additionally create
> user_data_dir_extractor_win.h that has the setter for the function.

Done.

https://codereview.chromium.org/12662033/diff/25001/chrome/browser/ui/views/u...
File chrome/browser/ui/views/user_data_dir_dialog_view.cc (right):

https://codereview.chromium.org/12662033/diff/25001/chrome/browser/ui/views/u...
chrome/browser/ui/views/user_data_dir_dialog_view.cc:106: base::Closure
g_custom_callback_for_test;
On 2013/03/22 23:14:27, sky wrote:
> Constants like this can not be objects.

Done. (Made it a pointer, and moved to extractor_win.cc).

[sky, 2013-03-25 15:43:14]

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
File chrome/browser/chrome_main_browsertest.cc (right):

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:147: class ChromeMainUserDataDirTest :
public InProcessBrowserTest {
Move this to user_data_dir_extractor_win_browsertest, or
chrome_main_browsertest_win.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:149: ChromeMainUserDataDirTest():
callback_invoked_(false) {}
nit: space between () and :

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:151: bool is_callback_invoked() {
return callback_invoked_; }
field should either be named is_callback_invoked_ to match.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:152: void ShowUserDataDirDialog() {
Move to private.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:159: base::FilePath user_data_dir_;
fields at end of section.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:182: };
private:: DISALLOW...

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
File chrome/browser/user_data_dir_extractor_win.cc (right):

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor_win.cc:5: #include
"chrome/browser/user_data_dir_extractor.h"
user_data_dir_extractor should be moved below with other includes.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor_win.cc:21: base::Closure*
g_custom_show_user_data_dir_dialog_callback = NULL;
This isn't a global.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor_win.cc:42: if
(g_custom_show_user_data_dir_dialog_callback) {
If this is set then invoke it first thing and early return. This way your test
can have whatever assertions it needs and doesn't depend on anything in this
function.

[hshi1, 2013-03-25 17:18:55]

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
File chrome/browser/chrome_main_browsertest.cc (right):

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:147: class ChromeMainUserDataDirTest :
public InProcessBrowserTest {
On 2013/03/25 15:43:14, sky wrote:
> Move this to user_data_dir_extractor_win_browsertest, or
> chrome_main_browsertest_win.

Done.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:151: bool is_callback_invoked() {
return callback_invoked_; }
On 2013/03/25 15:43:14, sky wrote:
> field should either be named is_callback_invoked_ to match.

Done.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:152: void ShowUserDataDirDialog() {
On 2013/03/25 15:43:14, sky wrote:
> Move to private.

Done.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:159: base::FilePath user_data_dir_;
On 2013/03/25 15:43:14, sky wrote:
> fields at end of section.

Done.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/chrome_mai...
chrome/browser/chrome_main_browsertest.cc:182: };
On 2013/03/25 15:43:14, sky wrote:
> private:: DISALLOW...

Done.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
File chrome/browser/user_data_dir_extractor_win.cc (right):

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor_win.cc:5: #include
"chrome/browser/user_data_dir_extractor.h"
On 2013/03/25 15:43:14, sky wrote:
> user_data_dir_extractor should be moved below with other includes.

Done.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor_win.cc:21: base::Closure*
g_custom_show_user_data_dir_dialog_callback = NULL;
On 2013/03/25 15:43:14, sky wrote:
> This isn't a global.

Done.

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor_win.cc:42: if
(g_custom_show_user_data_dir_dialog_callback) {
On 2013/03/25 15:43:14, sky wrote:
> If this is set then invoke it first thing and early return. This way your test
> can have whatever assertions it needs and doesn't depend on anything in this
> function.

Actually it wasn't my intention to make the callback for every GetUserDataDir
call. The callback should only be invoked when we attempt to show user data dir
dialog (in fact my previous patch set had the check in the user data dialog
view, but as you suggested I've moved it here.) If the user data dir already
exists then there won't be a dialog to show.

[sky, 2013-03-25 19:19:04]

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
File chrome/browser/user_data_dir_extractor_win.cc (right):

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor_win.cc:42: if
(g_custom_show_user_data_dir_dialog_callback) {
On 2013/03/25 17:18:55, hshi1 wrote:
> On 2013/03/25 15:43:14, sky wrote:
> > If this is set then invoke it first thing and early return. This way your
test
> > can have whatever assertions it needs and doesn't depend on anything in this
> > function.
> 
> Actually it wasn't my intention to make the callback for every GetUserDataDir
> call. The callback should only be invoked when we attempt to show user data
dir
> dialog (in fact my previous patch set had the check in the user data dialog
> view, but as you suggested I've moved it here.) If the user data dir already
> exists then there won't be a dialog to show.

This logic should be in the specified function and it should set the state
appropriately so that you can do your asserts.

[hshi1, 2013-03-25 19:31:15]

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
File chrome/browser/user_data_dir_extractor_win.cc (right):

https://codereview.chromium.org/12662033/diff/54001/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor_win.cc:42: if
(g_custom_show_user_data_dir_dialog_callback) {
On 2013/03/25 19:19:04, sky wrote:
> On 2013/03/25 17:18:55, hshi1 wrote:
> > On 2013/03/25 15:43:14, sky wrote:
> > > If this is set then invoke it first thing and early return. This way your
> test
> > > can have whatever assertions it needs and doesn't depend on anything in
this
> > > function.
> > 
> > Actually it wasn't my intention to make the callback for every
GetUserDataDir
> > call. The callback should only be invoked when we attempt to show user data
> dir
> > dialog (in fact my previous patch set had the check in the user data dialog
> > view, but as you suggested I've moved it here.) If the user data dir already
> > exists then there won't be a dialog to show.
> 
> This logic should be in the specified function and it should set the state
> appropriately so that you can do your asserts.

Understood, Done.

[sky, 2013-03-25 21:43:20]

LGTM with the following change.

https://codereview.chromium.org/12662033/diff/79004/chrome/browser/user_data_...
File chrome/browser/user_data_dir_extractor.cc (right):

https://codereview.chromium.org/12662033/diff/79004/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor.cc:14: #if !defined(OS_WIN)
Nuke this and instead update the gyp dependencies so this is only compiled on
non-win.

[hshi1, 2013-03-25 22:10:14]

https://codereview.chromium.org/12662033/diff/79004/chrome/browser/user_data_...
File chrome/browser/user_data_dir_extractor.cc (right):

https://codereview.chromium.org/12662033/diff/79004/chrome/browser/user_data_...
chrome/browser/user_data_dir_extractor.cc:14: #if !defined(OS_WIN)
On 2013/03/25 21:43:20, sky wrote:
> Nuke this and instead update the gyp dependencies so this is only compiled on
> non-win.

Done.

[commit-bot: I haz the power, 2013-03-26 00:06:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/12662033/93004

[hshi1, 2013-03-26 03:04:09]

On 2013/03/26 00:06:51, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/hshi%40chromium.org/12662033/93004

sky@ - unfortunately the browser test does not pass the trybots. The problem is
the following: BrowserTestBase::SetUp() runs the BrowserMain, so any setup such
as setting up the bogus user data dir must occur before that.

On the other hand, I can't get the current user data dir and override it with
the bogus one until InProcessBrowserTest::SetUp() is complete.

Is there any way to insert my custom SetUp() logic after
InProcessBrowserTest::SetUp() but before BrowserTestBase::SetUp()? Thanks.

[commit-bot: I haz the power, 2013-03-26 04:03:53]

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

[sky, 2013-03-26 14:33:49]

Off the top of my head I don't know. You'll have to trace through the code.
Worse case you could add a new protected virtual method call from
InProcessBrowserTest or BrowserTestBase.


On Mon, Mar 25, 2013 at 8:04 PM, <hshi@chromium.org> wrote:

> On 2013/03/26 00:06:51, I haz the power (commit-bot) wrote:
>
>> CQ is trying da patch. Follow status at
>> https://chromium-status.**appspot.com/cq/hshi%**
>>
40chromium.org/12662033/93004<https://chromium-status.appspot.com/cq/hshi%40chromium.org/12662033/93004>
>>
>
> sky@ - unfortunately the browser test does not pass the trybots. The
> problem is
> the following: BrowserTestBase::SetUp() runs the BrowserMain, so any setup
> such
> as setting up the bogus user data dir must occur before that.
>
> On the other hand, I can't get the current user data dir and override it
> with
> the bogus one until InProcessBrowserTest::SetUp() is complete.
>
> Is there any way to insert my custom SetUp() logic after
> InProcessBrowserTest::SetUp() but before BrowserTestBase::SetUp()? Thanks.
>
>
https://codereview.chromium.**org/12662033/<https://codereview.chromium.org/1...
>

[hshi1, 2013-03-26 17:30:36]

sky@ - can you review the latest patch set? I have decided to not pursue a
private method in InProcessBrowserTest to set the bogus user data dir; instead,
I'll just modify the test to verify the timing that GetUserDataDir happens
before CreateLocalState. Thanks.

[commit-bot: I haz the power, 2013-03-26 20:36:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/12662033/101004

[sky, 2013-03-26 20:50:48]

https://codereview.chromium.org/12662033/diff/101004/chrome/test/base/testing...
File chrome/test/base/testing_browser_process.h (right):

https://codereview.chromium.org/12662033/diff/101004/chrome/test/base/testing...
chrome/test/base/testing_browser_process.h:114: virtual bool
created_local_state() const OVERRIDE { return false; }
Shouldn't this return true if local_state_ is non-NULL?

[hshi1, 2013-03-26 20:54:11]

https://codereview.chromium.org/12662033/diff/101004/chrome/test/base/testing...
File chrome/test/base/testing_browser_process.h (right):

https://codereview.chromium.org/12662033/diff/101004/chrome/test/base/testing...
chrome/test/base/testing_browser_process.h:114: virtual bool
created_local_state() const OVERRIDE { return false; }
On 2013/03/26 20:50:48, sky wrote:
> Shouldn't this return true if local_state_ is non-NULL?

Done.

[hshi1, 2013-03-26 21:25:40]

Sorry, but this test doesn't seem to work. The problem is that during
InProcessBrowserTest::SetUp(), the global point g_browser_process is set to NULL
(see code at the beginning of SetUp()):

  // Undo TestingBrowserProcess creation in ChromeTestSuite.
  // TODO(phajdan.jr): Extract a smaller test suite so we don't need this.
  DCHECK(g_browser_process);
  delete g_browser_process;
  g_browser_process = NULL;

As a result we always hit SIGSEGV in the callback.

[hshi1, 2013-03-26 21:38:21]

The g_browser_process is created in ChromeBrowserMainParts::PreCreateThreads(),
after GetUserDataDir(), but before initialize LocalState. Thus it is impossible
to access g_browser_process->created_local_state() at the GetUserDataDir
callback because the local state does not exist.

I've updated the patch yet again (see Patch #22) where if the g_browser_process
does not exist then we treat it as a case where local state is not created
before GetUserDataDir() -- this is obviously true.

[commit-bot: I haz the power, 2013-03-26 21:49:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/12662033/117007

[commit-bot: I haz the power, 2013-03-27 06:44:46]

Change committed as 190833