fix: remove cyclic references of BrowserWindow (#22006)

* fix: remove cyclic references in BrowserWindow

* fix: prevent TopLevelWindow from garbage collection

* test: garbage collection of BrowserWindow

* chore: createIDWeakMap is used in tests

lib/browser/api/browser-window.js

@@ -25,26 +25,6 @@ BrowserWindow.prototype._init = function () {
     nativeSetBounds.call(this, bounds, ...opts)
   }
 
-  // window.resizeTo(...)
-  // window.moveTo(...)
-  this.webContents.on('move', (event, size) => {
-    this.setBounds(size)
-  })
-
-  // Hide the auto-hide menu when webContents is focused.
-  this.webContents.on('activate', () => {
-    if (process.platform !== 'darwin' && this.autoHideMenuBar && this.isMenuBarVisible()) {
-      this.setMenuBarVisibility(false)
-    }
-  })
-
-  // Change window title to page title.
-  this.webContents.on('page-title-updated', (event, title, ...args) => {
-    // Route the event to BrowserWindow.
-    this.emit('page-title-updated', event, title, ...args)
-    if (!this.isDestroyed() && !event.defaultPrevented) this.setTitle(title)
-  })
-
   // Sometimes the webContents doesn't get focus when window is shown, so we
   // have to force focusing on webContents in this case. The safest way is to
   // focus it when we first start to load URL, if we do it earlier it won't

shell/browser/api/electron_api_browser_window.cc

@@ -222,6 +222,31 @@ void BrowserWindow::OnDraggableRegionsUpdated(
   UpdateDraggableRegions(regions);
 }
 
+void BrowserWindow::OnSetContentBounds(const gfx::Rect& rect) {
+  // window.resizeTo(...)
+  // window.moveTo(...)
+  window()->SetBounds(rect, false);
+}
+
+void BrowserWindow::OnActivateContents() {
+  // Hide the auto-hide menu when webContents is focused.
+#if !defined(OS_MACOSX)
+  if (IsMenuBarAutoHide() && IsMenuBarVisible())
+    window()->SetMenuBarVisibility(false);
+#endif
+}
+
+void BrowserWindow::OnPageTitleUpdated(const base::string16& title,
+                                       bool explicit_set) {
+  // Change window title to page title.
+  auto self = GetWeakPtr();
+  if (!Emit("page-title-updated", title, explicit_set)) {
+    // |this| might be deleted, or marked as destroyed by close().
+    if (self && !IsDestroyed())
+      SetTitle(base::UTF16ToUTF8(title));
+  }
+}
+
 void BrowserWindow::RequestPreferredWidth(int* width) {
   *width = web_contents()->GetPreferredSize().width();
 }
@@ -251,6 +276,8 @@ void BrowserWindow::OnCloseButtonClicked(bool* prevent_default) {
 
 void BrowserWindow::OnWindowClosed() {
   Cleanup();
+  // See TopLevelWindow::OnWindowClosed on why calling InvalidateWeakPtrs.
+  weak_factory_.InvalidateWeakPtrs();
   TopLevelWindow::OnWindowClosed();
 }
 

shell/browser/api/electron_api_browser_window.h

@@ -57,6 +57,10 @@ class BrowserWindow : public TopLevelWindow,
   void OnRendererResponsive() override;
   void OnDraggableRegionsUpdated(
       const std::vector<mojom::DraggableRegionPtr>& regions) override;
+  void OnSetContentBounds(const gfx::Rect& rect) override;
+  void OnActivateContents() override;
+  void OnPageTitleUpdated(const base::string16& title,
+                          bool explicit_set) override;
 
   // NativeWindowObserver:
   void RequestPreferredWidth(int* width) override;

shell/browser/api/electron_api_top_level_window.cc

@@ -120,6 +120,9 @@ TopLevelWindow::~TopLevelWindow() {
   // Destroy the native window in next tick because the native code might be
   // iterating all windows.
   base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, window_.release());
+
+  // Remove global reference so the JS object can be garbage collected.
+  self_ref_.Reset();
 }
 
 void TopLevelWindow::InitWith(v8::Isolate* isolate,
@@ -135,6 +138,9 @@ void TopLevelWindow::InitWith(v8::Isolate* isolate,
     DCHECK(!parent.IsEmpty());
     parent->child_windows_.Set(isolate, weak_map_id(), wrapper);
   }
+
+  // Reference this object in case it got garbage collected.
+  self_ref_.Reset(isolate, wrapper);
 }
 
 void TopLevelWindow::WillCloseWindow(bool* prevent_default) {

shell/browser/api/electron_api_top_level_window.h

@@ -259,6 +259,9 @@ class TopLevelWindow : public gin_helper::TrackableObject<TopLevelWindow>,
 
   std::unique_ptr<NativeWindow> window_;
 
+  // Reference to JS wrapper to prevent garbage collection.
+  v8::Global<v8::Value> self_ref_;
+
   base::WeakPtrFactory<TopLevelWindow> weak_factory_;
 };
 

shell/browser/api/electron_api_web_contents.cc

@@ -705,8 +705,9 @@ void WebContents::BeforeUnloadFired(content::WebContents* tab,
 }
 
 void WebContents::SetContentsBounds(content::WebContents* source,
-                                    const gfx::Rect& pos) {
-  Emit("move", pos);
+                                    const gfx::Rect& rect) {
+  for (ExtendedWebContentsObserver& observer : observers_)
+    observer.OnSetContentBounds(rect);
 }
 
 void WebContents::CloseContents(content::WebContents* source) {
@@ -725,7 +726,8 @@ void WebContents::CloseContents(content::WebContents* source) {
 }
 
 void WebContents::ActivateContents(content::WebContents* source) {
-  Emit("activate");
+  for (ExtendedWebContentsObserver& observer : observers_)
+    observer.OnActivateContents();
 }
 
 void WebContents::UpdateTargetURL(content::WebContents* source,
@@ -1228,6 +1230,8 @@ void WebContents::TitleWasSet(content::NavigationEntry* entry) {
       final_title = title;
     }
   }
+  for (ExtendedWebContentsObserver& observer : observers_)
+    observer.OnPageTitleUpdated(final_title, explicit_set);
   Emit("page-title-updated", final_title, explicit_set);
 }
 

shell/browser/api/electron_api_web_contents.h

@@ -81,6 +81,10 @@ class ExtendedWebContentsObserver : public base::CheckedObserver {
   virtual void OnRendererResponsive() {}
   virtual void OnDraggableRegionsUpdated(
       const std::vector<mojom::DraggableRegionPtr>& regions) {}
+  virtual void OnSetContentBounds(const gfx::Rect& rect) {}
+  virtual void OnActivateContents() {}
+  virtual void OnPageTitleUpdated(const base::string16& title,
+                                  bool explicit_set) {}
 
  protected:
   ~ExtendedWebContentsObserver() override {}

shell/common/api/electron_api_v8_util.cc

@@ -126,12 +126,12 @@ void Initialize(v8::Local<v8::Object> exports,
                  &electron::RemoteCallbackFreer::BindTo);
   dict.SetMethod("setRemoteObjectFreer", &electron::RemoteObjectFreer::BindTo);
   dict.SetMethod("addRemoteObjectRef", &electron::RemoteObjectFreer::AddRef);
-  dict.SetMethod("createIDWeakMap",
-                 &electron::api::KeyWeakMap<int32_t>::Create);
   dict.SetMethod(
       "createDoubleIDWeakMap",
       &electron::api::KeyWeakMap<std::pair<std::string, int32_t>>::Create);
 #endif
+  dict.SetMethod("createIDWeakMap",
+                 &electron::api::KeyWeakMap<int32_t>::Create);
   dict.SetMethod("requestGarbageCollectionForTesting",
                  &RequestGarbageCollectionForTesting);
   dict.SetMethod("isSameOrigin", &IsSameOrigin);

spec-main/api-browser-window-spec.ts

@@ -9,7 +9,7 @@ import { app, BrowserWindow, BrowserView, ipcMain, OnBeforeSendHeadersListenerDe
 
 import { emittedOnce } from './events-helpers'
 import { ifit, ifdescribe } from './spec-helpers'
-import { closeWindow } from './window-helpers'
+import { closeWindow, closeAllWindows } from './window-helpers'
 
 const fixtures = path.resolve(__dirname, '..', 'spec', 'fixtures')
 
@@ -37,12 +37,6 @@ const expectBoundsEqual = (actual: any, expected: any) => {
   }
 }
 
-const closeAllWindows = async () => {
-  for (const w of BrowserWindow.getAllWindows()) {
-    await closeWindow(w, { assertNotWindows: false })
-  }
-}
-
 describe('BrowserWindow module', () => {
   describe('BrowserWindow constructor', () => {
     it('allows passing void 0 as the webContents', async () => {
@@ -58,6 +52,28 @@ describe('BrowserWindow module', () => {
     })
   })
 
+  describe('garbage collection', () => {
+    const v8Util = process.electronBinding('v8_util')
+    afterEach(closeAllWindows)
+
+    it('window does not get garbage collected when opened', (done) => {
+      const w = new BrowserWindow({ show: false })
+      // Keep a weak reference to the window.
+      const map = v8Util.createIDWeakMap<Electron.BrowserWindow>()
+      map.set(0, w)
+      setTimeout(() => {
+        // Do garbage collection, since |w| is not referenced in this closure
+        // it would be gone after next call if there is no other reference.
+        v8Util.requestGarbageCollectionForTesting()
+
+        setTimeout(() => {
+          expect(map.has(0)).to.equal(true)
+          done()
+        })
+      })
+    })
+  })
+
   describe('BrowserWindow.close()', () => {
     let w = null as unknown as BrowserWindow
     beforeEach(() => {