Remove RenderFrameHostObserver

Introducing a new observer interface along with WebContentObserver
should have a design doc and discussion, with a clear statement of the
problems that could be resolved. It would be more reasonable to remove
the RenderFrameHostObserver at this time being.

Bug: 1409410

Change-Id: I50f8907363c1b37e60d724eb40fe94bb27163ac2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4187600
Commit-Queue: Thomas Nguyen <tungnh@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1100329}

content/browser/permissions/permission_controller_impl.cc

@@ -705,6 +705,9 @@ bool PermissionControllerImpl::IsSubscribedToPermissionChangeEvent(
     RenderFrameHost* render_frame_host) {
   PermissionServiceContext* permission_service_context =
       PermissionServiceContext::GetForCurrentDocument(render_frame_host);
+  if (!permission_service_context) {
+    return false;
+  }
 
   return permission_service_context->GetOnchangeEventListeners().find(
              permission) !=

content/browser/permissions/permission_service_context.cc

@@ -122,17 +122,25 @@ class PermissionServiceContext::PermissionSubscription {
 };
 
 // static
-PermissionServiceContext* PermissionServiceContext::GetForCurrentDocument(
+PermissionServiceContext*
+PermissionServiceContext::GetOrCreateForCurrentDocument(
     RenderFrameHost* render_frame_host) {
   return &DocumentPermissionServiceContextHolder::GetOrCreateForCurrentDocument(
               render_frame_host)
               ->permission_service_context;
 }
 
+// static
+PermissionServiceContext* PermissionServiceContext::GetForCurrentDocument(
+    RenderFrameHost* render_frame_host) {
+  auto* holder = DocumentPermissionServiceContextHolder::GetForCurrentDocument(
+      render_frame_host);
+  return holder ? &holder->permission_service_context : nullptr;
+}
+
 PermissionServiceContext::PermissionServiceContext(
     RenderFrameHost* render_frame_host)
     : render_frame_host_(render_frame_host), render_process_host_(nullptr) {
-  render_frame_host->AddObserver(this);
   render_frame_host->GetProcess()->AddObserver(this);
 }
 
@@ -142,7 +150,6 @@ PermissionServiceContext::PermissionServiceContext(
 
 PermissionServiceContext::~PermissionServiceContext() {
   if (render_frame_host_) {
-    render_frame_host_->RemoveObserver(this);
     render_frame_host_->GetProcess()->RemoveObserver(this);
   }
 }
@@ -225,16 +232,15 @@ void PermissionServiceContext::RenderProcessHostDestroyed(
   // earlier so we need to listen to this event so we can do our clean up as
   // well.
   host->RemoveObserver(this);
-  render_frame_host_->RemoveObserver(this);
 }
 
-void PermissionServiceContext::DidEnterBackForwardCache() {
+void PermissionServiceContext::StoreStatusAtBFCacheEntry() {
   for (auto& iter : subscriptions_) {
     iter.second->StoreStatusAtBFCacheEntry();
   }
 }
 
-void PermissionServiceContext::DidRestoreFromBackForwardCache() {
+void PermissionServiceContext::NotifyPermissionStatusChangedIfNeeded() {
   for (auto& iter : subscriptions_) {
     iter.second->NotifyPermissionStatusChangedIfNeeded();
   }

content/browser/permissions/permission_service_context.h

@@ -11,7 +11,6 @@
 #include "base/memory/raw_ptr.h"
 #include "content/public/browser/document_user_data.h"
 #include "content/public/browser/permission_controller.h"
-#include "content/public/browser/render_frame_host_observer.h"
 #include "content/public/browser/render_process_host_observer.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
@@ -44,16 +43,20 @@ class RenderProcessHost;
 // created via the DocumentUserData static factories, as these
 // instances are deleted when a new document is committed.
 class CONTENT_EXPORT PermissionServiceContext
-    : public RenderProcessHostObserver,
-      public RenderFrameHostObserver {
+    : public RenderProcessHostObserver {
  public:
   explicit PermissionServiceContext(RenderProcessHost* render_process_host);
   PermissionServiceContext(const PermissionServiceContext&) = delete;
   PermissionServiceContext& operator=(const PermissionServiceContext&) = delete;
   ~PermissionServiceContext() override;
 
   // Return PermissionServiceContext associated with the current document in the
-  // given RenderFrameHost, lazily creatin gone, if needed.
+  // given RenderFrameHost, lazily creating one, if needed.
+  static PermissionServiceContext* GetOrCreateForCurrentDocument(
+      RenderFrameHost* render_frame_host);
+
+  // Return PermissionServiceContext associated with the current document.
+  // Return null if there's no associated one.
   static PermissionServiceContext* GetForCurrentDocument(
       RenderFrameHost* render_frame_host);
 
@@ -87,9 +90,8 @@ class CONTENT_EXPORT PermissionServiceContext
   // RenderProcessHostObserver:
   void RenderProcessHostDestroyed(RenderProcessHost* host) override;
 
-  // RenderFrameHostObserver:
-  void DidEnterBackForwardCache() override;
-  void DidRestoreFromBackForwardCache() override;
+  void StoreStatusAtBFCacheEntry();
+  void NotifyPermissionStatusChangedIfNeeded();
 
   std::set<blink::PermissionType>& GetOnchangeEventListeners() {
     return onchange_event_listeners_;

content/browser/permissions/permission_service_context_unittest.cc

@@ -11,7 +11,9 @@
 #include "content/browser/permissions/permission_controller_impl.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
 #include "content/public/browser/weak_document_ptr.h"
+#include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
+#include "content/test/test_render_frame_host.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/permissions/permission_utils.h"
 #include "third_party/blink/public/mojom/permissions/permission.mojom.h"
@@ -74,7 +76,8 @@ class PermissionServiceContextTest : public RenderViewHostTestHarness {
     render_frame_host_impl_ =
         static_cast<RenderFrameHostImpl*>(render_frame_host);
     permission_service_context_ =
-        PermissionServiceContext::GetForCurrentDocument(render_frame_host);
+        PermissionServiceContext::GetOrCreateForCurrentDocument(
+            render_frame_host);
   }
 
   std::unique_ptr<TestPermissionObserver> CreateSubscription(
@@ -141,9 +144,28 @@ TEST_F(PermissionServiceContextTest,
   // After dispatching changed events when the render frame host is active,
   // the event counter should increment as expected.
   EXPECT_EQ(observer->change_event_count(), 1U);
+
+  // Same origin child sub-frame should also receive changed events but should
+  // not double increment the parent's counter.
+  RenderFrameHost* child =
+      RenderFrameHostTester::For(render_frame_host())->AppendChild("");
+  RenderFrameHostTester::For(child)->InitializeRenderFrameIfNeeded();
+  auto navigation_simulator =
+      content::NavigationSimulator::CreateRendererInitiated(GURL(kTestUrl),
+                                                            child);
+  navigation_simulator->Commit();
+  child = navigation_simulator->GetFinalRenderFrameHost();
+  auto* permission_service_context =
+      PermissionServiceContext::GetOrCreateForCurrentDocument(child);
+  auto observer_child = std::make_unique<TestPermissionObserver>();
+  permission_service_context->CreateSubscription(
+      PermissionType::GEOLOCATION, url::Origin::Create(GURL(kTestUrl)),
+      blink::mojom::PermissionStatus::ASK, blink::mojom::PermissionStatus::ASK,
+      observer_child->GetRemote());
   SimulatePermissionChangedEvent(blink::PermissionType::GEOLOCATION,
                                  blink::mojom::PermissionStatus::ASK);
   EXPECT_EQ(observer->change_event_count(), 2U);
+  EXPECT_EQ(observer_child->change_event_count(), 1U);
 
   // Simulate the render frame host is put into the back/forward cache
   render_frame_host()->DidEnterBackForwardCache();
@@ -158,6 +180,7 @@ TEST_F(PermissionServiceContextTest,
 
   // Now the change events should not should increment the counter.
   EXPECT_EQ(observer->change_event_count(), 2U);
+  EXPECT_EQ(observer_child->change_event_count(), 1U);
 
   // Simulate the render frame host is back to active state by setting the
   // lifecycle state.
@@ -171,6 +194,7 @@ TEST_F(PermissionServiceContextTest,
   // Since the render frame host is active, the dispatched events should
   // increment the counter.
   EXPECT_EQ(observer->change_event_count(), 3U);
+  EXPECT_EQ(observer_child->change_event_count(), 2U);
 }
 
 TEST_F(PermissionServiceContextTest,

content/browser/renderer_host/render_frame_host_impl.cc

@@ -172,7 +172,6 @@
 #include "content/public/browser/document_service_internal.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/global_routing_id.h"
-#include "content/public/browser/render_frame_host_observer.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/shared_cors_origin_access_list.h"
@@ -1885,8 +1884,9 @@ void RenderFrameHostImpl::DidEnterBackForwardCacheInternal() {
   GetProcess()->PauseSocketManagerForRenderFrameHost(GetGlobalId());
 #endif  // BUILDFLAG(IS_P2P_ENABLED)
 
-  for (auto& observer : observers_) {
-    observer.DidEnterBackForwardCache();
+  if (auto* permission_service_context =
+          PermissionServiceContext::GetForCurrentDocument(this)) {
+    permission_service_context->StoreStatusAtBFCacheEntry();
   }
 }
 
@@ -3635,14 +3635,6 @@ bool RenderFrameHostImpl::IsCredentialless() const {
   return policy_container_host_->policies().is_credentialless;
 }
 
-void RenderFrameHostImpl::AddObserver(RenderFrameHostObserver* observer) {
-  observers_.AddObserver(observer);
-}
-
-void RenderFrameHostImpl::RemoveObserver(RenderFrameHostObserver* observer) {
-  observers_.RemoveObserver(observer);
-}
-
 void RenderFrameHostImpl::OnCreateChildFrame(
     int new_routing_id,
     mojo::PendingAssociatedRemote<mojom::Frame> frame_remote,
@@ -11069,7 +11061,7 @@ void RenderFrameHostImpl::CreateBucketManagerHost(
 
 void RenderFrameHostImpl::CreatePermissionService(
     mojo::PendingReceiver<blink::mojom::PermissionService> receiver) {
-  PermissionServiceContext::GetForCurrentDocument(this)->CreateService(
+  PermissionServiceContext::GetOrCreateForCurrentDocument(this)->CreateService(
       std::move(receiver));
 }
 
@@ -14088,12 +14080,6 @@ void RenderFrameHostImpl::SetLifecycleState(LifecycleStateImpl new_state) {
         }
       }
     }
-
-    if (lifecycle_state_ == LifecycleStateImpl::kInBackForwardCache) {
-      for (auto& observer : observers_) {
-        observer.DidRestoreFromBackForwardCache();
-      }
-    }
   }
 
   if (lifecycle_state() == LifecycleStateImpl::kInBackForwardCache)
@@ -14132,6 +14118,14 @@ void RenderFrameHostImpl::SetLifecycleState(LifecycleStateImpl new_state) {
                                              new_lifecycle_state);
     }
   }
+
+  if (new_state == LifecycleStateImpl::kActive &&
+      old_state == LifecycleStateImpl::kInBackForwardCache) {
+    if (auto* permission_service_context =
+            PermissionServiceContext::GetForCurrentDocument(this)) {
+      permission_service_context->NotifyPermissionStatusChangedIfNeeded();
+    }
+  }
 }
 
 void RenderFrameHostImpl::RecordDocumentCreatedUkmEvent(

content/browser/renderer_host/render_frame_host_impl.h

@@ -2062,9 +2062,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
 
   bool IsCredentialless() const override;
 
-  void AddObserver(RenderFrameHostObserver* observer) override;
-  void RemoveObserver(RenderFrameHostObserver* observer) override;
-
   bool is_fenced_frame_root_originating_from_opaque_url() const {
     return is_fenced_frame_root_originating_from_opaque_url_;
   }
@@ -4778,9 +4775,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // to change across MPArch activations like prerendering.
   const base::UnguessableToken devtools_frame_token_;
 
-  // The observers watching our state changed event.
-  base::ObserverList<RenderFrameHostObserver> observers_;
-
   // BrowserInterfaceBroker implementation through which this
   // RenderFrameHostImpl exposes document-scoped Mojo services to the currently
   // active document in the corresponding RenderFrame.

content/public/browser/BUILD.gn

@@ -321,8 +321,6 @@ source_set("browser_sources") {
     "reduce_accept_language_controller_delegate.h",
     "reload_type.h",
     "render_frame_host.h",
-    "render_frame_host_observer.cc",
-    "render_frame_host_observer.h",
     "render_frame_host_receiver_set.h",
     "render_frame_metadata_provider.h",
     "render_process_host.h",

content/public/browser/render_frame_host.h

@@ -108,7 +108,6 @@ namespace content {
 class BrowserContext;
 class DocumentRef;
 struct GlobalRenderFrameHostId;
-class RenderFrameHostObserver;
 class RenderProcessHost;
 class RenderViewHost;
 class RenderWidgetHost;
@@ -1055,11 +1054,6 @@ class CONTENT_EXPORT RenderFrameHost : public IPC::Listener,
   // Updated on every cross-document navigation.
   virtual bool IsCredentialless() const = 0;
 
-  // Add and remove observers for state changed events. Observers are
-  // responsible to remove themselves from observer list before they go away.
-  virtual void AddObserver(RenderFrameHostObserver* observer) = 0;
-  virtual void RemoveObserver(RenderFrameHostObserver* observer) = 0;
-
  private:
   // This interface should only be implemented inside content.
   friend class RenderFrameHostImpl;