[Mac] Re-enable AppShimInteractiveTest.Launch.

[Mac] Re-enable AppShimInteractiveTest.Launch.

The stack trace suggests LSOpenApplication fails to find the shim, and this
happens after waiting on a run loop. So a likely cause is that we're not
successfully waiting for the file operations to finish.

This CL changes the test to observe the parent directory instead of the shim
directory, and adds checks that the path exists before proceeding. If the test
still fails for this reason, at least it'll say so.

BUG=415422
Committed: https://crrev.com/4158cb9ac70b5f3b4b170e4bb89697bf01f00487
Cr-Commit-Position: refs/heads/master@{#296185}

[jackhou1, 2014-09-23 05:55:20]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2014-09-23 05:55:21]

Couldn't repro flakes on my machine, so not entirely sure why they're happening.

The stack trace suggests LSOpenApplication fails to find the shim, and this
happens after waiting on a run loop. So my suspicion is that we're not
successfully waiting for the file operations to finish.

I've changed it to observe the parent directory instead of the shim directory,
and added checks that the path exists before proceeding. If the test still fails
for this reason, at least it'll say so.

Example stack trace:

[3239:26883:0918/153744:ERROR:launch_services_util.cc(58)] LSOpenApplication:
procNotFound (-600)
BrowserTestBase signal handler received SIGTERM. Backtrace:
0   interactive_ui_tests                0x0000000111adbb93
base::debug::StackTrace::StackTrace() + 19
1   interactive_ui_tests                0x000000011c503459 content::(anonymous
namespace)::DumpStackTraceSignalHandler(int) + 185
2   libsystem_c.dylib                   0x00007fff82e8590a _sigtramp + 26
3   ???                                 0x000060e000009dc0 0x0 + 106515188981184
4   CoreFoundation                      0x00007fff8310c233
__CFRunLoopServiceMachPort + 195
5   CoreFoundation                      0x00007fff83111916 __CFRunLoopRun + 1078
6   CoreFoundation                      0x00007fff831110e2 CFRunLoopRunSpecific
+ 290
7   HIToolbox                           0x00007fff8babeeb4
RunCurrentEventLoopInMode + 209
8   HIToolbox                           0x00007fff8babec52
ReceiveNextEventCommon + 356
9   HIToolbox                           0x00007fff8babeae3
BlockUntilNextEventMatchingListInMode + 62
10  AppKit                              0x00007fff8922c533 _DPSNextEvent + 685
11  AppKit                              0x00007fff8922bdf2 -[NSApplication
nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
12  AppKit                              0x00007fff892231a3 -[NSApplication run]
+ 517
13  interactive_ui_tests                0x0000000111aa45c6
base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 886
14  interactive_ui_tests                0x0000000111aa2f9a
base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 474
15  interactive_ui_tests                0x0000000111bab0e2 base::RunLoop::Run()
+ 530
16  interactive_ui_tests                0x000000010f170ef8
apps::AppShimInteractiveTest_Launch_Test::RunTestOnMainThread() + 12296
17  interactive_ui_tests                0x00000001106b623a
InProcessBrowserTest::RunTestOnMainThreadLoop() + 810
18  interactive_ui_tests                0x000000011c502c23
content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() + 995
19  interactive_ui_tests                0x000000010f7396d4
ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 12132
20  interactive_ui_tests                0x000000010f736388
ChromeBrowserMainParts::PreMainMessageLoopRun() + 296
21  interactive_ui_tests                0x000000011962d547
content::BrowserMainLoop::PreMainMessageLoopRun() + 375
22  interactive_ui_tests                0x0000000119d5c1f6
content::StartupTaskRunner::RunAllTasksNow() + 294
23  interactive_ui_tests                0x0000000119628d5d
content::BrowserMainLoop::CreateStartupTasks() + 1901
24  interactive_ui_tests                0x00000001196343f8
content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) +
1000
25  interactive_ui_tests                0x0000000119622584
content::BrowserMain(content::MainFunctionParams const&) + 404
26  interactive_ui_tests                0x000000011c5000fc
content::ContentMainRunnerImpl::Run() + 476
27  interactive_ui_tests                0x000000011c4fe21e
content::ContentMain(content::ContentMainParams const&) + 142
28  interactive_ui_tests                0x000000011c502027
content::BrowserTestBase::SetUp() + 1799
29  interactive_ui_tests                0x00000001106b3ca9
InProcessBrowserTest::SetUp() + 745
30  interactive_ui_tests                0x0000000110dbdde7 testing::Test::Run()
+ 327
31  interactive_ui_tests                0x0000000110dc0261
testing::TestInfo::Run() + 2049
32  interactive_ui_tests                0x0000000110dc0f12
testing::TestCase::Run() + 1042
33  interactive_ui_tests                0x0000000110dd5d7d
testing::internal::UnitTestImpl::RunAllTests() + 4173
34  interactive_ui_tests                0x0000000110dd4ca5
testing::UnitTest::Run() + 389
35  interactive_ui_tests                0x000000011c4fb705
base::TestSuite::Run() + 565
36  interactive_ui_tests                0x000000010f647494
InteractiveUITestSuiteRunner::RunTestSuite(int, char**) + 212
37  interactive_ui_tests                0x000000011c50d2b3
content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) + 1763
38  interactive_ui_tests                0x00000001106b15d4
LaunchChromeTests(int, ChromeTestSuiteRunner*, int, char**) + 244
39  interactive_ui_tests                0x000000010f647352 main + 194
40  interactive_ui_tests                0x000000010f16d934 start + 52

