Fixed shutdown of LoginUtilsTest.

Fixed shutdown of LoginUtilsTest.

The shutdown of ProfileIOData started triggering a DCHECK on chrome_http_user_agent_settings.cc. Fixed this and similar issues by having a fake IO loop always available during the test.

BUG=158839
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167397

[Joao da Silva, 2012-11-12 10:01:07]

@Nikita: friendly ping :-)

[Nikita (slow), 2012-11-13 10:11:33]

lgtm

http://codereview.chromium.org/11358160/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/11358160/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/login_utils_browsertest.cc:105:
completion->Wait();
Is this a recommended way to handle fake IO look like that?
What do we do in other tests?

[Joao da Silva, 2012-11-13 13:11:03]

http://codereview.chromium.org/11358160/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/11358160/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/login_utils_browsertest.cc:105:
completion->Wait();
On 2012/11/13 10:11:33, Nikita Kostylev wrote:
> Is this a recommended way to handle fake IO look like that?
> What do we do in other tests?

This is not common at all, as you suspected.

Other unit_tests usually have a single loop that handles all UI/IO/FILE tasks;
other browser_tests usually have a thread per loop and they all execute their
tasks as well.

The peculiarity of this test is that it creates ProfileImpls in a unit_test
(notice the main test class derives from testing::Test, not
InProcessBrowserTests; that was to make sure this test executes in its own
process, since it modifies a lot of static stuff). However, ProfileIOData
creates a ton of stuff on the IO loop that this test can't handle because the
whole BrowserProcess stuff isn't there. This causes all sorts of crashes in
various places, especially at shutdown (that's why the test has been disabled).

So this function is the one single task that will ever execute on IO. This means
tasks can still be posted to IO (many places CHECK that, so we must have a IO
loop), but they can't really ever execute (because they will crash).

The completion event and work callback are used to execute selected tasks on IO,
and to wait for shutdown. This is used to invoke TearDownOnIO, since some of the
code it executes CHECKS that the current thread is IO.

Short answer: this test is a mess :-) But it's the only thing testing LoginUtils
right now. The Right Thing To Do would be to rewrite this as a browser_test
(which has with its own challenges...)

[commit-bot: I haz the power, 2012-11-13 13:11:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/11358160/8001

[commit-bot: I haz the power, 2012-11-13 13:11:22]

Presubmit check for 11358160-8001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[Joao da Silva, 2012-11-13 13:12:11]

@jochen: I kindly invoke your new-found powers to land this :-)

[Nikita (slow), 2012-11-13 13:21:22]

Trivial *.gypi changes could be landed as TBR.

[jochen (gone - plz use gerrit), 2012-11-13 13:25:49]

https://chromiumcodereview.appspot.com/11358160/diff/8001/chrome/chrome_tests...
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/11358160/diff/8001/chrome/chrome_tests...
chrome/chrome_tests.gypi:783:
'browser/chromeos/login/login_utils_browsertest.cc',
why was this entry missing?

[jochen (gone - plz use gerrit), 2012-11-13 13:26:59]

Joao tells that this was how the test was "disabled".... zomg

lgtm rubebrstamp

[Joao da Silva, 2012-11-13 13:27:41]

For reference, here's the disabling CL:
https://chromiumcodereview.appspot.com/11358028

[commit-bot: I haz the power, 2012-11-13 13:27:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/11358160/8001

[commit-bot: I haz the power, 2012-11-13 14:31:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/11358160/8001

[commit-bot: I haz the power, 2012-11-13 16:34:50]

Change committed as 167397