[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 adds a few customisations to `SidePanelRegistry`, namely, an
`Observer` that allows the communicating of `SidePanelRegistry` going
out of scope, as well as an event for `SidePanelEntry` going out of
scope.

With `SidePanelRegistry`, we want to keep observations to it entirely in
sync. That means that when a registry is destroyed, the observation is
removed, but also when `SidebarContainerView` is destroyed, all
observations are cancelled.

With `SidePanelEntry` though observation management can be a bit more
passive. We want to add observations to the entries, and we want to
avoid double observations. However, calling `RemoveObserver` is
something we don't have to do, and merely dropping it from the set is
good enough.

These customisation points are indeed cumbersome, but at this point it
is the only way it can be guaranteed that all observations for
individual entries in each individual registry is accounted for.

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();
@@ -188,8 +186,9 @@ void SidebarContainerView::WillShowSidePanel(bool show_on_deregistered) {
   StartObservingContextualSidePanelEntry(active_web_contents);
 
   auto* global_registry = side_panel_coordinator_->GetWindowRegistry();
+  AddSidePanelRegistryObservation(global_registry);
   for (const auto& entry : global_registry->entries()) {
-    StartObservingForEntry(entry.get());
+    AddSidePanelEntryObservation(entry.get());
   }
 }
 
@@ -773,11 +772,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 +803,12 @@ 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::OnEntryWillDestroy(SidePanelEntry* entry) {
+  side_panel_entries_observed_.erase(entry);
 }
 
-void SidebarContainerView::OnTabWillBeRemoved(content::WebContents* contents,
-                                              int index) {
-  // At this time, we can stop observing as TabFeatures is available.
-  StopObservingContextualSidePanelEntry(contents);
+void SidebarContainerView::OnRegistryWillDestroy(SidePanelRegistry* registry) {
+  side_panel_registry_observation_.RemoveObservation(registry);
 }
 
 void SidebarContainerView::UpdateActiveItemState() {
@@ -885,23 +865,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 +877,10 @@ void SidebarContainerView::StartObservingContextualSidePanelEntry(
     return;
   }
 
+  // Adding observations for the side panel entries from tab not seen before.
+  AddSidePanelRegistryObservation(registry);
   for (const auto& entry : registry->entries()) {
-    StartObservingForEntry(entry.get());
+    AddSidePanelEntryObservation(entry.get());
   }
 
   SharedPinnedTabService* shared_pinned_tab_service =
@@ -937,24 +902,24 @@ 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 (side_panel_entries_observed_.count(entry)) {
+    return;
   }
-}
 
-BraveBrowser* SidebarContainerView::GetBraveBrowser() const {
-  return static_cast<BraveBrowser*>(browser_.get());
+  side_panel_entries_observed_.insert(entry);
+  entry->AddObserver(this);
+}
+void SidebarContainerView::AddSidePanelRegistryObservation(
+    SidePanelRegistry* registry) {
+  if (side_panel_registry_observation_.IsObservingSource(registry)) {
+    return;
+  }
+  side_panel_registry_observation_.AddObservation(registry);
 }
 
 BEGIN_METADATA(SidebarContainerView)

browser/ui/views/sidebar/sidebar_container_view.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <optional>
 
+#include "base/containers/flat_set.h"
 #include "base/memory/raw_ptr.h"
 #include "base/scoped_multi_source_observation.h"
 #include "base/scoped_observation.h"
@@ -57,6 +58,7 @@ class SidebarContainerView
       public SidebarShowOptionsEventDetectWidget::Delegate,
       public sidebar::SidebarModel::Observer,
       public SidePanelEntryObserver,
+      public SidePanelRegistry::Observer,
       public SidePanelViewStateObserver,
       public TabStripModelObserver {
   METADATA_HEADER(SidebarContainerView, views::View)
@@ -86,8 +88,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,15 +131,16 @@ class SidebarContainerView
   // SidePanelEntryObserver:
   void OnEntryShown(SidePanelEntry* entry) override;
   void OnEntryHidden(SidePanelEntry* entry) override;
-  void OnEntryWillHide(SidePanelEntry* entry,
-                       SidePanelEntryHideReason reason) override;
+
+  // SidePanelRegistry::Observer:
+  void OnEntryWillDestroy(SidePanelEntry* entry) override;
+  void OnRegistryWillDestroy(SidePanelRegistry* registry) override;
 
   // TabStripModelObserver:
   void OnTabStripModelChanged(
       TabStripModel* tab_strip_model,
       const TabStripModelChange& change,
       const TabStripSelectionChange& selection) override;
-  void OnTabWillBeRemoved(content::WebContents* contents, int index) override;
 
   // SidePanelViewStateObserver:
   void OnSidePanelDidClose() override;
@@ -193,14 +195,18 @@ class SidebarContainerView
 
   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;
 
+  // Adds a passive observation for the given side panel entry.
+  void AddSidePanelEntryObservation(SidePanelEntry* entry);
+
+  // Adds a observation for the given side panel registry.
+  void AddSidePanelRegistryObservation(SidePanelRegistry* registry);
+
   raw_ptr<Browser> browser_ = nullptr;
   raw_ptr<SidePanelCoordinator> side_panel_coordinator_ = nullptr;
   raw_ptr<BraveSidePanel> side_panel_ = nullptr;
@@ -209,7 +215,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 +230,17 @@ class SidebarContainerView
   base::ScopedObservation<sidebar::SidebarModel,
                           sidebar::SidebarModel::Observer>
       sidebar_model_observation_{this};
-  base::ScopedMultiSourceObservation<SidePanelEntry, SidePanelEntryObserver>
-      panel_entry_observations_{this};
+
+  // The multishot observation for the side panel registries, which will
+  // deregister themselves when going out of scope.
+  base::ScopedMultiSourceObservation<SidePanelRegistry,
+                                     SidePanelRegistry::Observer>
+      side_panel_registry_observation_{this};
+
+  // This is a passive observations for side panel entries, to prevent double
+  // observer additions. These observations are supposed to outlive the observed
+  // objects, so `RemoveObservation` is never called on these entries.
+  base::flat_set<SidePanelEntry*> side_panel_entries_observed_;
 };
 
 #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_registry.cc

@@ -0,0 +1,36 @@
+/* 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_registry.h"
+
+#include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h"
+
+#define RemoveObserver(...)    \
+  RemoveObserver(__VA_ARGS__); \
+  OnEntryWillDestroy(entry)
+
+#include "src/chrome/browser/ui/views/side_panel/side_panel_registry.cc"
+
+#undef RemoveObserver
+
+RegistryScopeDestructionNotifier::RegistryScopeDestructionNotifier(
+    SidePanelRegistry* registry)
+    : registry_(registry) {}
+RegistryScopeDestructionNotifier::~RegistryScopeDestructionNotifier() {
+  registry_->observers_.Notify(
+      &SidePanelRegistry::Observer::OnRegistryWillDestroy, registry_);
+}
+
+void SidePanelRegistry::AddObserver(SidePanelRegistry::Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void SidePanelRegistry::RemoveObserver(SidePanelRegistry::Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void SidePanelRegistry::OnEntryWillDestroy(SidePanelEntry* entry) {
+  observers_.Notify(&SidePanelRegistry::Observer::OnEntryWillDestroy, entry);
+}