From 39fb556f275a96b4c5e5587b007f0190914606a6 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 13 Dec 2019 13:03:52 -0800 Subject: [PATCH] fix(webkit): detect expected/aborted navigations based on network requests (#238) --- src/webkit/FrameManager.ts | 16 +++------------- src/webkit/NetworkManager.ts | 25 +++++++++++++++++++------ test/navigation.spec.js | 34 +++++++++++++++------------------- 3 files changed, 37 insertions(+), 38 deletions(-) diff --git a/src/webkit/FrameManager.ts b/src/webkit/FrameManager.ts index 35df7b92f6..92d14e3517 100644 --- a/src/webkit/FrameManager.ts +++ b/src/webkit/FrameManager.ts @@ -39,8 +39,6 @@ import { PNG } from 'pngjs'; const UTILITY_WORLD_NAME = '__playwright_utility_world__'; const BINDING_CALL_MESSAGE = '__playwright_binding_call__'; -let lastDocumentId = 0; - export class FrameManager extends EventEmitter implements PageDelegate { readonly rawMouse: RawMouseImpl; readonly rawKeyboard: RawKeyboardImpl; @@ -65,7 +63,7 @@ export class FrameManager extends EventEmitter implements PageDelegate { this._page = new Page(this, browserContext); this._networkManager.on(NetworkManagerEvents.Request, event => this._page.emit(Events.Page.Request, event)); this._networkManager.on(NetworkManagerEvents.Response, event => this._page.emit(Events.Page.Response, event)); - this._networkManager.on(NetworkManagerEvents.RequestFailed, event => this._requestFailed(event)); + this._networkManager.on(NetworkManagerEvents.RequestFailed, event => this._page.emit(Events.Page.RequestFailed, event)); this._networkManager.on(NetworkManagerEvents.RequestFinished, event => this._page.emit(Events.Page.RequestFinished, event)); } @@ -230,8 +228,8 @@ export class FrameManager extends EventEmitter implements PageDelegate { } } - // Auto-increment to avoid cross-process loaderId clash. - const documentId = framePayload.loaderId + '::' + (++lastDocumentId); + // Append session id to avoid cross-process loaderId clash. + const documentId = this._session._sessionId + '::' + framePayload.loaderId; if (!initial) frame._onExpectedNewDocumentNavigation(documentId); frame._onCommittedNewDocumentNavigation(framePayload.url, framePayload.name, documentId); @@ -509,14 +507,6 @@ export class FrameManager extends EventEmitter implements PageDelegate { await this._session.send('Emulation.setDeviceMetricsOverride', { ...oldSize, deviceScaleFactor: 0 }); } - private _requestFailed(request: network.Request) { - if (request.isNavigationRequest() && !request.failure().errorText.includes('cancelled')) { - request.frame()._onExpectedNewDocumentNavigation('fake-loader-id', request.url()); - request.frame()._onAbortedNewDocumentNavigation('fake-loader-id', request.failure().errorText); - } - this._page.emit(Events.Page.RequestFailed, request); - } - async getContentFrame(handle: dom.ElementHandle): Promise { throw new Error('contentFrame() is not implemented'); } diff --git a/src/webkit/NetworkManager.ts b/src/webkit/NetworkManager.ts index c1a984cae4..a1bf48f210 100644 --- a/src/webkit/NetworkManager.ts +++ b/src/webkit/NetworkManager.ts @@ -97,8 +97,13 @@ export class NetworkManager extends EventEmitter { redirectChain = request.request._redirectChain; } } - const frame = event.frameId && this._frameManager ? this._frameManager.frame(event.frameId) : null; - const request = new InterceptableRequest(frame, undefined, event, redirectChain); + const frame = this._frameManager.frame(event.frameId); + // TODO(einbinder) this will fail if we are an XHR document request + const isNavigationRequest = event.type === 'Document'; + const documentId = isNavigationRequest ? this._session._sessionId + '::' + event.loaderId : undefined; + if (documentId) + frame._onExpectedNewDocumentNavigation(documentId, event.request.url); + const request = new InterceptableRequest(frame, undefined, event, redirectChain, documentId); this._requestIdToRequest.set(event.requestId, request); this.emit(NetworkManagerEvents.Request, request.request); } @@ -159,6 +164,14 @@ export class NetworkManager extends EventEmitter { response._requestFinished(); this._requestIdToRequest.delete(request._requestId); this._attemptedAuthentications.delete(request._interceptionId); + if (request._documentId) { + const isCurrentDocument = request.request.frame()._lastDocumentId === request._documentId; + // When frame was detached during load, "cancelled" comes before detach. + // Ignore it and hope for the best. + const wasCanceled = event.errorText === 'cancelled'; + if (!isCurrentDocument && !wasCanceled) + request.request.frame()._onAbortedNewDocumentNavigation(request._documentId, event.errorText); + } this.emit(NetworkManagerEvents.RequestFailed, request.request); } } @@ -173,13 +186,13 @@ class InterceptableRequest { readonly request: network.Request; _requestId: string; _interceptionId: string; + _documentId: string | undefined; - constructor(frame: frames.Frame | null, interceptionId: string, event: Protocol.Network.requestWillBeSentPayload, redirectChain: network.Request[]) { + constructor(frame: frames.Frame | null, interceptionId: string, event: Protocol.Network.requestWillBeSentPayload, redirectChain: network.Request[], documentId: string | undefined) { this._requestId = event.requestId; - // TODO(einbinder) this will fail if we are an XHR document request - const isNavigationRequest = event.type === 'Document'; this._interceptionId = interceptionId; - this.request = new network.Request(frame, redirectChain, isNavigationRequest, event.request.url, + this._documentId = documentId; + this.request = new network.Request(frame, redirectChain, !!documentId, event.request.url, event.type ? event.type.toLowerCase() : 'Unknown', event.request.method, event.request.postData, headersObject(event.request.headers)); (this.request as any)[interceptableRequestSymbol] = this; } diff --git a/test/navigation.spec.js b/test/navigation.spec.js index c2247673ab..1e3e7fc354 100644 --- a/test/navigation.spec.js +++ b/test/navigation.spec.js @@ -106,7 +106,8 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME }); await page.goto(server.PREFIX + '/frames/one-frame.html'); }); - it.skip(WEBKIT)('should fail when server returns 204', async({page, server}) => { + !WEBKIT && it('should fail when server returns 204', async({page, server}) => { + // Webkit just loads an empty page. server.setRoute('/empty.html', (req, res) => { res.statusCode = 204; res.end(); @@ -114,7 +115,7 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME let error = null; await page.goto(server.EMPTY_PAGE).catch(e => error = e); expect(error).not.toBe(null); - if (CHROME || WEBKIT) + if (CHROME) expect(error.message).toContain('net::ERR_ABORTED'); else expect(error.message).toContain('NS_BINDING_ABORTED'); @@ -123,7 +124,7 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME const response = await page.goto(server.EMPTY_PAGE, {waitUntil: 'domcontentloaded'}); expect(response.status()).toBe(200); }); - it.skip(WEBKIT)('should work when page calls history API in beforeunload', async({page, server}) => { + it('should work when page calls history API in beforeunload', async({page, server}) => { await page.goto(server.EMPTY_PAGE); await page.evaluate(() => { window.addEventListener('beforeunload', () => history.replaceState(null, 'initial', window.location.href), false); @@ -501,16 +502,11 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME it.skip(WEBKIT)('should work when subframe issues window.stop()', async({page, server}) => { server.setRoute('/frames/style.css', (req, res) => {}); const navigationPromise = page.goto(server.PREFIX + '/frames/one-frame.html'); - let frame; - await new Promise(fulfill => { - page.once('frameattached', attached => { - frame = attached; - page.on('framenavigated', f => { - if (f === frame) - fulfill(); - }); - }); - }); + const frame = await new Promise(f => page.once('frameattached', f)); + await new Promise(fulfill => page.on('framenavigated', f => { + if (f === frame) + fulfill(); + })); await Promise.all([ frame.evaluate(() => window.stop()), navigationPromise @@ -572,16 +568,16 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME const error = await navigationPromise; expect(error.message).toBe('Navigating frame was detached'); }); - it.skip(WEBKIT)('should return matching responses', async({page, server}) => { + it('should return matching responses', async({page, server}) => { // Disable cache: otherwise, chromium will cache similar requests. await page.setCacheEnabled(false); await page.goto(server.EMPTY_PAGE); // Attach three frames. - const frames = await Promise.all([ - utils.attachFrame(page, 'frame1', server.EMPTY_PAGE), - utils.attachFrame(page, 'frame2', server.EMPTY_PAGE), - utils.attachFrame(page, 'frame3', server.EMPTY_PAGE), - ]); + const frames = [ + await utils.attachFrame(page, 'frame1', server.EMPTY_PAGE), + await utils.attachFrame(page, 'frame2', server.EMPTY_PAGE), + await utils.attachFrame(page, 'frame3', server.EMPTY_PAGE), + ]; // Navigate all frames to the same URL. const serverResponses = []; server.setRoute('/one-style.html', (req, res) => serverResponses.push(res));