Fixed crash when toggling playlist panel while it's playing

fix brave/brave-browser#45144

Author: simonhong

browser/playlist/test/playlist_browsertest.cc

@@ -11,6 +11,7 @@
 #include "base/run_loop.h"
 #include "base/test/bind.h"
 #include "base/test/mock_callback.h"
+#include "base/test/run_until.h"
 #include "base/test/scoped_feature_list.h"
 #include "brave/browser/playlist/playlist_service_factory.h"
 #include "brave/browser/ui/brave_browser.h"
@@ -32,7 +33,10 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_command_controller.h"
+#include "chrome/browser/ui/browser_window/public/browser_window_features.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/side_panel/side_panel_entry_id.h"
+#include "chrome/browser/ui/views/side_panel/side_panel_ui.h"
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/test/base/platform_browser_test.h"
 #include "content/public/test/browser_test.h"
@@ -168,6 +172,44 @@ class PlaylistBrowserTest : public PlatformBrowserTest {
   content::ContentMockCertVerifier mock_cert_verifier_;
 };
 
+// Check toggling playlist panel while playing doesn't make crash.
+IN_PROC_BROWSER_TEST_F(PlaylistBrowserTest, PanelToggleTestWhilePlaying) {
+  auto* panel_ui = browser()->GetFeatures().side_panel_ui();
+
+  // Open playlist panel.
+  panel_ui->Show(SidePanelEntryId::kPlaylist);
+  WaitUntil(base::BindLambdaForTesting(
+      [&]() { return panel_ui->IsSidePanelShowing(); }));
+  ASSERT_TRUE(
+      base::test::RunUntil([&]() { return panel_ui->IsSidePanelShowing(); }));
+
+  auto* coordinator = PlaylistSidePanelCoordinator::FromBrowser(browser());
+  ASSERT_TRUE(coordinator);
+  coordinator->is_audible_for_testing_ = true;
+
+  // Close playlist panel check cached instances are still live.
+  panel_ui->Close();
+  EXPECT_TRUE(coordinator->contents_wrapper_);
+  EXPECT_TRUE(coordinator->side_panel_web_view_);
+  ASSERT_TRUE(
+      base::test::RunUntil([&]() { return !panel_ui->IsSidePanelShowing(); }));
+
+  // Re-open playlist panel.
+  panel_ui->Show(SidePanelEntryId::kPlaylist);
+  ASSERT_TRUE(
+      base::test::RunUntil([&]() { return panel_ui->IsSidePanelShowing(); }));
+
+  // Not audible. Check cached webview/contents are destroyed.
+  coordinator->is_audible_for_testing_ = false;
+
+  // Close playlist panel. Check cached instances are all freed.
+  panel_ui->Close();
+  EXPECT_FALSE(coordinator->contents_wrapper_);
+  EXPECT_FALSE(coordinator->side_panel_web_view_);
+  ASSERT_TRUE(
+      base::test::RunUntil([&]() { return !panel_ui->IsSidePanelShowing(); }));
+}
+
 IN_PROC_BROWSER_TEST_F(PlaylistBrowserTest, AddItemsToList) {
   ASSERT_TRUE(content::NavigateToURL(GetActiveWebContents(),
                                      GetURL("/playlist/site_with_video.html")));

browser/ui/views/side_panel/playlist/playlist_side_panel_coordinator.cc

@@ -22,6 +22,8 @@
 #include "chrome/browser/ui/views/side_panel/side_panel_util.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/grit/brave_components_strings.h"
+#include "content/public/browser/visibility.h"
+#include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/models/image_model.h"
 #include "ui/views/vector_icons.h"
@@ -88,22 +90,34 @@ std::unique_ptr<views::View> PlaylistSidePanelCoordinator::CreateWebView(
 
     Proxy::CreateForWebContents(contents_wrapper_->web_contents(),
                                 weak_ptr_factory_.GetWeakPtr());
-  }
 
-  auto web_view = std::make_unique<PlaylistSidePanelWebView>(
-      &GetBrowser(), scope, base::DoNothing(), contents_wrapper_.get());
-  side_panel_web_view_ = web_view->GetWeakPtr();
+    // |web_view_| is also created and cached together with |contents_wrapper_|.
+    // Why caching webview also? When SidePanelWebView is created, its contents
+    // is marked as side panel to make its loading faster at
+    // SidePanelLoadingVoter::MarkAsSidePanel(). However, upstream has
+    // assumption that it should have initial state(not url loaded before)
+    // except it's preloaded side panel. If we create PlaylistSidePanelWebView
+    // whenever opening, we conflict with it as playlist could still play even
+    // it's closed. By caching webview also, we could avoid that conflict.
+    CHECK(!side_panel_web_view_);
+    side_panel_web_view_ = std::make_unique<PlaylistSidePanelWebView>(
+        &GetBrowser(), scope, base::DoNothing(), contents_wrapper_.get());
+    side_panel_web_view_->set_owned_by_client();
+  }
 
   if (!should_create_contents_wrapper) {
     // SidePanelWebView's initial visibility is hidden. Thus, we need to
     // call this manually when we don't reload the web contents.
     // Calling this will also mark that the web contents is ready to go.
-    web_view->ShowUI();
+    side_panel_web_view_->ShowUI();
   }
 
