RemoteMacViews: Make all mojo interfaces be associated

All mojo interfaces that manipulate NSViews need to be associated (for
example, adding an NSView as a child of another requires that the
parent NSView be in a known state -- each NSView has its own interface,
and so they all need to be associated to the same pipe).

Also change all bindings to use ui::WindowResizeHelperMac's task
runner. This task runner is used to pump a nested run loop inside
a Cocoa resize function (for smooth resize). All bindings on the same
associated connection need to use that task runner (otherwise, all
messages will be blocked behind any un-handled message).

Change BridgeFactoryHost to not assign a new host id, but take one
in the constructor (just because the logic is easier to follow that
way).

R=domonickn
TBR=avi (for content/ OWNERship)

Bug: 821651
Change-Id: Ifb6aff292187b340d144d29ffe1258bb8d501267
Reviewed-on: https://chromium-review.googlesource.com/c/1253561
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596013}

chrome/app_shim/chrome_main_app_mode_mac.mm

@@ -133,7 +133,7 @@ class AppShimController : public chrome::mojom::AppShim {
   // chrome::mojom::AppShim implementation.
   void LaunchAppDone(apps::AppShimLaunchResult result) override;
   void CreateViewsBridgeFactory(
-      views_bridge_mac::mojom::BridgeFactoryRequest request) override;
+      views_bridge_mac::mojom::BridgeFactoryAssociatedRequest request) override;
   void CreateContentNSViewBridgeFactory(
       content::mojom::NSViewBridgeFactoryAssociatedRequest request) override;
   void Hide() override;
@@ -224,7 +224,8 @@ void AppShimController::CreateChannelAndSendLaunchApp(
       chrome::mojom::AppShimHostPtrInfo(std::move(message_pipe), 0));
 
   chrome::mojom::AppShimPtr app_shim_ptr;
-  shim_binding_.Bind(mojo::MakeRequest(&app_shim_ptr));
+  shim_binding_.Bind(mojo::MakeRequest(&app_shim_ptr),
+                     ui::WindowResizeHelperMac::Get()->task_runner());
   shim_binding_.set_connection_error_with_reason_handler(
       base::BindOnce(&AppShimController::ChannelError, base::Unretained(this)));
 
@@ -314,7 +315,7 @@ void AppShimController::LaunchAppDone(apps::AppShimLaunchResult result) {
 }
 
 void AppShimController::CreateViewsBridgeFactory(
-    views_bridge_mac::mojom::BridgeFactoryRequest request) {
+    views_bridge_mac::mojom::BridgeFactoryAssociatedRequest request) {
   views_bridge_mac::BridgeFactoryImpl::Get()->BindRequest(std::move(request));
 }
 

chrome/browser/apps/app_shim/app_shim_host_mac.cc

@@ -17,6 +17,13 @@
 #include "ui/views/cocoa/bridge_factory_host.h"
 #include "ui/views_bridge_mac/mojo/bridge_factory.mojom.h"
 
+namespace {
+
+// Start counting host ids at 1000 to help in debugging.
+uint64_t g_next_host_id = 1000;
+
+}  // namespace
+
 AppShimHost::AppShimHost()
     : host_binding_(this), initial_launch_finished_(false) {}
 
@@ -69,11 +76,14 @@ void AppShimHost::LaunchApp(chrome::mojom::AppShimPtr app_shim_ptr,
 
   app_shim_ = std::move(app_shim_ptr);
   if (features::HostWindowsInAppShimProcess()) {
+    uint64_t host_id = g_next_host_id++;
+
     // Create the interface that will be used by views::NativeWidgetMac to
     // create NSWindows hosted in the app shim process.
-    views_bridge_mac::mojom::BridgeFactoryRequest views_bridge_factory_request;
+    views_bridge_mac::mojom::BridgeFactoryAssociatedRequest
+        views_bridge_factory_request;
     views_bridge_factory_host_ = std::make_unique<views::BridgeFactoryHost>(
-        &views_bridge_factory_request);
+        host_id, &views_bridge_factory_request);
     app_shim_->CreateViewsBridgeFactory(
         std::move(views_bridge_factory_request));
 
@@ -83,8 +93,7 @@ void AppShimHost::LaunchApp(chrome::mojom::AppShimPtr app_shim_ptr,
         content_bridge_factory_request;
     content_bridge_factory_ =
         std::make_unique<content::NSViewBridgeFactoryHost>(
-            &content_bridge_factory_request,
-            views_bridge_factory_host_->GetHostId());
+            &content_bridge_factory_request, host_id);
     app_shim_->CreateContentNSViewBridgeFactory(
         std::move(content_bridge_factory_request));
   }

chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc

@@ -47,7 +47,8 @@ class TestingAppShim : public chrome::mojom::AppShim {
     launch_done_result_ = result;
   }
   void CreateViewsBridgeFactory(
-      views_bridge_mac::mojom::BridgeFactoryRequest request) override {}
+      views_bridge_mac::mojom::BridgeFactoryAssociatedRequest request)
+      override {}
   void CreateContentNSViewBridgeFactory(
       content::mojom::NSViewBridgeFactoryAssociatedRequest request) override {}
   void Hide() override {}

chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm

@@ -50,7 +50,8 @@ class TestShimClient : public chrome::mojom::AppShim {
   // chrome::mojom::AppShim implementation (not used in testing, but can be).
   void LaunchAppDone(apps::AppShimLaunchResult result) override {}
   void CreateViewsBridgeFactory(
-      views_bridge_mac::mojom::BridgeFactoryRequest request) override {}
+      views_bridge_mac::mojom::BridgeFactoryAssociatedRequest request)
+      override {}
   void CreateContentNSViewBridgeFactory(
       content::mojom::NSViewBridgeFactoryAssociatedRequest request) override {}
   void Hide() override {}

chrome/common/mac/app_shim.mojom

@@ -25,7 +25,7 @@ interface AppShim {
   // Create the interface through which BridgedNativeWidget instances may be
   // created (for views::Widgets whose NSWindows exist in the app shim process).
   CreateViewsBridgeFactory(
-      views_bridge_mac.mojom.BridgeFactory& views_bridge_factory);
+      associated views_bridge_mac.mojom.BridgeFactory& views_bridge_factory);
 
   // Create the interface through which a content structure
   // (RenderWidgetHostView or WebContentsView) may create an NSView that exists

content/browser/ns_view_bridge_factory_impl.mm

@@ -10,6 +10,7 @@
 #include "base/no_destructor.h"
 #include "content/browser/renderer_host/render_widget_host_ns_view_bridge_local.h"
 #include "content/browser/web_contents/web_contents_ns_view_bridge.h"
