[DanglingPtr] Fix UaFs with SidePanelEntry observations (#26288)

[DanglingPtr] Fix UaFs with `SidePanelEntry` observation

This change addresses the uaf cases we were having with the multisource
observation for `SidePanelEntry` on `SidebarContainerView`. The main
issue we were having is that we can't precisely be notified of when a
given `SidePanelEntry` is going out of scope, and in many cases the
instances would be destroyed before the lifetime of
`SidebarContainerView`. In such cases, the observation would be kept
around in the multisource observer, and eventually would cause an UaF,
as the multisource observer was being destroyed and trying to remove the
outstanding observations that were dangling.

`SidePanelEntry` observations do dangle in upstream. That's the model
they have for the lifetimes: FeatureFoo is destroyed before
`SidePanelEntry`. There are exactly 2 ways for a `SidePanelEntry` to be
destroyed in chromium: FeatureFoo destroyes it via Deregister, or
`SidePanelRegistry` is destroyed before `FeatureFoo`.
`SidebarContainerView` cannot in a reliable way keep track of these
guarantees.

This change is predicated on the fact that `SidebarContainerView` will
always outlive all the use of events by any living `SidePanelEntry`
instance.

In this change we introduce a customisation point to `SidePanelEntry`
that allows us to check if a certain entry is being observed or not. If
it is we don't double-observe. Observations are then removed when a tab
is destroyed, which includes when it gets moved to another Browser
window.

Resolves brave/brave-browser#39053
Resolves brave/brave-browser#41924

browser/ui/sidebar/sidebar_browsertest.cc

@@ -286,18 +286,6 @@ class SidebarBrowserTest : public InProcessBrowserTest {
     return std::distance(items.cbegin(), iter);
   }
 
-  bool SidebarContainerObserving(SidePanelEntryId id) {
-    auto* sidebar_container_view =
-        static_cast<SidebarContainerView*>(controller()->sidebar());
-    for (const auto& entry :
-         sidebar_container_view->panel_entry_observations_.sources()) {
-      if (entry->key().id() == id) {
-        return true;
-      }
-    }
-    return false;
-  }
-
   raw_ptr<views::View, DanglingUntriaged> item_added_bubble_anchor_ = nullptr;
   std::unique_ptr<base::RunLoop> run_loop_;
   base::WeakPtrFactory<SidebarBrowserTest> weak_factory_{this};
@@ -931,10 +919,6 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, TabSpecificAndGlobalPanelsTest) {
   WaitUntil(base::BindLambdaForTesting([&]() {
     return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kBookmarks;
   }));
-  // As previous customize panel per-url panel, it's closed by deregistering
-  // after loading another url. Then, sidebar container should stop observing
-  // its entry.
-  EXPECT_FALSE(SidebarContainerObserving(SidePanelEntryId::kCustomizeChrome));
 }
 
 IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, DisabledItemsTest) {

browser/ui/views/frame/brave_browser_view.cc

@@ -778,8 +778,8 @@ WalletButton* BraveBrowserView::GetWalletButton() {
   return static_cast<BraveToolbarView*>(toolbar())->wallet_button();
 }
 
-void BraveBrowserView::WillShowSidePanel(bool show_on_deregistered) {
-  sidebar_container_view_->WillShowSidePanel(show_on_deregistered);
+void BraveBrowserView::WillShowSidePanel() {
+  sidebar_container_view_->WillShowSidePanel();
 }
 
 void BraveBrowserView::NotifyDialogPositionRequiresUpdate() {

browser/ui/views/frame/brave_browser_view.h

@@ -81,7 +81,7 @@ class BraveBrowserView : public BrowserView,
   void CloseWalletBubble();
   WalletButton* GetWalletButton();
   views::View* GetWalletButtonAnchorView();
-  void WillShowSidePanel(bool show_on_deregistered);
+  void WillShowSidePanel();
 
   // Triggers layout of web modal dialogs
   void NotifyDialogPositionRequiresUpdate();

browser/ui/views/side_panel/brave_side_panel_coordinator.cc

@@ -152,22 +152,7 @@ void BraveSidePanelCoordinator::PopulateSidePanel(
   // global or active tab's contextual registry.
   auto* brave_browser_view = static_cast<BraveBrowserView*>(browser_view_);
   CHECK(browser_view_->unified_side_panel()->children().size() == 1);
-  const auto content_wrapper =
-      browser_view_->unified_side_panel()->children()[0];
-  bool show_on_deregistered = false;
-  // If current entry is not null and its content is already removed from
-  // wrapper when new panel is about to be shown, new panel is shown by
-  // deregistering current panel. See the comment of
-  // SidebarContainerView::WillShowSidePanel() about why |show_on_deregistered|
-  // is needed.
-  // Instead, we can override base class' Show(const UniqueKey& entry, ...)
-  // method as we can know whether it's shown by deregistering with
-  // |open_trigger| arg. However, need patching to make it virtual as base class
-  // already have overridden same name methods.
-  if (current_entry_.get() && content_wrapper->children().empty()) {
-    show_on_deregistered = true;
-  }
-  brave_browser_view->WillShowSidePanel(show_on_deregistered);
+  brave_browser_view->WillShowSidePanel();
   SidePanelCoordinator::PopulateSidePanel(supress_animations, unique_key, entry,
                                           std::move(content_view));
 }

browser/ui/views/sidebar/sidebar_container_view.cc

@@ -175,9 +175,7 @@ bool SidebarContainerView::IsSidebarVisible() const {
   return sidebar_control_view_ && sidebar_control_view_->GetVisible();
 }
 
-void SidebarContainerView::WillShowSidePanel(bool show_on_deregistered) {
-  deregister_when_hidden_ = show_on_deregistered;
-
+void SidebarContainerView::WillShowSidePanel() {
   // It's good timing to start observing any panel entries
   // from global and contextual if not yet observed.
   auto* tab_model = browser_->tab_strip_model();
@@ -189,7 +187,7 @@ void SidebarContainerView::WillShowSidePanel(bool show_on_deregistered) {
 
   auto* global_registry = side_panel_coordinator_->GetWindowRegistry();
   for (const auto& entry : global_registry->entries()) {
-    StartObservingForEntry(entry.get());
+    AddSidePanelEntryObservation(entry.get());
   }
 }
 
@@ -773,11 +771,6 @@ void SidebarContainerView::OnEntryShown(SidePanelEntry* entry) {
 void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) {
   DVLOG(1) << "Panel hidden: " << SidePanelEntryIdToString(entry->key().id());
 
-  if (deregister_when_hidden_) {
-    StopObservingForEntry(entry);
-    deregister_when_hidden_ = false;
-  }
-
   auto* controller = GetBraveBrowser()->sidebar_controller();
 
   // Handling if |entry| is managed one.
@@ -809,26 +802,22 @@ void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) {
   }
 }
 
-void SidebarContainerView::OnEntryWillHide(SidePanelEntry* entry,
-                                           SidePanelEntryHideReason reason) {
-  DVLOG(1) << "Panel will hide: "
-           << SidePanelEntryIdToString(entry->key().id());
-
-  // If |reason| is panel closing, we could deregister. And it'll be
-  // re-registered when panel is shown if that entry is still live in tab's
-  // registry.
-  // We only stop observing when |entry|'s panel is hidden by closing.
-  // If it's hidden by replacing with other panel, we should not stop
-  // to know the timing that it's shown again.
-  if (reason == SidePanelEntryHideReason::kSidePanelClosed) {
-    deregister_when_hidden_ = true;
-  }
-}
-
 void SidebarContainerView::OnTabWillBeRemoved(content::WebContents* contents,
                                               int index) {
   // At this time, we can stop observing as TabFeatures is available.
-  StopObservingContextualSidePanelEntry(contents);
+  auto* tab = tabs::TabInterface::GetFromContents(contents);
+  if (!tab->GetTabFeatures()) {
+    return;
+  }
+
+  auto* registry = tab->GetTabFeatures()->side_panel_registry();
+  if (!registry) {
+    return;
+  }
+
+  for (const auto& entry : registry->entries()) {
+    RemoveSidePanelEntryObservation(entry.get());
+  }
 }
 
 void SidebarContainerView::UpdateActiveItemState() {
@@ -885,23 +874,6 @@ void SidebarContainerView::OnTabStripModelChanged(
   }
 }
 
-void SidebarContainerView::StopObservingContextualSidePanelEntry(
-    content::WebContents* contents) {
-  auto* tab = tabs::TabInterface::GetFromContents(contents);
-  if (!tab->GetTabFeatures()) {
-    return;
-  }
-
-  auto* registry = tab->GetTabFeatures()->side_panel_registry();
-  if (!registry) {
-    return;
-  }
-
-  for (const auto& entry : registry->entries()) {
-    StopObservingForEntry(entry.get());
-  }
-}
-
 void SidebarContainerView::StartObservingContextualSidePanelEntry(
     content::WebContents* contents) {
   auto* tab = tabs::TabInterface::GetFromContents(contents);
@@ -914,8 +886,9 @@ void SidebarContainerView::StartObservingContextualSidePanelEntry(
     return;
   }
 
+  // Adding observations for the side panel entries from tab not seen before.
   for (const auto& entry : registry->entries()) {
-    StartObservingForEntry(entry.get());
+    AddSidePanelEntryObservation(entry.get());
   }
 
   SharedPinnedTabService* shared_pinned_tab_service =
@@ -937,24 +910,23 @@ void SidebarContainerView::StartObservingContextualSidePanelEntry(
   }
 }
 
-void SidebarContainerView::StartObservingForEntry(SidePanelEntry* entry) {
-  if (!panel_entry_observations_.IsObservingSource(entry)) {
-    DVLOG(1) << "Observing panel entry: "
-             << SidePanelEntryIdToString(entry->key().id());
-    panel_entry_observations_.AddObservation(entry);
-  }
+BraveBrowser* SidebarContainerView::GetBraveBrowser() const {
+  return static_cast<BraveBrowser*>(browser_.get());
 }
 
-void SidebarContainerView::StopObservingForEntry(SidePanelEntry* entry) {
-  if (panel_entry_observations_.IsObservingSource(entry)) {
-    DVLOG(1) << "Removing panel entry observation: "
-             << SidePanelEntryIdToString(entry->key().id());
-    panel_entry_observations_.RemoveObservation(entry);
+void SidebarContainerView::AddSidePanelEntryObservation(SidePanelEntry* entry) {
+  if (entry->IsBeingObservedBy(this)) {
+    return;
   }
+  entry->AddObserver(this);
 }
 
-BraveBrowser* SidebarContainerView::GetBraveBrowser() const {
-  return static_cast<BraveBrowser*>(browser_.get());
+void SidebarContainerView::RemoveSidePanelEntryObservation(
+    SidePanelEntry* entry) {
+  if (!entry->IsBeingObservedBy(this)) {
+    return;
+  }
+  entry->RemoveObserver(this);
 }
 
 BEGIN_METADATA(SidebarContainerView)

browser/ui/views/sidebar/sidebar_container_view.h

@@ -10,8 +10,6 @@
 #include <optional>
 
 #include "base/memory/raw_ptr.h"
-#include "base/scoped_multi_source_observation.h"
-#include "base/scoped_observation.h"
 #include "base/timer/timer.h"
 #include "brave/browser/ui/sidebar/sidebar.h"
 #include "brave/browser/ui/sidebar/sidebar_model.h"
@@ -86,8 +84,7 @@ class SidebarContainerView
   // By using OnEntryWillHide()'s arg, we can know this hidden is from
   // closing or not. Fortunately, we can know when WillShowSidePanel()
   // is called whether it's closing from deregistration.
-  // With |show_on_deregistered|, we can stop observing OnEntryHidden().
-  void WillShowSidePanel(bool show_on_deregistered);
+  void WillShowSidePanel();
   bool IsFullscreenForCurrentEntry() const;
 
   void set_operation_from_active_tab_change(bool tab_change) {
@@ -130,8 +127,6 @@ class SidebarContainerView
   // SidePanelEntryObserver:
   void OnEntryShown(SidePanelEntry* entry) override;
   void OnEntryHidden(SidePanelEntry* entry) override;
-  void OnEntryWillHide(SidePanelEntry* entry,
-                       SidePanelEntryHideReason reason) override;
 
   // TabStripModelObserver:
   void OnTabStripModelChanged(
@@ -192,15 +187,15 @@ class SidebarContainerView
   void StopBrowserWindowEventMonitoring();
 
   void StartObservingContextualSidePanelEntry(content::WebContents* contents);
-  void StopObservingContextualSidePanelEntry(content::WebContents* contents);
-  void StartObservingForEntry(SidePanelEntry* entry);
-  void StopObservingForEntry(SidePanelEntry* entry);
   void UpdateActiveItemState();
 
   // Casts |browser_| to BraveBrowser, as storing it as BraveBrowser would cause
   // a precocious downcast.
   BraveBrowser* GetBraveBrowser() const;
 
+  void AddSidePanelEntryObservation(SidePanelEntry* entry);
+  void RemoveSidePanelEntryObservation(SidePanelEntry* entry);
+
   raw_ptr<Browser> browser_ = nullptr;
   raw_ptr<SidePanelCoordinator> side_panel_coordinator_ = nullptr;
   raw_ptr<BraveSidePanel> side_panel_ = nullptr;
@@ -209,7 +204,6 @@ class SidebarContainerView
   bool initialized_ = false;
   bool sidebar_on_left_ = true;
   bool operation_from_active_tab_change_ = false;
-  bool deregister_when_hidden_ = false;
   base::OneShotTimer sidebar_hide_timer_;
   sidebar::SidebarService::ShowSidebarOption show_sidebar_option_ =
       sidebar::SidebarService::ShowSidebarOption::kShowAlways;
@@ -225,8 +219,6 @@ class SidebarContainerView
   base::ScopedObservation<sidebar::SidebarModel,
                           sidebar::SidebarModel::Observer>
       sidebar_model_observation_{this};
-  base::ScopedMultiSourceObservation<SidePanelEntry, SidePanelEntryObserver>
-      panel_entry_observations_{this};
 };
 
 #endif  // BRAVE_BROWSER_UI_VIEWS_SIDEBAR_SIDEBAR_CONTAINER_VIEW_H_

chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h

@@ -34,11 +34,9 @@
   virtual NotifyPinnedContainerOfActiveStateChange
 
 #define PopulateSidePanel virtual PopulateSidePanel
-#define OnEntryWillDeregister virtual OnEntryWillDeregister
 
 #include "src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h"  // IWYU pragma: export
 
-#undef OnEntryWillDeregister
 #undef PopulateSidePanel
 #undef NotifyPinnedContainerOfActiveStateChange
 #undef CreateHeader

chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.cc

@@ -0,0 +1,12 @@
+/* Copyright (c) 2024 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#include "chrome/browser/ui/views/side_panel/side_panel_entry.h"
+
+#include "src/chrome/browser/ui/views/side_panel/side_panel_entry.cc"
+
+bool SidePanelEntry::IsBeingObservedBy(SidePanelEntryObserver* observer) {
+  return observers_.HasObserver(observer);
+}

chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.h

@@ -0,0 +1,17 @@
+/* Copyright (c) 2024 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_SIDE_PANEL_SIDE_PANEL_ENTRY_H_
+#define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_SIDE_PANEL_SIDE_PANEL_ENTRY_H_
+
+#define SupportsNewTabButton(...)    \
+  SupportsNewTabButton(__VA_ARGS__); \
+  bool IsBeingObservedBy(SidePanelEntryObserver* observer)
+
+#include "src/chrome/browser/ui/views/side_panel/side_panel_entry.h"  // IWYU pragma: export
+
+#undef SupportsNewTabButton
+
+#endif  // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_SIDE_PANEL_SIDE_PANEL_ENTRY_H_