The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6072cbbb4bbf076716833b07db995302d4cb87ff

commit 6072cbbb4bbf076716833b07db995302d4cb87ff
Author: Fernando Serboncini <fserb@chromium.org>
Date: Fri Aug 11 13:55:06 2017

Disable TestLaunchSchemeUnknownChromeDefault due to flakiness

TBR=benwells

Bug:  754703 
Change-Id: Ie73f643c776237a9235fdd636989a56f4ff85d29
Reviewed-on: https://chromium-review.googlesource.com/612320
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493716}
[modify] https://crrev.com/6072cbbb4bbf076716833b07db995302d4cb87ff/chrome/browser/external_protocol/external_protocol_handler_unittest.cc

Are you confident about the suspect CLs? Why not revert the suspect CLs rather than disable the test?

I'm not confident about the suspect CLs.

https://chromium-review.googlesource.com/c/585732 is a no-op

I suspect https://codereview.chromium.org/2888693003 (pmonette@) may have highlighted an existing race [1]

Either way, seems like the problem is likely that this test assumed synchronicity of something that didn't have to be. With the TaskScheduler migration some of these come out.

Disabling the test isn't great IMO. It should at least be P1 and Assigned. Unlooked at disabled tests are an issue.

[1] causing errors like 

[ RUN      ] ExternalProtocolHandlerTest.TestLaunchSchemeUnknownChromeDefault
../../chrome/browser/external_protocol/external_protocol_handler_unittest.cc:148: Failure
      Expected: should_block
      Which is: true
To be equal to: delegate_.has_blocked()
      Which is: false

in tests that depend on shell_integration's reaction.

I agree there is probably a pre-existing race, which is now more obvious.

It's surprising this test - and the whole external handler system - is enabled on Android.

Matt - do you think it is needed on Android? It seems to be deliberately be left in for non-main-frame navigations [1] but I'd be surprised if the dialog etc. were hooked up. The shell integration stuff is mostly NOTIMPLEMENTED on Android.

[1] https://cs.chromium.org/chromium/src/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc?l=680

Looking. Note: I'm just wandering into protocol handler code for the first time; certainly not experienced with this code at all.

Probably not needed on Android but I also don't know why it would only be flaky on Android. Probably is just exacerbated on Android.

Right now, the test TestLaunchSchemeUnknownChromeDefault is disabled everywhere. Should we re-enable it everywhere but Android and see if it's flaky elsewhere? I don't see any failures outside of Android on flakiness dashboard.

Also we are seeing the failure (in those logs) on all TestLaunch* tests, not just TestLaunchSchemeUnknownChromeDefault.

The specific failures I can see (across all three logs):

TestLaunchSchemeUnBlockedChromeNotDefault (has_launched is false)
TestLaunchSchemeUnBlockedChromeUnknown (has_launched is false)
TestLaunchSchemeUnknownChromeDefault (has_blocked is false)
TestLaunchSchemeUnknownChromeNotDefault (has_prompted is false)
TestLaunchSchemeUnknownChromeUnknown (has_prompted is false)

So there are 9 tests. The TestLaunchSchemeBlocked* tests are synchronous and they don't seem to fail. There are 6 async tests and 5 are failing (don't know why TestLaunchSchemeUnBlockedChromeDefault is not failing).

I can only assume that the content::RunAllBlockingPoolTasksUntilIdle() call is not actually blocking until the async call in LaunchUrlWithDelegate is actually completed. I have no idea how this stuff works. gab@, can you take a quick look and see if there's anything obviously wrong with this call?

As I said above, we can disable this on Android but it's going to take me some time to get acquainted with this code to do anything more serious.

Could be related to https://chromium-review.googlesource.com/c/602470, can you try on top of that CL which addresses shutdown (i.e. RunUntilIdle() AFAIU) issues on Android.

So it's kind of tricky to work out exactly what's going on with RunAllBlockingPoolTasksUntilIdle(), but it can't be related to message_pump_android because if base::RunLoop().RunUntilIdle() were calling into message_pump_android it would crash immediately after hitting NOTREACHED().

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a5dabed4d51c8665fe8280474e65a708fbfc309

commit 5a5dabed4d51c8665fe8280474e65a708fbfc309
Author: Matt Giuca <mgiuca@chromium.org>
Date: Fri Aug 18 01:04:55 2017

ExternalProtocolHandlerTest: Small cleanup.

Refactor to make it obvious what's expected in each test.

Change ASSERT to EXPECT throughout. There's no reason to assert here. It
just masks other failures.

Bug:  754703 
Change-Id: Iff21cef8cc789ac358710cda4a0bb7cd34605a64
Reviewed-on: https://chromium-review.googlesource.com/618536
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495408}
[modify] https://crrev.com/5a5dabed4d51c8665fe8280474e65a708fbfc309/chrome/browser/external_protocol/external_protocol_handler_unittest.cc

Seems to have been fixed in r507898 by benwells