Service CreateNewWindow on the UI thread with a new mojo interface

content/test/test_render_frame.cc

Index: content/test/test_render_frame.cc
diff --git a/content/test/test_render_frame.cc b/content/test/test_render_frame.cc
index 6c0bb0133b81b4af5f5e4e615a24e6bb8f48f943..3ae3715cd7cd14e106b25aa520611186ec60014e 100644
--- a/content/test/test_render_frame.cc
+++ b/content/test/test_render_frame.cc
@@ -4,14 +4,42 @@
 
 #include "content/test/test_render_frame.h"
 
+#include "base/memory/ptr_util.h"
 #include "content/common/navigation_params.h"
 #include "content/common/resource_request_body_impl.h"
+#include "content/public/common/associated_interface_provider.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/resource_response.h"
+#include "content/public/test/mock_render_thread.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 
 namespace content {
 
+class MockFrameHost : public mojom::FrameHost {
+ public:
+  MockFrameHost() : binding_(this) {}
+  ~MockFrameHost() override = default;
+
+  void CreateNewWindow(mojom::CreateNewWindowParamsPtr params,
+                       const CreateNewWindowCallback& callback) override {
+    mojom::CreateNewWindowReplyPtr reply = mojom::CreateNewWindowReply::New();
+    MockRenderThread* mock_render_thread =
+        static_cast<MockRenderThread*>(RenderThread::Get());
+    mock_render_thread->OnCreateWindow(*params, reply.get());
+    callback.Run(std::move(reply));
+  }
+
+  void Bind(mojo::ScopedInterfaceEndpointHandle handle) {
+    binding_.Bind(
+        mojo::MakeAssociatedRequest<mojom::FrameHost>(std::move(handle)));
+  }
+
+ private:
+  mojo::AssociatedBinding<mojom::FrameHost> binding_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockFrameHost);
+};
+
 // static
 RenderFrameImpl* TestRenderFrame::CreateTestRenderFrame(
     const RenderFrameImpl::CreateParams& params) {
@@ -19,7 +47,12 @@ RenderFrameImpl* TestRenderFrame::CreateTestRenderFrame(
 }
 
 TestRenderFrame::TestRenderFrame(const RenderFrameImpl::CreateParams& params)
-    : RenderFrameImpl(params) {
+    : RenderFrameImpl(params),
+      mock_frame_host_(base::MakeUnique<MockFrameHost>()) {
+  GetRemoteAssociatedInterfaces()->OverrideBinderForTesting(
+      mojom::FrameHost::Name_,
+      base::Bind(&MockFrameHost::Bind,
+                 base::Unretained(mock_frame_host_.get())));
 }
 
 TestRenderFrame::~TestRenderFrame() {
@@ -89,4 +122,12 @@ blink::WebNavigationPolicy TestRenderFrame::DecidePolicyForNavigation(
   return RenderFrameImpl::DecidePolicyForNavigation(info);
 }
 
+mojom::FrameHostAssociatedPtr TestRenderFrame::GetFrameHost() {
+  mojom::FrameHostAssociatedPtr ptr = RenderFrameImpl::GetFrameHost();
+
+  // Needed to ensure no deadlocks when waiting for sync IPC.
+  ptr.FlushForTesting();

[Ken Rockot(use gerrit already), 2017/05/31 23:58:02]

Can you please remind me exactly why this flush was necessary? i.e. how the sync
CreateNewWindow IPC be sent at a time where it would cause the main thread to
deadlock? And how wasn't this a problem before the IPC was converted to mojom,
given that the ordering and threading behavior should be exactly the same as
before? Any specific tests that were failing?

I'm asking because this turns out to be quite problematic: we effectively can't
use the FrameHost interface for all kinds of things now, because calling
GetFrameHost() from various points in production code will deadlock browser
tests on this FlushForTesting().

[Charlie Harrison, 2017/06/01 01:35:12]

Hey Ken, agreed this is not ideal especially if it is making testing harder in
general. I've filed crbug.com/728463 to track this with some extra context for
how we got here.

Thanks for bringing this up

+  return ptr;
+}
+
 }  // namespace content