fix: crash due to race between attach and destruction of webview (#24344)

lib/browser/guest-view-manager.ts

@@ -15,6 +15,7 @@ interface GuestInstance {
 }
 
 const webViewManager = process._linkedBinding('electron_browser_web_view_manager');
+const eventBinding = process._linkedBinding('electron_browser_event');
 
 const supportedWebViewEvents = Object.keys(webViewEvents);
 
@@ -75,7 +76,7 @@ function makeWebPreferences (embedder: Electron.WebContents, params: Record<stri
 }
 
 // Create a new guest instance.
-const createGuest = function (embedder: Electron.WebContents, params: Record<string, any>) {
+const createGuest = function (embedder: Electron.WebContents, embedderFrameId: number, elementInstanceId: number, params: Record<string, any>) {
   // eslint-disable-next-line no-undef
   const guest = (webContents as typeof ElectronInternal.WebContents).create({
     type: 'webview',
@@ -159,20 +160,22 @@ const createGuest = function (embedder: Electron.WebContents, params: Record<str
     }
   });
 
-  return guestInstanceId;
+  if (attachGuest(embedder, embedderFrameId, elementInstanceId, guestInstanceId, params)) {
+    return guestInstanceId;
+  }
+
+  return -1;
 };
 
 // Attach the guest to an element of embedder.
-const attachGuest = function (event: Electron.IpcMainInvokeEvent,
+const attachGuest = function (embedder: Electron.WebContents, embedderFrameId: number, elementInstanceId: number, guestInstanceId: number, params: Record<string, any>) {
-  embedderFrameId: number, elementInstanceId: number, guestInstanceId: number, params: Record<string, any>) {
-  const embedder = event.sender;
   // Destroy the old guest when attaching.
   const key = `${embedder.id}-${elementInstanceId}`;
   const oldGuestInstanceId = embedderElementsMap.get(key);
   if (oldGuestInstanceId != null) {
     // Reattachment to the same guest is just a no-op.
     if (oldGuestInstanceId === guestInstanceId) {
-      return;
+      return false;
     }
 
     const oldGuestInstance = guestInstances.get(oldGuestInstanceId);
@@ -184,11 +187,13 @@ const attachGuest = function (event: Electron.IpcMainInvokeEvent,
   const guestInstance = guestInstances.get(guestInstanceId);
   // If this isn't a valid guest instance then do nothing.
   if (!guestInstance) {
-    throw new Error(`Invalid guestInstanceId: ${guestInstanceId}`);
+    console.error(new Error(`Guest attach failed: Invalid guestInstanceId ${guestInstanceId}`));
+    return false;
   }
   const { guest } = guestInstance;
   if (guest.hostWebContents !== embedder) {
-    throw new Error(`Access denied to guestInstanceId: ${guestInstanceId}`);
+    console.error(new Error(`Guest attach failed: Access denied to guestInstanceId ${guestInstanceId}`));
+    return false;
   }
 
   // If this guest is already attached to an element then remove it
@@ -205,11 +210,12 @@ const attachGuest = function (event: Electron.IpcMainInvokeEvent,
 
   const webPreferences = makeWebPreferences(embedder, params);
 
+  const event = eventBinding.createWithSender(embedder);
   embedder.emit('will-attach-webview', event, webPreferences, params);
   if (event.defaultPrevented) {
     if (guest.viewInstanceId == null) guest.viewInstanceId = params.instanceId;
     guest.destroy();
-    return;
+    return false;
   }
 
   guest.attachParams = params;
@@ -223,6 +229,7 @@ const attachGuest = function (event: Electron.IpcMainInvokeEvent,
 
   webViewManager.addGuest(guestInstanceId, embedder, guest, webPreferences);
   guest.attachToIframe(embedder, embedderFrameId);
+  return true;
 };
 
 // Remove an guest-embedder relationship.
@@ -307,16 +314,8 @@ const handleMessageSync = function (channel: string, handler: (event: ElectronIn
   ipcMainUtils.handleSync(channel, makeSafeHandler(channel, handler));
 };
 
-handleMessage(IPC_MESSAGES.GUEST_VIEW_MANAGER_CREATE_GUEST, function (event, params) {
+handleMessage(IPC_MESSAGES.GUEST_VIEW_MANAGER_CREATE_AND_ATTACH_GUEST, function (event, embedderFrameId: number, elementInstanceId: number, params) {
-  return createGuest(event.sender, params);
+  return createGuest(event.sender, embedderFrameId, elementInstanceId, params);
-});
-
-handleMessage(IPC_MESSAGES.GUEST_VIEW_MANAGER_ATTACH_GUEST, function (event, embedderFrameId: number, elementInstanceId: number, guestInstanceId: number, params) {
-  try {
-    attachGuest(event, embedderFrameId, elementInstanceId, guestInstanceId, params);
-  } catch (error) {
-    console.error(`Guest attach failed: ${error}`);
-  }
 });
 
 handleMessageSync(IPC_MESSAGES.GUEST_VIEW_MANAGER_DETACH_GUEST, function (event, guestInstanceId: number) {

lib/common/ipc-messages.ts

@@ -11,8 +11,7 @@ export const enum IPC_MESSAGES {
   GUEST_VIEW_INTERNAL_DISPATCH_EVENT = 'GUEST_VIEW_INTERNAL_DISPATCH_EVENT',
   GUEST_VIEW_INTERNAL_IPC_MESSAGE = 'GUEST_VIEW_INTERNAL_IPC_MESSAGE',
 
-  GUEST_VIEW_MANAGER_CREATE_GUEST = 'GUEST_VIEW_MANAGER_CREATE_GUEST',
+  GUEST_VIEW_MANAGER_CREATE_AND_ATTACH_GUEST = 'GUEST_VIEW_MANAGER_CREATE_AND_ATTACH_GUEST',
-  GUEST_VIEW_MANAGER_ATTACH_GUEST = 'GUEST_VIEW_MANAGER_ATTACH_GUEST',
   GUEST_VIEW_MANAGER_DETACH_GUEST = 'GUEST_VIEW_MANAGER_DETACH_GUEST',
   GUEST_VIEW_MANAGER_FOCUS_CHANGE = 'GUEST_VIEW_MANAGER_FOCUS_CHANGE',
   GUEST_VIEW_MANAGER_CALL = 'GUEST_VIEW_MANAGER_CALL',

lib/renderer/web-view/guest-view-internal.ts

@@ -48,11 +48,7 @@ export function deregisterEvents (viewInstanceId: number) {
   ipcRendererInternal.removeAllListeners(`${IPC_MESSAGES.GUEST_VIEW_INTERNAL_IPC_MESSAGE}-${viewInstanceId}`);
 }
 
-export function createGuest (params: Record<string, any>): Promise<number> {
+export function createGuest (iframe: HTMLIFrameElement, elementInstanceId: number, params: Record<string, any>): Promise<number> {
-  return ipcRendererInternal.invoke(IPC_MESSAGES.GUEST_VIEW_MANAGER_CREATE_GUEST, params);
-}
-
-export function attachGuest (iframe: HTMLIFrameElement, elementInstanceId: number, guestInstanceId: number, params: Record<string, any>) {
   if (!(iframe instanceof HTMLIFrameElement)) {
     throw new Error('Invalid embedder frame');
   }
@@ -62,7 +58,7 @@ export function attachGuest (iframe: HTMLIFrameElement, elementInstanceId: numbe
     throw new Error('Invalid embedder frame');
   }
 
-  return ipcRendererInternal.invoke(IPC_MESSAGES.GUEST_VIEW_MANAGER_ATTACH_GUEST, embedderFrameId, elementInstanceId, guestInstanceId, params);
+  return ipcRendererInternal.invoke(IPC_MESSAGES.GUEST_VIEW_MANAGER_CREATE_AND_ATTACH_GUEST, embedderFrameId, elementInstanceId, params);
 }
 
 export function detachGuest (guestInstanceId: number) {

lib/renderer/web-view/web-view-impl.ts

@@ -115,9 +115,11 @@ export class WebViewImpl {
   }
 
   createGuest () {
-    this.hooks.guestViewInternal.createGuest(this.buildParams()).then(guestInstanceId => {
+    this.internalInstanceId = getNextId();
-      this.attachGuestInstance(guestInstanceId);
+    this.hooks.guestViewInternal.createGuest(this.internalElement, this.internalInstanceId, this.buildParams())
-    });
+      .then(guestInstanceId => {
+        this.attachGuestInstance(guestInstanceId);
+      });
   }
 
   dispatchEvent (eventName: string, props: Record<string, any> = {}) {
@@ -192,20 +194,19 @@ export class WebViewImpl {
   }
 
   attachGuestInstance (guestInstanceId: number) {
-    if (!this.elementAttached) {
+    if (guestInstanceId === -1) {
-      // The element could be detached before we got response from browser.
+      // Do nothing
       return;
     }
-    this.internalInstanceId = getNextId();
+
+    if (!this.elementAttached) {
+      // The element could be detached before we got response from browser.
+      // Destroy the backing webContents to avoid any zombie nodes in the frame tree.
+      this.hooks.guestViewInternal.detachGuest(guestInstanceId);
+      return;
+    }
+
     this.guestInstanceId = guestInstanceId;
-
-    this.hooks.guestViewInternal.attachGuest(
-      this.internalElement,
-      this.internalInstanceId,
-      this.guestInstanceId,
-      this.buildParams()
-    );
-
     // TODO(zcbenz): Should we deprecate the "resize" event? Wait, it is not
     // even documented.
     this.resizeObserver = new ResizeObserver(this.onElementResize.bind(this));