UI test for browser termination with beforeunload handler.

UI test for browser termination with beforeunload handler and some cleanup.

This test will pass only after http://codereview.chromium.org/8262009 lands.
* Rename CloseAllBrowsersAndExitPosted to ExitPosted.
* Add VerifyCleanExit to UITest, code which existed in BrowserTest.SessionEnd.
* Move ui tests in browser_uitest.cc to non-anonymous namespace; the test names need to be unique.
* Create UnloadTest.BrowserTerminateBeforeUnload which sets up onbeforeunload handler and sends a SIGTERM.

BUG=chromium-os:21961
TEST=UnloadTest.BrowserTerminateBeforeUnload passes.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107163

[achuithb, 2011-10-23 00:47:14]

Oshima-san, please take a look.

I'm not happy with the changes to pref_names.* and browser_list.cc. I'm setting
a preference upon observing the app terminating notification and checking the
value of this preference in the ui_test code. There is no way to observe this
notification from the ui test, since it is outside the browser process, and
there are no hooks in the automation controller/browser proxy to make this
possible.

There is also no way to run this kind of test within a browser test. It has to
be done with a ui test. 

Any other suggestions on how to test that the notification is only sent once are
welcome.

I think it's actually better to not test this rather than go with this hacky
approach.

I think the rest of the changes for BrowserTerminateBeforeUnload are good.

I've incorporated your CL (http://codereview.chromium.org/8262009/) to
browser_list.cc, and confirmed that the test BrowserTerminateBeforeUnload fails
without your CL, and passes with.

[achuithb, 2011-10-23 01:09:56]

Oshima-san, ignore my last comment.

I've removed the hacky test for app terminating notification. The rest of the
code changes are described in the CL description.

Thanks!

[achuithb, 2011-10-23 01:10:34]

Dave, please take a look at your convenience.

[DaveMoore, 2011-10-23 19:46:40]

lgtm. Thanks for taking this on. It'll give us better coverage on all platforms.

[achuithb, 2011-10-23 21:05:03]

Thanks for the review, Dave. We lucked out that there were existing ui tests for
unload handling. If you could also LGTM Oshima's CL
http://codereview.chromium.org/8262009. Thanks again!

[Paweł Hajdan Jr., 2011-10-24 07:32:51]

Drive-by, chrome/test/OWNERS here and you must get positive review *before*
committing.

I'm happy to see this change, just let's do a little bit more work to make it
even more effective. Changes like this are helpful to detect general flaky
problems in the browser and/or tests.

http://codereview.chromium.org/8376001/diff/9002/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/8376001/diff/9002/chrome/test/ui/ui_test.cc#ne...
chrome/test/ui/ui_test.cc:806: // Make sure the UMA metrics say we didn't crash.
How about doing that in TerminateBrowser instead? Every time we shutdown the
browser it should shutdown cleanly.

In fact, I'd like this to be done for most UI tests, unless they say they expect
some crashes or assertions to happen.

[achuithb, 2011-10-24 10:12:39]

http://codereview.chromium.org/8376001/diff/9002/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/8376001/diff/9002/chrome/test/ui/ui_test.cc#ne...
chrome/test/ui/ui_test.cc:806: // Make sure the UMA metrics say we didn't crash.
Since this hardens the tests, I expect there'll be a few failures that result
from your suggestion, pointing to bugs both in the ui tests and the shutdown
code (from my understanding, both these areas have fragile code).

I'd like to land this CL as is, and then post a separate CL that calls
VerifyCleanExit in TerminateBrowser, to reduce the risk of having this CL
reverted. WDYT?

Please note that this CL is a companion to
http://codereview.chromium.org/8262009/, and we will probably merge these 2 CLs
before landing.

[Paweł Hajdan Jr., 2011-10-24 12:32:14]

LGTM with notes.

http://codereview.chromium.org/8376001/diff/9002/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/8376001/diff/9002/chrome/test/ui/ui_test.cc#ne...
chrome/test/ui/ui_test.cc:806: // Make sure the UMA metrics say we didn't crash.
On 2011/10/24 10:12:39, achuith.bhandarkar wrote:
> Since this hardens the tests, I expect there'll be a few failures that result
> from your suggestion, pointing to bugs both in the ui tests and the shutdown
> code (from my understanding, both these areas have fragile code).
> 
> I'd like to land this CL as is, and then post a separate CL that calls
> VerifyCleanExit in TerminateBrowser, to reduce the risk of having this CL
> reverted. WDYT?
> 
> Please note that this CL is a companion to
> http://codereview.chromium.org/8262009/, and we will probably merge these 2
CLs
> before landing.

Generally sounds good, please add a TODO then. Please note that in my opinion
doing VerifyOnExit in more cases is quite important, so please do that as the
very next thing after landing. Thanks!

[oshima, 2011-10-24 16:20:39]

Awesome, LGTM.

I'll let you know when my change lands.

[achuithb, 2011-10-24 18:57:18]

Thanks for the reviews

http://codereview.chromium.org/8376001/diff/9002/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/8376001/diff/9002/chrome/test/ui/ui_test.cc#ne...
chrome/test/ui/ui_test.cc:806: // Make sure the UMA metrics say we didn't crash.
Done. I've opened http://crbug.com/101390 to track this.

[achuithb, 2011-10-25 00:31:19]

Oshima's CL has landed: http://crrev.com/106988

I've had to make another change to this patch. Oshima's CL ensures that
SIGTERM/power down results in a clean exit, but only on cros. So this test only
passes on cros. After talking with Oshima, we've decided to make this new test
cros-specific. 

Oshima is planning to make further changes to shutdown on all platforms that
will allow this test to pass everywhere.

Awaiting comments from Dave and Pawel. Thanks!

[Paweł Hajdan Jr., 2011-10-25 09:00:25]

LGTM