From de30ee0aa8bb32ff9a54803df7abf562b9369c35 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 5 Feb 2021 11:30:44 -0800 Subject: [PATCH] fix(oopifs): account for various races between processes (#5320) Consider the following situation (one among many possible). - FrameA has an oopif child FrameB; - FrameA navigates to same-process origin (e.g. about:blank); - at the same time, FrameC is attached to the FrameB in the FrameB's process. In this case, we get `frameNavigated` event for FrameA, immediately followed by `frameAttached` event for FrameC. Since we detach all FrameA's child frames on navigation, including the oopif FrameB, there is no parent frame for FrameC to attach to. In general, multiple processes coming from oopif may send their events in wildly different order, and their view about the frame tree may not always correspond to the "up to date" frame tree as seen from the main frame's process. We try to keep our frame tree aligned with what main process thinks, and ignore events that reference frames absent in this tree. Drive-by: handle filechooser exceptions because of async processing. --- src/server/chromium/crPage.ts | 37 +++++++++++++++++++++----- src/server/page.ts | 8 +++++- src/server/webkit/wkPage.ts | 12 ++++++--- test/page-set-input-files.spec.ts | 44 +++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 11 deletions(-) diff --git a/src/server/chromium/crPage.ts b/src/server/chromium/crPage.ts index b0a0fdbc750a3..86dbdbac8e1c7 100644 --- a/src/server/chromium/crPage.ts +++ b/src/server/chromium/crPage.ts @@ -532,14 +532,25 @@ class FrameSession { if (frameSession && frameId !== this._targetId) { // This is a remote -> local frame transition. frameSession._swappedIn = true; - const frame = this._page._frameManager.frame(frameId)!; - this._page._frameManager.removeChildFramesRecursively(frame); + const frame = this._page._frameManager.frame(frameId); + // Frame or even a whole subtree may be already gone, because some ancestor did navigate. + if (frame) + this._page._frameManager.removeChildFramesRecursively(frame); + return; + } + if (parentFrameId && !this._page._frameManager.frame(parentFrameId)) { + // Parent frame may be gone already because some ancestor frame navigated and + // destroyed the whole subtree of some oopif, while oopif's process is still sending us events. + // Be careful to not confuse this with "main frame navigated cross-process" scenario + // where parentFrameId is null. return; } this._page._frameManager.frameAttached(frameId, parentFrameId); } _onFrameNavigated(framePayload: Protocol.Page.Frame, initial: boolean) { + if (!this._page._frameManager.frame(framePayload.id)) + return; // Subtree may be already gone because some ancestor navigation destroyed the oopif. this._page._frameManager.frameCommittedNewDocumentNavigation(framePayload.id, framePayload.url + (framePayload.urlFragment || ''), framePayload.name || '', framePayload.loaderId, initial); if (!initial) this._firstNonInitialNavigationCommittedFulfill(); @@ -609,7 +620,9 @@ class FrameSession { if (event.targetInfo.type === 'iframe') { // Frame id equals target id. const targetId = event.targetInfo.targetId; - const frame = this._page._frameManager.frame(targetId)!; + const frame = this._page._frameManager.frame(targetId); + if (!frame) + return; // Subtree may be already gone due to renderer/browser race. this._page._frameManager.removeChildFramesRecursively(frame); const frameSession = new FrameSession(this._crPage, session, targetId, this); this._crPage._sessions.set(targetId, frameSession); @@ -715,6 +728,8 @@ class FrameSession { } _onDialog(event: Protocol.Page.javascriptDialogOpeningPayload) { + if (!this._page._frameManager.frame(this._targetId)) + return; // Our frame/subtree may be gone already. this._page.emit(Page.Events.Dialog, new dialog.Dialog( this._page, event.type, @@ -749,10 +764,18 @@ class FrameSession { } async _onFileChooserOpened(event: Protocol.Page.fileChooserOpenedPayload) { - const frame = this._page._frameManager.frame(event.frameId)!; - const utilityContext = await frame._utilityContext(); - const handle = await this._adoptBackendNodeId(event.backendNodeId, utilityContext); - this._page._onFileChooserOpened(handle); + const frame = this._page._frameManager.frame(event.frameId); + if (!frame) + return; + let handle; + try { + const utilityContext = await frame._utilityContext(); + handle = await this._adoptBackendNodeId(event.backendNodeId, utilityContext); + } catch (e) { + // During async processing, frame/context may go away. We should not throw. + return; + } + await this._page._onFileChooserOpened(handle); } _onDownloadWillBegin(payload: Protocol.Page.downloadWillBeginPayload) { diff --git a/src/server/page.ts b/src/server/page.ts index 7abb26db77250..b7a630858c918 100644 --- a/src/server/page.ts +++ b/src/server/page.ts @@ -226,7 +226,13 @@ export class Page extends EventEmitter { } async _onFileChooserOpened(handle: dom.ElementHandle) { - const multiple = await handle.evaluate(element => !!(element as HTMLInputElement).multiple); + let multiple; + try { + multiple = await handle.evaluate(element => !!(element as HTMLInputElement).multiple); + } catch (e) { + // Frame/context may be gone during async processing. Do not throw. + return; + } if (!this.listenerCount(Page.Events.FileChooser)) { handle.dispose(); return; diff --git a/src/server/webkit/wkPage.ts b/src/server/webkit/wkPage.ts index 85699fc4f9252..fccab1846beb7 100644 --- a/src/server/webkit/wkPage.ts +++ b/src/server/webkit/wkPage.ts @@ -559,9 +559,15 @@ export class WKPage implements PageDelegate { } private async _onFileChooserOpened(event: {frameId: Protocol.Network.FrameId, element: Protocol.Runtime.RemoteObject}) { - const context = await this._page._frameManager.frame(event.frameId)!._mainContext(); - const handle = context.createHandle(event.element).asElement()!; - this._page._onFileChooserOpened(handle); + let handle; + try { + const context = await this._page._frameManager.frame(event.frameId)!._mainContext(); + handle = context.createHandle(event.element).asElement()!; + } catch (e) { + // During async processing, frame/context may go away. We should not throw. + return; + } + await this._page._onFileChooserOpened(handle); } private static async _setEmulateMedia(session: WKSession, mediaType: types.MediaType | null, colorScheme: types.ColorScheme | null): Promise { diff --git a/test/page-set-input-files.spec.ts b/test/page-set-input-files.spec.ts index d41e8dc6e313f..cf69149b62ab6 100644 --- a/test/page-set-input-files.spec.ts +++ b/test/page-set-input-files.spec.ts @@ -115,6 +115,50 @@ it('should work when file input is not attached to DOM', async ({page, server}) expect(chooser).toBeTruthy(); }); +it('should not throw when filechooser belongs to iframe', (test, { browserName }) => { + test.skip(browserName === 'firefox', 'Firefox ignores filechooser from child frame'); +}, async ({page, server}) => { + await page.goto(server.PREFIX + '/frames/one-frame.html'); + const frame = page.mainFrame().childFrames()[0]; + await frame.setContent(` +
Click me
+ + `); + await Promise.all([ + page.waitForEvent('filechooser'), + frame.click('div') + ]); + await page.waitForFunction(() => (window as any).__done); +}); + +it('should not throw when frame is detached immediately', async ({page, server}) => { + await page.goto(server.PREFIX + '/frames/one-frame.html'); + const frame = page.mainFrame().childFrames()[0]; + await frame.setContent(` +
Click me
+ + `); + page.on('filechooser', () => {}); // To ensure we handle file choosers. + await frame.click('div'); + await page.waitForFunction(() => (window as any).__done); +}); + it('should work with CSP', async ({page, server}) => { server.setCSP('/empty.html', 'default-src "none"'); await page.goto(server.EMPTY_PAGE);