fix: reparenting UAF crash on macOS (#38677)

* fix: reparenting UAF crash on macOS

Co-authored-by: Shelley Vohr <[email protected]>

* Update api-browser-window-spec.ts

* Update api-browser-window-spec.ts

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

shell/browser/native_window_mac.h

@@ -157,6 +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);
   // Attach child windows, if the window is visible.
   void AttachChildren() override;
   // Detach window from parent without destroying it.

shell/browser/native_window_mac.mm

@@ -389,6 +389,9 @@ void NativeWindowMac::Close() {
     return;
   }
 
+  // Ensure we're detached from the parent window before closing.
+  RemoveChildFromParentWindow(this);
+
   // If a sheet is attached to the window when we call
   // [window_ performClose:nil], the window won't close properly
   // even after the user has ended the sheet.
@@ -406,6 +409,8 @@ void NativeWindowMac::Close() {
 }
 
 void NativeWindowMac::CloseImmediately() {
+  RemoveChildFromParentWindow(this);
+
   // 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.
@@ -599,6 +604,11 @@ void NativeWindowMac::RemoveChildWindow(NativeWindow* child) {
   [window_ removeChildWindow:child->GetNativeWindow().GetNativeNSWindow()];
 }
 
+void NativeWindowMac::RemoveChildFromParentWindow(NativeWindow* child) {
+  if (parent())
+    parent()->RemoveChildWindow(child);
+}
+
 void NativeWindowMac::AttachChildren() {
   for (auto* child : child_windows_) {
     auto* child_nswindow = child->GetNativeWindow().GetNativeNSWindow();
@@ -1779,27 +1789,27 @@ void NativeWindowMac::InternalSetWindowButtonVisibility(bool visible) {
   [[window_ standardWindowButton:NSWindowZoomButton] setHidden:!visible];
 }
 
-void NativeWindowMac::InternalSetParentWindow(NativeWindow* parent,
+void NativeWindowMac::InternalSetParentWindow(NativeWindow* new_parent,
                                               bool attach) {
   if (is_modal())
     return;
 
-  NativeWindow::SetParentWindow(parent);
-
   // Do not remove/add if we are already properly attached.
-  if (attach && parent &&
-      [window_ parentWindow] == parent->GetNativeWindow().GetNativeNSWindow())
+  if (attach && new_parent &&
+      [window_ parentWindow] ==
+          new_parent->GetNativeWindow().GetNativeNSWindow())
     return;
 
   // Remove current parent window.
-  if ([window_ parentWindow])
-    parent->RemoveChildWindow(this);
+  RemoveChildFromParentWindow(this);
 
   // Set new parent window.
-  if (parent && attach) {
-    parent->add_child_window(this);
-    parent->AttachChildren();
+  if (new_parent && attach) {
+    new_parent->add_child_window(this);
+    new_parent->AttachChildren();
   }
+
+  NativeWindow::SetParentWindow(new_parent);
 }
 
 void NativeWindowMac::SetForwardMouseMessages(bool forward) {

spec/api-browser-window-spec.ts

@@ -4267,6 +4267,21 @@ describe('BrowserWindow module', () => {
         expect(w.getChildWindows().length).to.equal(0);
       });
 
+      it('can handle child window close and reparent multiple times', async () => {
+        const w = new BrowserWindow({ show: false });
+        let c: BrowserWindow | null;
+
+        for (let i = 0; i < 5; i++) {
+          c = new BrowserWindow({ show: false, parent: w });
+          const closed = emittedOnce(c, 'closed');
+          c.close();
+          await closed;
+        }
+
+        await delay();
+        expect(w.getChildWindows().length).to.equal(0);
+      });
+
       ifit(process.platform === 'darwin')('child window matches visibility when visibility changes', async () => {
         const w = new BrowserWindow({ show: false });
         const c = new BrowserWindow({ show: false, parent: w });