From 1b84ec902309c3f5cd39c4b444cbed3f0da2d770 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 14 Jul 2020 13:34:49 -0700 Subject: [PATCH] fix(binding): dispatch binding after the page has been initialized (#2938) ... but not after it was closed. --- src/chromium/crPage.ts | 6 ++++-- src/firefox/ffPage.ts | 6 ++++-- src/page.ts | 2 ++ src/webkit/wkPage.ts | 5 ++++- test/browsercontext.spec.js | 18 ++++++++++++++++- test/popup.spec.js | 39 ++++++++++++++++++++++++++++++++++--- 6 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index 598fe5fadd..d9b5084142 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -643,9 +643,11 @@ class FrameSession { ]); } - _onBindingCalled(event: Protocol.Runtime.bindingCalledPayload) { + async _onBindingCalled(event: Protocol.Runtime.bindingCalledPayload) { const context = this._contextIdToContext.get(event.executionContextId)!; - this._page._onBindingCalled(event.payload, context); + const pageOrError = await this._crPage.pageOrError(); + if (!(pageOrError instanceof Error)) + this._page._onBindingCalled(event.payload, context); } _onDialog(event: Protocol.Page.javascriptDialogOpeningPayload) { diff --git a/src/firefox/ffPage.ts b/src/firefox/ffPage.ts index 3886cbbd60..444a655fc4 100644 --- a/src/firefox/ffPage.ts +++ b/src/firefox/ffPage.ts @@ -195,9 +195,11 @@ export class FFPage implements PageDelegate { params.defaultValue)); } - _onBindingCalled(event: Protocol.Page.bindingCalledPayload) { + async _onBindingCalled(event: Protocol.Page.bindingCalledPayload) { const context = this._contextIdToContext.get(event.executionContextId)!; - this._page._onBindingCalled(event.payload, context); + const pageOrError = await this.pageOrError(); + if (!(pageOrError instanceof Error)) + this._page._onBindingCalled(event.payload, context); } async _onFileChooserOpened(payload: Protocol.Page.fileChooserOpenedPayload) { diff --git a/src/page.ts b/src/page.ts index ced9ebe8c4..a810fe4d26 100644 --- a/src/page.ts +++ b/src/page.ts @@ -293,6 +293,8 @@ export class Page extends EventEmitter { } async _onBindingCalled(payload: string, context: dom.FrameExecutionContext) { + if (this._disconnected || this._closedState === 'closed') + return; await PageBinding.dispatch(this, payload, context); } diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index a04c128ee9..6726195831 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -455,7 +455,10 @@ export class WKPage implements PageDelegate { if (level === 'debug' && parameters && parameters[0].value === BINDING_CALL_MESSAGE) { const parsedObjectId = JSON.parse(parameters[1].objectId!); const context = this._contextIdToContext.get(parsedObjectId.injectedScriptId)!; - this._page._onBindingCalled(parameters[2].value, context); + this.pageOrError().then(pageOrError => { + if (!(pageOrError instanceof Error)) + this._page._onBindingCalled(parameters[2].value, context); + }); return; } if (level === 'error' && source === 'javascript') { diff --git a/test/browsercontext.spec.js b/test/browsercontext.spec.js index df90a2acf4..8df4c0d7e0 100644 --- a/test/browsercontext.spec.js +++ b/test/browsercontext.spec.js @@ -131,7 +131,7 @@ describe('BrowserContext', function() { }); it('should not report frameless pages on error', async({browser, server}) => { const context = await browser.newContext(); - page = await context.newPage(); + const page = await context.newPage(); server.setRoute('/empty.html', (req, res) => { res.end(`Click me`); }); @@ -145,6 +145,22 @@ describe('BrowserContext', function() { expect(popup.isClosed()).toBeTruthy(); expect(popup.mainFrame()).toBeTruthy(); } + }); + it('should not call binding on errored pages', async({browser, server}) => { + const context = await browser.newContext(); + let gotBinding = 0; + await context.exposeFunction('add', (a, b) => { + gotBinding++; + return a + b; + }); + const page = await context.newPage(); + server.setRoute('/empty.html', (req, res) => { + res.end(`Click me`); + }); + await page.goto(server.EMPTY_PAGE); + await page.click('"Click me"'); + await context.close(); + expect(gotBinding).toBe(1); }) }); diff --git a/test/popup.spec.js b/test/popup.spec.js index 87db944cbe..a142dd3458 100644 --- a/test/popup.spec.js +++ b/test/popup.spec.js @@ -200,11 +200,15 @@ describe('window.open', function() { expect(await popup.evaluate('injected')).toBe(123); await context.close(); }); - it('should expose function from browser context', async function({browser, server}) { const context = await browser.newContext(); - await context.exposeFunction('add', (a, b) => a + b); + const messages = []; + await context.exposeFunction('add', (a, b) => { + messages.push('binding'); + return a + b; + }); const page = await context.newPage(); + context.on('page', () => messages.push('page')); await page.goto(server.EMPTY_PAGE); const added = await page.evaluate(async () => { const win = window.open('about:blank'); @@ -212,6 +216,35 @@ describe('window.open', function() { }); await context.close(); expect(added).toBe(13); + expect(messages.join('|')).toBe('page|binding'); + }); + it('should not dispatch binding on a closed page', async function({browser, server}) { + const context = await browser.newContext(); + const messages = []; + await context.exposeFunction('add', (a, b) => { + messages.push('binding'); + return a + b; + }); + const page = await context.newPage(); + await page.goto(server.EMPTY_PAGE); + await Promise.all([ + page.waitForEvent('popup').then(popup => { + if (popup.isClosed()) + messages.push('alreadyclosed'); + else + return popup.waitForEvent('close').then(() => messages.push('close')); + }), + page.evaluate(async () => { + const win = window.open('about:blank'); + win.add(9, 4); + win.close(); + }), + ]); + await context.close(); + if (FFOX) + expect(messages.join('|')).toBe('alreadyclosed'); + else + expect(messages.join('|')).toBe('binding|close'); }); }); @@ -252,7 +285,7 @@ describe('Page.Events.Popup', function() { expect(popup).toBeTruthy(); await context.close(); }); - it('should emit for immediately closed popups', async({browser, server}) => { + it('should emit for immediately closed popups 2', async({browser, server}) => { const context = await browser.newContext(); const page = await context.newPage(); await page.goto(server.EMPTY_PAGE);