Remove AgentSchedulingGroup disconnect handler.

Currently, renderer-side objects (RenderFrame, RenderView, etc.) assume
the AgentSchedulingGroup outlives them, but this is not the case if the
disconnect handler is called before they are destroyed.

After this CL, the ASG's lifetime will match the RenderThread's, and it
will be destroyed when the RenderThread is destroyed. This is OK for
now, since we currently only have a single ASG per process and the
host's lifetime matches the RenderProcessHost's. In the meantime,
kouhei@ is working on properly shutting down ASGs via an IPC, similar to
other objects.

Bug: 1137580, 1138942
Change-Id: Iab02cecf3cd828a0b84f2bc29e451d1a591422ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2478622
Commit-Queue: Tal Pressman <talp@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819238}

content/public/test/render_view_test.cc

@@ -236,8 +236,7 @@ std::unique_ptr<AgentSchedulingGroup> CreateAgentSchedulingGroup(
 
   return std::make_unique<MockAgentSchedulingGroup>(
       render_thread, std::move(agent_scheduling_group_host),
-      std::move(agent_scheduling_group_receiver),
-      base::OnceCallback<void(const AgentSchedulingGroup*)>());
+      std::move(agent_scheduling_group_receiver));
 }
 
 }  // namespace
@@ -285,8 +284,7 @@ RenderViewTest::RendererBlinkPlatformImplTestOverride::
 }
 
 RenderViewTest::RendererBlinkPlatformImplTestOverride::
-    ~RendererBlinkPlatformImplTestOverride() {
-}
+    ~RendererBlinkPlatformImplTestOverride() = default;
 
 RendererBlinkPlatformImpl*
 RenderViewTest::RendererBlinkPlatformImplTestOverride::Get() const {
@@ -696,7 +694,6 @@ void RenderViewTest::SimulatePointClick(const gfx::Point& point) {
       base::DoNothing());
 }
 
-
 bool RenderViewTest::SimulateElementRightClick(const std::string& element_id) {
   gfx::Rect bounds = GetElementBounds(element_id);
   if (bounds.IsEmpty())
@@ -761,8 +758,7 @@ void RenderViewTest::Reload(const GURL& url) {
       blink::DocumentUpdateReason::kTest);
 }
 
-void RenderViewTest::Resize(gfx::Size new_size,
-                            bool is_fullscreen_granted) {
+void RenderViewTest::Resize(gfx::Size new_size, bool is_fullscreen_granted) {
   blink::VisualProperties visual_properties;
   visual_properties.screen_info = blink::ScreenInfo();
   visual_properties.new_size = new_size;

content/renderer/agent_scheduling_group.cc

@@ -34,28 +34,18 @@ RenderThreadImpl& ToImpl(RenderThread& render_thread) {
 // MaybeAssociatedReceiver:
 AgentSchedulingGroup::MaybeAssociatedReceiver::MaybeAssociatedReceiver(
     AgentSchedulingGroup& impl,
-    PendingReceiver<mojom::AgentSchedulingGroup> receiver,
-    base::OnceClosure disconnect_handler)
+    PendingReceiver<mojom::AgentSchedulingGroup> receiver)
     : 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));
-}
+                std::move(receiver)) {}
 
 AgentSchedulingGroup::MaybeAssociatedReceiver::MaybeAssociatedReceiver(
     AgentSchedulingGroup& impl,
-    PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
-    base::OnceClosure disconnect_handler)
+    PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver)
     : 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));
-}
+          std::move(receiver)) {}
 
 AgentSchedulingGroup::MaybeAssociatedReceiver::~MaybeAssociatedReceiver() =
     default;
@@ -83,17 +73,11 @@ AgentSchedulingGroup::MaybeAssociatedRemote::get() {
 AgentSchedulingGroup::AgentSchedulingGroup(
     RenderThread& render_thread,
     PendingRemote<mojom::AgentSchedulingGroupHost> host_remote,
-    PendingReceiver<mojom::AgentSchedulingGroup> receiver,
-    base::OnceCallback<void(const AgentSchedulingGroup*)>
-        mojo_disconnect_handler)
+    PendingReceiver<mojom::AgentSchedulingGroup> receiver)
     // TODO(crbug.com/1111231): Mojo interfaces should be associated with
     // per-ASG task runners instead of default.
     : render_thread_(render_thread),