-  view_observation_.Observe(web_view.get());
+  auto view = std::make_unique<views::View>();
+  view->SetUseDefaultFillLayout(true);
+  view->AddChildViewRaw(side_panel_web_view_.get());
+  view_observation_.Observe(view.get());
 
-  return web_view;
+  return view;
 }
 
 BrowserView* PlaylistSidePanelCoordinator::GetBrowserView() {
@@ -118,8 +132,10 @@ void PlaylistSidePanelCoordinator::OnViewIsDeleting(views::View* view) {
 
 void PlaylistSidePanelCoordinator::DestroyWebContentsIfNeeded() {
   DCHECK(contents_wrapper_);
-  if (!contents_wrapper_->web_contents()->IsCurrentlyAudible()) {
+  if (!is_audible_for_testing_ &&
+      !contents_wrapper_->web_contents()->IsCurrentlyAudible()) {
     contents_wrapper_.reset();
+    side_panel_web_view_.reset();
   }
 }
 

browser/ui/views/side_panel/playlist/playlist_side_panel_coordinator.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <string>
 
+#include "base/gtest_prod_util.h"
 #include "base/scoped_observation.h"
 #include "brave/browser/ui/views/side_panel/playlist/playlist_contents_wrapper.h"
 #include "brave/browser/ui/views/side_panel/playlist/playlist_side_panel_web_view.h"
@@ -18,6 +19,7 @@
 
 namespace playlist {
 class PlaylistUI;
+FORWARD_DECLARE_TEST(PlaylistBrowserTest, PanelToggleTestWhilePlaying);
 }  // namespace playlist
 
 class Browser;
@@ -61,7 +63,7 @@ class PlaylistSidePanelCoordinator
   void LoadPlaylist(const std::string& playlist_id, const std::string& item_id);
 
   base::WeakPtr<PlaylistSidePanelWebView> side_panel_web_view() {
-    return side_panel_web_view_;
+    return side_panel_web_view_ ? side_panel_web_view_->GetWeakPtr() : nullptr;
   }
 
   BrowserView* GetBrowserView();
@@ -71,16 +73,20 @@ class PlaylistSidePanelCoordinator
 
  private:
   friend class BrowserUserData<PlaylistSidePanelCoordinator>;
+  FRIEND_TEST_ALL_PREFIXES(playlist::PlaylistBrowserTest,
+                           PanelToggleTestWhilePlaying);
 
   void DestroyWebContentsIfNeeded();
 
   std::unique_ptr<views::View> CreateWebView(SidePanelEntryScope& scope);
 
+  bool is_audible_for_testing_ = false;
   raw_ptr<Browser> browser_ = nullptr;
 
+  // Both are cached while it's playing even if playlist panel is closed.
+  // Destroyed when panel is closed and not played.
   std::unique_ptr<PlaylistContentsWrapper> contents_wrapper_;
-
-  base::WeakPtr<PlaylistSidePanelWebView> side_panel_web_view_;
+  std::unique_ptr<PlaylistSidePanelWebView> side_panel_web_view_;
 
   base::ScopedObservation<views::View, views::ViewObserver> view_observation_{
       this};

browser/ui/views/side_panel/playlist/playlist_side_panel_web_view.cc

@@ -5,6 +5,8 @@
 
 #include "brave/browser/ui/views/side_panel/playlist/playlist_side_panel_web_view.h"
 
+#include "ui/base/metadata/metadata_impl_macros.h"
+
 PlaylistSidePanelWebView::PlaylistSidePanelWebView(
     Browser* browser,
     SidePanelEntryScope& scope,
@@ -20,3 +22,6 @@ PlaylistSidePanelWebView::~PlaylistSidePanelWebView() = default;
 base::WeakPtr<PlaylistSidePanelWebView> PlaylistSidePanelWebView::GetWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
 }
+
+BEGIN_METADATA(PlaylistSidePanelWebView)
+END_METADATA

browser/ui/views/side_panel/playlist/playlist_side_panel_web_view.h

@@ -13,8 +13,11 @@ class SidePanelEntryScope;
 #include "base/functional/callback_forward.h"
 #include "brave/browser/ui/webui/playlist_ui.h"
 #include "chrome/browser/ui/views/side_panel/side_panel_web_ui_view.h"
+#include "ui/base/metadata/metadata_header_macros.h"
 
 class PlaylistSidePanelWebView : public SidePanelWebUIView {
+  METADATA_HEADER(PlaylistSidePanelWebView, SidePanelWebUIView)
+
  public:
   PlaylistSidePanelWebView(Browser* browser,
                            SidePanelEntryScope& scope,