From 8b3d70a2a34b7fdcf91fdda5566bf2663551ff60 Mon Sep 17 00:00:00 2001 From: Sam Maddock Date: Fri, 11 Oct 2024 18:33:53 -0400 Subject: [PATCH] feat: add WebFrameMain detached property (#43473) * feat: add WebFrameMain detached property fix: throw instead of returning null senderFrame test: detached frames fix: ensure IPCs of pending deletion RFHs are dispatched fix: lookup WFM by FTN ID to dispatch IPCs feat: add frame.isDestroyed() return null fix: return undefined docs: add null to all frame properties refactor: option c, return null and emit warning refactor: add routingId & processId to navigation events test: null frame property docs: clarify warning message better wording clarify null frame fix: browserwindow spec * maybe fix :shrug: * fix: use updated util #43722 * docs: add notice for frame change of behavior * docs: clarify why frame properties may be null * lint * wip * fix: content::FrameTreeNodeId lookup and converter * refactor: avoid holey array deoptimization --------- Co-authored-by: John Kleinschmidt --- docs/api/session.md | 21 ++- docs/api/structures/ipc-main-event.md | 2 +- docs/api/structures/ipc-main-invoke-event.md | 2 +- docs/api/web-contents.md | 31 ++-- docs/api/web-frame-main.md | 11 ++ docs/api/web-request.md | 24 ++- docs/breaking-changes.md | 28 ++++ lib/browser/api/web-contents.ts | 64 +++++--- lib/browser/guest-view-manager.ts | 4 +- .../browser/api/electron_api_web_contents.cc | 7 +- .../api/electron_api_web_frame_main.cc | 151 +++++++++++++----- .../browser/api/electron_api_web_frame_main.h | 15 +- .../common/gin_converters/frame_converter.cc | 21 ++- shell/common/gin_converters/frame_converter.h | 8 + spec/api-browser-window-spec.ts | 2 +- spec/api-ipc-spec.ts | 18 +++ spec/api-web-frame-main-spec.ts | 113 ++++++++++--- spec/fixtures/sub-frames/preload.js | 4 + typings/internal-ambient.d.ts | 3 +- typings/internal-electron.d.ts | 7 + 20 files changed, 410 insertions(+), 126 deletions(-) diff --git a/docs/api/session.md b/docs/api/session.md index 90c407dd31..8b56af1897 100644 --- a/docs/api/session.md +++ b/docs/api/session.md @@ -268,7 +268,8 @@ Returns: * `event` Event * `details` Object * `deviceList` [HIDDevice[]](structures/hid-device.md) - * `frame` [WebFrameMain](web-frame-main.md) + * `frame` [WebFrameMain](web-frame-main.md) | null - The frame initiating this event. + May be `null` if accessed after the frame has either navigated or been destroyed. * `callback` Function * `deviceId` string | null (optional) @@ -332,7 +333,8 @@ Returns: * `event` Event * `details` Object * `device` [HIDDevice](structures/hid-device.md) - * `frame` [WebFrameMain](web-frame-main.md) + * `frame` [WebFrameMain](web-frame-main.md) | null - The frame initiating this event. + May be `null` if accessed after the frame has either navigated or been destroyed. Emitted after `navigator.hid.requestDevice` has been called and `select-hid-device` has fired if a new device becomes available before @@ -347,7 +349,8 @@ Returns: * `event` Event * `details` Object * `device` [HIDDevice](structures/hid-device.md) - * `frame` [WebFrameMain](web-frame-main.md) + * `frame` [WebFrameMain](web-frame-main.md) | null - The frame initiating this event. + May be `null` if accessed after the frame has either navigated or been destroyed. Emitted after `navigator.hid.requestDevice` has been called and `select-hid-device` has fired if a device has been removed before the callback @@ -473,7 +476,8 @@ Returns: * `event` Event * `details` Object * `port` [SerialPort](structures/serial-port.md) - * `frame` [WebFrameMain](web-frame-main.md) + * `frame` [WebFrameMain](web-frame-main.md) | null - The frame initiating this event. + May be `null` if accessed after the frame has either navigated or been destroyed. * `origin` string - The origin that the device has been revoked from. Emitted after `SerialPort.forget()` has been called. This event can be used @@ -517,7 +521,8 @@ Returns: * `event` Event * `details` Object * `deviceList` [USBDevice[]](structures/usb-device.md) - * `frame` [WebFrameMain](web-frame-main.md) + * `frame` [WebFrameMain](web-frame-main.md) | null - The frame initiating this event. + May be `null` if accessed after the frame has either navigated or been destroyed. * `callback` Function * `deviceId` string (optional) @@ -957,7 +962,8 @@ session.fromPartition('some-partition').setPermissionCheckHandler((webContents, * `handler` Function | null * `request` Object - * `frame` [WebFrameMain](web-frame-main.md) - Frame that is requesting access to media. + * `frame` [WebFrameMain](web-frame-main.md) | null - Frame that is requesting access to media. + May be `null` if accessed after the frame has either navigated or been destroyed. * `securityOrigin` String - Origin of the page making the request. * `videoRequested` Boolean - true if the web content requested a video stream. * `audioRequested` Boolean - true if the web content requested an audio stream. @@ -1158,7 +1164,8 @@ app.whenReady().then(() => { pin displayed on the device. * `providePin` This prompt is requesting that a pin be provided for the device. - * `frame` [WebFrameMain](web-frame-main.md) + * `frame` [WebFrameMain](web-frame-main.md) | null - The frame initiating this handler. + May be `null` if accessed after the frame has either navigated or been destroyed. * `pin` string (optional) - The pin value to verify if `pairingKind` is `confirmPin`. * `callback` Function * `response` Object diff --git a/docs/api/structures/ipc-main-event.md b/docs/api/structures/ipc-main-event.md index eb06bb9315..2bf1e84aff 100644 --- a/docs/api/structures/ipc-main-event.md +++ b/docs/api/structures/ipc-main-event.md @@ -4,7 +4,7 @@ * `frameId` Integer - The ID of the renderer frame that sent this message * `returnValue` any - Set this to the value to be returned in a synchronous message * `sender` [WebContents](../web-contents.md) - Returns the `webContents` that sent the message -* `senderFrame` [WebFrameMain](../web-frame-main.md) _Readonly_ - The frame that sent this message +* `senderFrame` [WebFrameMain](../web-frame-main.md) | null _Readonly_ - The frame that sent this message. May be `null` if accessed after the frame has either navigated or been destroyed. * `ports` [MessagePortMain](../message-port-main.md)[] - A list of MessagePorts that were transferred with this message * `reply` Function - A function that will send an IPC message to the renderer frame that sent the original message that you are currently handling. You should use this method to "reply" to the sent message in order to guarantee the reply will go to the correct process and frame. * `channel` string diff --git a/docs/api/structures/ipc-main-invoke-event.md b/docs/api/structures/ipc-main-invoke-event.md index 9e24be40dc..b5c9e20438 100644 --- a/docs/api/structures/ipc-main-invoke-event.md +++ b/docs/api/structures/ipc-main-invoke-event.md @@ -3,4 +3,4 @@ * `processId` Integer - The internal ID of the renderer process that sent this message * `frameId` Integer - The ID of the renderer frame that sent this message * `sender` [WebContents](../web-contents.md) - Returns the `webContents` that sent the message -* `senderFrame` [WebFrameMain](../web-frame-main.md) _Readonly_ - The frame that sent this message +* `senderFrame` [WebFrameMain](../web-frame-main.md) | null _Readonly_ - The frame that sent this message. May be `null` if accessed after the frame has either navigated or been destroyed. diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index e0feef19d2..e76ecf0587 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -242,8 +242,9 @@ Returns: * `isSameDocument` boolean - This event does not fire for same document navigations using window.history api and reference fragment navigations. This property is always set to `false` for this event. * `isMainFrame` boolean - True if the navigation is taking place in a main frame. - * `frame` WebFrameMain - The frame to be navigated. - * `initiator` WebFrameMain (optional) - The frame which initiated the + * `frame` WebFrameMain | null - The frame to be navigated. + May be `null` if accessed after the frame has either navigated or been destroyed. + * `initiator` WebFrameMain | null (optional) - The frame which initiated the navigation, which can be a parent frame (e.g. via `window.open` with a frame's name), or null if the navigation was not initiated by a frame. This can also be null if the initiating frame was deleted before the event was @@ -275,8 +276,9 @@ Returns: * `isSameDocument` boolean - This event does not fire for same document navigations using window.history api and reference fragment navigations. This property is always set to `false` for this event. * `isMainFrame` boolean - True if the navigation is taking place in a main frame. - * `frame` WebFrameMain - The frame to be navigated. - * `initiator` WebFrameMain (optional) - The frame which initiated the + * `frame` WebFrameMain | null - The frame to be navigated. + May be `null` if accessed after the frame has either navigated or been destroyed. + * `initiator` WebFrameMain | null (optional) - The frame which initiated the navigation, which can be a parent frame (e.g. via `window.open` with a frame's name), or null if the navigation was not initiated by a frame. This can also be null if the initiating frame was deleted before the event was @@ -306,8 +308,9 @@ Returns: document. Examples of same document navigations are reference fragment navigations, pushState/replaceState, and same page history navigation. * `isMainFrame` boolean - True if the navigation is taking place in a main frame. - * `frame` WebFrameMain - The frame to be navigated. - * `initiator` WebFrameMain (optional) - The frame which initiated the + * `frame` WebFrameMain | null - The frame to be navigated. + May be `null` if accessed after the frame has either navigated or been destroyed. + * `initiator` WebFrameMain | null (optional) - The frame which initiated the navigation, which can be a parent frame (e.g. via `window.open` with a frame's name), or null if the navigation was not initiated by a frame. This can also be null if the initiating frame was deleted before the event was @@ -330,8 +333,9 @@ Returns: document. Examples of same document navigations are reference fragment navigations, pushState/replaceState, and same page history navigation. * `isMainFrame` boolean - True if the navigation is taking place in a main frame. - * `frame` WebFrameMain - The frame to be navigated. - * `initiator` WebFrameMain (optional) - The frame which initiated the + * `frame` WebFrameMain | null - The frame to be navigated. + May be `null` if accessed after the frame has either navigated or been destroyed. + * `initiator` WebFrameMain | null (optional) - The frame which initiated the navigation, which can be a parent frame (e.g. via `window.open` with a frame's name), or null if the navigation was not initiated by a frame. This can also be null if the initiating frame was deleted before the event was @@ -361,8 +365,9 @@ Returns: document. Examples of same document navigations are reference fragment navigations, pushState/replaceState, and same page history navigation. * `isMainFrame` boolean - True if the navigation is taking place in a main frame. - * `frame` WebFrameMain - The frame to be navigated. - * `initiator` WebFrameMain (optional) - The frame which initiated the + * `frame` WebFrameMain | null - The frame to be navigated. + May be `null` if accessed after the frame has either navigated or been destroyed. + * `initiator` WebFrameMain | null (optional) - The frame which initiated the navigation, which can be a parent frame (e.g. via `window.open` with a frame's name), or null if the navigation was not initiated by a frame. This can also be null if the initiating frame was deleted before the event was @@ -743,7 +748,8 @@ Returns: * `params` Object * `x` Integer - x coordinate. * `y` Integer - y coordinate. - * `frame` WebFrameMain - Frame from which the context menu was invoked. + * `frame` WebFrameMain | null - Frame from which the context menu was invoked. + May be `null` if accessed after the frame has either navigated or been destroyed. * `linkURL` string - URL of the link that encloses the node the context menu was invoked on. * `linkText` string - Text associated with the link. May be an empty @@ -1010,7 +1016,8 @@ Returns: * `event` Event * `details` Object - * `frame` WebFrameMain + * `frame` WebFrameMain | null - The created frame. + May be `null` if accessed after the frame has either navigated or been destroyed. Emitted when the [mainFrame](web-contents.md#contentsmainframe-readonly), an ``); await emittedUntil(w.webContents, 'did-frame-finish-load', (e: any, isMainFrame: boolean) => !isMainFrame); - let initiator: WebFrameMain | undefined; + let initiator: WebFrameMain | null | undefined; w.webContents.on('will-navigate', (e) => { initiator = e.initiator; }); diff --git a/spec/api-ipc-spec.ts b/spec/api-ipc-spec.ts index 38ff8f14ae..88eaaf4a0b 100644 --- a/spec/api-ipc-spec.ts +++ b/spec/api-ipc-spec.ts @@ -835,5 +835,23 @@ describe('ipc module', () => { const [, arg] = await once(w.webContents.mainFrame.frames[0].ipc, 'test'); expect(arg).to.equal(42); }); + + it('receives ipcs from unloading frames in the main frame', async () => { + const server = http.createServer((req, res) => { + res.setHeader('content-type', 'text/html'); + res.end(''); + }); + const { port } = await listen(server); + defer(() => { + server.close(); + }); + const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, contextIsolation: false } }); + await w.loadURL(`http://localhost:${port}`); + await w.webContents.executeJavaScript('window.onunload = () => require(\'electron\').ipcRenderer.send(\'unload\'); void 0'); + const onUnloadIpc = once(w.webContents.mainFrame.ipc, 'unload'); + w.loadURL(`http://127.0.0.1:${port}`); // cross-origin navigation + const [{ senderFrame }] = await onUnloadIpc; + expect(senderFrame.detached).to.be.true(); + }); }); }); diff --git a/spec/api-web-frame-main-spec.ts b/spec/api-web-frame-main-spec.ts index 54c95c9ef4..9d1c9edae7 100644 --- a/spec/api-web-frame-main-spec.ts +++ b/spec/api-web-frame-main-spec.ts @@ -18,10 +18,10 @@ describe('webFrameMain module', () => { const fileUrl = (filename: string) => url.pathToFileURL(path.join(subframesPath, filename)).href; - type Server = { server: http.Server, url: string } + type Server = { server: http.Server, url: string, crossOriginUrl: string } /** Creates an HTTP server whose handler embeds the given iframe src. */ - const createServer = async () => { + const createServer = async (): Promise => { const server = http.createServer((req, res) => { const params = new URLSearchParams(new URL(req.url || '', `http://${req.headers.host}`).search || ''); if (params.has('frameSrc')) { @@ -30,7 +30,15 @@ describe('webFrameMain module', () => { res.end(''); } }); - return { server, url: (await listen(server)).url + '/' }; + const serverUrl = (await listen(server)).url + '/'; + // HACK: Use 'localhost' instead of '127.0.0.1' so Chromium treats it as + // a separate origin because differing ports aren't enough 🤔 + const crossOriginUrl = serverUrl.replace('127.0.0.1', 'localhost'); + return { + server, + url: serverUrl, + crossOriginUrl + }; }; afterEach(closeAllWindows); @@ -285,8 +293,15 @@ describe('webFrameMain module', () => { }); describe('RenderFrame lifespan', () => { + let server: Awaited>; let w: BrowserWindow; + before(async () => { + server = await createServer(); + }); + after(() => { + server.server.close(); + }); beforeEach(async () => { w = new BrowserWindow({ show: false }); }); @@ -302,19 +317,15 @@ describe('webFrameMain module', () => { }); it('persists through cross-origin navigation', async () => { - const server = await createServer(); - // 'localhost' is treated as a separate origin. - const crossOriginUrl = server.url.replace('127.0.0.1', 'localhost'); await w.loadURL(server.url); const { mainFrame } = w.webContents; expect(mainFrame.url).to.equal(server.url); - await w.loadURL(crossOriginUrl); + await w.loadURL(server.crossOriginUrl); expect(w.webContents.mainFrame).to.equal(mainFrame); - expect(mainFrame.url).to.equal(crossOriginUrl); + expect(mainFrame.url).to.equal(server.crossOriginUrl); }); it('recovers from renderer crash on same-origin', async () => { - const server = await createServer(); // Keep reference to mainFrame alive throughout crash and recovery. const { mainFrame } = w.webContents; await w.webContents.loadURL(server.url); @@ -328,9 +339,6 @@ describe('webFrameMain module', () => { // Fixed by #34411 it('recovers from renderer crash on cross-origin', async () => { - const server = await createServer(); - // 'localhost' is treated as a separate origin. - const crossOriginUrl = server.url.replace('127.0.0.1', 'localhost'); // Keep reference to mainFrame alive throughout crash and recovery. const { mainFrame } = w.webContents; await w.webContents.loadURL(server.url); @@ -339,10 +347,79 @@ describe('webFrameMain module', () => { await crashEvent; // A short wait seems to be required to reproduce the crash. await setTimeout(100); - await w.webContents.loadURL(crossOriginUrl); + await w.webContents.loadURL(server.crossOriginUrl); // Log just to keep mainFrame in scope. console.log('mainFrame.url', mainFrame.url); }); + + it('returns null upon accessing senderFrame after cross-origin navigation', async () => { + w = new BrowserWindow({ + show: false, + webPreferences: { + preload: path.join(subframesPath, 'preload.js') + } + }); + const preloadPromise = once(ipcMain, 'preload-ran'); + await w.webContents.loadURL(server.url); + const [event] = await preloadPromise; + await w.webContents.loadURL(server.crossOriginUrl); + // senderFrame now points to a disposed RenderFrameHost. It should + // be null when attempting to access the lazily evaluated property. + expect(event.senderFrame).to.be.null(); + }); + + it('is detached when unload handler sends IPC', async () => { + w = new BrowserWindow({ + show: false, + webPreferences: { + preload: path.join(subframesPath, 'preload.js') + } + }); + await w.webContents.loadURL(server.url); + const unloadPromise = new Promise((resolve, reject) => { + ipcMain.once('preload-unload', (event) => { + try { + const { senderFrame } = event; + expect(senderFrame).to.not.be.null(); + expect(senderFrame!.detached).to.be.true(); + expect(senderFrame!.processId).to.equal(event.processId); + expect(senderFrame!.routingId).to.equal(event.frameId); + resolve(); + } catch (error) { + reject(error); + } + }); + }); + await w.webContents.loadURL(server.crossOriginUrl); + await expect(unloadPromise).to.eventually.be.fulfilled(); + }); + + it('disposes detached frame after cross-origin navigation', async () => { + w = new BrowserWindow({ + show: false, + webPreferences: { + preload: path.join(subframesPath, 'preload.js') + } + }); + await w.webContents.loadURL(server.url); + // eslint-disable-next-line prefer-const + let crossOriginPromise: Promise; + const unloadPromise = new Promise((resolve, reject) => { + ipcMain.once('preload-unload', async (event) => { + try { + const { senderFrame } = event; + expect(senderFrame!.detached).to.be.true(); + await crossOriginPromise; + expect(() => senderFrame!.url).to.throw(/Render frame was disposed/); + resolve(); + } catch (error) { + reject(error); + } + }); + }); + crossOriginPromise = w.webContents.loadURL(server.crossOriginUrl); + await expect(unloadPromise).to.eventually.be.fulfilled(); + }); }); describe('webFrameMain.fromId', () => { @@ -388,10 +465,6 @@ describe('webFrameMain module', () => { it('is not emitted upon cross-origin navigation', async () => { const server = await createServer(); - // HACK: Use 'localhost' instead of '127.0.0.1' so Chromium treats it as - // a separate origin because differing ports aren't enough 🤔 - const secondUrl = server.url.replace('127.0.0.1', 'localhost'); - const w = new BrowserWindow({ show: false }); await w.webContents.loadURL(server.url); @@ -401,7 +474,7 @@ describe('webFrameMain module', () => { frameCreatedEmitted = true; }); - await w.webContents.loadURL(secondUrl); + await w.webContents.loadURL(server.crossOriginUrl); expect(frameCreatedEmitted).to.be.false(); }); @@ -419,8 +492,8 @@ describe('webFrameMain module', () => { const w = new BrowserWindow({ show: false }); const promise = new Promise(resolve => { w.webContents.on('frame-created', (e, { frame }) => { - frame.on('dom-ready', () => { - if (frame.name === 'frameA') { + frame!.on('dom-ready', () => { + if (frame!.name === 'frameA') { resolve(); } }); diff --git a/spec/fixtures/sub-frames/preload.js b/spec/fixtures/sub-frames/preload.js index 52e87657f1..cc36e49e4b 100644 --- a/spec/fixtures/sub-frames/preload.js +++ b/spec/fixtures/sub-frames/preload.js @@ -7,3 +7,7 @@ ipcRenderer.send('preload-ran', window.location.href, webFrame.routingId); ipcRenderer.on('preload-ping', () => { ipcRenderer.send('preload-pong', webFrame.routingId); }); + +window.addEventListener('unload', () => { + ipcRenderer.send('preload-unload', window.location.href); +}); diff --git a/typings/internal-ambient.d.ts b/typings/internal-ambient.d.ts index 27c0b9f32e..4b5470e775 100644 --- a/typings/internal-ambient.d.ts +++ b/typings/internal-ambient.d.ts @@ -125,7 +125,8 @@ declare namespace NodeJS { interface WebFrameMainBinding { WebFrameMain: typeof Electron.WebFrameMain; fromId(processId: number, routingId: number): Electron.WebFrameMain; - fromIdOrNull(processId: number, routingId: number): Electron.WebFrameMain | null; + _fromIdIfExists(processId: number, routingId: number): Electron.WebFrameMain | null; + _fromFtnIdIfExists(frameTreeNodeId: number): Electron.WebFrameMain | null; } interface InternalWebPreferences { diff --git a/typings/internal-electron.d.ts b/typings/internal-electron.d.ts index 8de940b1ec..b213b031a6 100644 --- a/typings/internal-electron.d.ts +++ b/typings/internal-electron.d.ts @@ -177,10 +177,12 @@ declare namespace Electron { interface IpcMainEvent { _replyChannel: ReplyChannel; + frameTreeNodeId?: number; } interface IpcMainInvokeEvent { _replyChannel: ReplyChannel; + frameTreeNodeId?: number; } // Deprecated / undocumented BrowserWindow methods @@ -225,6 +227,11 @@ declare namespace Electron { once(event: 'destroyed', listener: (event: Electron.Event) => void): this; } + + interface WebContentsWillFrameNavigateEventParams { + processId: number; + routingId: number; + } } declare namespace ElectronInternal {