fix: reparenting after BrowserWindow.destroy() (#39308)

fix: reparenting after BrowserWindow.destroy() Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
electron · Aug 1, 2023 · b8f05c3 · b8f05c3
1 parent 990fb72
commit b8f05c3
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 9 deletions.
diff --git a/shell/browser/native_window.h b/shell/browser/native_window.h
@@ -148,6 +148,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
@@ -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
@@ -431,7 +431,12 @@ void ReorderChildWindowAbove(NSWindow* child_window, NSWindow* other_window) {
   }
 
   // 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
@@ -450,13 +455,20 @@ void ReorderChildWindowAbove(NSWindow* child_window, NSWindow* other_window) {
 }
 
 void NativeWindowMac::CloseImmediately() {
-  RemoveChildFromParentWindow(this);
+  // Ensure we're detached from the parent window before closing.
+  RemoveChildFromParentWindow();
 
   // Retain the child window before closing it. If the last reference to the
   // NSWindow goes away inside -[NSWindow close], then bad stuff can happen.
   // See e.g. http://crbug.com/616701.
   base::scoped_nsobject<NSWindow> child_window(window_,
                                                base::scoped_policy::RETAIN);
+
+  while (!child_windows_.empty()) {
+    auto* child = child_windows_.back();
+    child->RemoveChildFromParentWindow();
+  }
+
   [window_ close];
 }
 
@@ -645,9 +657,11 @@ void ReorderChildWindowAbove(NSWindow* child_window, NSWindow* other_window) {
   [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() {
@@ -1867,12 +1881,13 @@ int NonClientHitTest(const gfx::Point& point) override {
     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
@@ -4790,6 +4790,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 });
@@ -4799,6 +4800,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 });
@@ -4810,6 +4812,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', () => {