-      receiver_(*this,
-                std::move(receiver),
-                mojo_disconnect_handler
-                    ? base::BindOnce(std::move(mojo_disconnect_handler), this)
-                    : base::OnceClosure()),
+      receiver_(*this, std::move(receiver)),
       host_remote_(std::move(host_remote)) {
   DCHECK(base::FeatureList::IsEnabled(
       features::kMbiDetachAgentSchedulingGroupFromChannel));
@@ -102,17 +86,11 @@ AgentSchedulingGroup::AgentSchedulingGroup(
 AgentSchedulingGroup::AgentSchedulingGroup(
     RenderThread& render_thread,
     PendingAssociatedRemote<mojom::AgentSchedulingGroupHost> host_remote,
-    PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
-    base::OnceCallback<void(const AgentSchedulingGroup*)>
-        mojo_disconnect_handler)
+    PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver)
     // TODO(crbug.com/1111231): Mojo interfaces should be associated with
     // per-ASG task runners instead of default.
     : render_thread_(render_thread),
-      receiver_(*this,
-                std::move(receiver),
-                mojo_disconnect_handler
-                    ? base::BindOnce(std::move(mojo_disconnect_handler), this)
-                    : base::OnceClosure()),
+      receiver_(*this, std::move(receiver)),
       host_remote_(std::move(host_remote)) {
   DCHECK(!base::FeatureList::IsEnabled(
       features::kMbiDetachAgentSchedulingGroupFromChannel));

content/renderer/agent_scheduling_group.h

@@ -5,7 +5,6 @@
 #ifndef CONTENT_RENDERER_AGENT_SCHEDULING_GROUP_H_
 #define CONTENT_RENDERER_AGENT_SCHEDULING_GROUP_H_
 
-#include "base/callback.h"
 #include "content/common/agent_scheduling_group.mojom.h"
 #include "content/common/associated_interfaces.mojom.h"
 #include "content/common/content_export.h"
@@ -36,21 +35,15 @@ class CONTENT_EXPORT AgentSchedulingGroup
       public mojom::RouteProvider,
       public blink::mojom::AssociatedInterfaceProvider {
  public:
-  // |mojo_disconnect_handler| is an optional callback that will be called with
-  // |this| when |receiver| is disconnected.
   AgentSchedulingGroup(
       RenderThread& render_thread,
       mojo::PendingRemote<mojom::AgentSchedulingGroupHost> host_remote,
-      mojo::PendingReceiver<mojom::AgentSchedulingGroup> receiver,
-      base::OnceCallback<void(const AgentSchedulingGroup*)>
-          mojo_disconnect_handler);
+      mojo::PendingReceiver<mojom::AgentSchedulingGroup> receiver);
   AgentSchedulingGroup(
       RenderThread& render_thread,
       mojo::PendingAssociatedRemote<mojom::AgentSchedulingGroupHost>
           host_remote,
-      mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
-      base::OnceCallback<void(const AgentSchedulingGroup*)>
-          mojo_disconnect_handler);
+      mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver);
   ~AgentSchedulingGroup() override;
 
   AgentSchedulingGroup(const AgentSchedulingGroup&) = delete;
@@ -79,12 +72,10 @@ class CONTENT_EXPORT AgentSchedulingGroup
    public:
     MaybeAssociatedReceiver(
         AgentSchedulingGroup& impl,
-        mojo::PendingReceiver<mojom::AgentSchedulingGroup> receiver,
-        base::OnceClosure disconnect_handler);
+        mojo::PendingReceiver<mojom::AgentSchedulingGroup> receiver);
     MaybeAssociatedReceiver(
         AgentSchedulingGroup& impl,
-        mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
-        base::OnceClosure disconnect_handler);
+        mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver);
     ~MaybeAssociatedReceiver();
 
    private:

content/renderer/mock_agent_scheduling_group.cc

