Fix time outs in ephemeral apps tests

Fix time outs in ephemeral apps tests

EphemeralAppBrowserTest.GarbageCollectData and similar browser tests 
have been reported to be flaky. There is a time out despite the tests 
passing.

Move up the creation of the ResultCatcher to handle the case where an
unlucky context switch after launching a test could mean that the test
fully completes before the ResultCatcher has installed observers,
causing GetNextResult() to block indefinitely.

BUG=363033
TEST=browser_tests (EphemeralAppBrowserTest.*)

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

[tmdiep, 2014-05-14 00:01:09]

This patch attempts to fix time outs in some ephemeral apps tests by moving up
the creation of the ResultCatcher. Also ensures the app window is always closed.

The logs also occasionally show some IPC or mojo channel errors when the tests
time out, but these seem to occur randomly for any test.

tapted: PTAL, thanks in advance.

[tapted, 2014-05-14 01:54:57]

lgtm

You could also say in the description ~"An unlucky context switch after
launching a test could mean that the test fully completes before the
ReplyCatcher has installed observers, causing GetNextResult() to block
indefinitely." (which I think is what's happening :)

https://codereview.chromium.org/284743004/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_browsertest.cc (right):

https://codereview.chromium.org/284743004/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_browsertest.cc:135: if (!result)
nit: this `if` probably isn't needed (i.e. do `message_ = catcher.message();`
unconditionally).

ExtensionApiTest::ResultCatcher::Observe(..) seems designed to support message()
returning an empty string on success. (theoretical benefit of this: you could
call LaunchAppAndRunTest multiple times and message_ would be correctly cleared
on success).

[tmdiep, 2014-05-14 02:08:13]

Thanks Trent. Nit addressed and updated the description to what you wrote
verbatim :)

[tmdiep, 2014-05-14 04:11:47]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-05-14 04:11:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/284743004/20001

[commit-bot: I haz the power, 2014-05-14 04:53:18]

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...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-14 04:55:13]

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

[commit-bot: I haz the power, 2014-05-14 04:55:13]

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

[tmdiep, 2014-05-14 23:26:37]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-05-14 23:29:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/284743004/20001

[commit-bot: I haz the power, 2014-05-15 01:04:01]

Change committed as 270551