Speculative fix for guest view message filter stale BrowserContext

It appears to be possible for the guest view message filter to outlive
the BrowserContext during profile shutdown. We now observe this and
clear the filter's pointer to the BrowserContext.

We also now destroy the filter on the UI thread for simplicity. The
filter does not use weak pointers on the IO thread as a comment
previously claimed.

Bug: 1129705
Change-Id: If8483653d55c13cde0594e551ee27027060255af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2419376
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809487}

components/guest_view/browser/BUILD.gn

@@ -32,6 +32,8 @@ static_library("browser") {
     "//components/guest_view/common",
   ]
   deps = [
+    "//components/keyed_service/content",
+    "//components/keyed_service/core",
     "//components/zoom",
     "//content/public/browser",
     "//content/public/common",

components/guest_view/browser/DEPS

 include_rules = [
   "+components/crash/core/common/crash_key.h",
+  "+components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h",
+  "+components/keyed_service/core/keyed_service_shutdown_notifier.h",
   "+components/zoom",
   "+content/public/browser",
   "+content/public/common",

components/guest_view/browser/guest_view_message_filter.cc

@@ -6,11 +6,13 @@
 
 #include <memory>
 
+#include "base/memory/singleton.h"
 #include "components/guest_view/browser/bad_message.h"
 #include "components/guest_view/browser/guest_view_base.h"
 #include "components/guest_view/browser/guest_view_manager.h"
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
 #include "components/guest_view/common/guest_view_messages.h"
+#include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
@@ -23,13 +25,25 @@ using content::WebContents;
 
 namespace guest_view {
 
-GuestViewMessageFilter::GuestViewMessageFilter(int render_process_id,
-                                               BrowserContext* context)
-    : BrowserMessageFilter(GuestViewMsgStart),
-      render_process_id_(render_process_id),
-      browser_context_(context) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-}
+namespace {
+
+class ShutdownNotifierFactory
+    : public BrowserContextKeyedServiceShutdownNotifierFactory {
+ public:
+  static ShutdownNotifierFactory* GetInstance() {
+    return base::Singleton<ShutdownNotifierFactory>::get();
+  }
+
+ private:
+  friend struct base::DefaultSingletonTraits<ShutdownNotifierFactory>;
+
+  ShutdownNotifierFactory()
+      : BrowserContextKeyedServiceShutdownNotifierFactory(
+            "GuestViewMessageFilter") {}
+  ~ShutdownNotifierFactory() override = default;
+};
+
+}  // namespace
 
 GuestViewMessageFilter::GuestViewMessageFilter(
     const uint32_t* message_classes_to_filter,
@@ -41,13 +55,30 @@ GuestViewMessageFilter::GuestViewMessageFilter(
       render_process_id_(render_process_id),
       browser_context_(context) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  browser_context_shutdown_subscription_ =
+      ShutdownNotifierFactory::GetInstance()->Get(context)->Subscribe(
+          base::BindRepeating(&GuestViewMessageFilter::OnBrowserContextShutdown,
+                              base::Unretained(this)));
 }
 
 GuestViewMessageFilter::~GuestViewMessageFilter() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  // The filter is destroyed on the UI thread as
+  // |browser_context_shutdown_subscription_| was created there.
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
+
+void GuestViewMessageFilter::EnsureShutdownNotifierFactoryBuilt() {
+  ShutdownNotifierFactory::GetInstance();
+}
+
+void GuestViewMessageFilter::OnBrowserContextShutdown() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  browser_context_ = nullptr;
+  browser_context_shutdown_subscription_.reset();
 }
 
 GuestViewManager* GuestViewMessageFilter::GetOrCreateGuestViewManager() {
+  DCHECK(browser_context_);
   auto* manager = GuestViewManager::FromBrowserContext(browser_context_);
   if (!manager) {
     manager = GuestViewManager::CreateWithDelegate(
@@ -57,6 +88,7 @@ GuestViewManager* GuestViewMessageFilter::GetOrCreateGuestViewManager() {
 }
 
 GuestViewManager* GuestViewMessageFilter::GetGuestViewManagerOrKill() {
+  DCHECK(browser_context_);
   auto* manager = GuestViewManager::FromBrowserContext(browser_context_);
   if (!manager) {
     bad_message::ReceivedBadMessage(
@@ -73,9 +105,7 @@ void GuestViewMessageFilter::OverrideThreadForMessage(
 }
 
 void GuestViewMessageFilter::OnDestruct() const {
-  // Destroy the filter on the IO thread since that's where its weak pointers
-  // are being used.
-  BrowserThread::DeleteOnIOThread::Destruct(this);
+  BrowserThread::DeleteOnUIThread::Destruct(this);
 }
 
 bool GuestViewMessageFilter::OnMessageReceived(const IPC::Message& message) {
@@ -95,12 +125,16 @@ bool GuestViewMessageFilter::OnMessageReceived(const IPC::Message& message) {
 void GuestViewMessageFilter::OnViewCreated(int view_instance_id,
                                            const std::string& view_type) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!browser_context_)
+    return;
   GetOrCreateGuestViewManager()->ViewCreated(render_process_id_,
                                              view_instance_id, view_type);
 }
 
 void GuestViewMessageFilter::OnViewGarbageCollected(int view_instance_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!browser_context_)
+    return;
   GetOrCreateGuestViewManager()->ViewGarbageCollected(render_process_id_,
                                                       view_instance_id);
 }
@@ -110,6 +144,9 @@ void GuestViewMessageFilter::OnAttachGuest(
     int guest_instance_id,
     const base::DictionaryValue& params) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!browser_context_)
+    return;
+
   // We should have a GuestViewManager at this point. If we don't then the
   // embedder is misbehaving.
   auto* manager = GetGuestViewManagerOrKill();
@@ -127,6 +164,9 @@ void GuestViewMessageFilter::OnAttachToEmbedderFrame(
     int element_instance_id,
     int guest_instance_id,
     const base::DictionaryValue& params) {
+  if (!browser_context_)
+    return;
+
   // We should have a GuestViewManager at this point. If we don't then the
   // embedder is misbehaving.
   auto* manager = GetGuestViewManagerOrKill();

components/guest_view/browser/guest_view_message_filter.h

@@ -11,7 +11,7 @@
 #include <string>
 
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
+#include "components/keyed_service/core/keyed_service_shutdown_notifier.h"
 #include "content/public/browser/browser_message_filter.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -31,8 +31,7 @@ class GuestViewManager;
 // the IO thread or the UI thread.
 class GuestViewMessageFilter : public content::BrowserMessageFilter {
  public:
-  GuestViewMessageFilter(int render_process_id,
-                         content::BrowserContext* context);
+  static void EnsureShutdownNotifierFactoryBuilt();
 
  protected:
   GuestViewMessageFilter(const uint32_t* message_classes_to_filter,
@@ -61,10 +60,8 @@ class GuestViewMessageFilter : public content::BrowserMessageFilter {
   const int render_process_id_;
 
   // Should only be accessed on the UI thread.
-  content::BrowserContext* const browser_context_;
-
-  // Weak pointers produced by this factory are bound to the IO thread.
-  base::WeakPtrFactory<GuestViewMessageFilter> weak_ptr_factory_{this};
+  // May become null if this filter outlives the BrowserContext.
+  content::BrowserContext* browser_context_;
 
  private:
   friend class content::BrowserThread;
@@ -81,6 +78,11 @@ class GuestViewMessageFilter : public content::BrowserMessageFilter {
   void OnViewCreated(int view_instance_id, const std::string& view_type);
   void OnViewGarbageCollected(int view_instance_id);
 
+  void OnBrowserContextShutdown();
+
+  std::unique_ptr<KeyedServiceShutdownNotifier::Subscription>
+      browser_context_shutdown_subscription_;
+
   DISALLOW_COPY_AND_ASSIGN(GuestViewMessageFilter);
 };
 

extensions/browser/browser_context_keyed_service_factories.cc

@@ -39,6 +39,7 @@
 #include "extensions/browser/event_router_factory.h"
 #include "extensions/browser/extension_message_filter.h"
 #include "extensions/browser/extension_prefs_factory.h"
+#include "extensions/browser/guest_view/extensions_guest_view_message_filter.h"
 #include "extensions/browser/process_manager_factory.h"
 #include "extensions/browser/renderer_startup_helper.h"
 #include "extensions/browser/updater/update_service_factory.h"
@@ -84,6 +85,7 @@ void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
   DisplaySourceEventRouterFactory::GetInstance();
   EventRouterFactory::GetInstance();
   ExtensionMessageFilter::EnsureShutdownNotifierFactoryBuilt();
+  ExtensionsGuestViewMessageFilter::EnsureShutdownNotifierFactoryBuilt();
   ExtensionPrefsFactory::GetInstance();
   FeedbackPrivateAPI::GetFactoryInstance();
   HidDeviceManager::GetFactoryInstance();

extensions/browser/guest_view/extensions_guest_view_message_filter.cc

@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/guid.h"
 #include "base/stl_util.h"
-#include "base/time/time.h"
 #include "components/guest_view/browser/bad_message.h"
 #include "components/guest_view/browser/guest_view_manager.h"
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
@@ -58,10 +57,6 @@ ExtensionsGuestViewMessageFilter::ExtensionsGuestViewMessageFilter(
       content::BrowserAssociatedInterface<mojom::GuestView>(this, this) {
 }
 
-ExtensionsGuestViewMessageFilter::~ExtensionsGuestViewMessageFilter() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-}
-
 void ExtensionsGuestViewMessageFilter::OverrideThreadForMessage(
     const IPC::Message& message,
     BrowserThread::ID* thread) {
@@ -89,6 +84,7 @@ bool ExtensionsGuestViewMessageFilter::OnMessageReceived(
 
 GuestViewManager* ExtensionsGuestViewMessageFilter::
     GetOrCreateGuestViewManager() {
+  DCHECK(browser_context_);
   auto* manager = GuestViewManager::FromBrowserContext(browser_context_);
   if (!manager) {
     manager = GuestViewManager::CreateWithDelegate(
@@ -153,6 +149,8 @@ void ExtensionsGuestViewMessageFilter::CreateMimeHandlerViewGuestOnUIThread(
         before_unload_control,
     bool is_full_page_plugin) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!browser_context_)
+    return;
 
   auto* manager = GetOrCreateGuestViewManager();
 
@@ -178,6 +176,9 @@ void ExtensionsGuestViewMessageFilter::OnResizeGuest(
     int render_frame_id,
     int element_instance_id,
     const gfx::Size& new_size) {
+  if (!browser_context_)
+    return;
+
   // We should have a GuestViewManager at this point. If we don't then the
   // embedder is misbehaving.
   auto* manager = GetGuestViewManagerOrKill();

extensions/browser/guest_view/extensions_guest_view_message_filter.h

@@ -7,10 +7,8 @@
 
 #include <stdint.h>
 
-#include <map>
 #include <string>
 
-#include "base/memory/weak_ptr.h"
 #include "components/guest_view/browser/guest_view_message_filter.h"
 #include "content/public/browser/browser_associated_interface.h"
 #include "content/public/browser/browser_message_filter.h"
@@ -33,7 +31,7 @@ class GuestViewManager;
 
 namespace extensions {
 // This class filters out incoming extensions GuestView-specific IPC messages
-// from thw renderer process. It is created on the UI thread. Messages may be
+// from the renderer process. It is created on the UI thread. Messages may be
 // handled on the IO thread or the UI thread.
 class ExtensionsGuestViewMessageFilter
     : public guest_view::GuestViewMessageFilter,
@@ -51,7 +49,7 @@ class ExtensionsGuestViewMessageFilter
   friend class content::BrowserThread;
   friend class base::DeleteHelper<ExtensionsGuestViewMessageFilter>;
 
-  ~ExtensionsGuestViewMessageFilter() override;
+  ~ExtensionsGuestViewMessageFilter() override = default;
 
   // GuestViewMessageFilter implementation.
   void OverrideThreadForMessage(const IPC::Message& message,