Repository: electron/electron
Pull request: fix: potentially closed webContents in BrowserView

#### Description of Change

Fixes https://github.com/electron/electron/issues/42564.

This PR reworks some BrowserView logic to account for the fact that it's possible for a BrowserView to be actively attached to a BrowserWindow, but for the BrowserView's webContents to be closed. Previously, we were handling BrowserView ownerWindow logic by routing through the webContents' ownerWindow, and this would break in the above scenario. To fix this, we need to do the following:

- Ensure that when a child BrowserView is removed, it's removed even if its webContents is closed. Otherwise, if we go to add another BrowserView to the same window, our logic will have the incorrect number of existing children and DCHECK.
- Ensure that when a BrowserView's webContents is closed/destroyed, its owner window removes the BrowserView's webContentsView as a child.
 
#### Checklist
<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->

- [x] PR description included and stakeholders cc'd
- [x] `npm test` passes
- [x] tests are [changed or added](https://github.com/electron/electron/blob/main/docs/development/testing.md)
- [x] [PR release notes](https://github.com/electron/clerk/blob/main/README.md) describe the change in a way relevant to app developers, and are [capitalized, punctuated, and past tense](https://github.com/electron/clerk/blob/main/README.md#examples).

#### Release Notes

Notes: Fixed a potential error when calling `webContents.close()` on a BrowserView.


Changed files:
filename: lib/browser/api/browser-view.ts
patch: @@ -4,6 +4,9 @@ const v8Util = process._linkedBinding('electron_common_v8_util');
 
 export default class BrowserView {
   #webContentsView: WebContentsView;
+  #ownerWindow: BrowserWindow | null = null;
+
+  #destroyListener: ((e: any) => void) | null = null;
 
   // AutoResize state
   #resizeListener: ((...args: any[]) => void) | null = null;
@@ -17,6 +20,9 @@ export default class BrowserView {
     }
     webPreferences.type = 'browserView';
     this.#webContentsView = new WebContentsView({ webPreferences });
+
+    this.#destroyListener = this.#onDestroy.bind(this);
+    this.#webContentsView.webContents.once('destroyed', this.#destroyListener);
   }
 
   get webContents () {
@@ -55,23 +61,39 @@ export default class BrowserView {
 
   // Internal methods
   get ownerWindow (): BrowserWindow | null {
-    return !this.webContents.isDestroyed() ? this.webContents.getOwnerBrowserWindow() : null;
+    return this.#ownerWindow;
   }
 
+  // We can't rely solely on the webContents' owner window because
+  // a webContents can be closed by the user while the BrowserView
+  // remains alive and attached to a BrowserWindow.
   set ownerWindow (w: BrowserWindow | null) {
-    if (this.webContents.isDestroyed()) return;
-    const oldWindow = this.webContents.getOwnerBrowserWindow();
-    if (oldWindow && this.#resizeListener) {
-      oldWindow.off('resize', this.#resizeListener);
+    if (this.#ownerWindow && this.#resizeListener) {
+      this.#ownerWindow.off('resize', this.#resizeListener);
       this.#resizeListener = null;
     }
-    this.webContents._setOwnerWindow(w);
+
+    if (this.webContents && !this.webContents.isDestroyed()) {
+      this.webContents._setOwnerWindow(w);
+    }
+
+    this.#ownerWindow = w;
     if (w) {
       this.#lastWindowSize = w.getBounds();
       w.on('resize', this.#resizeListener = this.#autoResize.bind(this));
+      w.on('closed', () => {
+        this.#ownerWindow = null;
+        this.#destroyListener = null;
+      });
     }
   }
 
+  #onDestroy () {
+    // Ensure that if #webContentsView's webContents is destroyed,
+    // the WebContentsView is removed from the view hierarchy.
+    this.#ownerWindow?.contentView.removeChildView(this.webContentsView);
+  }
+
   #autoHorizontalProportion: {width: number, left: number} | null = null;
   #autoVerticalProportion: {height: number, top: number} | null = null;
   #autoResize () {

filename: lib/browser/api/browser-window.ts
patch: @@ -222,7 +222,9 @@ BrowserWindow.prototype.removeBrowserView = function (browserView: BrowserView)
 };
 
 BrowserWindow.prototype.getBrowserView = function () {
-  if (this._browserViews.length > 1) { throw new Error('This BrowserWindow has multiple BrowserViews, use getBrowserViews() instead'); }
+  if (this._browserViews.length > 1) {
+    throw new Error('This BrowserWindow has multiple BrowserViews - use getBrowserViews() instead');
+  }
   return this._browserViews[0] ?? null;
 };
 
@@ -231,7 +233,9 @@ BrowserWindow.prototype.getBrowserViews = function () {
 };
 
 BrowserWindow.prototype.setTopBrowserView = function (browserView: BrowserView) {
-  if (browserView.ownerWindow !== this) { throw new Error('Given BrowserView is not attached to the window'); }
+  if (browserView.ownerWindow !== this) {
+    throw new Error('Given BrowserView is not attached to the window');
+  }
   const idx = this._browserViews.indexOf(browserView);
   if (idx >= 0) {
     this.contentView.addChildView(browserView.webContentsView);

filename: shell/browser/api/electron_api_view.cc
patch: @@ -249,14 +249,16 @@ void View::AddChildViewAt(gin::Handle<View> child,
 void View::RemoveChildView(gin::Handle<View> child) {
   if (!view_)
     return;
-  if (!child->view())
-    return;
+
   const auto it = base::ranges::find(child_views_, child.ToV8());
   if (it != child_views_.end()) {
 #if BUILDFLAG(IS_MAC)
     ScopedCAActionDisabler disable_animations;
 #endif
-    view_->RemoveChildView(child->view());
+    // It's possible for the child's view to be invalid here
+    // if the child's webContents was closed or destroyed.
+    if (child->view())
+      view_->RemoveChildView(child->view());
     child_views_.erase(it);
   }
 }

filename: spec/api-browser-view-spec.ts
patch: @@ -370,6 +370,19 @@ describe('BrowserView module', () => {
       const view = w.getBrowserView();
       expect(view).to.be.null('view');
     });
+
+    it('throws if multiple BrowserViews are attached', () => {
+      view = new BrowserView();
+      w.setBrowserView(view);
+      const view2 = new BrowserView();
+      defer(() => view2.webContents.destroy());
+      w.addBrowserView(view2);
+      defer(() => w.removeBrowserView(view2));
+
+      expect(() => {
+        w.getBrowserView();
+      }).to.throw(/has multiple BrowserViews/);
+    });
   });
 
   describe('BrowserWindow.addBrowserView()', () => {
@@ -391,14 +404,6 @@ describe('BrowserView module', () => {
       w.addBrowserView(view);
     });
 
-    it('does not crash if the BrowserView webContents are destroyed prior to window addition', () => {
-      expect(() => {
-        const view1 = new BrowserView();
-        view1.webContents.destroy();
-        w.addBrowserView(view1);
-      }).to.not.throw();
-    });
-
     it('does not crash if the webContents is destroyed after a URL is loaded', () => {
       view = new BrowserView();
       expect(async () => {
@@ -411,13 +416,19 @@ describe('BrowserView module', () => {
     it('can handle BrowserView reparenting', async () => {
       view = new BrowserView();
 
+      expect(view.ownerWindow).to.be.null('ownerWindow');
+
       w.addBrowserView(view);
       view.webContents.loadURL('about:blank');
       await once(view.webContents, 'did-finish-load');
 
+      expect(view.ownerWindow).to.equal(w);
+
       const w2 = new BrowserWindow({ show: false });
       w2.addBrowserView(view);
 
+      expect(view.ownerWindow).to.equal(w2);
+
       w.close();
 
       view.webContents.loadURL(`file://${fixtures}/pages/blank.html`);
@@ -429,15 +440,31 @@ describe('BrowserView module', () => {
       w2.destroy();
     });
 
-    it('does not cause a crash when used for view with destroyed web contents', async () => {
+    it('allows attaching a BrowserView with a previously-closed webContents', async () => {
       const w2 = new BrowserWindow({ show: false });
       const view = new BrowserView();
+
+      expect(view.ownerWindow).to.be.null('ownerWindow');
       view.webContents.close();
       w2.addBrowserView(view);
+      expect(view.ownerWindow).to.equal(w2);
+
       w2.webContents.loadURL('about:blank');
       await once(w2.webContents, 'did-finish-load');
       w2.close();
     });
+
+    it('allows attaching a BrowserView with a previously-destroyed webContents', async () => {
+      const view = new BrowserView();
+
+      expect(view.ownerWindow).to.be.null('ownerWindow');
+      view.webContents.destroy();
+      w.addBrowserView(view);
+      expect(view.ownerWindow).to.equal(w);
+
+      w.webContents.loadURL('about:blank');
+      await once(w.webContents, 'did-finish-load');
+    });
   });
 
   describe('BrowserWindow.removeBrowserView()', () => {
@@ -535,16 +562,47 @@ describe('BrowserView module', () => {
     });
   });
 
-  describe('BrowserView.webContents.getOwnerBrowserWindow()', () => {
+  describe('BrowserView owning window', () => {
     it('points to owning window', () => {
       view = new BrowserView();
       expect(view.webContents.getOwnerBrowserWindow()).to.be.null('owner browser window');
+      expect(view.ownerWindow).to.be.null('ownerWindow');
 
       w.setBrowserView(view);
       expect(view.webContents.getOwnerBrowserWindow()).to.equal(w);
+      expect(view.ownerWindow).to.equal(w);
 
       w.setBrowserView(null);
       expect(view.webContents.getOwnerBrowserWindow()).to.be.null('owner browser window');
+      expect(view.ownerWindow).to.be.null('ownerWindow');
+    });
+
+    it('works correctly when the webContents is destroyed', async () => {
+      view = new BrowserView();
+      w.setBrowserView(view);
+
+      expect(view.webContents.getOwnerBrowserWindow()).to.equal(w);
+      expect(view.ownerWindow).to.equal(w);
+
+      const destroyed = once(view.webContents, 'destroyed');
+      view.webContents.close();
+      await destroyed;
+
+      expect(view.ownerWindow).to.equal(w);
+    });
+
+    it('works correctly when owner window is closed', async () => {
+      view = new BrowserView();
+      w.setBrowserView(view);
+
+      expect(view.webContents.getOwnerBrowserWindow()).to.equal(w);
+      expect(view.ownerWindow).to.equal(w);
+
+      const destroyed = once(w, 'closed');
+      w.close();
+      await destroyed;
+
+      expect(view.ownerWindow).to.equal(null);
     });
   });