Service CreateNewWindow on the UI thread with a new mojo interface

content/common/associated_interface_provider_impl.cc

Index: content/common/associated_interface_provider_impl.cc
diff --git a/content/common/associated_interface_provider_impl.cc b/content/common/associated_interface_provider_impl.cc
index 647ba6a161407b5e698863420df35cb043c4d096..69988b93a00245ae3a10c7a8a8e589fef409f2c7 100644
--- a/content/common/associated_interface_provider_impl.cc
+++ b/content/common/associated_interface_provider_impl.cc
@@ -3,6 +3,8 @@
 // found in the LICENSE file.
 
 #include "content/common/associated_interface_provider_impl.h"
+#include "base/callback.h"
+#include "base/run_loop.h"
 #include "mojo/public/cpp/bindings/associated_binding.h"
 
 namespace content {
@@ -24,6 +26,14 @@ class AssociatedInterfaceProviderImpl::LocalProvider
     binders_[name] = binder;
   }
 
+  void WaitForBinding(const std::string& name) {

[ncarter (slow), 2017/04/21 20:42:13]

Assuming this is test-only code, we should find a way to put it in a test-only
file that's not linked against production content.

[Charlie Harrison, 2017/04/21 20:48:02]

Sorry, this is no longer up to date since rockot@ gave me a better suggestion.

+    DCHECK(binders_.find(name) != binders_.end());
+    base::RunLoop run_loop;
+    quit_closure_ = run_loop.QuitClosure();
+    name_to_wait_for_ = name;
+    run_loop.Run();
+  }
+
  private:
   // mojom::AssociatedInterfaceProvider:
   void GetAssociatedInterface(
@@ -32,6 +42,11 @@ class AssociatedInterfaceProviderImpl::LocalProvider
     auto it = binders_.find(name);
     if (it != binders_.end())
       it->second.Run(request.PassHandle());
+
+    if (name == name_to_wait_for_) {
+      name_to_wait_for_.clear();
+      std::move(quit_closure_).Run();
+    }
   }
 
   using BinderMap =
@@ -39,6 +54,9 @@ class AssociatedInterfaceProviderImpl::LocalProvider
                base::Callback<void(mojo::ScopedInterfaceEndpointHandle)>>;
   BinderMap binders_;
 
+  base::Closure quit_closure_;
+  std::string name_to_wait_for_;
+
   mojo::AssociatedBinding<mojom::AssociatedInterfaceProvider>
       associated_interface_provider_binding_;
 };
@@ -59,7 +77,12 @@ void AssociatedInterfaceProviderImpl::GetInterface(
     mojo::ScopedInterfaceEndpointHandle handle) {
   mojom::AssociatedInterfaceAssociatedRequest request;
   request.Bind(std::move(handle));
-  return proxy_->GetAssociatedInterface(name, std::move(request));
+  proxy_->GetAssociatedInterface(name, std::move(request));
+
+  // In tests, make sure the interface is bound before continuing, to ensure we
+  // don't deadlock for sync messages.
+  if (local_provider_)
+    local_provider_->WaitForBinding(name);

[Ken Rockot(use gerrit already), 2017/04/21 18:26:58]

Hmm. On one hand, I'm really not a fan of code which quietly runs nested message
loops, especially when the normal non-test behavior is to be totally async. On
the other hand, I can see the possibility of developers tripping over this
subtle behavior when writing tests.

Given that AssociatedInterfaceProvider is not something we intend to keep around
for too long, and that its use (as well as sync message use in general) is
relatively infrequent, I would prefer that your test code instead simply call
FlushForTesting on the relevant AssociatedInterfacePtr. That does a ping-pong
behind the scenes and waits for the reply in a nested loop. It should have an
equivalent outcome. i.e.

  FooAssociatedPtr foo;
  provider->GetInterface(&foo);
  foo.FlushForTesting();
  foo->PrettyCoolSyncIPC();

[Charlie Harrison, 2017/04/21 18:46:10]

Hm, this makes sense but it's a bit tricky because the tests do not have a
pointer to the AssociatedInterfacePtr. I think we'd just expose a getting
RenderFrameImpl for the interface, then override it in TestRenderFrame with an
extra call to FLushForTesting.

 }
 
 void AssociatedInterfaceProviderImpl::OverrideBinderForTesting(