Merge pull request #13029 from brave/shields_bugs_1.38.x

UI and crash fixes for Shields v2 panel (uplift to 1.38.x)

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,108 +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());
 }
 
-BraveShieldsDataController*
-ShieldsPanelDataHandler::GetActiveShieldsDataController() {
-  auto* profile = Profile::FromWebUI(webui_controller_->web_ui());
-  DCHECK(profile);
-
-  Browser* browser = chrome::FindLastActiveWithProfile(profile);
-  if (!browser)
-    return nullptr;
+void ShieldsPanelDataHandler::UpdateFavicon() {
+  if (!active_shields_data_controller_)
+    return;
 
-  auto* web_contents = browser->tab_strip_model()->GetActiveWebContents();
+  // TODO(nullhook): Don't update favicon if previous site is the current site
+  site_block_info_.favicon_url =
+      active_shields_data_controller_->GetFaviconURL(true);
 
-  if (web_contents) {
-    return BraveShieldsDataController::FromWebContents(web_contents);
+  // Notify remote that favicon changed
+  if (ui_handler_remote_) {
+    ui_handler_remote_.get()->OnSiteBlockInfoChanged(site_block_info_.Clone());
   }
-
-  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
@@ -167,35 +178,30 @@ void ShieldsPanelDataHandler::OnResourcesChanged() {
 }
 
 void ShieldsPanelDataHandler::OnFaviconUpdated() {
-  auto* shields_data_ctrlr = GetActiveShieldsDataController();
-  if (!shields_data_ctrlr)
-    return;
-
-  site_block_info_.favicon_url = shields_data_ctrlr->GetFaviconURL(true);
-
-  // Notify remote that favicon changed
-  if (ui_handler_remote_) {
-    ui_handler_remote_.get()->OnSiteBlockInfoChanged(site_block_info_.Clone());
-  }
+  UpdateFavicon();
 }
 
 void ShieldsPanelDataHandler::OnTabStripModelChanged(
     TabStripModel* tab_strip_model,
     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;
@@ -47,9 +47,9 @@ class ShieldsPanelDataHandler : public brave_shields::mojom::DataHandler,
   void SetHTTPSEverywhereEnabled(bool is_enabled) override;
   void SetBraveShieldsEnabled(bool is_enabled) override;
   void OpenWebCompatWindow() override;
+  void UpdateFavicon() override;
 
  private:
-  BraveShieldsDataController* GetActiveShieldsDataController();
   void UpdateSiteBlockInfo();
 
   // BraveShieldsDataController::Observer
@@ -64,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();
 };