Another flake incident can be found here https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Ozone%20Tests%20%281%29/builds/47278.

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

commit 66169e7ebec215a59409823ab08f5210ce1fde24
Author: kenrb <kenrb@chromium.org>
Date: Sat May 20 00:37:43 2017

Speculative fix for PopupMenuTest flakiness

SitePerProcessBrowserTest.PopupMenuTest is occasionally flaky on
ChromeOS bots, possibly because a Linux-only section was added in
r459476 that relies on browser process hit testing, but the test does
not call WaitForChildFrameSurfaceReady() to prevent races between hit
tests and compositor frames.

This adds a call to attempt to resolve the flakiness, and does some
unrelated cleanup.

BUG= 723657 

Review-Url: https://codereview.chromium.org/2895653003
Cr-Commit-Position: refs/heads/master@{#473396}

[modify] https://crrev.com/66169e7ebec215a59409823ab08f5210ce1fde24/content/browser/site_per_process_browsertest.cc

I don't see any more flakes in a quick scan of recent logs. Has anyone seen any recent instances, or can I close this bug?

Test flaked on https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/38852, right before your change landed.  I looked at the next 20, and some more after that, and didn't see the flake shown in the first call stack.

However, I did see one instance of the second call stack crash in those 30 runs:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FLinux_ChromiumOS_Tests__1_%2F39036%2F%2B%2Frecipes%2Fsteps%2Fcontent_browsertests%2F0%2Fstdout

Empirically, it looks like these were two different problems.  This first (1-in-5) seems to be fixed, the second (1-in-30+?) still seems to happen occasionally.

A local build of content_browsertests (Linux, non-Chrome OS, release build, DCHECK on) is consistently hitting this failure on every run. This is on ToT with no local changes applied.

rockot@: I still can't repro locally, using the configuration you describe. Is there anything else notable about it?

I am wondering if there is something wrong with the MouseEvent being passed to RenderWidgetHostViewEventHandler::ForwardMouseEventToParent(), but it would be a lot easier to diagnose if I could reproduce it.

Adding rockot@ to cc -- Please see comment above.

I can try to repro in a debugger some time today to help track this down.

Thanks.

One possibility that just occurred to me is that I might need to move this test to ui_interactive_tests. It might be possible that other tests are causing the popup to be dismissed during one of the times when this test yields thread execution, and the RenderWidgetHostView is sometimes getting destroyed prematurely, resulting in a crash when it tries to route a mouse event to the RWHV.

Any help confirming or refuting that hypothesis would be greatly appreciated.

Thought I posted this already btw but I don't see it... Here's some other output which appears within the same test shortly before the crash:

../../content/browser/site_per_process_browsertest.cc:6457: Failure
      Expected: popup_rect.y() - rwhv_root->GetViewBounds().y()
      Which is: 96
To be equal to: 94

From a debugger it seems pretty clear that RenderWidgetHost::FromID on line 6479 of site_per_process_browsertest.cc is returning null.

I wonder if this is just because the MessageLoop is allowed to run for too long after the test message is received, and ShowWidgetMessageFilter should either switch to just using a base::RunLoop to Wait, or at least construct its content::MessageLoopRunner with content::MessageLoopRunner::QuitMode::IMMEDIATE?

I just tried constructing the MessageLoopRunner with content::MessageLoopRunner::QuitMode::IMMEDIATE, with interesting results. Whereas I was previously not able to repro this bug, after that change I see this crash stack on every run.

That suggests the problem is the other way around -- we aren't waiting long enough. RenderWidgetHost::FromID is returning null because WebContentsImpl hasn't yet processed the ViewHostMsg_ShowWidget message, and the popup's RWH doesn't yet exist.

A simple solution would just be to check the FromID return for null, and if it isn't there yet, yield and try again.

Somehow that sounds likely to have even more potential for flakiness. If
the ViewHostMsg_ShowWidget IPC does not correspond to the expected RWH
being created, then isn't waiting for ViewHostMsg_ShowWidget simply the
wrong thing to do here?

You're right, I was thinking that the widget gets created by the WebContentsImpl upon receipt of ViewHostMsg_ShowWidget, and since the IO thread is posting both those events to the UI thread, we might have a race.

But the RenderWidgetHost already exists, and incidentally in my test runs FromID always returns a RenderWidgetHost, even though I see a crash. I still think this is a race between ShowWidget handlers, but possibly what is happening is that we are calling RWHVA::OnMouseEvent() before WebContentsImpl calls RWHVA::InitAsPopup. This means that the RenderWidgetHostView doesn't have a window_ object yet, which would result in the crash stack we are seeing.

Creating a loop that waits for popup_view->window() to be non-null fixes the crash for me locally.

CL here: https://codereview.chromium.org/2917173002

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

commit 37003a1722d89acc689cd3911efceb583bad43e0
Author: kenrb <kenrb@chromium.org>
Date: Sat Jun 03 00:17:03 2017

Make PopupMenuTest wait for RenderWidgetHostView initialization

SitePerProcessBrowserTest.PopupMenuTest has been observed to have
occasional flakes, and the cause seems to be a race between handlers
of ViewMsg_ShowWidget messages. The test code uses a message filter
which notifies upon receipt of ShowWidget, and WebContentsImpl is the
ultimate recipient of that message which causes it to initialize the
popup widget.

Occasionally the test code will run before WebContentsImpl's handler
has fired, in which case the RenderWidgetHostView has not been
initialized, resulting in a crash. This CL adds a loop to wait for
initialization to be completed before proceeding with the test.

BUG= 723657 

Review-Url: https://codereview.chromium.org/2917173002
Cr-Commit-Position: refs/heads/master@{#476839}

[modify] https://crrev.com/37003a1722d89acc689cd3911efceb583bad43e0/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/37003a1722d89acc689cd3911efceb583bad43e0/content/browser/site_per_process_browsertest.cc

I haven't seen any incidences of this flake since my CL landed.

I checked the 40 builds after your CL landed and didn't see any flakes.  Thanks for your diligence in getting this fixed!