From 2b897c8ad83296aeee8c1321fcbd411a0f353fcb Mon Sep 17 00:00:00 2001 From: Robo Date: Mon, 2 Aug 2021 08:35:57 -0700 Subject: [PATCH] fix: crash due to race between attach and destruction of webview (#24344) --- lib/browser/guest-view-manager.ts | 37 ++++++++++---------- lib/common/ipc-messages.ts | 3 +- lib/renderer/web-view/guest-view-internal.ts | 8 ++--- lib/renderer/web-view/web-view-impl.ts | 29 +++++++-------- 4 files changed, 36 insertions(+), 41 deletions(-) diff --git a/lib/browser/guest-view-manager.ts b/lib/browser/guest-view-manager.ts index 236b821c6d37..9d0afa2f2fb7 100644 --- a/lib/browser/guest-view-manager.ts +++ b/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) { +const createGuest = function (embedder: Electron.WebContents, embedderFrameId: number, elementInstanceId: number, params: Record) { // 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) { - const embedder = event.sender; +const attachGuest = function (embedder: Electron.WebContents, embedderFrameId: number, elementInstanceId: number, guestInstanceId: number, params: Record) { // 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) { - return createGuest(event.sender, 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}`); - } +handleMessage(IPC_MESSAGES.GUEST_VIEW_MANAGER_CREATE_AND_ATTACH_GUEST, function (event, embedderFrameId: number, elementInstanceId: number, params) { + return createGuest(event.sender, embedderFrameId, elementInstanceId, params); }); handleMessageSync(IPC_MESSAGES.GUEST_VIEW_MANAGER_DETACH_GUEST, function (event, guestInstanceId: number) { diff --git a/lib/common/ipc-messages.ts b/lib/common/ipc-messages.ts index feb790a11bc7..e9cd21684359 100644 --- a/lib/common/ipc-messages.ts +++ b/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_ATTACH_GUEST = 'GUEST_VIEW_MANAGER_ATTACH_GUEST', + GUEST_VIEW_MANAGER_CREATE_AND_ATTACH_GUEST = 'GUEST_VIEW_MANAGER_CREATE_AND_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', diff --git a/lib/renderer/web-view/guest-view-internal.ts b/lib/renderer/web-view/guest-view-internal.ts index c3701311e3f9..e66ac8c64852 100644 --- a/lib/renderer/web-view/guest-view-internal.ts +++ b/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): Promise { - return ipcRendererInternal.invoke(IPC_MESSAGES.GUEST_VIEW_MANAGER_CREATE_GUEST, params); -} - -export function attachGuest (iframe: HTMLIFrameElement, elementInstanceId: number, guestInstanceId: number, params: Record) { +export function createGuest (iframe: HTMLIFrameElement, elementInstanceId: number, params: Record): Promise { 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) { diff --git a/lib/renderer/web-view/web-view-impl.ts b/lib/renderer/web-view/web-view-impl.ts index 75bde104b7a3..2a4097e7a836 100644 --- a/lib/renderer/web-view/web-view-impl.ts +++ b/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.attachGuestInstance(guestInstanceId); - }); + this.internalInstanceId = getNextId(); + this.hooks.guestViewInternal.createGuest(this.internalElement, this.internalInstanceId, this.buildParams()) + .then(guestInstanceId => { + this.attachGuestInstance(guestInstanceId); + }); } dispatchEvent (eventName: string, props: Record = {}) { @@ -192,20 +194,19 @@ export class WebViewImpl { } attachGuestInstance (guestInstanceId: number) { - if (!this.elementAttached) { - // The element could be detached before we got response from browser. + if (guestInstanceId === -1) { + // 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));