fix: potentially closed webContents in BrowserView (#42811)

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

lib/browser/api/browser-view.ts

@@ -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 () {

lib/browser/api/browser-window.ts

@@ -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);

shell/browser/api/electron_api_view.cc

@@ -248,14 +248,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);
   }
 }

spec/api-browser-view-spec.ts

@@ -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);
     });
   });