Service CreateNewWindow on the UI thread with a new mojo interface

content/test/test_render_frame.cc

 // Copyright 2015 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/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) {
   return new TestRenderFrame(params);
 }
 
 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() {
 }
 
 void TestRenderFrame::Navigate(const CommonNavigationParams& common_params,
                                const StartNavigationParams& start_params,
                                const RequestNavigationParams& request_params) {
   // PlzNavigate
   if (IsBrowserSideNavigationEnabled()) {
@@ skipping 49 matching lines @@
       info.url_request.CheckForBrowserSideNavigation() &&
       GetWebFrame()->Parent() && info.form.IsNull()) {
     // RenderViewTest::LoadHTML already disables PlzNavigate for the main frame
     // requests. However if the loaded html has a subframe, the WebURLRequest
     // will be created inside Blink and it won't have this flag set.
     info.url_request.SetCheckForBrowserSideNavigation(false);
   }
   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