BrowserNonClientFrameViewThemeTest.ActiveTabTextColor is flaky.

Findit has detected 3 new flake occurrences of this test. List
of all flake occurrences can be found at:
https://findit-for-me.appspot.com/flake/occurrences?key=ag9zfmZpbmRpdC1mb3ItbWVyVAsSBUZsYWtlIkljaHJvbWl1bUB1bml0X3Rlc3RzQEJyb3dzZXJOb25DbGllbnRGcmFtZVZpZXdUaGVtZVRlc3QuQWN0aXZlVGFiVGV4dENvbG9yDA.

Since this test is still flaky, this issue has been moved back onto the Sheriff
Bug Queue if it's not already there.

If the result above is wrong, please file a bug using this link:
https://bugs.chromium.org/p/chromium/issues/entry?status=Unconfirmed&labels=Pri-1,Test-Findit-Wrong&components=Tools%3ETest%3EFindit%3EFlakiness&summary=%5BFindit%5D%20Flake%20Detection%20-%20Wrong%20result%20for%20BrowserNonClientFrameViewThemeTest.ActiveTabTextColor&comment=Link%20to%20flake%20occurrences%3A%20https://findit-for-me.appspot.com/flake/occurrences?key=ag9zfmZpbmRpdC1mb3ItbWVyVAsSBUZsYWtlIkljaHJvbWl1bUB1bml0X3Rlc3RzQEJyb3dzZXJOb25DbGllbnRGcmFtZVZpZXdUaGVtZVRlc3QuQWN0aXZlVGFiVGV4dENvbG9yDA

Automatically posted by the findit-for-me app (https://goo.gl/Ot9f7N).

+collinbaker@: This test was added in https://chromium.googlesource.com/chromium/src/+/059b37110ad2d6dcd3c177b66f26bbfdfe197386, could you please look into this flake? In the meantime, I'll be disabling the test.

Thanks!

Disable landing in https://chromium-review.googlesource.com/c/chromium/src/+/1237863

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

commit 758685cf3c66f1ec376744aacef8b3f46f040149
Author: Anthony Vallee-Dubois <anthonyvd@chromium.org>
Date: Fri Sep 21 15:11:11 2018

Disable flaky BrowserNonClientFrameViewThemeTest test

TBR= collinbaker@chromium.org,pkasting@chromium.org

Bug:  887815 
Change-Id: Iff08fcfe7efeac0363d40302c57a08005b069bbb
Reviewed-on: https://chromium-review.googlesource.com/1237863
Commit-Queue: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593186}
[modify] https://crrev.com/758685cf3c66f1ec376744aacef8b3f46f040149/chrome/browser/ui/views/frame/browser_non_client_frame_view_unittest.cc

I found the conditions under which the test hangs: if another window is focused when this test tries to activate its window, this test's window doesn't get activated. Then, the WidgetActivationWaiter waits indefinitely.

This appears to be a result of the way Windows handles activation. Calling BrowserFrame::Activate() ultimately leads to the WinAPI function SetForegroundWindow() being called. The WinAPI documentation (https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-setforegroundwindow#remarks) states:

> An application cannot force a window to the foreground while the user is working with another window. Instead, Windows flashes the taskbar button of the window to notify the user.

It also lists the specific conditions under which a window *can* activate itself. By those conditions, if another test runs and has an active window at the same time as this test, it will not be able to activate its window. This explains why it didn't happen when I was running my test manually. I am able to duplicate the hang by running the test command and switching to a different window before the test process starts.

For now, we probably need to keep the test disabled on Windows. It looks like there's no way to reliably activate the window.

+pkasting@ I'm assuming there's no way to unit test this class without opening a real window?

Good work tracking this down as far as you have!

There was some work done a while back on making sure the test runner could consistently activate windows, I thought.  I can't think who the right contact people are.  Maybe ask dpranke@ (+CCed), who still isn't the right person, but might know who is?  It seems like that could cause flakiness on a lot of things and we should try reasonably hard to fix in general.  (For example, maybe we can fake mouse movement to go click the windows taskbar button, or maybe we can determine what thing is running on the bots that's stealing activation and then stop it from running.)

As for unit testing, we can probably unittest, though it may involve some nontrivial plumbing, and might not be worth it in the end.

Nope, I have no idea. Maybe sky@ knows.

It looks like this test is relying on activation from the OS, and it's running a message loop. That is obviously problematic when the bot is running a ton of other things in parallel. Tests of this sort belong in interactive_ui_tests, when we know nothing else will be running in parallel.

The alternative is to make it possible (in a test) to force the active status to be true, ignoring the value from the OS. That is probably possible by having DesktopBrowserFrameAura override IsActive(). Some investigation would be needed to know if that's enough (I'm not sure if some code in DesktopNativeWidgetAura/DesktopWindowTreeHost is directly calling to HWNDMessageHandler).

Sure, I'll move it into an interactive UI test. Similarly, I think the other tests deriving from TestWithBrowserView also interact with the OS. My understanding is that this test fixture creates a native window (through BrowserView). Should these other unit tests be moved as well?

It looks like this test's flakiness was a symptom of this problem. Ideally, I could write this test as a unit test since all it needs to do is control how ShouldPaintAsActive() returns. Similarly, I don't think the other tests deriving from TestWithBrowserView need any OS interaction. Can a BrowserView be created without a backing native window? If so, changing the tests to create a fake window may be the better solution. If not, a longer-term solution would be to reduce this coupling, though I imagine this would be a large change.

I've deleted this test so I'm closing this.