@@ -12,13 +12,10 @@ namespace content {
 MockAgentSchedulingGroup::MockAgentSchedulingGroup(
     RenderThread& render_thread,
     mojo::PendingAssociatedRemote<mojom::AgentSchedulingGroupHost> host_remote,
-    mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
-    base::OnceCallback<void(const AgentSchedulingGroup*)>
-        mojo_disconnect_handler)
+    mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver)
     : AgentSchedulingGroup(render_thread,
                            std::move(host_remote),
-                           std::move(receiver),
-                           std::move(mojo_disconnect_handler)) {}
+                           std::move(receiver)) {}
 
 mojom::RouteProvider* MockAgentSchedulingGroup::GetRemoteRouteProvider() {
   DCHECK(!RenderThreadImpl::current());

content/renderer/mock_agent_scheduling_group.h

@@ -28,9 +28,7 @@ class MockAgentSchedulingGroup : public AgentSchedulingGroup {
       RenderThread& render_thread,
       mojo::PendingAssociatedRemote<mojom::AgentSchedulingGroupHost>
           host_remote,
-      mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver,
-      base::OnceCallback<void(const AgentSchedulingGroup*)>
-          mojo_disconnect_handler);
+      mojo::PendingAssociatedReceiver<mojom::AgentSchedulingGroup> receiver);
 
   mojom::RouteProvider* GetRemoteRouteProvider() override;
 };

content/renderer/render_thread_impl.cc

@@ -575,10 +575,6 @@ RenderThreadImpl::RenderThreadImpl(
 void RenderThreadImpl::Init() {
   TRACE_EVENT0("startup", "RenderThreadImpl::Init");
 
-  remove_agent_scheduling_group_callback_ =
-      base::BindRepeating(&RenderThreadImpl::RemoveAgentSchedulingGroup,
-                          weak_factory_.GetWeakPtr());
-
   GetContentClient()->renderer()->PostIOThreadCreated(GetIOTaskRunner().get());
 
   base::trace_event::TraceLog::GetInstance()->SetThreadSortIndex(
@@ -1709,22 +1705,13 @@ void RenderThreadImpl::CreateFrame(mojom::CreateFrameParamsPtr params,
       params->has_committed_real_load);
 }
 
-void RenderThreadImpl::RemoveAgentSchedulingGroup(
-    const AgentSchedulingGroup* agent_scheduling_group) {
-  DCHECK(agent_scheduling_group);
-  DCHECK(base::Contains(agent_scheduling_groups_, agent_scheduling_group));
-  auto it = agent_scheduling_groups_.find(agent_scheduling_group);
-  agent_scheduling_groups_.erase(it);
-}
-
 void RenderThreadImpl::CreateAgentSchedulingGroup(
     mojo::PendingRemote<mojom::AgentSchedulingGroupHost>
         agent_scheduling_group_host,
     mojo::PendingReceiver<mojom::AgentSchedulingGroup> agent_scheduling_group) {
   agent_scheduling_groups_.emplace(std::make_unique<AgentSchedulingGroup>(
       *this, std::move(agent_scheduling_group_host),
-      std::move(agent_scheduling_group),
-      remove_agent_scheduling_group_callback_));
+      std::move(agent_scheduling_group)));
 }
 
 void RenderThreadImpl::CreateAssociatedAgentSchedulingGroup(
@@ -1734,8 +1721,7 @@ void RenderThreadImpl::CreateAssociatedAgentSchedulingGroup(
         agent_scheduling_group) {
   agent_scheduling_groups_.emplace(std::make_unique<AgentSchedulingGroup>(
       *this, std::move(agent_scheduling_group_host),
-      std::move(agent_scheduling_group),
-      remove_agent_scheduling_group_callback_));
+      std::move(agent_scheduling_group)));
 }
 
 void RenderThreadImpl::CreateFrameProxy(

content/renderer/render_thread_impl.h

@@ -514,11 +514,6 @@ class CONTENT_EXPORT RenderThreadImpl
   void OnRendererInterfaceReceiver(
       mojo::PendingAssociatedReceiver<mojom::Renderer> receiver);
 
-  void RemoveAgentSchedulingGroup(
-      const AgentSchedulingGroup* agent_scheduling_group);
-  base::RepeatingCallback<void(const AgentSchedulingGroup*)>
-      remove_agent_scheduling_group_callback_;
-
   std::unique_ptr<discardable_memory::ClientDiscardableSharedMemoryManager>
       discardable_memory_allocator_;