Service CreateNewWindow on the UI thread with a new mojo interface

content/common/associated_interface_provider_impl.cc

 // Copyright 2016 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/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 {
 
 class AssociatedInterfaceProviderImpl::LocalProvider
     : public mojom::AssociatedInterfaceProvider {
  public:
   explicit LocalProvider(mojom::AssociatedInterfaceProviderAssociatedPtr* proxy)
       : associated_interface_provider_binding_(this) {
     associated_interface_provider_binding_.Bind(
         mojo::MakeIsolatedRequest(proxy));
   }
 
   ~LocalProvider() override {}
 
   void SetBinderForName(
       const std::string& name,
       const base::Callback<void(mojo::ScopedInterfaceEndpointHandle)>& binder) {
     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(
       const std::string& name,
       mojom::AssociatedInterfaceAssociatedRequest request) override {
     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 =
       std::map<std::string,
                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_;
 };
 
 AssociatedInterfaceProviderImpl::AssociatedInterfaceProviderImpl(
     mojom::AssociatedInterfaceProviderAssociatedPtr proxy)
     : proxy_(std::move(proxy)) {
   DCHECK(proxy_.is_bound());
 }
 
 AssociatedInterfaceProviderImpl::AssociatedInterfaceProviderImpl()
     : local_provider_(new LocalProvider(&proxy_)) {}
 
 AssociatedInterfaceProviderImpl::~AssociatedInterfaceProviderImpl() {}
 
 void AssociatedInterfaceProviderImpl::GetInterface(
     const std::string& name,
     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(
     const std::string& name,
     const base::Callback<void(mojo::ScopedInterfaceEndpointHandle)>& binder) {
   DCHECK(local_provider_);
   local_provider_->SetBinderForName(name, binder);
 }
 
 }  // namespace content