[tapted, 2014-09-23 06:28:41]

Yeah, I think the reasoning is sound. But we should keep an eye on it as much as
we can - flakes are annoying for everyone

https://codereview.chromium.org/598443002/diff/1/apps/app_shim/app_shim_inter...
File apps/app_shim/app_shim_interactive_uitest_mac.mm (right):

https://codereview.chromium.org/598443002/diff/1/apps/app_shim/app_shim_inter...
apps/app_shim/app_shim_interactive_uitest_mac.mm:76: void Watch(const
base::FilePath& path) {
I don't think the function argument here is needed any more (ie call
Watch(path_.DirName().. below). It does raise the question about whether it's
safe to set it in the UI thread and then read it immediately, here. But a read
is already happening in Observe(..), and I think it's nicer at least to
construct and destroy on the same thread rather than try to pass strings around
(they're probably reference-counted between threads anyway).

https://codereview.chromium.org/598443002/diff/1/apps/app_shim/app_shim_inter...
apps/app_shim/app_shim_interactive_uitest_mac.mm:84:
content::BrowserThread::PostTask(
watcher_->Cancel() before this? Not entirely sure how safe it is if another file
is created in path_.DirName() after this triggers.

https://codereview.chromium.org/598443002/diff/1/apps/app_shim/app_shim_inter...
apps/app_shim/app_shim_interactive_uitest_mac.mm:98: base::FilePath path_;
nit: const?

[jackhou1, 2014-09-23 07:34:07]

https://codereview.chromium.org/598443002/diff/1/apps/app_shim/app_shim_inter...
File apps/app_shim/app_shim_interactive_uitest_mac.mm (right):

https://codereview.chromium.org/598443002/diff/1/apps/app_shim/app_shim_inter...
apps/app_shim/app_shim_interactive_uitest_mac.mm:76: void Watch(const
base::FilePath& path) {
On 2014/09/23 06:28:41, tapted wrote:
> I don't think the function argument here is needed any more (ie call
> Watch(path_.DirName().. below). It does raise the question about whether it's
> safe to set it in the UI thread and then read it immediately, here. But a read
> is already happening in Observe(..), and I think it's nicer at least to
> construct and destroy on the same thread rather than try to pass strings
around
> (they're probably reference-counted between threads anyway).

Done.

https://codereview.chromium.org/598443002/diff/1/apps/app_shim/app_shim_inter...
apps/app_shim/app_shim_interactive_uitest_mac.mm:84:
content::BrowserThread::PostTask(
On 2014/09/23 06:28:41, tapted wrote:
> watcher_->Cancel() before this? Not entirely sure how safe it is if another
file
> is created in path_.DirName() after this triggers.

Done.

https://codereview.chromium.org/598443002/diff/1/apps/app_shim/app_shim_inter...
apps/app_shim/app_shim_interactive_uitest_mac.mm:98: base::FilePath path_;
On 2014/09/23 06:28:41, tapted wrote:
> nit: const?

Done.

[tapted, 2014-09-23 07:52:13]

lgtm

(I'd probably be a bit more verbose in the CL description too, but I'm crazy
like that :p. E.g. 

The stack trace suggests LSOpenApplication fails to find the shim, and this
happens after waiting on a run loop. So a likely cause is that we're not
successfully waiting for the file operations to finish.

This CL changes the test to observe the parent directory instead of the shim
directory,
and adds checks that the path exists before proceeding. If the test still fails
for this reason, at least it'll say so.

https://codereview.chromium.org/598443002/diff/20001/apps/app_shim/app_shim_i...
File apps/app_shim/app_shim_interactive_uitest_mac.mm (right):

https://codereview.chromium.org/598443002/diff/20001/apps/app_shim/app_shim_i...
apps/app_shim/app_shim_interactive_uitest_mac.mm:78:
watcher_->Watch(path_.DirName(),
optional-nit: I just noticed this returns a bool which you could EXPECT_TRUE on
:). Unlikely to be related to the flakes though, so probably fine to skip

https://codereview.chromium.org/598443002/diff/20001/apps/app_shim/app_shim_i...
apps/app_shim/app_shim_interactive_uitest_mac.mm:86: watcher_.reset();
ah - oops I thought Cancel() was on a different class. But this looks nice (yay
RefCountedThreadSafe \o/).

[jackhou1, 2014-09-23 13:59:55]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-09-23 14:27:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/598443002/20001

[commit-bot: I haz the power, 2014-09-23 14:27:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/598443002/20001

[commit-bot: I haz the power, 2014-09-23 15:05:54]

Committed patchset #2 (id:20001) as 283878cfe1bf579d507c2c47c8f5b1cc06e8507d

[commit-bot: I haz the power, 2014-09-23 15:06:42]

Patchset 2 (id:??) landed as
https://crrev.com/4158cb9ac70b5f3b4b170e4bb89697bf01f00487
Cr-Commit-Position: refs/heads/master@{#296185}