Fix flakiness in postMessage-related SitePerProcessBrowserTests.

content/public/test/browser_test_utils.cc

Index: content/public/test/browser_test_utils.cc
diff --git a/content/public/test/browser_test_utils.cc b/content/public/test/browser_test_utils.cc
index 60423961dd6d2b7f4ad41539431a0318f92d0157..214b2fd61d678cca452e21f5e3fcde487f0bc5a8 100644
--- a/content/public/test/browser_test_utils.cc
+++ b/content/public/test/browser_test_utils.cc
@@ -75,9 +75,11 @@ class DOMOperationObserver : public NotificationObserver,
                const NotificationDetails& details) override {
     DCHECK(type == NOTIFICATION_DOM_OPERATION_RESPONSE);
     Details<DomOperationNotificationDetails> dom_op_details(details);
-    response_ = dom_op_details->json;
-    did_respond_ = true;
-    message_loop_runner_->Quit();
+    if (!did_respond_) {

[Charlie Reis, 2015/08/14 22:54:13]

I'm a bit concerned about this one, but partly because I don't fully understand
the message loop behavior.

I'm guessing that after the Quit call below, the message loop continues to
process any messages that have already queued up but stops queueing new
messages.  Is that correct?

Won't this change mean that any queued up messages after the first
DOM_OPERATION_RESPONSE will get consumed here and thus get ignored / dropped on
the floor?  I think that will make your msg_queue.WaitForMessage line time out
as well.

Or do such messages go to both observer and the DOMMessageQueue you create in
the test?  In that case, this one ignores it and the DOMMessageQueue processes
it, and the test would pass.  Is that what happens?

[alexmos, 2015/08/14 23:51:32]

Yes, it definitely continues to process any messages that have already queued
up, since I saw that while debugging.  Looking at ExecuteScriptHelper, which
uses this, DOMOperationObserver registers for these notifications in its
constructor and removes them when it is destroyed, which would be after
returning from WaitAndGetResponse (after the message loop has quit) and from
ExecuteScriptHelper.
It is the latter: the messages will go to both the observer and the
DOMMessageQueue I create in the test, since DOMMessageQueue registers for these
same notifications in its constructor.  In the race condition case, this one
will ignore it, and the queue will pick it up.

Declaring the queue earlier does mean that it will now always pick up the
return-value message from the preceding ExecuteScript that it doesn't care
about, but that's already handled properly in the WaitForMessage while loop.

[Charlie Reis, 2015/08/15 00:09:50]

Ok, sounds good.

+      response_ = dom_op_details->json;
+      did_respond_ = true;
+      message_loop_runner_->Quit();
+    }
   }
 
   // Overridden from WebContentsObserver: