Merge pull request #26194 from brave/fix_sidebar_asan_failure

Fixed sidebar asan failure when closing browser window

browser/ui/sidebar/sidebar_browsertest.cc

@@ -284,6 +284,18 @@ 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> item_added_bubble_anchor_ = nullptr;
   std::unique_ptr<base::RunLoop> run_loop_;
   base::WeakPtrFactory<SidebarBrowserTest> weak_factory_{this};
@@ -917,6 +929,10 @@ 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

@@ -779,8 +779,8 @@ WalletButton* BraveBrowserView::GetWalletButton() {
   return static_cast<BraveToolbarView*>(toolbar())->wallet_button();
 }
 
-void BraveBrowserView::WillShowSidePanel() {
-  sidebar_container_view_->WillShowSidePanel();
+void BraveBrowserView::WillShowSidePanel(bool show_on_deregistered) {
+  sidebar_container_view_->WillShowSidePanel(show_on_deregistered);
 }
 
 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();
+  void WillShowSidePanel(bool show_on_deregistered);
 
   // Triggers layout of web modal dialogs
   void NotifyDialogPositionRequiresUpdate();

browser/ui/views/side_panel/brave_side_panel_coordinator.cc

@@ -151,8 +151,23 @@ void BraveSidePanelCoordinator::PopulateSidePanel(
   // Notify to give opportunity to observe another panel entries from
   // global or active tab's contextual registry.
   auto* brave_browser_view = static_cast<BraveBrowserView*>(browser_view_);
-  brave_browser_view->WillShowSidePanel();
-
+  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);
   SidePanelCoordinator::PopulateSidePanel(supress_animations, unique_key, entry,
                                           std::move(content_view));
 }

browser/ui/views/sidebar/sidebar_container_view.cc

@@ -174,7 +174,9 @@ bool SidebarContainerView::IsSidebarVisible() const {
   return sidebar_control_view_ && sidebar_control_view_->GetVisible();
 }
 
-void SidebarContainerView::WillShowSidePanel() {
+void SidebarContainerView::WillShowSidePanel(bool show_on_deregistered) {
+  deregister_when_hidden_ = show_on_deregistered;
+
   // 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();
@@ -764,8 +766,13 @@ void SidebarContainerView::OnEntryShown(SidePanelEntry* entry) {
 }
 
 void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) {
-  // Make sure item is deselected
   DVLOG(1) << "Panel hidden: " << SidePanelEntryIdToString(entry->key().id());
+
+  if (deregister_when_hidden_) {
+    StopObservingForEntry(entry);
+    deregister_when_hidden_ = false;
+  }
+
   auto* controller = browser_->sidebar_controller();
 
   // Handling if |entry| is managed one.
@@ -806,10 +813,10 @@ void SidebarContainerView::OnEntryWillHide(SidePanelEntry* entry,
   // 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 shoudl not stop
+  // 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) {
-    StopObservingForEntry(entry);
+    deregister_when_hidden_ = true;
   }
 }
 

browser/ui/views/sidebar/sidebar_container_view.h

@@ -78,7 +78,16 @@ class SidebarContainerView
 
   BraveSidePanel* side_panel() { return side_panel_; }
 
-  void WillShowSidePanel();
+  // Need to know this showing comes from deregistering current entry
+  // or not. We're observing contextual/global panel entries when panel is
+  // shown. And stop observing when it's hidden by closing. Unfortunately,
+  // OnEntryWillHide() is not called when current entry is closed by
+  // deregistering. Only OnEntryHidden() is called.
+  // 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);
   bool IsFullscreenForCurrentEntry() const;
 
   void set_operation_from_active_tab_change(bool tab_change) {
@@ -196,6 +205,7 @@ 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;