+#include "ui/accelerated_widget_mac/window_resize_helper_mac.h"
 
 namespace content {
 
@@ -21,7 +22,8 @@ NSViewBridgeFactoryImpl* NSViewBridgeFactoryImpl::Get() {
 
 void NSViewBridgeFactoryImpl::BindRequest(
     mojom::NSViewBridgeFactoryAssociatedRequest request) {
-  binding_.Bind(std::move(request));
+  binding_.Bind(std::move(request),
+                ui::WindowResizeHelperMac::Get()->task_runner());
 }
 
 void NSViewBridgeFactoryImpl::CreateRenderWidgetHostNSViewBridge(

content/browser/renderer_host/render_widget_host_ns_view_bridge_local.mm

@@ -9,6 +9,7 @@
 #include "content/browser/renderer_host/render_widget_host_ns_view_client_helper.h"
 #include "content/common/cursors/webcursor.h"
 #import "skia/ext/skia_utils_mac.h"
+#include "ui/accelerated_widget_mac/window_resize_helper_mac.h"
 #import "ui/base/cocoa/animation_utils.h"
 #include "ui/base/cocoa/ns_view_ids.h"
 #include "ui/display/screen.h"
@@ -28,7 +29,8 @@ RenderWidgetHostNSViewBridgeLocal::RenderWidgetHostNSViewBridgeLocal(
     mojom::RenderWidgetHostNSViewClientAssociatedPtr client,
     mojom::RenderWidgetHostNSViewBridgeAssociatedRequest bridge_request)
     : remote_client_(std::move(client)), binding_(this) {
-  binding_.Bind(std::move(bridge_request));
+  binding_.Bind(std::move(bridge_request),
+                ui::WindowResizeHelperMac::Get()->task_runner());
   // This object will be destroyed when its connection is closed.
   binding_.set_connection_error_handler(
       base::BindOnce(&RenderWidgetHostNSViewBridgeLocal::OnConnectionError,

content/browser/web_contents/web_contents_ns_view_bridge.mm

@@ -4,6 +4,8 @@
 
 #include "content/browser/web_contents/web_contents_ns_view_bridge.h"
 
+#include "ui/accelerated_widget_mac/window_resize_helper_mac.h"
+
 namespace content {
 
 WebContentsNSViewBridge::WebContentsNSViewBridge(
@@ -11,7 +13,8 @@ WebContentsNSViewBridge::WebContentsNSViewBridge(
     mojom::WebContentsNSViewClientAssociatedPtr client,
     mojom::WebContentsNSViewBridgeAssociatedRequest bridge_request)
     : client_(std::move(client)), binding_(this) {
-  binding_.Bind(std::move(bridge_request));
+  binding_.Bind(std::move(bridge_request),
+                ui::WindowResizeHelperMac::Get()->task_runner());
   // This object will be destroyed when its connection is closed.
   binding_.set_connection_error_handler(base::BindOnce(
       &WebContentsNSViewBridge::OnConnectionError, base::Unretained(this)));

ui/views/cocoa/bridge_factory_host.cc

@@ -8,14 +8,10 @@
 
 namespace views {
 
-namespace {
-// Start the ids at something far from zero to help in debugging.
-uint64_t g_next_bridge_factory_host_id_ = 0x1000;
-}  // namespace
-
 BridgeFactoryHost::BridgeFactoryHost(
-    views_bridge_mac::mojom::BridgeFactoryRequest* request)
-    : host_id_(g_next_bridge_factory_host_id_++) {
+    uint64_t host_id,
+    views_bridge_mac::mojom::BridgeFactoryAssociatedRequest* request)
+    : host_id_(host_id) {
   *request = mojo::MakeRequest(&bridge_factory_ptr_);
 }
 

ui/views/cocoa/bridge_factory_host.h

@@ -22,7 +22,9 @@ class VIEWS_EXPORT BridgeFactoryHost {
     ~Observer() override {}
   };
 
-  BridgeFactoryHost(views_bridge_mac::mojom::BridgeFactoryRequest* request);
+  BridgeFactoryHost(
+      uint64_t host_id,
+      views_bridge_mac::mojom::BridgeFactoryAssociatedRequest* request);
   ~BridgeFactoryHost();
 
   // Return an id for the host process. This can be used to look up other
@@ -36,7 +38,7 @@ class VIEWS_EXPORT BridgeFactoryHost {
 
  private:
   const uint64_t host_id_;
-  views_bridge_mac::mojom::BridgeFactoryPtr bridge_factory_ptr_;
+  views_bridge_mac::mojom::BridgeFactoryAssociatedPtr bridge_factory_ptr_;
   base::ObserverList<Observer> observers_;
 };
 

ui/views/cocoa/bridged_native_widget_host_impl.h

@@ -9,7 +9,7 @@
 
 #include "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
-#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/associated_binding.h"
 #include "ui/accelerated_widget_mac/accelerated_widget_mac.h"
 #include "ui/accelerated_widget_mac/display_link_mac.h"
 #include "ui/base/ime/input_method_delegate.h"
@@ -312,7 +312,7 @@ class VIEWS_EXPORT BridgedNativeWidgetHostImpl
 
   // The mojo pointer to a BridgedNativeWidget, which may exist in another
   // process.
-  views_bridge_mac::mojom::BridgedNativeWidgetPtr bridge_ptr_;
+  views_bridge_mac::mojom::BridgedNativeWidgetAssociatedPtr bridge_ptr_;
 
   // TODO(ccameron): Rather than instantiate a BridgedNativeWidgetImpl here,
   // we will instantiate a mojo BridgedNativeWidgetImpl interface to a Cocoa
@@ -353,7 +353,7 @@ class VIEWS_EXPORT BridgedNativeWidgetHostImpl
   // Contains NativeViewHost->gfx::NativeView associations.
   std::map<const views::View*, NSView*> associated_views_;
 
-  mojo::Binding<views_bridge_mac::mojom::BridgedNativeWidgetHost>
+  mojo::AssociatedBinding<views_bridge_mac::mojom::BridgedNativeWidgetHost>
       host_mojo_binding_;
   DISALLOW_COPY_AND_ASSIGN(BridgedNativeWidgetHostImpl);
 };

ui/views/cocoa/bridged_native_widget_host_impl.mm

@@ -89,7 +89,8 @@ BridgedNativeWidgetHostImpl::BridgedNativeWidgetHostImpl(
 
 BridgedNativeWidgetHostImpl::~BridgedNativeWidgetHostImpl() {
   if (bridge_factory_host_) {
-    bridge_factory_host_->GetFactory()->DestroyBridge(widget_id_);
+    bridge_ptr_.reset();
+    host_mojo_binding_.Unbind();
     bridge_factory_host_->RemoveObserver(this);
     bridge_factory_host_ = nullptr;
   }
@@ -147,11 +148,11 @@ void BridgedNativeWidgetHostImpl::CreateRemoteBridge(
   [local_window_ setBridgedNativeWidgetId:widget_id_];
 
   // Initialize |bridge_ptr_| to point to a bridge created by |factory|.
-  views_bridge_mac::mojom::BridgedNativeWidgetHostPtr host_ptr;
+  views_bridge_mac::mojom::BridgedNativeWidgetHostAssociatedPtr host_ptr;
   host_mojo_binding_.Bind(mojo::MakeRequest(&host_ptr),
                           ui::WindowResizeHelperMac::Get()->task_runner());
-  bridge_factory_host_->GetFactory()->CreateBridge(
-      widget_id_, mojo::MakeRequest(&bridge_ptr_), std::move(host_ptr));
+  bridge_factory_host_->GetFactory()->CreateBridgedNativeWidget(
+      widget_id_, mojo::MakeRequest(&bridge_ptr_), host_ptr.PassInterface());
 
   // Create the window in its process, and attach it to its parent window.
   bridge()->CreateWindow(std::move(window_create_params), parent_bridge_id);

ui/views_bridge_mac/bridge_factory_impl.h

@@ -5,7 +5,7 @@
 #ifndef UI_VIEWS_BRIDGE_MAC_BRIDGE_FACTORY_IMPL_H_
 #define UI_VIEWS_BRIDGE_MAC_BRIDGE_FACTORY_IMPL_H_
 
-#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/associated_binding.h"
 #include "ui/views/views_export.h"
 #include "ui/views_bridge_mac/mojo/bridge_factory.mojom.h"
 #include "ui/views_bridge_mac/mojo/bridged_native_widget.mojom.h"
@@ -20,20 +20,20 @@ namespace views_bridge_mac {
 class VIEWS_EXPORT BridgeFactoryImpl : public mojom::BridgeFactory {
  public:
   static BridgeFactoryImpl* Get();
-  void BindRequest(mojom::BridgeFactoryRequest request);
+  void BindRequest(mojom::BridgeFactoryAssociatedRequest request);
 
   // mojom::BridgeFactory:
-  void CreateBridge(uint64_t bridge_id,
-                    mojom::BridgedNativeWidgetRequest bridge_request,
-                    mojom::BridgedNativeWidgetHostPtr host) override;
-  void DestroyBridge(uint64_t bridge_id) override;
+  void CreateBridgedNativeWidget(
+      uint64_t bridge_id,
+      mojom::BridgedNativeWidgetAssociatedRequest bridge_request,
+      mojom::BridgedNativeWidgetHostAssociatedPtrInfo host) override;
 
  private:
   friend class base::NoDestructor<BridgeFactoryImpl>;
   BridgeFactoryImpl();
   ~BridgeFactoryImpl() override;
 
-  mojo::Binding<mojom::BridgeFactory> binding_;
+  mojo::AssociatedBinding<mojom::BridgeFactory> binding_;
 };
 
 }  // namespace views_bridge_mac

ui/views_bridge_mac/bridge_factory_impl.mm

@@ -5,6 +5,7 @@
 #include "ui/views_bridge_mac/bridge_factory_impl.h"
 
 #include "base/no_destructor.h"
+#include "ui/accelerated_widget_mac/window_resize_helper_mac.h"
 #include "ui/views_bridge_mac/bridged_native_widget_host_helper.h"
 #include "ui/views_bridge_mac/bridged_native_widget_impl.h"
 
@@ -18,15 +19,22 @@ namespace {
 class Bridge : public BridgedNativeWidgetHostHelper {
  public:
   Bridge(uint64_t bridge_id,
-         mojom::BridgedNativeWidgetHostPtr host_ptr,
-         mojom::BridgedNativeWidgetRequest bridge_request) {
-    host_ptr_ = std::move(host_ptr);
+         mojom::BridgedNativeWidgetHostAssociatedPtrInfo host_ptr,
+         mojom::BridgedNativeWidgetAssociatedRequest bridge_request) {
+    host_ptr_.Bind(std::move(host_ptr),
+                   ui::WindowResizeHelperMac::Get()->task_runner());
     bridge_impl_ = std::make_unique<BridgedNativeWidgetImpl>(
         bridge_id, host_ptr_.get(), this);
-    bridge_impl_->BindRequest(std::move(bridge_request));
+    bridge_impl_->BindRequest(
+        std::move(bridge_request),
+        base::BindOnce(&Bridge::OnConnectionError, base::Unretained(this)));
   }
+
+ private:
   ~Bridge() override {}
 
+  void OnConnectionError() { delete this; }
+
   // BridgedNativeWidgetHostHelper:
   NSView* GetNativeViewAccessible() override { return nil; }
   void DispatchKeyEvent(ui::KeyEvent* event) override {}
@@ -45,15 +53,10 @@ class Bridge : public BridgedNativeWidgetHostHelper {
     return nullptr;
   }
 
-  mojom::BridgedNativeWidgetHostPtr host_ptr_;
+  mojom::BridgedNativeWidgetHostAssociatedPtr host_ptr_;
   std::unique_ptr<BridgedNativeWidgetImpl> bridge_impl_;
 };
 
-std::map<uint64_t, std::unique_ptr<Bridge>>& GetBridgeMap() {
-  static base::NoDestructor<std::map<uint64_t, std::unique_ptr<Bridge>>> map;
-  return *map;
-}
-
 }  // namespace
 
 // static
@@ -62,20 +65,18 @@ BridgeFactoryImpl* BridgeFactoryImpl::Get() {
   return factory.get();
 }
 
-void BridgeFactoryImpl::BindRequest(mojom::BridgeFactoryRequest request) {
+void BridgeFactoryImpl::BindRequest(
+    mojom::BridgeFactoryAssociatedRequest request) {
   binding_.Bind(std::move(request));
 }
 
-void BridgeFactoryImpl::CreateBridge(
+void BridgeFactoryImpl::CreateBridgedNativeWidget(
     uint64_t bridge_id,
-    mojom::BridgedNativeWidgetRequest bridge_request,
-    mojom::BridgedNativeWidgetHostPtr host_ptr) {
-  GetBridgeMap()[bridge_id] = std::make_unique<Bridge>(
-      bridge_id, std::move(host_ptr), std::move(bridge_request));
-}
-
-void BridgeFactoryImpl::DestroyBridge(uint64_t bridge_id) {
-  GetBridgeMap().erase(bridge_id);
+    mojom::BridgedNativeWidgetAssociatedRequest bridge_request,
+    mojom::BridgedNativeWidgetHostAssociatedPtrInfo host) {
+  // The resulting object will be destroyed when its message pipe is closed.
+  ignore_result(
+      new Bridge(bridge_id, std::move(host), std::move(bridge_request)));
 }
 
 BridgeFactoryImpl::BridgeFactoryImpl() : binding_(this) {}

ui/views_bridge_mac/bridged_native_widget_impl.h

@@ -13,7 +13,7 @@
 #import "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
 #include "components/viz/common/surfaces/parent_local_surface_id_allocator.h"
-#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/associated_binding.h"
 #include "ui/accelerated_widget_mac/ca_transaction_observer.h"
 #include "ui/accelerated_widget_mac/display_ca_layer_tree.h"
 #include "ui/base/cocoa/ns_view_ids.h"
@@ -84,7 +84,13 @@ class VIEWS_EXPORT BridgedNativeWidgetImpl
                           BridgedNativeWidgetHost* host,
                           BridgedNativeWidgetHostHelper* host_helper);
   ~BridgedNativeWidgetImpl() override;
-  void BindRequest(views_bridge_mac::mojom::BridgedNativeWidgetRequest request);
+
+  // Bind |bridge_mojo_binding_| to |request|, and set the connection error
+  // callback for |bridge_mojo_binding_| to |connection_closed_callback| (which
+  // will delete |this| when the connection is closed).
+  void BindRequest(
+      views_bridge_mac::mojom::BridgedNativeWidgetAssociatedRequest request,
+      base::OnceClosure connection_closed_callback);
 
   // Initialize the NSWindow by taking ownership of the specified object.
   // TODO(ccameron): When a BridgedNativeWidgetImpl is allocated across a
@@ -341,7 +347,7 @@ class VIEWS_EXPORT BridgedNativeWidgetImpl
   // shadow needs to be invalidated when a frame is received for the new shape.
   bool invalidate_shadow_on_frame_swap_ = false;
 
-  mojo::Binding<views_bridge_mac::mojom::BridgedNativeWidget>
+  mojo::AssociatedBinding<views_bridge_mac::mojom::BridgedNativeWidget>
       bridge_mojo_binding_;
   DISALLOW_COPY_AND_ASSIGN(BridgedNativeWidgetImpl);
 };

ui/views_bridge_mac/bridged_native_widget_impl.mm

@@ -282,9 +282,12 @@ BridgedNativeWidgetImpl::~BridgedNativeWidgetImpl() {
 }
 
 void BridgedNativeWidgetImpl::BindRequest(
-    views_bridge_mac::mojom::BridgedNativeWidgetRequest request) {
+    views_bridge_mac::mojom::BridgedNativeWidgetAssociatedRequest request,
+    base::OnceClosure connection_closed_callback) {
   bridge_mojo_binding_.Bind(std::move(request),
                             ui::WindowResizeHelperMac::Get()->task_runner());
+  bridge_mojo_binding_.set_connection_error_handler(
+      std::move(connection_closed_callback));
 }
 
 void BridgedNativeWidgetImpl::SetWindow(

ui/views_bridge_mac/mojo/bridge_factory.mojom

@@ -9,10 +9,12 @@ import "ui/views_bridge_mac/mojo/bridged_native_widget_host.mojom";
 
 // The interface through which a bridge is created and connected to its host.
 interface BridgeFactory {
-  CreateBridge(
+  // Create a bridge for a native widget. The resulting object will be owned by
+  // the connection for |host|. Closing that connection will result in deleting
+  // the bridge.
+  CreateBridgedNativeWidget(
       uint64 bridge_id,
-      BridgedNativeWidget& bridge_request,
-      BridgedNativeWidgetHost host);
-  DestroyBridge(uint64 bridge_id);
+      associated BridgedNativeWidget& bridge_request,
+      associated BridgedNativeWidgetHost host);
 };