[MBI] Conditionally associate AgentSchedulingGroup mojos.

As part of MBI, most widget/view/frame-related messages will go through
the AgentSchedulingGroup instead of RenderProcess/RenderThread. That can
change the ordering in which messages are handled, and may break some
assumptions we are currently making. In order to enable testing and
experimentation with this ordering change, this CL makes the ASG
interfaces channel-associated, thus preserving the current message order.
This will allow us to migrate all the currently channel-associated
interfaces to be associated with the ASG, and control the ordering via a
flag.

Bug: 1111231
Change-Id: Id04d90c748a2ce2cbb47d1cde7892fa99b1aac70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386859
Commit-Queue: Tal Pressman <talp@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806525}

content/browser/renderer_host/agent_scheduling_group_host.cc

@@ -5,10 +5,13 @@
 
 #include <memory>
 
+#include "base/feature_list.h"
 #include "base/supports_user_data.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/common/agent_scheduling_group.mojom.h"
 #include "content/common/renderer.mojom.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/common/content_features.h"
 
 namespace content {
 
@@ -16,8 +19,14 @@ namespace {
 
 using ::IPC::ChannelProxy;
 using ::IPC::Listener;
+using ::mojo::AssociatedReceiver;
+using ::mojo::AssociatedRemote;
+using ::mojo::PendingAssociatedReceiver;
+using ::mojo::PendingAssociatedRemote;
+using ::mojo::PendingReceiver;
 using ::mojo::PendingRemote;
 using ::mojo::Receiver;
+using ::mojo::Remote;
 
 static constexpr char kAgentGroupHostDataKey[] =
     "AgentSchedulingGroupHostUserDataKey";
@@ -39,6 +48,63 @@ class AgentGroupHostUserData : public base::SupportsUserData::Data {
 
 }  // namespace
 
+// MaybeAssociatedReceiver:
+AgentSchedulingGroupHost::MaybeAssociatedReceiver::MaybeAssociatedReceiver(
+    AgentSchedulingGroupHost& host,
+    bool should_associate) {
+  if (should_associate) {
+    receiver_.emplace<AssociatedReceiver<mojom::AgentSchedulingGroupHost>>(
+        &host);
+  } else {
+    receiver_.emplace<Receiver<mojom::AgentSchedulingGroupHost>>(&host);
+  }
+}
+
+AgentSchedulingGroupHost::MaybeAssociatedReceiver::~MaybeAssociatedReceiver() =
+    default;
+
+PendingRemote<mojom::AgentSchedulingGroupHost>
+AgentSchedulingGroupHost::MaybeAssociatedReceiver::BindNewPipeAndPassRemote() {
+  return absl::get<Receiver<mojom::AgentSchedulingGroupHost>>(receiver_)
+      .BindNewPipeAndPassRemote();
+}
+
+PendingAssociatedRemote<mojom::AgentSchedulingGroupHost>
+AgentSchedulingGroupHost::MaybeAssociatedReceiver::
+    BindNewEndpointAndPassRemote() {
+  return absl::get<AssociatedReceiver<mojom::AgentSchedulingGroupHost>>(
+             receiver_)
+      .BindNewEndpointAndPassRemote();
+}
+
+// MaybeAssociatedRemote:
+AgentSchedulingGroupHost::MaybeAssociatedRemote::MaybeAssociatedRemote(
+    bool should_associate) {
+  if (should_associate) {
+    remote_ = AssociatedRemote<mojom::AgentSchedulingGroup>();
+  } else {
+    remote_ = Remote<mojom::AgentSchedulingGroup>();
+  }
+}
+
+AgentSchedulingGroupHost::MaybeAssociatedRemote::~MaybeAssociatedRemote() =
+    default;
+
+PendingReceiver<mojom::AgentSchedulingGroup>
+AgentSchedulingGroupHost::MaybeAssociatedRemote::BindNewPipeAndPassReceiver() {
+  return absl::get<Remote<mojom::AgentSchedulingGroup>>(remote_)
+      .BindNewPipeAndPassReceiver();
+}
+
+PendingAssociatedReceiver<mojom::AgentSchedulingGroup>
+AgentSchedulingGroupHost::MaybeAssociatedRemote::
+    BindNewEndpointAndPassReceiver() {
+  return absl::get<AssociatedRemote<mojom::AgentSchedulingGroup>>(remote_)
+      .BindNewEndpointAndPassReceiver();
+}
+
+// AgentSchedulingGroupHost:
+
 // static
 AgentSchedulingGroupHost* AgentSchedulingGroupHost::Get(
     const SiteInstance& instance,
@@ -57,11 +123,26 @@ AgentSchedulingGroupHost* AgentSchedulingGroupHost::Get(
 }
 
 AgentSchedulingGroupHost::AgentSchedulingGroupHost(RenderProcessHost& process)
-    : process_(process) {
+    : AgentSchedulingGroupHost(
+          process,
+          !base::FeatureList::IsEnabled(
+              features::kMbiDetachAgentSchedulingGroupFromChannel)) {}
+
+AgentSchedulingGroupHost::AgentSchedulingGroupHost(RenderProcessHost& process,
+                                                   bool should_associate)
+    : process_(process),
+      receiver_(*this, should_associate),
+      mojo_remote_(should_associate) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  process_.GetRendererInterface()->CreateAgentSchedulingGroup(
-      receiver_.BindNewPipeAndPassRemote(),
-      mojo_remote_.BindNewPipeAndPassReceiver());
+  if (should_associate) {
+    process_.GetRendererInterface()->CreateAssociatedAgentSchedulingGroup(
+        receiver_.BindNewEndpointAndPassRemote(),
+        mojo_remote_.BindNewEndpointAndPassReceiver());
+  } else {
+    process_.GetRendererInterface()->CreateAgentSchedulingGroup(
+        receiver_.BindNewPipeAndPassRemote(),
+        mojo_remote_.BindNewPipeAndPassReceiver());
+  }
 }
 
 // DO NOT USE |process_| HERE! At this point it (or at least parts of it) is no

content/browser/renderer_host/agent_scheduling_group_host.h

@@ -11,8 +11,11 @@
 #include "content/common/associated_interfaces.mojom-forward.h"
 #include "content/common/content_export.h"
 #include "content/common/renderer.mojom-forward.h"
+#include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "mojo/public/cpp/bindings/associated_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "third_party/abseil-cpp/absl/types/variant.h"
 
 namespace IPC {
 class ChannelProxy;
@@ -43,7 +46,8 @@ class CONTENT_EXPORT AgentSchedulingGroupHost
   static AgentSchedulingGroupHost* Get(const SiteInstance& instance,
                                        RenderProcessHost& process);
 
-  // Should not be called explicitly. Use Get() instead.
+  // Utility ctor, forwarding to the main ctor below.
+  // Should not be called explicitly. Use `Get()` instead.
   explicit AgentSchedulingGroupHost(RenderProcessHost& process);
   ~AgentSchedulingGroupHost() override;
 
@@ -64,16 +68,70 @@ class CONTENT_EXPORT AgentSchedulingGroupHost
   void DestroyView(int32_t routing_id);
 
  private:
+  // `MaybeAssociatedReceiver` and `MaybeAssociatedRemote` are temporary helper
+  // classes that allow us to switch between using associated and non-associated
+  // mojo interfaces. This behavior is controlled by the
+  // `kMbiDetachAgentSchedulingGroupFromChannel` feature flag.
+  // Associated interfaces are associated with the IPC channel (transitively,
+  // via the `Renderer` interface), thus preserving cross-agent scheduling group
+  // message order. Non-associated interfaces are independent from each other
+  // and do not preserve message order between agent scheduling groups.
+  // TODO(crbug.com/1111231): Remove these once we can remove the flag.
+  class MaybeAssociatedReceiver {
+   public:
+    MaybeAssociatedReceiver(AgentSchedulingGroupHost& host,
+                            bool should_associate);
+    ~MaybeAssociatedReceiver();
+
+    mojo::PendingRemote<mojom::AgentSchedulingGroupHost>
+    BindNewPipeAndPassRemote() WARN_UNUSED_RESULT;
+    mojo::PendingAssociatedRemote<mojom::AgentSchedulingGroupHost>
+    BindNewEndpointAndPassRemote() WARN_UNUSED_RESULT;
+
+   private:
+    absl::variant<
+        // This is required to make the variant default constructible. After the
+        // ctor body finishes, the variant will never hold this alternative.
+        absl::monostate,
+        mojo::Receiver<mojom::AgentSchedulingGroupHost>,
+        mojo::AssociatedReceiver<mojom::AgentSchedulingGroupHost>>
+        receiver_;
+  };
+
+  class MaybeAssociatedRemote {
+   public:
+    explicit MaybeAssociatedRemote(bool should_associate);
+    ~MaybeAssociatedRemote();
+
+    mojo::PendingReceiver<mojom::AgentSchedulingGroup>
+    BindNewPipeAndPassReceiver() WARN_UNUSED_RESULT;
+    mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup>
+    BindNewEndpointAndPassReceiver() WARN_UNUSED_RESULT;
+
+   private:
+    absl::variant<mojo::Remote<mojom::AgentSchedulingGroup>,
+                  mojo::AssociatedRemote<mojom::AgentSchedulingGroup>>
+        remote_;
+  };
+
+  // Main constructor.
+  // |should_associate| determines whether the `AgentSchedulingGroupHost` and
+  // `AgentSchedulingGroup` mojos should be associated with the `Renderer` or
+  // not. If they are, message order will be preserved across the entire
+  // process. If not, ordering will only be preserved inside an
+  // `AgentSchedulingGroup`.
+  AgentSchedulingGroupHost(RenderProcessHost& process, bool should_associate);
+
   // The RenderProcessHost this AgentSchedulingGroup is assigned to.
   RenderProcessHost& process_;
 
   // Implementation of `mojom::AgentSchedulingGroupHost`, used for responding to
   // calls from the (renderer-side) `AgentSchedulingGroup`.
-  mojo::Receiver<mojom::AgentSchedulingGroupHost> receiver_{this};
+  MaybeAssociatedReceiver receiver_;
 
   // Remote stub of `mojom::AgentSchedulingGroup`, used for sending calls to the
   // (renderer-side) `AgentSchedulingGroup`.
-  mojo::Remote<mojom::AgentSchedulingGroup> mojo_remote_;
+  MaybeAssociatedRemote mojo_remote_;
 };
 
 }  // namespace content

content/common/renderer.mojom

@@ -260,12 +260,24 @@ interface Renderer {
 
   // Tells the renderer to create a new AgentSchedulingGroup, that will
   // communicate via the pending |agent_scheduling_group_host| and
-  // |agent_scheduling_group|.
+  // |agent_scheduling_group|. This will create "independent" interfaces that
+  // are not associated with the IPC channel, which will not guarantee any order
+  // across agent scheduling groups.
   CreateAgentSchedulingGroup(
     pending_remote<AgentSchedulingGroupHost> agent_scheduling_group_host,
     pending_receiver<AgentSchedulingGroup> agent_scheduling_group
   );
 
+  // Tells the renderer to create a new AgentSchedulingGroup, that will
+  // communicate via the pending |agent_scheduling_group_host| and
+  // |agent_scheduling_group|. This will create channel-associated interfaces
+  // that will preserve message ordering across agent scheduling groups.
+  CreateAssociatedAgentSchedulingGroup(
+    pending_associated_remote<AgentSchedulingGroupHost>
+      agent_scheduling_group_host,
+    pending_associated_receiver<AgentSchedulingGroup> agent_scheduling_group
+  );
+
   // Tells the renderer to create a new RenderFrameProxy object with
   // |routing_id|.  |render_view_routing_id| identifies the
   // RenderView to be associated with this proxy. The new proxy's opener should

content/public/common/content_features.cc

@@ -349,6 +349,14 @@ const base::Feature kLogJsConsoleMessages {
 const base::Feature kLowPriorityIframes{"LowPriorityIframes",
                                         base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Removes the association between the `AgentSchedulingGroup` interfaces and the
+// IPC Channel. This will break ordering guarantees between different agent
+// scheduling groups (ordering withing a group is still preserved).
+// DO NOT USE! The feature is not yet fully implemented. See crbug.com/1111231.
+const base::Feature kMbiDetachAgentSchedulingGroupFromChannel{
+    "MbiDetachAgentSchedulingGroupFromChannel",
+    base::FEATURE_DISABLED_BY_DEFAULT};
+
 // If this feature is enabled, media-device enumerations use a cache that is
 // invalidated upon notifications sent by base::SystemMonitor. If disabled, the
 // cache is considered invalid on every enumeration request.

content/public/common/content_features.h

@@ -82,6 +82,8 @@ CONTENT_EXPORT extern const base::Feature kLazyInitializeMediaControls;
 CONTENT_EXPORT extern const base::Feature kLegacyWindowsDWriteFontFallback;
 CONTENT_EXPORT extern const base::Feature kLogJsConsoleMessages;
 CONTENT_EXPORT extern const base::Feature kLowPriorityIframes;
+CONTENT_EXPORT extern const base::Feature
+    kMbiDetachAgentSchedulingGroupFromChannel;
 CONTENT_EXPORT extern const base::Feature kMediaDevicesSystemMonitorCache;
 CONTENT_EXPORT extern const base::Feature kMojoDedicatedThread;
 CONTENT_EXPORT extern const base::Feature kMojoVideoCapture;

content/renderer/agent_scheduling_group.cc

@@ -4,19 +4,92 @@
 
 #include "content/renderer/agent_scheduling_group.h"
 
+#include "base/feature_list.h"
+#include "content/public/common/content_features.h"
+
 namespace content {
 
+using ::mojo::AssociatedReceiver;
+using ::mojo::AssociatedRemote;
+using ::mojo::PendingAssociatedReceiver;
+using ::mojo::PendingAssociatedRemote;
+using ::mojo::PendingReceiver;
+using ::mojo::PendingRemote;
+using ::mojo::Receiver;
+using ::mojo::Remote;
+
+// MaybeAssociatedReceiver:
+AgentSchedulingGroup::MaybeAssociatedReceiver::MaybeAssociatedReceiver(
+    AgentSchedulingGroup& impl,
+    PendingReceiver<mojom::AgentSchedulingGroup> receiver,
+    base::OnceClosure disconnect_handler)
+    : receiver_(absl::in_place_type<Receiver<mojom::AgentSchedulingGroup>>,
+                &impl,
+                std::move(receiver)) {
+  if (disconnect_handler)
+    absl::get<Receiver<mojom::AgentSchedulingGroup>>(receiver_)
+        .set_disconnect_handler(std::move(disconnect_handler));
+}
+
+AgentSchedulingGroup::MaybeAssociatedReceiver::MaybeAssociatedReceiver(
+    AgentSchedulingGroup& impl,
+    PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
+    base::OnceClosure disconnect_handler)
+    : receiver_(
+          absl::in_place_type<AssociatedReceiver<mojom::AgentSchedulingGroup>>,
+          &impl,
+          std::move(receiver)) {
+  if (disconnect_handler)
+    absl::get<AssociatedReceiver<mojom::AgentSchedulingGroup>>(receiver_)
+        .set_disconnect_handler(std::move(disconnect_handler));
+}
+
+AgentSchedulingGroup::MaybeAssociatedReceiver::~MaybeAssociatedReceiver() =
+    default;
+
+// MaybeAssociatedRemote:
+AgentSchedulingGroup::MaybeAssociatedRemote::MaybeAssociatedRemote(
+    PendingRemote<mojom::AgentSchedulingGroupHost> host_remote)
+    : remote_(absl::in_place_type<Remote<mojom::AgentSchedulingGroupHost>>,
+              std::move(host_remote)) {}
+
+AgentSchedulingGroup::MaybeAssociatedRemote::MaybeAssociatedRemote(
+    PendingAssociatedRemote<mojom::AgentSchedulingGroupHost> host_remote)
+    : remote_(absl::in_place_type<
+                  AssociatedRemote<mojom::AgentSchedulingGroupHost>>,
+              std::move(host_remote)) {}
+
+AgentSchedulingGroup::MaybeAssociatedRemote::~MaybeAssociatedRemote() = default;
+
+// AgentSchedulingGroup:
+AgentSchedulingGroup::AgentSchedulingGroup(
+    PendingRemote<mojom::AgentSchedulingGroupHost> host_remote,
+    PendingReceiver<mojom::AgentSchedulingGroup> receiver,
+    base::OnceCallback<void(const AgentSchedulingGroup*)>
+        mojo_disconnect_handler)
+    // TODO(crbug.com/1111231): Mojo interfaces should be associated with
+    // per-ASG task runners instead of default.
+    : receiver_(*this,
+                std::move(receiver),
+                base::BindOnce(std::move(mojo_disconnect_handler), this)),
+      host_remote_(std::move(host_remote)) {
+  DCHECK(base::FeatureList::IsEnabled(
+      features::kMbiDetachAgentSchedulingGroupFromChannel));
+}
+
 AgentSchedulingGroup::AgentSchedulingGroup(
-    mojo::PendingRemote<mojom::AgentSchedulingGroupHost> host_remote,
-    mojo::PendingReceiver<mojom::AgentSchedulingGroup> receiver,
+    PendingAssociatedRemote<mojom::AgentSchedulingGroupHost> host_remote,
+    PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
     base::OnceCallback<void(const AgentSchedulingGroup*)>
         mojo_disconnect_handler)
     // TODO(crbug.com/1111231): Mojo interfaces should be associated with
     // per-ASG task runners instead of default.
-    : receiver_(this, std::move(receiver)),
+    : receiver_(*this,
+                std::move(receiver),
+                base::BindOnce(std::move(mojo_disconnect_handler), this)),
       host_remote_(std::move(host_remote)) {
-  receiver_.set_disconnect_handler(
-      base::BindOnce(std::move(mojo_disconnect_handler), this));
+  DCHECK(!base::FeatureList::IsEnabled(
+      features::kMbiDetachAgentSchedulingGroupFromChannel));
 }
 
 AgentSchedulingGroup::~AgentSchedulingGroup() = default;

content/renderer/agent_scheduling_group.h

@@ -8,8 +8,11 @@
 #include "base/callback.h"
 #include "content/common/agent_scheduling_group.mojom.h"
 #include "content/common/content_export.h"
+#include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "mojo/public/cpp/bindings/associated_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "third_party/abseil-cpp/absl/types/variant.h"
 
 namespace content {
 
@@ -27,6 +30,12 @@ class CONTENT_EXPORT AgentSchedulingGroup : public mojom::AgentSchedulingGroup {
       mojo::PendingReceiver<mojom::AgentSchedulingGroup> receiver,
       base::OnceCallback<void(const AgentSchedulingGroup*)>
           mojo_disconnect_handler);
+  AgentSchedulingGroup(
+      mojo::PendingAssociatedRemote<mojom::AgentSchedulingGroupHost>
+          host_remote,
+      mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
+      base::OnceCallback<void(const AgentSchedulingGroup*)>
+          mojo_disconnect_handler);
   ~AgentSchedulingGroup() override;
 
   AgentSchedulingGroup(const AgentSchedulingGroup&) = delete;
@@ -35,13 +44,55 @@ class CONTENT_EXPORT AgentSchedulingGroup : public mojom::AgentSchedulingGroup {
   AgentSchedulingGroup& operator=(const AgentSchedulingGroup&&) = delete;
 
  private:
+  // `MaybeAssociatedReceiver` and `MaybeAssociatedRemote` are temporary helper
+  // classes that allow us to switch between using associated and non-associated
+  // mojo interfaces. This behavior is controlled by the
+  // `kMbiDetachAgentSchedulingGroupFromChannel` feature flag.
+  // Associated interfaces are associated with the IPC channel (transitively,
+  // via the `Renderer` interface), thus preserving cross-agent scheduling group
+  // message order. Non-associated interfaces are independent from each other
+  // and do not preserve message order between agent scheduling groups.
+  // TODO(crbug.com/1111231): Remove these once we can remove the flag.
+  class MaybeAssociatedReceiver {
+   public:
+    MaybeAssociatedReceiver(
+        AgentSchedulingGroup& impl,
+        mojo::PendingReceiver<mojom::AgentSchedulingGroup> receiver,
+        base::OnceClosure disconnect_handler);
+    MaybeAssociatedReceiver(
+        AgentSchedulingGroup& impl,
+        mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
+        base::OnceClosure disconnect_handler);
+    ~MaybeAssociatedReceiver();
+
+   private:
+    absl::variant<mojo::Receiver<mojom::AgentSchedulingGroup>,
+                  mojo::AssociatedReceiver<mojom::AgentSchedulingGroup>>
+        receiver_;
+  };
+
+  class MaybeAssociatedRemote {
+   public:
+    explicit MaybeAssociatedRemote(
+        mojo::PendingRemote<mojom::AgentSchedulingGroupHost> host_remote);
+    explicit MaybeAssociatedRemote(
+        mojo::PendingAssociatedRemote<mojom::AgentSchedulingGroupHost>
+            host_remote);
+    ~MaybeAssociatedRemote();
+
+   private:
+    absl::variant<mojo::Remote<mojom::AgentSchedulingGroupHost>,
+                  mojo::AssociatedRemote<mojom::AgentSchedulingGroupHost>>
+        remote_;
+  };
+
   // Implementation of `mojom::AgentSchedulingGroup`, used for responding to
   // calls from the (browser-side) `AgentSchedulingGroupHost`.
-  mojo::Receiver<mojom::AgentSchedulingGroup> receiver_;
+  MaybeAssociatedReceiver receiver_;
 
   // Remote stub of mojom::AgentSchedulingGroupHost, used for sending calls to
   // the (browser-side) AgentSchedulingGroupHost.
-  mojo::Remote<mojom::AgentSchedulingGroupHost> host_remote_;
+  MaybeAssociatedRemote host_remote_;
 };
 
 }  // namespace content

content/renderer/render_thread_impl.cc

@@ -1875,6 +1875,16 @@ void RenderThreadImpl::CreateAgentSchedulingGroup(
       remove_agent_scheduling_group_callback_));
 }
 
+void RenderThreadImpl::CreateAssociatedAgentSchedulingGroup(
+    mojo::PendingAssociatedRemote<mojom::AgentSchedulingGroupHost>
+        agent_scheduling_group_host,
+    mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup>
+        agent_scheduling_group) {
+  agent_scheduling_groups_.emplace(std::make_unique<AgentSchedulingGroup>(
+      std::move(agent_scheduling_group_host), std::move(agent_scheduling_group),
+      remove_agent_scheduling_group_callback_));
+}
+
 void RenderThreadImpl::CreateFrameProxy(
     int32_t routing_id,
     int32_t render_view_routing_id,

content/renderer/render_thread_impl.h

@@ -448,6 +448,11 @@ class CONTENT_EXPORT RenderThreadImpl
           agent_scheduling_group_host,
       mojo::PendingReceiver<mojom::AgentSchedulingGroup> agent_scheduling_group)
       override;
+  void CreateAssociatedAgentSchedulingGroup(
+      mojo::PendingAssociatedRemote<mojom::AgentSchedulingGroupHost>
+          agent_scheduling_group_host,
+      mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup>
+          agent_scheduling_group) override;
   void CreateFrameProxy(
       int32_t routing_id,
       int32_t render_view_routing_id,