Nullify widget state on NativeWidget classes post widget destruction

This CL introduces a new ClientDestroyedWidget() method on
NativeWidgetPrivate. This the associated NativeWidget to clear
any Widget-associated state and avoid dangling pointers.

This is specifically necessary under the CLIENT_OWNS_WIDGET
ownership model where the NativeWidget (managed by the OS) can
outlive the Widget.

This patch leverages this method to clear widget-associated state on
  - DesktopBrowserFrameAura
  - BrowserDesktopWindowTreeHostWin
  - BrowserDesktopWindowTreeHostLinux

A follow up will leverage this new method to additionally clear
such state from Mac NativeWidget classes.

Bug: 413168662, 413385730
Change-Id: Ie3825b624cf2853865f3f501e673d3ddcf24aa5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6497480
Reviewed-by: Darryl James <dljames@chromium.org>
Reviewed-by: Keren Zhu <kerenzhu@chromium.org>
Commit-Queue: Tom Lukaszewicz <tluk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1454902}

chrome/browser/ui/views/frame/browser_desktop_window_tree_host.h

@@ -34,6 +34,10 @@ class BrowserDesktopWindowTreeHost {
   // Returns true if the OS takes care of showing the system menu. Returning
   // false means BrowserFrame handles showing the system menu.
   virtual bool UsesNativeSystemMenu() const = 0;
+
+  // A lifecycle hook invoked when the views::Widget associated with this window
+  // tree host was destroyed by the client.
+  virtual void ClientDestroyedWidget() = 0;
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_DESKTOP_WINDOW_TREE_HOST_H_

chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.cc

@@ -108,7 +108,6 @@ void BrowserDesktopWindowTreeHostLinux::AddAdditionalInitProperties(
     ui::PlatformWindowInitProperties* properties) {
   views::DesktopWindowTreeHostLinux::AddAdditionalInitProperties(params,
                                                                  properties);
-
   auto* profile = browser_view_->browser()->profile();
   const auto* linux_ui_theme = ui::LinuxUiTheme::GetForProfile(profile);
   properties->prefer_dark_theme =
@@ -128,6 +127,14 @@ bool BrowserDesktopWindowTreeHostLinux::UsesNativeSystemMenu() const {
   return false;
 }
 
+void BrowserDesktopWindowTreeHostLinux::ClientDestroyedWidget() {
+#if BUILDFLAG(USE_DBUS)
+  dbus_appmenu_.reset();
+#endif
+  browser_frame_ = nullptr;
+  browser_view_ = nullptr;
+}
+
 void BrowserDesktopWindowTreeHostLinux::FrameTypeChanged() {
   DesktopWindowTreeHostPlatform::FrameTypeChanged();
   UpdateFrameHints();
@@ -164,6 +171,8 @@ void BrowserDesktopWindowTreeHostLinux::UnlockMouse(aura::Window* window) {
 
 void BrowserDesktopWindowTreeHostLinux::TabDraggingKindChanged(
     TabDragKind tab_drag_kind) {
+  CHECK(browser_frame_);
+  CHECK(browser_view_);
   // If there's no tabs left, the browser window is about to close, so don't
   // call SetOverrideRedirect() to prevent the window from flashing.
   if (!browser_view_->tabstrip()->GetModelCount()) {
@@ -200,6 +209,10 @@ bool BrowserDesktopWindowTreeHostLinux::SupportsClientFrameShadow() const {
 }
 
 void BrowserDesktopWindowTreeHostLinux::UpdateFrameHints() {
+  if (!browser_frame_) {
+    return;
+  }
+
   auto* window = platform_window();
   auto window_state = window->GetPlatformWindowState();
   float scale = device_scale_factor();
@@ -320,6 +333,7 @@ void BrowserDesktopWindowTreeHostLinux::CloseNow() {
 void BrowserDesktopWindowTreeHostLinux::Show(
     ui::mojom::WindowShowState show_state,
     const gfx::Rect& restore_bounds) {
+  CHECK(browser_view_);
   DesktopWindowTreeHostLinux::Show(show_state, restore_bounds);
 
   const std::string& startup_id =
@@ -332,14 +346,16 @@ void BrowserDesktopWindowTreeHostLinux::Show(
 
 bool BrowserDesktopWindowTreeHostLinux::IsOverrideRedirect(
     const ui::X11Extension& x11_extension) const {
-  return (browser_frame_->tab_drag_kind() == TabDragKind::kAllTabs) &&
+  return (browser_frame_ &&
+          browser_frame_->tab_drag_kind() == TabDragKind::kAllTabs) &&
          x11_extension.IsWmTiling() && x11_extension.CanResetOverrideRedirect();
 }
 
 gfx::Insets BrowserDesktopWindowTreeHostLinux::CalculateInsetsInDIP(
     ui::PlatformWindowState window_state) const {
   // If we are not showing frame, the insets should be zero.
-  if (!IsShowingFrame(browser_frame_->native_browser_frame()->UseCustomFrame(),
+  if (!browser_frame_ ||
+      !IsShowingFrame(browser_frame_->native_browser_frame()->UseCustomFrame(),
                       window_state)) {
     return gfx::Insets();
   }
@@ -351,6 +367,10 @@ gfx::Insets BrowserDesktopWindowTreeHostLinux::CalculateInsetsInDIP(
 void BrowserDesktopWindowTreeHostLinux::OnWindowStateChanged(
     ui::PlatformWindowState old_state,
     ui::PlatformWindowState new_state) {
+  if (!browser_view_) {
+    return;
+  }
+
   DesktopWindowTreeHostLinux::OnWindowStateChanged(old_state, new_state);
 
   bool fullscreen_changed = new_state == ui::PlatformWindowState::kFullScreen ||
@@ -373,6 +393,10 @@ void BrowserDesktopWindowTreeHostLinux::OnWindowStateChanged(
 
 void BrowserDesktopWindowTreeHostLinux::OnWindowTiledStateChanged(
     ui::WindowTiledEdges new_tiled_edges) {
+  if (!browser_frame_) {
+    return;
+  }
+
   bool maximized = new_tiled_edges.top && new_tiled_edges.left &&
                    new_tiled_edges.bottom && new_tiled_edges.right;
   bool tiled = new_tiled_edges.top || new_tiled_edges.left ||

chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.h

@@ -71,6 +71,7 @@ class BrowserDesktopWindowTreeHostLinux
   DesktopWindowTreeHost* AsDesktopWindowTreeHost() override;
   int GetMinimizeButtonOffset() const override;
   bool UsesNativeSystemMenu() const override;
+  void ClientDestroyedWidget() override;
 
   // BrowserWindowTreeHostPlatform:
   void FrameTypeChanged() override;

chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc

@@ -241,6 +241,7 @@ BrowserDesktopWindowTreeHostWin::~BrowserDesktopWindowTreeHostWin() = default;
 
 views::NativeMenuWin* BrowserDesktopWindowTreeHostWin::GetSystemMenu() {
   if (!system_menu_.get()) {
+    CHECK(browser_frame_);
     SystemMenuInsertionDelegateWin insertion_delegate;
     system_menu_ = std::make_unique<views::NativeMenuWin>(
         browser_frame_->GetSystemMenuModel(), GetHWND());
@@ -265,6 +266,13 @@ bool BrowserDesktopWindowTreeHostWin::UsesNativeSystemMenu() const {
   return true;
 }
 
+void BrowserDesktopWindowTreeHostWin::ClientDestroyedWidget() {
+  system_menu_.reset();
+  browser_window_property_manager_.reset();
+  browser_frame_ = nullptr;
+  browser_view_ = nullptr;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserDesktopWindowTreeHostWin, views::DesktopWindowTreeHostWin overrides:
 
@@ -298,7 +306,9 @@ void BrowserDesktopWindowTreeHostWin::HandleWindowMinimizedOrRestored(
     bool restored) {
   DesktopWindowTreeHostWin::HandleWindowMinimizedOrRestored(restored);
 
-  browser_view_->UpdateLoadingAnimations(restored);
+  if (browser_view_) {
+    browser_view_->UpdateLoadingAnimations(restored);
+  }
 }
 
 void BrowserDesktopWindowTreeHostWin::HandleRequestClose() {
@@ -328,7 +338,7 @@ bool BrowserDesktopWindowTreeHostWin::GetClientAreaInsets(
     gfx::Insets* insets,
     int frame_thickness) const {
   // Always use default insets for opaque frame.
-  if (!ShouldUseNativeFrame()) {
+  if (!browser_view_ || !ShouldUseNativeFrame()) {
     return false;
   }
 
@@ -373,7 +383,8 @@ bool BrowserDesktopWindowTreeHostWin::GetDwmFrameInsetsInPixels(
   // an opaque frame, leading to graphical glitches behind the opaque frame.
   // Instead, we use that function below to tell us whether the frame is
   // currently native or opaque.
-  if (!GetWidget()->client_view() || !browser_view_->GetIsNormalType() ||
+  if (!browser_view_ || !browser_frame_ || !GetWidget()->client_view() ||
+      !browser_view_->GetIsNormalType() ||
       !DesktopWindowTreeHostWin::ShouldUseNativeFrame()) {
     return false;
   }
@@ -395,6 +406,10 @@ bool BrowserDesktopWindowTreeHostWin::GetDwmFrameInsetsInPixels(
 }
 
 void BrowserDesktopWindowTreeHostWin::HandleCreate() {
+  if (!browser_view_) {
+    return;
+  }
+
   DesktopWindowTreeHostWin::HandleCreate();
   browser_window_property_manager_ =
       BrowserWindowPropertyManager::CreateBrowserWindowPropertyManager(
@@ -484,6 +499,11 @@ void BrowserDesktopWindowTreeHostWin::PostHandleMSG(UINT message,
 }
 
 views::FrameMode BrowserDesktopWindowTreeHostWin::GetFrameMode() const {
+  if (!browser_view_) {
+    // If there is no browser view the frame should be system drawn.
+    return views::FrameMode::SYSTEM_DRAWN;
+  }
+
   const views::FrameMode system_frame_mode =
       ShouldBrowserCustomDrawTitlebar(browser_view_)
           ? views::FrameMode::SYSTEM_DRAWN_NO_CONTROLS
@@ -506,7 +526,8 @@ views::FrameMode BrowserDesktopWindowTreeHostWin::GetFrameMode() const {
 }
 
 bool BrowserDesktopWindowTreeHostWin::ShouldUseNativeFrame() const {
-  if (!views::DesktopWindowTreeHostWin::ShouldUseNativeFrame()) {
+  if (!browser_view_ ||
+      !views::DesktopWindowTreeHostWin::ShouldUseNativeFrame()) {
     return false;
   }
   // This function can get called when the Browser window is closed i.e. in the
@@ -528,6 +549,7 @@ bool BrowserDesktopWindowTreeHostWin::ShouldUseNativeFrame() const {
 
 bool BrowserDesktopWindowTreeHostWin::ShouldWindowContentsBeTransparent()
     const {
+  CHECK(browser_view_);
   return !ShouldBrowserCustomDrawTitlebar(browser_view_) &&
          views::DesktopWindowTreeHostWin::ShouldWindowContentsBeTransparent();
 }
@@ -539,6 +561,7 @@ void BrowserDesktopWindowTreeHostWin::OnProfileAvatarChanged(
     const base::FilePath& profile_path) {
   // If we're currently badging the window icon (>1 available profile),
   // and this window's profile's avatar changed, update the window icon.
+  CHECK(browser_view_);
   if (browser_view_->browser()->profile()->GetPath() == profile_path &&
       g_browser_process->profile_manager()
               ->GetProfileAttributesStorage()
@@ -613,6 +636,7 @@ void BrowserDesktopWindowTreeHostWin::SetWindowIcon(bool badged) {
   // icon is valid until replaced with the new icon.
   base::win::ScopedGDIObject<HICON> previous_icon = std::move(icon_handle_);
   if (badged) {
+    CHECK(browser_view_);
     icon_handle_ = IconUtil::CreateHICONFromSkBitmap(
         GetBadgedIconBitmapForProfile(browser_view_->browser()->profile()));
   } else {

chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.h

@@ -56,6 +56,7 @@ class BrowserDesktopWindowTreeHostWin
   DesktopWindowTreeHost* AsDesktopWindowTreeHost() override;
   int GetMinimizeButtonOffset() const override;
   bool UsesNativeSystemMenu() const override;
+  void ClientDestroyedWidget() override;
 
   // Overridden from DesktopWindowTreeHostWin:
   void Init(const views::Widget::InitParams& params) override;

chrome/browser/ui/views/frame/desktop_browser_frame_aura.cc

@@ -31,7 +31,6 @@ DesktopBrowserFrameAura::DesktopBrowserFrameAura(BrowserFrame* browser_frame,
       browser_view_(browser_view),
       browser_frame_(browser_frame),
       browser_desktop_window_tree_host_(nullptr) {
-  widget_observation_.Observe(browser_frame);
   GetNativeWindow()->SetName("BrowserFrameAura");
 }
 
@@ -132,8 +131,8 @@ bool DesktopBrowserFrameAura::ShouldUseInitialVisibleOnAllWorkspaces() const {
   return true;
 }
 
-void DesktopBrowserFrameAura::OnWidgetDestroyed(views::Widget* widget) {
+void DesktopBrowserFrameAura::ClientDestroyedWidget() {
+  browser_desktop_window_tree_host_->ClientDestroyedWidget();
   browser_frame_ = nullptr;
   browser_view_ = nullptr;
-  widget_observation_.Reset();
 }

chrome/browser/ui/views/frame/desktop_browser_frame_aura.h

@@ -8,13 +8,11 @@
 #include <memory>
 
 #include "base/memory/raw_ptr.h"
-#include "base/scoped_observation.h"
 #include "chrome/browser/ui/views/frame/native_browser_frame.h"
 #include "ui/aura/window.h"
 #include "ui/base/mojom/window_show_state.mojom-forward.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/widget/desktop_aura/desktop_native_widget_aura.h"
-#include "ui/views/widget/widget_observer.h"
 
 class BrowserDesktopWindowTreeHost;
 class BrowserFrame;
@@ -31,8 +29,7 @@ class VisibilityController;
 //  the window frame for the Chrome browser window.
 //
 class DesktopBrowserFrameAura : public views::DesktopNativeWidgetAura,
-                                public NativeBrowserFrame,
-                                public views::WidgetObserver {
+                                public NativeBrowserFrame {
  public:
   DesktopBrowserFrameAura(BrowserFrame* browser_frame,
                           BrowserView* browser_view);
@@ -68,9 +65,7 @@ class DesktopBrowserFrameAura : public views::DesktopNativeWidgetAura,
   bool HandleKeyboardEvent(const input::NativeWebKeyboardEvent& event) override;
   bool ShouldRestorePreviousBrowserWidgetState() const override;
   bool ShouldUseInitialVisibleOnAllWorkspaces() const override;
-
-  // views::WidgetObserver:
-  void OnWidgetDestroyed(views::Widget* widget) override;
+  void ClientDestroyedWidget() override;
 
  private:
   // The BrowserView is our ClientView. This is a pointer to it.
@@ -82,9 +77,6 @@ class DesktopBrowserFrameAura : public views::DesktopNativeWidgetAura,
       browser_desktop_window_tree_host_;
 
   std::unique_ptr<wm::VisibilityController> visibility_controller_;
-
-  base::ScopedObservation<views::Widget, views::WidgetObserver>
-      widget_observation_{this};
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_FRAME_DESKTOP_BROWSER_FRAME_AURA_H_

ui/views/test/mock_native_widget.cc

@@ -8,10 +8,13 @@ using testing::Return;
 
 namespace views {
 
-MockNativeWidget::MockNativeWidget(Widget* widget) : widget_(widget) {}
+MockNativeWidget::MockNativeWidget(Widget* widget)
+    : widget_(widget->GetWeakPtr()) {}
 
 MockNativeWidget::~MockNativeWidget() {
-  widget_->OnNativeWidgetDestroyed();
+  if (widget_) {
+    widget_->OnNativeWidgetDestroyed();
+  }
 }
 
 base::WeakPtr<internal::NativeWidgetPrivate> MockNativeWidget::GetWeakPtr() {

ui/views/test/mock_native_widget.h

@@ -46,6 +46,7 @@ class MockNativeWidget : public internal::NativeWidgetPrivate {
   MOCK_METHOD(const ui::Layer*, GetLayer, (), (const override));
   MOCK_METHOD(void, ReorderNativeViews, (), (override));
   MOCK_METHOD(void, ViewRemoved, (View * view), (override));
+  MOCK_METHOD(void, ClientDestroyedWidget, (), (override));
   MOCK_METHOD(void,
               SetNativeWindowProperty,
               (const char* name, void* value),
@@ -187,7 +188,7 @@ class MockNativeWidget : public internal::NativeWidgetPrivate {
   base::WeakPtr<NativeWidgetPrivate> GetWeakPtr() override;
 
  private:
-  raw_ptr<Widget> widget_;
+  base::WeakPtr<Widget> widget_;
   base::WeakPtrFactory<MockNativeWidget> weak_factory_{this};
 };
 

ui/views/widget/native_widget_private.cc

@@ -26,6 +26,8 @@ gfx::Rect NativeWidgetPrivate::ConstrainBoundsToDisplayWorkArea(
   return new_bounds;
 }
 
+void NativeWidgetPrivate::ClientDestroyedWidget() {}
+
 void NativeWidgetPrivate::PaintAsActiveChanged() {}
 
 void NativeWidgetPrivate::ShowWindowControlsMenu(const gfx::Point& point) {}

ui/views/widget/native_widget_private.h

@@ -130,6 +130,17 @@ class VIEWS_EXPORT NativeWidgetPrivate : public NativeWidget {
   // hierarchy.
   virtual void ViewRemoved(View* view) = 0;
 
+  // Notifies the NativeWidget that its Widget was destroyed by the client.
+  // NativeWidgets should override this to clear any references to its
+  // associated Widget. The NativeWidget destruction will be initiated
+  // separately by the host platform.
+  //
+  // Only relevant for CLIENT_OWNS_WIDGET ownership schemes. Not relevant for
+  // other widget ownership schemes
+  //  - NATIVE_WIDGET_OWNS_WIDGET - NativeWidget initiates Widget destruction.
+  //  - WIDGET_OWNS_NATIVE_WIDGET - Widget synchronously destroys NativeWidget.
+  virtual void ClientDestroyedWidget();
+
   // Sets/Gets a native window property on the underlying native window object.
   // Returns NULL if the property does not exist. Setting the property value to
   // NULL removes the property.

ui/views/widget/widget.cc

@@ -281,6 +281,15 @@ Widget::~Widget() {
       native_widget_->Close();
     }
     HandleWidgetDestroying();
+
+    // Specifically in the case of CLIENT_OWNS_WIDGET the native widget is
+    // notified to allow clearing of any widget-associated state. Do so before
+    // the call to `HandleWidgetDestroyed()` below which will invalidate
+    // `native_widget_`.
+    if (native_widget_) {
+      native_widget_->ClientDestroyedWidget();
+    }
+
     HandleWidgetDestroyed();
     if (widget_delegate_) {
       widget_delegate_->WidgetDestroying();

ui/views/widget/widget_unittest.cc

@@ -5795,6 +5795,23 @@ TEST_F(WidgetTest, ChildWidgetNotifiesObserverWhenReparented) {
   EXPECT_EQ(observer_2.child_widget(), nullptr);
 }
 
+TEST_F(WidgetTest, NativeWidgetNotifiedOfWidgetDestructionForClientOwnsWidget) {
+  auto widget = std::make_unique<Widget>();
+  Widget::InitParams params = CreateParams(
+      Widget::InitParams::CLIENT_OWNS_WIDGET, Widget::InitParams::TYPE_WINDOW);
+
+  auto native_widget =
+      std::make_unique<testing::NiceMock<MockNativeWidget>>(widget.get());
+  ON_CALL(*native_widget, CreateNonClientFrameView).WillByDefault([]() {
+    return std::make_unique<NonClientFrameView>();
+  });
+  params.native_widget = native_widget.get();
+  widget->Init(std::move(params));
+
+  EXPECT_CALL(*native_widget, ClientDestroyedWidget());
+  widget.reset();
+}
+
 // Parameterized test that verifies the behavior of SetAspectRatio with respect
 // to the excluded margin.
 class WidgetSetAspectRatioTest