Fixed shields v2 crash with multiple windows

fix brave/brave-browser#22224

ShieldsPanelDataHandler should keep observed data controller
instead of getting data controller from active web contents of current
active window because active window could be different on it's created
or destroyed.
Crash happened because ShieldsPanelDataHandler tried to remove itself
from wrong data controller due to above reason.

browser/ui/webui/brave_shields/shields_panel_data_handler.cc

@@ -3,40 +3,48 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this file,
 // you can obtain one at http://mozilla.org/MPL/2.0/.
 
+#include "brave/browser/ui/webui/brave_shields/shields_panel_data_handler.h"
+
 #include <utility>
 
-#include "brave/browser/ui/webui/brave_shields/shields_panel_data_handler.h"
 #include "brave/browser/webcompat_reporter/webcompat_reporter_dialog.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "ui/webui/mojo_bubble_web_ui_controller.h"
 
+using brave_shields::BraveShieldsDataController;
+using brave_shields::mojom::SiteSettings;
+
 ShieldsPanelDataHandler::ShieldsPanelDataHandler(
     mojo::PendingReceiver<brave_shields::mojom::DataHandler>
         data_handler_receiver,
-    ui::MojoBubbleWebUIController* webui_controller)
+    ui::MojoBubbleWebUIController* webui_controller,
+    TabStripModel* tab_strip_model)
     : data_handler_receiver_(this, std::move(data_handler_receiver)),
       webui_controller_(webui_controller) {
-  auto* profile = Profile::FromWebUI(webui_controller_->web_ui());
-  DCHECK(profile);
-  Browser* browser = chrome::FindLastActiveWithProfile(profile);
-  browser->tab_strip_model()->AddObserver(this);
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  if (!shields_data_ctrlr)
+  DCHECK(tab_strip_model);
+  tab_strip_model->AddObserver(this);
+
+  auto* web_contents = tab_strip_model->GetActiveWebContents();
+  if (!web_contents)
+    return;
+  active_shields_data_controller_ =
+      BraveShieldsDataController::FromWebContents(web_contents);
+  if (!active_shields_data_controller_)
     return;
+
   UpdateSiteBlockInfo();
-  shields_data_ctrlr->AddObserver(this);
+  active_shields_data_controller_->AddObserver(this);
 }
 
 ShieldsPanelDataHandler::~ShieldsPanelDataHandler() {
   /* The lifecycle of this class is similar to ShieldsPanelUI and
    * ShieldsPanelUI's cache gets destryed after ~300ms of being idle.
    */
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  if (!shields_data_ctrlr)
+  if (!active_shields_data_controller_)
     return;
-  shields_data_ctrlr->RemoveObserver(this);
+
+  active_shields_data_controller_->RemoveObserver(this);
+  active_shields_data_controller_ = nullptr;
 }
 
 void ShieldsPanelDataHandler::RegisterUIHandler(
@@ -52,122 +60,111 @@ void ShieldsPanelDataHandler::GetSiteBlockInfo(
 
 void ShieldsPanelDataHandler::GetSiteSettings(
     GetSiteSettingsCallback callback) {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  DCHECK(shields_data_ctrlr);
+  if (!active_shields_data_controller_)
+    return;
 
   SiteSettings settings;
-  settings.ad_block_mode = shields_data_ctrlr->GetAdBlockMode();
-  settings.fingerprint_mode = shields_data_ctrlr->GetFingerprintMode();
-  settings.cookie_block_mode = shields_data_ctrlr->GetCookieBlockMode();
+  settings.ad_block_mode = active_shields_data_controller_->GetAdBlockMode();
+  settings.fingerprint_mode =
+      active_shields_data_controller_->GetFingerprintMode();
+  settings.cookie_block_mode =
+      active_shields_data_controller_->GetCookieBlockMode();
   settings.is_https_everywhere_enabled =
-      shields_data_ctrlr->GetHTTPSEverywhereEnabled();
-  settings.is_noscript_enabled = shields_data_ctrlr->GetNoScriptEnabled();
+      active_shields_data_controller_->GetHTTPSEverywhereEnabled();
+  settings.is_noscript_enabled =
+      active_shields_data_controller_->GetNoScriptEnabled();
 
   std::move(callback).Run(settings.Clone());
 }
 
 void ShieldsPanelDataHandler::SetAdBlockMode(AdBlockMode mode) {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  DCHECK(shields_data_ctrlr);
+  if (!active_shields_data_controller_)
+    return;
 
-  shields_data_ctrlr->SetAdBlockMode(mode);
+  active_shields_data_controller_->SetAdBlockMode(mode);
 }
 
 void ShieldsPanelDataHandler::SetFingerprintMode(FingerprintMode mode) {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  DCHECK(shields_data_ctrlr);
+  if (!active_shields_data_controller_)
+    return;
 
-  shields_data_ctrlr->SetFingerprintMode(mode);
+  active_shields_data_controller_->SetFingerprintMode(mode);
 }
 
 void ShieldsPanelDataHandler::SetCookieBlockMode(CookieBlockMode mode) {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  DCHECK(shields_data_ctrlr);
+  if (!active_shields_data_controller_)
+    return;
 
-  shields_data_ctrlr->SetCookieBlockMode(mode);
+  active_shields_data_controller_->SetCookieBlockMode(mode);
 }
 
 void ShieldsPanelDataHandler::SetIsNoScriptsEnabled(bool is_enabled) {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  DCHECK(shields_data_ctrlr);
+  if (!active_shields_data_controller_)
+    return;
 
-  shields_data_ctrlr->SetIsNoScriptEnabled(is_enabled);
+  active_shields_data_controller_->SetIsNoScriptEnabled(is_enabled);
 }
 
 void ShieldsPanelDataHandler::SetHTTPSEverywhereEnabled(bool is_enabled) {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  DCHECK(shields_data_ctrlr);
+  if (!active_shields_data_controller_)
+    return;
 
-  shields_data_ctrlr->SetIsHTTPSEverywhereEnabled(is_enabled);
+  active_shields_data_controller_->SetIsHTTPSEverywhereEnabled(is_enabled);
 }
 
 void ShieldsPanelDataHandler::SetBraveShieldsEnabled(bool is_enabled) {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  DCHECK(shields_data_ctrlr);
+  if (!active_shields_data_controller_)
+    return;
 
-  shields_data_ctrlr->SetBraveShieldsEnabled(is_enabled);
+  active_shields_data_controller_->SetBraveShieldsEnabled(is_enabled);
 }
 
 void ShieldsPanelDataHandler::OpenWebCompatWindow() {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  DCHECK(shields_data_ctrlr);
+  if (!active_shields_data_controller_)
+    return;
 
-  OpenWebcompatReporterDialog(shields_data_ctrlr->web_contents());
+  OpenWebcompatReporterDialog(active_shields_data_controller_->web_contents());
 }
 
 void ShieldsPanelDataHandler::UpdateFavicon() {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  if (!shields_data_ctrlr)
+  if (!active_shields_data_controller_)
     return;
 
   // TODO(nullhook): Don't update favicon if previous site is the current site
-  site_block_info_.favicon_url = shields_data_ctrlr->GetFaviconURL(true);
+  site_block_info_.favicon_url =
+      active_shields_data_controller_->GetFaviconURL(true);
 
   // Notify remote that favicon changed
   if (ui_handler_remote_) {
     ui_handler_remote_.get()->OnSiteBlockInfoChanged(site_block_info_.Clone());
   }
 }
 
-BraveShieldsDataController*
-ShieldsPanelDataHandler::GetActiveShieldsDataController() {
-  auto* profile = Profile::FromWebUI(webui_controller_->web_ui());
-  DCHECK(profile);
-
-  Browser* browser = chrome::FindLastActiveWithProfile(profile);
-  if (!browser)
-    return nullptr;
-
-  auto* web_contents = browser->tab_strip_model()->GetActiveWebContents();
-
-  if (web_contents) {
-    return BraveShieldsDataController::FromWebContents(web_contents);
-  }
-
-  return nullptr;
-}
-
 void ShieldsPanelDataHandler::UpdateSiteBlockInfo() {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  if (!shields_data_ctrlr)
+  if (!active_shields_data_controller_)
     return;
 
-  site_block_info_.host = shields_data_ctrlr->GetCurrentSiteURL().host();
+  site_block_info_.host =
+      active_shields_data_controller_->GetCurrentSiteURL().host();
   site_block_info_.total_blocked_resources =
-      shields_data_ctrlr->GetTotalBlockedCount();
-  site_block_info_.ads_list = shields_data_ctrlr->GetBlockedAdsList();
-  site_block_info_.js_list = shields_data_ctrlr->GetJsList();
+      active_shields_data_controller_->GetTotalBlockedCount();
+  site_block_info_.ads_list =
+      active_shields_data_controller_->GetBlockedAdsList();
+  site_block_info_.js_list = active_shields_data_controller_->GetJsList();
   site_block_info_.fingerprints_list =
-      shields_data_ctrlr->GetFingerprintsList();
+      active_shields_data_controller_->GetFingerprintsList();
   site_block_info_.http_redirects_list =
-      shields_data_ctrlr->GetHttpRedirectsList();
+      active_shields_data_controller_->GetHttpRedirectsList();
   site_block_info_.is_shields_enabled =
-      shields_data_ctrlr->GetBraveShieldsEnabled();
+      active_shields_data_controller_->GetBraveShieldsEnabled();
+  site_block_info_.favicon_url =
+      active_shields_data_controller_->GetFaviconURL(false);
 
   // This method gets called from various callsites. Constantly updating favicon
   // url will replace the hashed version too. So, we update this once only
   if (site_block_info_.favicon_url.is_empty()) {
-    site_block_info_.favicon_url = shields_data_ctrlr->GetFaviconURL(false);
+    site_block_info_.favicon_url =
+        active_shields_data_controller_->GetFaviconURL(false);
   }
 
   // Notify remote that data changed
@@ -189,18 +186,22 @@ void ShieldsPanelDataHandler::OnTabStripModelChanged(
     const TabStripModelChange& change,
     const TabStripSelectionChange& selection) {
   if (selection.active_tab_changed()) {
-    // OnResourcesChanged doesnt get triggered instantly on active tab change so
-    // trigger this explicitly
-    UpdateSiteBlockInfo();
-
-    if (selection.new_contents) {
-      BraveShieldsDataController::FromWebContents(selection.new_contents)
-          ->AddObserver(this);
+    // To make logic simpler, always remove observer when active tab changed.
+    // And then, start observing when there is new active web contents.
+    if (active_shields_data_controller_) {
+      active_shields_data_controller_->RemoveObserver(this);
+      active_shields_data_controller_ = nullptr;
     }
 
-    if (selection.old_contents) {
-      BraveShieldsDataController::FromWebContents(selection.old_contents)
-          ->RemoveObserver(this);
+    if (selection.new_contents) {
+      active_shields_data_controller_ =
+          BraveShieldsDataController::FromWebContents(selection.new_contents);
+      active_shields_data_controller_->AddObserver(this);
+
+      // OnResourcesChanged doesnt get triggered instantly on active tab change
+      // so trigger this explicitly. Call this after new
+      // |active_shields_data_controller_| is set.
+      UpdateSiteBlockInfo();
     }
   }
 }

browser/ui/webui/brave_shields/shields_panel_data_handler.h

@@ -6,30 +6,30 @@
 #ifndef BRAVE_BROWSER_UI_WEBUI_BRAVE_SHIELDS_SHIELDS_PANEL_DATA_HANDLER_H_
 #define BRAVE_BROWSER_UI_WEBUI_BRAVE_SHIELDS_SHIELDS_PANEL_DATA_HANDLER_H_
 
+#include "base/memory/raw_ptr.h"
 #include "brave/browser/ui/brave_shields_data_controller.h"
 #include "brave/components/brave_shields/common/brave_shields_panel.mojom.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver.h"
-#include "mojo/public/cpp/bindings/remote_set.h"
+#include "mojo/public/cpp/bindings/remote.h"
+
+class TabStripModel;
 
 namespace ui {
 class MojoBubbleWebUIController;
 }  // namespace ui
 
-using brave_shields::BraveShieldsDataController;
-using brave_shields::mojom::SiteBlockInfo;
-using brave_shields::mojom::SiteSettings;
-using favicon::FaviconDriver;
-
-class ShieldsPanelDataHandler : public brave_shields::mojom::DataHandler,
-                                public BraveShieldsDataController::Observer,
-                                public TabStripModelObserver {
+class ShieldsPanelDataHandler
+    : public brave_shields::mojom::DataHandler,
+      public brave_shields::BraveShieldsDataController::Observer,
+      public TabStripModelObserver {
  public:
   ShieldsPanelDataHandler(
       mojo::PendingReceiver<brave_shields::mojom::DataHandler>
           data_handler_receiver,
-      ui::MojoBubbleWebUIController* webui_controller);
+      ui::MojoBubbleWebUIController* webui_controller,
+      TabStripModel* browser);
 
   ShieldsPanelDataHandler(const ShieldsPanelDataHandler&) = delete;
   ShieldsPanelDataHandler& operator=(const ShieldsPanelDataHandler&) = delete;
@@ -50,7 +50,6 @@ class ShieldsPanelDataHandler : public brave_shields::mojom::DataHandler,
   void UpdateFavicon() override;
 
  private:
-  BraveShieldsDataController* GetActiveShieldsDataController();
   void UpdateSiteBlockInfo();
 
   // BraveShieldsDataController::Observer
@@ -65,8 +64,11 @@ class ShieldsPanelDataHandler : public brave_shields::mojom::DataHandler,
 
   mojo::Receiver<brave_shields::mojom::DataHandler> data_handler_receiver_;
   mojo::Remote<brave_shields::mojom::UIHandler> ui_handler_remote_;
-  ui::MojoBubbleWebUIController* const webui_controller_;
-  SiteBlockInfo site_block_info_;
+  raw_ptr<ui::MojoBubbleWebUIController> const webui_controller_ = nullptr;
+  raw_ptr<brave_shields::BraveShieldsDataController>
+      active_shields_data_controller_ = nullptr;
+
+  brave_shields::mojom::SiteBlockInfo site_block_info_;
 };
 
 #endif  // BRAVE_BROWSER_UI_WEBUI_BRAVE_SHIELDS_SHIELDS_PANEL_DATA_HANDLER_H_

browser/ui/webui/brave_shields/shields_panel_ui.cc

@@ -12,15 +12,23 @@
 #include "brave/components/brave_shields/common/brave_shield_constants.h"
 #include "brave/components/brave_shields/resources/panel/grit/brave_shields_panel_generated_map.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/webui/favicon_source.h"
 #include "chrome/browser/ui/webui/webui_util.h"
 #include "components/favicon_base/favicon_url_parser.h"
 #include "components/grit/brave_components_resources.h"
 #include "content/public/browser/web_ui.h"
 
+// Cache active Browser instance's TabStripModel to give
+// to ShieldsPanelDataHandler when this is created because
+// CreatePanelHandler() is run in async.
 ShieldsPanelUI::ShieldsPanelUI(content::WebUI* web_ui)
     : ui::MojoBubbleWebUIController(web_ui, true),
       profile_(Profile::FromWebUI(web_ui)) {
+  auto* browser = chrome::FindLastActiveWithProfile(profile_);
+  tab_strip_model_ = browser->tab_strip_model();
+
   content::WebUIDataSource* source =
       content::WebUIDataSource::Create(kShieldsPanelHost);
 
@@ -58,7 +66,6 @@ void ShieldsPanelUI::CreatePanelHandler(
 
   panel_handler_ =
       std::make_unique<ShieldsPanelHandler>(std::move(panel_receiver), this);
-
   data_handler_ = std::make_unique<ShieldsPanelDataHandler>(
-      std::move(data_handler_receiver), this);
+      std::move(data_handler_receiver), this, tab_strip_model_);
 }

browser/ui/webui/brave_shields/shields_panel_ui.h

@@ -15,6 +15,8 @@
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "ui/webui/mojo_bubble_web_ui_controller.h"
 
+class TabStripModel;
+
 class ShieldsPanelUI : public ui::MojoBubbleWebUIController,
                        public brave_shields::mojom::PanelHandlerFactory {
  public:
@@ -41,7 +43,8 @@ class ShieldsPanelUI : public ui::MojoBubbleWebUIController,
   mojo::Receiver<brave_shields::mojom::PanelHandlerFactory>
       panel_factory_receiver_{this};
 
-  raw_ptr<Profile> profile_;
+  raw_ptr<Profile> profile_ = nullptr;
+  raw_ptr<TabStripModel> tab_strip_model_ = nullptr;
 
   WEB_UI_CONTROLLER_TYPE_DECL();
 };