From 7bd924673afc1f7f4af1c526eea9d1b00bdb6ee3 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 18 Mar 2020 17:14:18 -0700 Subject: [PATCH] fix(PageEvent): properly wait for initial navigation in chromium and webkit (#1412) --- package.json | 2 +- src/chromium/crBrowser.ts | 23 +++++++++++++- src/chromium/crPage.ts | 9 +++++- src/chromium/crTarget.ts | 8 +++-- src/webkit/wkBrowser.ts | 19 ++++++++++- src/webkit/wkPage.ts | 13 +++++++- test/browsercontext.spec.js | 28 ++++++++++++++--- test/page.spec.js | 5 +-- test/popup.spec.js | 63 +++++++++++++++++++++++++++++++++++-- 9 files changed, 154 insertions(+), 16 deletions(-) diff --git a/package.json b/package.json index dcce29d197815..71519da04d8cb 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,7 @@ "playwright": { "chromium_revision": "750417", "firefox_revision": "1043", - "webkit_revision": "1179" + "webkit_revision": "1180" }, "scripts": { "ctest": "cross-env BROWSER=chromium node test/test.js", diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 876e35f644980..2574fd9ce3ab9 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -40,6 +40,7 @@ export class CRBrowser extends platform.EventEmitter implements Browser { _targets = new Map(); readonly _firstPagePromise: Promise; private _firstPageCallback = () => {}; + private _popupOpeners: string[] = []; private _tracingRecording = false; private _tracingPath: string | null = ''; @@ -104,6 +105,15 @@ export class CRBrowser extends platform.EventEmitter implements Browser { return createPageInNewContext(this, options); } + _onWindowOpen(openerId: string, payload: Protocol.Page.windowOpenPayload) { + // window.open($url) with noopener forces a new browser-initiated window, not + // a renderer-initiated one. When url is about:blank, we only get an + // initial navigation, and no real navigation to $url. + if (payload.windowFeatures.includes('noopener') && payload.url.startsWith('about:blank')) + return; + this._popupOpeners.push(openerId); + } + _onAttachedToTarget({targetInfo, sessionId, waitingForDebugger}: Protocol.Target.attachedToTargetPayload) { const session = this._connection.session(sessionId)!; if (!CRTarget.isPageType(targetInfo.type)) { @@ -148,7 +158,18 @@ export class CRBrowser extends platform.EventEmitter implements Browser { private _createTarget(targetInfo: Protocol.Target.TargetInfo, session: CRSession | null) { const {browserContextId} = targetInfo; const context = (browserContextId && this._contexts.has(browserContextId)) ? this._contexts.get(browserContextId)! : this._defaultContext; - const target = new CRTarget(this, targetInfo, context, session, () => this._connection.createSession(targetInfo)); + let hasInitialAboutBlank = false; + if (CRTarget.isPageType(targetInfo.type) && targetInfo.openerId) { + const openerIndex = this._popupOpeners.indexOf(targetInfo.openerId); + if (openerIndex !== -1) { + this._popupOpeners.splice(openerIndex, 1); + // When this page is a result of window.open($url) call, we should have it's opener + // in the list of popup openers. In this case we know there is an initial + // about:blank navigation, followed by a navigation to $url. + hasInitialAboutBlank = true; + } + } + const target = new CRTarget(this, targetInfo, context, session, () => this._connection.createSession(targetInfo), hasInitialAboutBlank); assert(!this._targets.has(targetInfo.targetId), 'Target should not exist before targetCreated'); this._targets.set(targetInfo.targetId, target); return { context, target }; diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index 3d79d0fcc5db5..e7c3483aead98 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -53,6 +53,8 @@ export class CRPage implements PageDelegate { private readonly _pdf: CRPDF; private readonly _coverage: CRCoverage; private readonly _browserContext: CRBrowserContext; + private _firstNonInitialNavigationCommittedPromise: Promise; + private _firstNonInitialNavigationCommittedCallback = () => {}; constructor(client: CRSession, browser: CRBrowser, browserContext: CRBrowserContext) { this._client = client; @@ -64,9 +66,10 @@ export class CRPage implements PageDelegate { this._browserContext = browserContext; this._page = new Page(this, browserContext); this._networkManager = new CRNetworkManager(client, this._page); + this._firstNonInitialNavigationCommittedPromise = new Promise(f => this._firstNonInitialNavigationCommittedCallback = f); } - async initialize() { + async initialize(hasInitialAboutBlank: boolean) { const promises: Promise[] = [ this._client.send('Page.enable'), this._client.send('Page.getFrameTree').then(({frameTree}) => { @@ -142,6 +145,8 @@ export class CRPage implements PageDelegate { for (const source of this._browserContext._evaluateOnNewDocumentSources) promises.push(this.evaluateOnNewDocument(source)); promises.push(this._client.send('Runtime.runIfWaitingForDebugger')); + if (hasInitialAboutBlank) + promises.push(this._firstNonInitialNavigationCommittedPromise); await Promise.all(promises); } @@ -189,6 +194,8 @@ export class CRPage implements PageDelegate { _onFrameNavigated(framePayload: Protocol.Page.Frame, initial: boolean) { this._page._frameManager.frameCommittedNewDocumentNavigation(framePayload.id, framePayload.url, framePayload.name || '', framePayload.loaderId, initial); + if (!initial) + this._firstNonInitialNavigationCommittedCallback(); } _onFrameRequestedNavigation(payload: Protocol.Page.frameRequestedNavigationPayload) { diff --git a/src/chromium/crTarget.ts b/src/chromium/crTarget.ts index e06b72b0adb6a..fac521c1a5863 100644 --- a/src/chromium/crTarget.ts +++ b/src/chromium/crTarget.ts @@ -19,7 +19,7 @@ import { CRBrowser, CRBrowserContext } from './crBrowser'; import { CRSession, CRSessionEvents } from './crConnection'; import { Page, Worker } from '../page'; import { Protocol } from './protocol'; -import { debugError, assert } from '../helper'; +import { debugError, assert, helper } from '../helper'; import { CRPage } from './crPage'; import { CRExecutionContext } from './crExecutionContext'; @@ -49,7 +49,8 @@ export class CRTarget { targetInfo: Protocol.Target.TargetInfo, browserContext: CRBrowserContext, session: CRSession | null, - sessionFactory: () => Promise) { + sessionFactory: () => Promise, + hasInitialAboutBlank: boolean) { this._targetInfo = targetInfo; this._browser = browser; this._browserContext = browserContext; @@ -58,10 +59,11 @@ export class CRTarget { if (CRTarget.isPageType(targetInfo.type)) { assert(session, 'Page target must be created with existing session'); this._crPage = new CRPage(session, this._browser, this._browserContext); + helper.addEventListener(session, 'Page.windowOpen', event => browser._onWindowOpen(targetInfo.targetId, event)); const page = this._crPage.page(); (page as any)[targetSymbol] = this; session.once(CRSessionEvents.Disconnected, () => page._didDisconnect()); - this._pagePromise = this._crPage.initialize().then(() => this._initializedPage = page).catch(e => e); + this._pagePromise = this._crPage.initialize(hasInitialAboutBlank).then(() => this._initializedPage = page).catch(e => e); } } diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 9e89bbe707461..a9f1f5d4af54f 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -38,6 +38,7 @@ export class WKBrowser extends platform.EventEmitter implements Browser { readonly _contexts = new Map(); readonly _wkPages = new Map(); private readonly _eventListeners: RegisteredListener[]; + private _popupOpeners: string[] = []; private _firstPageCallback: () => void = () => {}; private readonly _firstPagePromise: Promise; @@ -59,6 +60,7 @@ export class WKBrowser extends platform.EventEmitter implements Browser { helper.addEventListener(this._browserSession, 'Playwright.pageProxyCreated', this._onPageProxyCreated.bind(this)), helper.addEventListener(this._browserSession, 'Playwright.pageProxyDestroyed', this._onPageProxyDestroyed.bind(this)), helper.addEventListener(this._browserSession, 'Playwright.provisionalLoadFailed', event => this._onProvisionalLoadFailed(event)), + helper.addEventListener(this._browserSession, 'Playwright.windowOpen', this._onWindowOpen.bind(this)), helper.addEventListener(this._browserSession, kPageProxyMessageReceived, this._onPageProxyMessageReceived.bind(this)), ]; @@ -98,6 +100,10 @@ export class WKBrowser extends platform.EventEmitter implements Browser { return this._firstPagePromise; } + _onWindowOpen(payload: Protocol.Playwright.windowOpenPayload) { + this._popupOpeners.push(payload.pageProxyId); + } + _onPageProxyCreated(event: Protocol.Playwright.pageProxyCreatedPayload) { const { pageProxyInfo } = event; const pageProxyId = pageProxyInfo.pageProxyId; @@ -117,7 +123,18 @@ export class WKBrowser extends platform.EventEmitter implements Browser { this._connection.rawSend({ ...message, pageProxyId }); }); const opener = pageProxyInfo.openerId ? this._wkPages.get(pageProxyInfo.openerId) : undefined; - const wkPage = new WKPage(context, pageProxySession, opener || null); + let hasInitialAboutBlank = false; + if (pageProxyInfo.openerId) { + const openerIndex = this._popupOpeners.indexOf(pageProxyInfo.openerId); + if (openerIndex !== -1) { + this._popupOpeners.splice(openerIndex, 1); + // When this page is a result of window.open($url) call, we should have it's opener + // in the list of popup openers. In this case we know there is an initial + // about:blank navigation, followed by a navigation to $url. + hasInitialAboutBlank = true; + } + } + const wkPage = new WKPage(context, pageProxySession, opener || null, hasInitialAboutBlank); this._wkPages.set(pageProxyId, wkPage); const pageEvent = new PageEvent(context, wkPage.pageOrError()); diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index ca101ecbd3723..045ab33bbffb4 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -57,10 +57,14 @@ export class WKPage implements PageDelegate { private readonly _evaluateOnNewDocumentSources: string[] = []; readonly _browserContext: WKBrowserContext; private _initialized = false; + private _hasInitialAboutBlank: boolean; + private _firstNonInitialNavigationCommittedPromise: Promise; + private _firstNonInitialNavigationCommittedCallback = () => {}; - constructor(browserContext: WKBrowserContext, pageProxySession: WKSession, opener: WKPage | null) { + constructor(browserContext: WKBrowserContext, pageProxySession: WKSession, opener: WKPage | null, hasInitialAboutBlank: boolean) { this._pageProxySession = pageProxySession; this._opener = opener; + this._hasInitialAboutBlank = hasInitialAboutBlank; this.rawKeyboard = new RawKeyboardImpl(pageProxySession); this.rawMouse = new RawMouseImpl(pageProxySession); this._contextIdToContext = new Map(); @@ -76,6 +80,7 @@ export class WKPage implements PageDelegate { helper.addEventListener(this._pageProxySession, 'Target.didCommitProvisionalTarget', this._onDidCommitProvisionalTarget.bind(this)), ]; this._pagePromise = new Promise(f => this._pagePromiseCallback = f); + this._firstNonInitialNavigationCommittedPromise = new Promise(f => this._firstNonInitialNavigationCommittedCallback = f); } _initializedPage(): Page | null { @@ -251,6 +256,8 @@ export class WKPage implements PageDelegate { } if (targetInfo.isPaused) this._pageProxySession.send('Target.resume', { targetId: targetInfo.targetId }).catch(debugError); + if (this._hasInitialAboutBlank) + await this._firstNonInitialNavigationCommittedPromise; this._initialized = true; this._pagePromiseCallback(pageOrError); } else { @@ -330,6 +337,8 @@ export class WKPage implements PageDelegate { private _handleFrameTree(frameTree: Protocol.Page.FrameResourceTree) { this._onFrameAttached(frameTree.frame.id, frameTree.frame.parentId || null); this._onFrameNavigated(frameTree.frame, true); + this._page._frameManager.frameLifecycleEvent(frameTree.frame.id, 'domcontentloaded'); + this._page._frameManager.frameLifecycleEvent(frameTree.frame.id, 'load'); if (!frameTree.childFrames) return; @@ -348,6 +357,8 @@ export class WKPage implements PageDelegate { if (!framePayload.parentId) this._workers.clear(); this._page._frameManager.frameCommittedNewDocumentNavigation(framePayload.id, framePayload.url, framePayload.name || '', framePayload.loaderId, initial); + if (!initial) + this._firstNonInitialNavigationCommittedCallback(); } private _onFrameNavigatedWithinDocument(frameId: string, url: string) { diff --git a/test/browsercontext.spec.js b/test/browsercontext.spec.js index bf6cad1420250..6a0b2b655840e 100644 --- a/test/browsercontext.spec.js +++ b/test/browsercontext.spec.js @@ -480,7 +480,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF }); describe('Events.BrowserContext.PageEvent', function() { - it.fail(true)('should have url with nowait', async({browser, server}) => { + it.fail(FFOX)('should have url with nowait', async({browser, server}) => { const context = await browser.newContext(); const page = await context.newPage(); const [otherPage] = await Promise.all([ @@ -490,7 +490,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF expect(otherPage.url()).toBe(server.EMPTY_PAGE); await context.close(); }); - it.fail(CHROMIUM)('should have url with domcontentloaded', async({browser, server}) => { + it('should have url with domcontentloaded', async({browser, server}) => { const context = await browser.newContext(); const page = await context.newPage(); const [otherPage] = await Promise.all([ @@ -500,6 +500,26 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF expect(otherPage.url()).toBe(server.EMPTY_PAGE); await context.close(); }); + it('should have about:blank url with domcontentloaded', async({browser, server}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + const [otherPage] = await Promise.all([ + context.waitForEvent('page').then(event => event.page({ waitUntil: 'domcontentloaded' })), + page.evaluate(url => window.open(url), 'about:blank') + ]); + expect(otherPage.url()).toBe('about:blank'); + await context.close(); + }); + it.fail(FFOX)('should have about:blank for empty url with domcontentloaded', async({browser, server}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + const [otherPage] = await Promise.all([ + context.waitForEvent('page').then(event => event.page({ waitUntil: 'domcontentloaded' })), + page.evaluate(() => window.open()) + ]); + expect(otherPage.url()).toBe('about:blank'); + await context.close(); + }); it('should report when a new page is created and closed', async({browser, server}) => { const context = await browser.newContext(); const page = await context.newPage(); @@ -527,7 +547,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF }); it('should report initialized pages', async({browser, server}) => { const context = await browser.newContext(); - const pagePromise = context.waitForEvent('page').then(e => e.page({ waitUntil: 'nowait' })); + const pagePromise = context.waitForEvent('page').then(e => e.page()); context.newPage(); const newPage = await pagePromise; expect(newPage.url()).toBe('about:blank'); @@ -576,7 +596,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF const context = await browser.newContext(); const events = []; context.on('page', async event => { - const page = await event.page({ waitUntil: 'nowait' }); + const page = await event.page(); events.push('CREATED: ' + page.url()); page.on('close', () => events.push('DESTROYED: ' + page.url())); }); diff --git a/test/page.spec.js b/test/page.spec.js index 59fc8076ce015..f9ddf6a34772e 100644 --- a/test/page.spec.js +++ b/test/page.spec.js @@ -224,13 +224,14 @@ module.exports.describe = function({testRunner, expect, headless, playwright, FF }); }); // @see https://github.com/GoogleChrome/puppeteer/issues/3865 - it('should not throw when there are console messages in detached iframes', async({page, server}) => { + it.fail(FFOX)('should not throw when there are console messages in detached iframes', async({page, server}) => { await page.goto(server.EMPTY_PAGE); const [popup] = await Promise.all([ page.waitForEvent('popup').then(e => e.page()), page.evaluate(async() => { // 1. Create a popup that Playwright is not connected to. - const win = window.open(window.location.href); + const win = window.open(''); + window._popup = win; if (window.document.readyState !== 'complete') await new Promise(f => window.addEventListener('load', f)); // 2. In this popup, create an iframe that console.logs a message. diff --git a/test/popup.spec.js b/test/popup.spec.js index fa9767eaf6b54..090116568558b 100644 --- a/test/popup.spec.js +++ b/test/popup.spec.js @@ -292,17 +292,28 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, WE await evaluatePromise; await context.close(); }); - it('should work with empty url', async({browser}) => { + it.fail(FFOX)('should work with empty url', async({browser}) => { const context = await browser.newContext(); const page = await context.newPage(); const [popup] = await Promise.all([ - page.waitForEvent('popup').then(e => e.page({ waitUntil: 'nowait' })), + page.waitForEvent('popup').then(e => e.page()), page.evaluate(() => window.__popup = window.open('')), ]); expect(await page.evaluate(() => !!window.opener)).toBe(false); expect(await popup.evaluate(() => !!window.opener)).toBe(true); await context.close(); }); + it('should work with empty url and nowait', async({browser}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + const [popup] = await Promise.all([ + page.waitForEvent('popup').then(e => e.page({ waitUntil: 'nowait' })), + page.evaluate(() => window.__popup = window.open()), + ]); + expect(await page.evaluate(() => !!window.opener)).toBe(false); + expect(await popup.evaluate(() => !!window.opener)).toBe(true); + await context.close(); + }); it('should work with noopener', async({browser}) => { const context = await browser.newContext(); const page = await context.newPage(); @@ -314,6 +325,41 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, WE expect(await popup.evaluate(() => !!window.opener)).toBe(false); await context.close(); }); + it('should work with noopener and nowait', async({browser}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + const [popup] = await Promise.all([ + page.waitForEvent('popup').then(e => e.page({ waitUntil: 'nowait' })), + page.evaluate(() => window.__popup = window.open('about:blank', null, 'noopener')), + ]); + expect(await page.evaluate(() => !!window.opener)).toBe(false); + expect(await popup.evaluate(() => !!window.opener)).toBe(false); + await context.close(); + }); + it('should work with noopener and url', async({browser, server}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + await page.goto(server.EMPTY_PAGE); + const [popup] = await Promise.all([ + page.waitForEvent('popup').then(e => e.page()), + page.evaluate(url => window.__popup = window.open(url, null, 'noopener'), server.EMPTY_PAGE), + ]); + expect(await page.evaluate(() => !!window.opener)).toBe(false); + expect(await popup.evaluate(() => !!window.opener)).toBe(false); + await context.close(); + }); + it('should work with noopener and url and nowait', async({browser, server}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + await page.goto(server.EMPTY_PAGE); + const [popup] = await Promise.all([ + page.waitForEvent('popup').then(e => e.page({ waitUntil: 'nowait' })), + page.evaluate(url => window.__popup = window.open(url, null, 'noopener'), server.EMPTY_PAGE), + ]); + expect(await page.evaluate(() => !!window.opener)).toBe(false); + expect(await popup.evaluate(() => !!window.opener)).toBe(false); + await context.close(); + }); it('should work with clicking target=_blank', async({browser, server}) => { const context = await browser.newContext(); const page = await context.newPage(); @@ -327,6 +373,19 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, WE expect(await popup.evaluate(() => !!window.opener)).toBe(true); await context.close(); }); + it('should work with clicking target=_blank and nowait', async({browser, server}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + await page.goto(server.EMPTY_PAGE); + await page.setContent('yo'); + const [popup] = await Promise.all([ + page.waitForEvent('popup').then(e => e.page({ waitUntil: 'nowait' })), + page.click('a'), + ]); + expect(await page.evaluate(() => !!window.opener)).toBe(false); + expect(await popup.evaluate(() => !!window.opener)).toBe(true); + await context.close(); + }); it('should work with fake-clicking target=_blank and rel=noopener', async({browser, server}) => { const context = await browser.newContext(); const page = await context.newPage();