Fix flakiness in postMessage-related SitePerProcessBrowserTests.

content/public/test/browser_test_utils.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/public/test/browser_test_utils.h"
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
@@ skipping 57 matching lines @@
     registrar_.Add(this, NOTIFICATION_DOM_OPERATION_RESPONSE,
                    Source<WebContents>(web_contents()));
     message_loop_runner_ = new MessageLoopRunner;
   }
 
   void Observe(int type,
                const NotificationSource& source,
                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:
   void RenderProcessGone(base::TerminationStatus status) override {
     message_loop_runner_->Quit();
   }
 
   bool WaitAndGetResponse(std::string* response) WARN_UNUSED_RESULT {
     message_loop_runner_->Run();
     *response = response_;
@@ skipping 894 matching lines @@
 void FrameWatcher::WaitFrames(int frames_to_wait) {
   if (frames_to_wait <= 0)
     return;
   base::RunLoop run_loop;
   base::AutoReset<base::Closure> reset_quit(&quit_, run_loop.QuitClosure());
   base::AutoReset<int> reset_frames_to_wait(&frames_to_wait_, frames_to_wait);
   run_loop.Run();
 }
 
 }  // namespace content