Move ContentSettingsManagerImpl to be a per-process rather than per-frame object.

This is an attempt to avoid a renderer hang that may be explained by trying to
get an interface via BrowserInterfaceBroker (BIB) and then make synchronous calls
on that interface.

This is an experimental change. It is unfortunate to introduce render_frame_id
parameters here, and my intent is to come back and remove them once I understand
how to do so without the observed hang.

The experiment with this CL is to confirm that the observed hang goes away. If it
does, then it confirms that there is some interaction with BIB that is causing the
issue. If it does not, then we know BIB is off the hook, and we can look elsewhere.

Bug: 1023519
Change-Id: I696b63f057605b3cd5fe7f0c8182b044f2a60578
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1949036Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Darin Fisher <darin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721382}

chrome/browser/chrome_browser_interface_binders.cc

@@ -10,7 +10,6 @@
 #include "build/build_config.h"
 #include "chrome/browser/accessibility/accessibility_labels_service.h"
 #include "chrome/browser/accessibility/accessibility_labels_service_factory.h"
-#include "chrome/browser/content_settings/content_settings_manager_impl.h"
 #include "chrome/browser/dom_distiller/dom_distiller_service_factory.h"
 #include "chrome/browser/language/translate_frame_binder.h"
 #include "chrome/browser/navigation_predictor/navigation_predictor.h"
