From 695fcf3cb265aa2b635e9bfdad4e0affe527c917 Mon Sep 17 00:00:00 2001
From: Shelley Vohr <shelley.vohr@gmail.com>
Date: Wed, 26 Jul 2023 16:47:32 +0200
Subject: [PATCH] fix: reparenting after `BrowserWindow.destroy()` (#39062)

fix: reparenting after BrowserWindow.destroy()
---
 shell/browser/native_window.h      |  1 +
 shell/browser/native_window_mac.h  |  2 +-
 shell/browser/native_window_mac.mm | 31 ++++++++++++++++++++++--------
 spec/api-browser-window-spec.ts    | 21 ++++++++++++++++++++
 4 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/shell/browser/native_window.h b/shell/browser/native_window.h
index ef3792300c9f..8ce87494582a 100644
--- a/shell/browser/native_window.h
+++ b/shell/browser/native_window.h
@@ -149,6 +149,7 @@ class NativeWindow : public base::SupportsUserData,
   virtual std::string GetAlwaysOnTopLevel() = 0;
   virtual void SetActive(bool is_key) = 0;
   virtual bool IsActive() const = 0;
+  virtual void RemoveChildFromParentWindow() = 0;
   virtual void RemoveChildWindow(NativeWindow* child) = 0;
   virtual void AttachChildren() = 0;
   virtual void DetachChildren() = 0;
diff --git a/shell/browser/native_window_mac.h b/shell/browser/native_window_mac.h
index 39246fddb6a6..9b089c52640f 100644
--- a/shell/browser/native_window_mac.h
+++ b/shell/browser/native_window_mac.h
@@ -157,7 +157,7 @@ class NativeWindowMac : public NativeWindow,
   bool IsActive() const override;
   // Remove the specified child window without closing it.
   void RemoveChildWindow(NativeWindow* child) override;
-  void RemoveChildFromParentWindow(NativeWindow* child);
+  void RemoveChildFromParentWindow() override;
   // Attach child windows, if the window is visible.
   void AttachChildren() override;
   // Detach window from parent without destroying it.
diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm
index 7af696339ed4..7c44cfbed18a 100644
--- a/shell/browser/native_window_mac.mm
+++ b/shell/browser/native_window_mac.mm
@@ -434,7 +434,12 @@ void NativeWindowMac::Close() {
   }
 
   // Ensure we're detached from the parent window before closing.
-  RemoveChildFromParentWindow(this);
+  RemoveChildFromParentWindow();
+
+  while (!child_windows_.empty()) {
+    auto* child = child_windows_.back();
+    child->RemoveChildFromParentWindow();
+  }
 
   // If a sheet is attached to the window when we call
   // [window_ performClose:nil], the window won't close properly
@@ -453,7 +458,14 @@ void NativeWindowMac::Close() {
 }
 
 void NativeWindowMac::CloseImmediately() {
-  RemoveChildFromParentWindow(this);
+  // Ensure we're detached from the parent window before closing.
+  RemoveChildFromParentWindow();
+
+  while (!child_windows_.empty()) {
+    auto* child = child_windows_.back();
+    child->RemoveChildFromParentWindow();
+  }
+
   [window_ close];
 }
 
@@ -642,9 +654,11 @@ void NativeWindowMac::RemoveChildWindow(NativeWindow* child) {
   [window_ removeChildWindow:child->GetNativeWindow().GetNativeNSWindow()];
 }
 
-void NativeWindowMac::RemoveChildFromParentWindow(NativeWindow* child) {
-  if (parent())
-    parent()->RemoveChildWindow(child);
+void NativeWindowMac::RemoveChildFromParentWindow() {
+  if (parent() && !is_modal()) {
+    parent()->RemoveChildWindow(this);
+    NativeWindow::SetParentWindow(nullptr);
+  }
 }
 
 void NativeWindowMac::AttachChildren() {
@@ -1842,12 +1856,13 @@ void NativeWindowMac::InternalSetParentWindow(NativeWindow* new_parent,
     return;
 
   // Remove current parent window.
-  RemoveChildFromParentWindow(this);
+  RemoveChildFromParentWindow();
 
   // Set new parent window.
-  if (new_parent && attach) {
+  if (new_parent) {
     new_parent->add_child_window(this);
-    new_parent->AttachChildren();
+    if (attach)
+      new_parent->AttachChildren();
   }
 
   NativeWindow::SetParentWindow(new_parent);
diff --git a/spec/api-browser-window-spec.ts b/spec/api-browser-window-spec.ts
index 25071881ff51..ed55d9af8bc0 100644
--- a/spec/api-browser-window-spec.ts
+++ b/spec/api-browser-window-spec.ts
@@ -4797,6 +4797,7 @@ describe('BrowserWindow module', () => {
         c.setParentWindow(null);
         expect(c.getParentWindow()).to.be.null('c.parent');
       });
+
       it('adds window to child windows of parent', () => {
         const w = new BrowserWindow({ show: false });
         const c = new BrowserWindow({ show: false });
@@ -4806,6 +4807,7 @@ describe('BrowserWindow module', () => {
         c.setParentWindow(null);
         expect(w.getChildWindows()).to.deep.equal([]);
       });
+
       it('removes from child windows of parent when window is closed', async () => {
         const w = new BrowserWindow({ show: false });
         const c = new BrowserWindow({ show: false });
@@ -4817,6 +4819,25 @@ describe('BrowserWindow module', () => {
         await setTimeout();
         expect(w.getChildWindows().length).to.equal(0);
       });
+
+      ifit(process.platform === 'darwin')('can reparent when the first parent is destroyed', async () => {
+        const w1 = new BrowserWindow({ show: false });
+        const w2 = new BrowserWindow({ show: false });
+        const c = new BrowserWindow({ show: false });
+
+        c.setParentWindow(w1);
+        expect(w1.getChildWindows().length).to.equal(1);
+
+        const closed = once(w1, 'closed');
+        w1.destroy();
+        await closed;
+
+        c.setParentWindow(w2);
+        await setTimeout();
+
+        const children = w2.getChildWindows();
+        expect(children[0]).to.equal(c);
+      });
     });
 
     describe('modal option', () => {