@@ -202,9 +201,6 @@ void BindBeforeUnloadControl(
 
 void PopulateChromeFrameBinders(
     service_manager::BinderMapWithContext<content::RenderFrameHost*>* map) {
-  map->Add<mojom::ContentSettingsManager>(
-      base::BindRepeating(&ContentSettingsManagerImpl::Create));
-
   map->Add<image_annotation::mojom::Annotator>(
       base::BindRepeating(&BindImageAnnotator));
 

chrome/browser/chrome_content_browser_client_receiver_bindings.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/cache_stats_recorder.h"
 #include "chrome/browser/chrome_browser_interface_binders.h"
 #include "chrome/browser/chrome_content_browser_client_parts.h"
+#include "chrome/browser/content_settings/content_settings_manager_impl.h"
 #include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings.h"
 #include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings_factory.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
@@ -250,6 +251,13 @@ void ChromeContentBrowserClient::BindGpuHostReceiver(
 void ChromeContentBrowserClient::BindHostReceiverForRenderer(
     content::RenderProcessHost* render_process_host,
     mojo::GenericPendingReceiver receiver) {
+  if (auto host_receiver =
+          receiver.As<chrome::mojom::ContentSettingsManager>()) {
+    chrome::ContentSettingsManagerImpl::Create(render_process_host,
+                                               std::move(host_receiver));
+    return;
+  }
+
   if (auto host_receiver =
           receiver.As<network_hints::mojom::NetworkHintsHandler>()) {
     predictors::NetworkHintsHandlerImpl::Create(render_process_host->GetID(),

chrome/browser/content_settings/content_settings_manager_impl.cc

@@ -87,10 +87,10 @@ ContentSettingsManagerImpl::~ContentSettingsManagerImpl() = default;
 
 // static
 void ContentSettingsManagerImpl::Create(
-    content::RenderFrameHost* render_frame_host,
+    content::RenderProcessHost* render_process_host,
     mojo::PendingReceiver<mojom::ContentSettingsManager> receiver) {
   mojo::MakeSelfOwnedReceiver(
-      base::WrapUnique(new ContentSettingsManagerImpl(render_frame_host)),
+      base::WrapUnique(new ContentSettingsManagerImpl(render_process_host)),
       std::move(receiver));
 }
 
@@ -102,6 +102,7 @@ void ContentSettingsManagerImpl::Clone(
 }
 
 void ContentSettingsManagerImpl::AllowStorageAccess(
+    int32_t render_frame_id,
     StorageType storage_type,
     const url::Origin& origin,
     const GURL& site_for_cookies,
@@ -115,35 +116,35 @@ void ContentSettingsManagerImpl::AllowStorageAccess(
   switch (storage_type) {
     case StorageType::DATABASE:
       TabSpecificContentSettings::WebDatabaseAccessed(
-          render_process_id_, render_frame_id_, url, !allowed);
+          render_process_id_, render_frame_id, url, !allowed);
       break;
     case StorageType::LOCAL_STORAGE:
-      OnDomStorageAccessed(render_process_id_, render_frame_id_, url,
+      OnDomStorageAccessed(render_process_id_, render_frame_id, url,
                            top_frame_origin.GetURL(), true, !allowed);
       break;
     case StorageType::SESSION_STORAGE:
-      OnDomStorageAccessed(render_process_id_, render_frame_id_, url,
+      OnDomStorageAccessed(render_process_id_, render_frame_id, url,
                            top_frame_origin.GetURL(), false, !allowed);
       break;
     case StorageType::FILE_SYSTEM:
 #if BUILDFLAG(ENABLE_EXTENSIONS)
       if (extensions::WebViewRendererState::GetInstance()->IsGuest(
               render_process_id_)) {
-        OnFileSystemAccessedInGuestView(render_process_id_, render_frame_id_,
+        OnFileSystemAccessedInGuestView(render_process_id_, render_frame_id,
                                         url, allowed, std::move(callback));
         return;
       }
 #endif
       TabSpecificContentSettings::FileSystemAccessed(
-          render_process_id_, render_frame_id_, url, !allowed);
+          render_process_id_, render_frame_id, url, !allowed);
       break;
     case StorageType::INDEXED_DB:
       TabSpecificContentSettings::IndexedDBAccessed(
-          render_process_id_, render_frame_id_, url, !allowed);
+          render_process_id_, render_frame_id, url, !allowed);
       break;
     case StorageType::CACHE:
       TabSpecificContentSettings::CacheStorageAccessed(
-          render_process_id_, render_frame_id_, url, !allowed);
+          render_process_id_, render_frame_id, url, !allowed);
       break;
     case StorageType::WEB_LOCKS:
       // State not persisted, no need to record anything.
@@ -153,9 +154,10 @@ void ContentSettingsManagerImpl::AllowStorageAccess(
   std::move(callback).Run(allowed);
 }
 
-void ContentSettingsManagerImpl::OnContentBlocked(ContentSettingsType type) {
+void ContentSettingsManagerImpl::OnContentBlocked(int32_t render_frame_id,
+                                                  ContentSettingsType type) {
   content::RenderFrameHost* frame =
-      content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
+      content::RenderFrameHost::FromID(render_process_id_, render_frame_id);
   content::WebContents* web_contents =
       content::WebContents::FromRenderFrameHost(frame);
   if (!web_contents)
@@ -168,17 +170,15 @@ void ContentSettingsManagerImpl::OnContentBlocked(ContentSettingsType type) {
 }
 
 ContentSettingsManagerImpl::ContentSettingsManagerImpl(
-    content::RenderFrameHost* render_frame_host)
-    : render_process_id_(render_frame_host->GetProcess()->GetID()),
-      render_frame_id_(render_frame_host->GetRoutingID()),
+    content::RenderProcessHost* render_process_host)
+    : render_process_id_(render_process_host->GetID()),
       cookie_settings_(
           CookieSettingsFactory::GetForProfile(Profile::FromBrowserContext(
-              render_frame_host->GetProcess()->GetBrowserContext()))) {}
+              render_process_host->GetBrowserContext()))) {}
 
 ContentSettingsManagerImpl::ContentSettingsManagerImpl(
     const ContentSettingsManagerImpl& other)
     : render_process_id_(other.render_process_id_),
-      render_frame_id_(other.render_frame_id_),
       cookie_settings_(other.cookie_settings_) {}
 
 }  // namespace chrome

chrome/browser/content_settings/content_settings_manager_impl.h

@@ -9,7 +9,7 @@
 #include "chrome/common/content_settings_manager.mojom.h"
 
 namespace content {
-class RenderFrameHost;
+class RenderProcessHost;
 }  // namespace content
 
 namespace content_settings {
@@ -23,27 +23,28 @@ class ContentSettingsManagerImpl : public mojom::ContentSettingsManager {
   ~ContentSettingsManagerImpl() override;
 
   static void Create(
-      content::RenderFrameHost* render_frame_host,
+      content::RenderProcessHost* render_process_host,
       mojo::PendingReceiver<mojom::ContentSettingsManager> receiver);
 
   // mojom::ContentSettingsManager methods:
   void Clone(
       mojo::PendingReceiver<mojom::ContentSettingsManager> receiver) override;
-  void AllowStorageAccess(StorageType storage_type,
+  void AllowStorageAccess(int32_t render_frame_id,
+                          StorageType storage_type,
                           const url::Origin& origin,
                           const GURL& site_for_cookies,
                           const url::Origin& top_frame_origin,
                           base::OnceCallback<void(bool)> callback) override;
-  void OnContentBlocked(ContentSettingsType type) override;
+  void OnContentBlocked(int32_t render_frame_id,
+                        ContentSettingsType type) override;
 
  private:
   explicit ContentSettingsManagerImpl(
-      content::RenderFrameHost* render_frame_host);
+      content::RenderProcessHost* render_process_host);
   explicit ContentSettingsManagerImpl(const ContentSettingsManagerImpl& other);
 
   // Use these IDs to hold a weak reference back to the RenderFrameHost.
-  int render_process_id_;
-  int render_frame_id_;
+  const int render_process_id_;
 
   // Used to look up storage permissions.
   scoped_refptr<content_settings::CookieSettings> cookie_settings_;

chrome/common/content_settings_manager.mojom

@@ -33,6 +33,7 @@ interface ContentSettingsManager {
   // Then these parameters would not need to be passed here.
   [Sync]
   AllowStorageAccess(
+      int32 render_frame_id,
       StorageType storage_type,
       url.mojom.Origin origin,
       url.mojom.Url site_for_cookies,
@@ -40,5 +41,7 @@ interface ContentSettingsManager {
 
   // Tells the browser that content in the current page was blocked due to the
   // user's content settings.
-  OnContentBlocked(content_settings.mojom.ContentSettingsType type);
+  OnContentBlocked(
+      int32 render_frame_id,
+      content_settings.mojom.ContentSettingsType type);
 };

chrome/renderer/content_settings_agent_impl.cc

@@ -19,6 +19,7 @@
 #include "components/content_settings/core/common/content_settings.mojom.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/content_settings/core/common/content_settings_utils.h"
+#include "content/public/child/child_thread.h"
 #include "content/public/common/client_hints.mojom.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/common/previews_state.h"
@@ -187,13 +188,13 @@ void ContentSettingsAgentImpl::DidBlockContentType(
     ContentSettingsType settings_type) {
   bool newly_blocked = content_blocked_.insert(settings_type).second;
   if (newly_blocked)
-    GetContentSettingsManager().OnContentBlocked(settings_type);
+    GetContentSettingsManager().OnContentBlocked(routing_id(), settings_type);
 }
 
 void ContentSettingsAgentImpl::BindContentSettingsManager(
     mojo::Remote<chrome::mojom::ContentSettingsManager>* manager) {
   DCHECK(!*manager);
-  render_frame()->GetBrowserInterfaceBroker()->GetInterface(
+  content::ChildThread::Get()->BindHostReceiver(
       manager->BindNewPipeAndPassReceiver());
 }
 
@@ -271,6 +272,7 @@ void ContentSettingsAgentImpl::RequestFileSystemAccessAsync(
   }
 
   GetContentSettingsManager().AllowStorageAccess(
+      routing_id(),
       chrome::mojom::ContentSettingsManager::StorageType::FILE_SYSTEM,
       frame->GetSecurityOrigin(), frame->GetDocument().SiteForCookies(),
       frame->GetDocument().TopFrameOrigin(), std::move(callback));
@@ -374,6 +376,7 @@ bool ContentSettingsAgentImpl::AllowStorage(bool local) {
 
   bool result = false;
   GetContentSettingsManager().AllowStorageAccess(
+      routing_id(),
       local
           ? chrome::mojom::ContentSettingsManager::StorageType::LOCAL_STORAGE
           : chrome::mojom::ContentSettingsManager::StorageType::SESSION_STORAGE,
@@ -648,7 +651,7 @@ bool ContentSettingsAgentImpl::AllowStorageAccess(
 
   bool result = false;
   GetContentSettingsManager().AllowStorageAccess(
-      storage_type, frame->GetSecurityOrigin(),
+      routing_id(), storage_type, frame->GetSecurityOrigin(),
       frame->GetDocument().SiteForCookies(),
       frame->GetDocument().TopFrameOrigin(), &result);
   return result;

chrome/renderer/content_settings_agent_impl_browsertest.cc

@@ -61,7 +61,8 @@ class MockContentSettingsManagerImpl
                  receiver) override {
     ADD_FAILURE() << "Not reached";
   }
-  void AllowStorageAccess(StorageType storage_type,
+  void AllowStorageAccess(int32_t render_frame_id,
+                          StorageType storage_type,
                           const url::Origin& origin,
                           const GURL& site_for_cookies,
                           const url::Origin& top_frame_origin,
@@ -69,7 +70,8 @@ class MockContentSettingsManagerImpl
     ++log_->allow_storage_access_count;
     std::move(callback).Run(true);
   }
-  void OnContentBlocked(ContentSettingsType type) override {
+  void OnContentBlocked(int32_t render_frame_id,
+                        ContentSettingsType type) override {
     ++log_->on_content_blocked_count;
     log_->on_content_blocked_type = type;
   }
@@ -442,6 +444,9 @@ TEST_F(ContentSettingsAgentImplBrowserTest,
 // allow JS and reload the page. In each case, only one of noscript or script
 // tags should be enabled, but never both.
 TEST_F(ContentSettingsAgentImplBrowserTest, ContentSettingsNoscriptTag) {
+  MockContentSettingsAgentImpl mock_agent(view_->GetMainRenderFrame(),
+                                          registry_.get());
+
   // 1. Block JavaScript.
   RendererContentSettingRules content_setting_rules;
   ContentSettingsForOneType& script_setting_rules =

chrome/renderer/worker_content_settings_client.cc

@@ -16,7 +16,8 @@
 #include "url/origin.h"
 
 WorkerContentSettingsClient::WorkerContentSettingsClient(
-    content::RenderFrame* render_frame) {
+    content::RenderFrame* render_frame)
+    : render_frame_id_(render_frame->GetRoutingID()) {
   blink::WebLocalFrame* frame = render_frame->GetWebFrame();
   const blink::WebDocument& document = frame->GetDocument();
   if (document.GetSecurityOrigin().IsOpaque() ||
@@ -27,7 +28,7 @@ WorkerContentSettingsClient::WorkerContentSettingsClient(
   site_for_cookies_ = document.SiteForCookies();
   top_frame_origin_ = document.TopFrameOrigin();
 
-  render_frame->GetBrowserInterfaceBroker()->GetInterface(
+  content::ChildThread::Get()->BindHostReceiver(
       pending_content_settings_manager_.InitWithNewPipeAndPassReceiver());
 
   ContentSettingsAgentImpl* agent = ContentSettingsAgentImpl::Get(render_frame);
@@ -42,6 +43,7 @@ WorkerContentSettingsClient::WorkerContentSettingsClient(
       site_for_cookies_(other.site_for_cookies_),
       top_frame_origin_(other.top_frame_origin_),
       allow_running_insecure_content_(other.allow_running_insecure_content_),
+      render_frame_id_(other.render_frame_id_),
       content_setting_rules_(other.content_setting_rules_) {
   other.EnsureContentSettingsManager();
   other.content_settings_manager_->Clone(
@@ -81,7 +83,7 @@ bool WorkerContentSettingsClient::AllowRunningInsecureContent(
   if (!allow_running_insecure_content_ && !allowed_per_settings) {
     EnsureContentSettingsManager();
     content_settings_manager_->OnContentBlocked(
-        ContentSettingsType::MIXEDSCRIPT);
+        render_frame_id_, ContentSettingsType::MIXEDSCRIPT);
     return false;
   }
 
@@ -106,7 +108,7 @@ bool WorkerContentSettingsClient::AllowScriptFromSource(
   if (!allow) {
     EnsureContentSettingsManager();
     content_settings_manager_->OnContentBlocked(
-        ContentSettingsType::JAVASCRIPT);
+        render_frame_id_, ContentSettingsType::JAVASCRIPT);
     return false;
   }
 
@@ -132,9 +134,9 @@ bool WorkerContentSettingsClient::AllowStorageAccess(
   EnsureContentSettingsManager();
 
   bool result = false;
-  content_settings_manager_->AllowStorageAccess(storage_type, document_origin_,
-                                                site_for_cookies_,
-                                                top_frame_origin_, &result);
+  content_settings_manager_->AllowStorageAccess(
+      render_frame_id_, storage_type, document_origin_, site_for_cookies_,
+      top_frame_origin_, &result);
   return result;
 }
 

chrome/renderer/worker_content_settings_client.h

@@ -52,6 +52,7 @@ class WorkerContentSettingsClient : public blink::WebContentSettingsClient {
   GURL site_for_cookies_;
   url::Origin top_frame_origin_;
   bool allow_running_insecure_content_;
+  const int32_t render_frame_id_;
   const RendererContentSettingRules* content_setting_rules_;
 
   // Because instances of this class are created on the parent's thread (i.e,