From dd2ce94de92d85b64b774f03a7c3c9a02305e40d Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 13 Dec 2019 16:35:10 -0800 Subject: [PATCH] fix(navigation): waitForNavigation to pick up aborted navigation (#244) --- src/chromium/FrameManager.ts | 2 +- src/chromium/NetworkManager.ts | 21 ++++++++++-- src/chromium/features/permissions.ts | 4 +-- src/firefox/FrameManager.ts | 2 +- src/frames.ts | 2 +- src/transport.ts | 4 +-- src/webkit/FrameManager.ts | 2 +- src/webkit/NetworkManager.ts | 12 +++---- test/navigation.spec.js | 48 +++++++++++++++------------- 9 files changed, 58 insertions(+), 39 deletions(-) diff --git a/src/chromium/FrameManager.ts b/src/chromium/FrameManager.ts index 3fca1061a8..73dda0f7a6 100644 --- a/src/chromium/FrameManager.ts +++ b/src/chromium/FrameManager.ts @@ -201,7 +201,7 @@ export class FrameManager extends EventEmitter implements PageDelegate { return; if (event.name === 'init') { frame._firedLifecycleEvents.clear(); - frame._onExpectedNewDocumentNavigation(event.loaderId); + frame._expectNewDocumentNavigation(event.loaderId); } else if (event.name === 'load') { frame._lifecycleEvent('load'); } else if (event.name === 'DOMContentLoaded') { diff --git a/src/chromium/NetworkManager.ts b/src/chromium/NetworkManager.ts index 3da800b001..ae105cc479 100644 --- a/src/chromium/NetworkManager.ts +++ b/src/chromium/NetworkManager.ts @@ -197,8 +197,13 @@ export class NetworkManager extends EventEmitter { redirectChain = request.request._redirectChain; } } + // TODO: how can frame be null here? const frame = event.frameId ? this._frameManager.frame(event.frameId) : null; - const request = new InterceptableRequest(this._client, frame, interceptionId, this._userRequestInterceptionEnabled, event, redirectChain); + const isNavigationRequest = event.requestId === event.loaderId && event.type === 'Document'; + const documentId = isNavigationRequest ? event.loaderId : undefined; + if (documentId && frame) + frame._expectNewDocumentNavigation(documentId, event.request.url); + const request = new InterceptableRequest(this._client, frame, interceptionId, documentId, this._userRequestInterceptionEnabled, event, redirectChain); this._requestIdToRequest.set(event.requestId, request); this.emit(NetworkManagerEvents.Request, request.request); } @@ -259,6 +264,14 @@ export class NetworkManager extends EventEmitter { response._requestFinished(); this._requestIdToRequest.delete(request._requestId); this._attemptedAuthentications.delete(request._interceptionId); + if (request._documentId && request.request.frame()) { + const isCurrentDocument = request.request.frame()._lastDocumentId === request._documentId; + let errorText = event.errorText; + if (event.canceled) + errorText += '; maybe frame was detached?'; + if (!isCurrentDocument) + request.request.frame()._onAbortedNewDocumentNavigation(request._documentId, errorText); + } this.emit(NetworkManagerEvents.RequestFailed, request.request); } } @@ -273,17 +286,19 @@ class InterceptableRequest { readonly request: network.Request; _requestId: string; _interceptionId: string; + _documentId: string; private _client: CDPSession; private _allowInterception: boolean; private _interceptionHandled = false; - constructor(client: CDPSession, frame: frames.Frame | null, interceptionId: string, allowInterception: boolean, event: Protocol.Network.requestWillBeSentPayload, redirectChain: network.Request[]) { + constructor(client: CDPSession, frame: frames.Frame | null, interceptionId: string, documentId: string | undefined, allowInterception: boolean, event: Protocol.Network.requestWillBeSentPayload, redirectChain: network.Request[]) { this._client = client; this._requestId = event.requestId; this._interceptionId = interceptionId; + this._documentId = documentId; this._allowInterception = allowInterception; - this.request = new network.Request(frame, redirectChain, event.requestId === event.loaderId && event.type === 'Document', + this.request = new network.Request(frame, redirectChain, !!documentId, event.request.url, event.type.toLowerCase(), event.request.method, event.request.postData, headersObject(event.request.headers)); (this.request as any)[interceptableRequestSymbol] = this; } diff --git a/src/chromium/features/permissions.ts b/src/chromium/features/permissions.ts index ce75e6418d..831cfb7617 100644 --- a/src/chromium/features/permissions.ts +++ b/src/chromium/features/permissions.ts @@ -40,8 +40,8 @@ export class Permissions { ['gyroscope', 'sensors'], ['magnetometer', 'sensors'], ['accessibility-events', 'accessibilityEvents'], - ['clipboard-read', 'clipboardRead'], - ['clipboard-write', 'clipboardWrite'], + ['clipboard-read', 'clipboardReadWrite'], + ['clipboard-write', 'clipboardSanitizedWrite'], ['payment-handler', 'paymentHandler'], // chrome-specific permissions we have. ['midi-sysex', 'midiSysex'], diff --git a/src/firefox/FrameManager.ts b/src/firefox/FrameManager.ts index 8567355dd0..ac3cfa6919 100644 --- a/src/firefox/FrameManager.ts +++ b/src/firefox/FrameManager.ts @@ -138,7 +138,7 @@ export class FrameManager extends EventEmitter implements PageDelegate { _onNavigationStarted(params) { const frame = this._frames.get(params.frameId); - frame._onExpectedNewDocumentNavigation(params.navigationId, params.url); + frame._expectNewDocumentNavigation(params.navigationId, params.url); } _onNavigationAborted(params) { diff --git a/src/frames.ts b/src/frames.ts index 6cb99853ff..90bd56f21a 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -422,7 +422,7 @@ export class Frame { return context.evaluate(() => document.title); } - _onExpectedNewDocumentNavigation(documentId: string, url?: string) { + _expectNewDocumentNavigation(documentId: string, url?: string) { for (const watcher of this._page._lifecycleWatchers) watcher._onExpectedNewDocumentNavigation(this, documentId, url); } diff --git a/src/transport.ts b/src/transport.ts index 884464a82f..f7334b0d10 100644 --- a/src/transport.ts +++ b/src/transport.ts @@ -178,11 +178,11 @@ export class SlowMoTransport { } send(s: string) { - this._delegate.send(s); + this._delegate.send(s); } close() { this._closed = true; - this._delegate.close(); + this._delegate.close(); } } diff --git a/src/webkit/FrameManager.ts b/src/webkit/FrameManager.ts index 92d14e3517..a97e6c5fe0 100644 --- a/src/webkit/FrameManager.ts +++ b/src/webkit/FrameManager.ts @@ -231,7 +231,7 @@ export class FrameManager extends EventEmitter implements PageDelegate { // Append session id to avoid cross-process loaderId clash. const documentId = this._session._sessionId + '::' + framePayload.loaderId; if (!initial) - frame._onExpectedNewDocumentNavigation(documentId); + frame._expectNewDocumentNavigation(documentId); frame._onCommittedNewDocumentNavigation(framePayload.url, framePayload.name, documentId); this._page.emit(Events.Page.FrameNavigated, frame); diff --git a/src/webkit/NetworkManager.ts b/src/webkit/NetworkManager.ts index b44264ae7f..2fa1fb6896 100644 --- a/src/webkit/NetworkManager.ts +++ b/src/webkit/NetworkManager.ts @@ -102,7 +102,7 @@ export class NetworkManager extends EventEmitter { const isNavigationRequest = event.type === 'Document'; const documentId = isNavigationRequest ? this._session._sessionId + '::' + event.loaderId : undefined; if (documentId) - frame._onExpectedNewDocumentNavigation(documentId, event.request.url); + frame._expectNewDocumentNavigation(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); @@ -166,11 +166,11 @@ export class NetworkManager extends EventEmitter { 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.includes('cancelled'); - if (!isCurrentDocument && !wasCanceled) - request.request.frame()._onAbortedNewDocumentNavigation(request._documentId, event.errorText); + let errorText = event.errorText; + if (errorText.includes('cancelled')) + errorText += '; maybe frame was detached?'; + if (!isCurrentDocument) + request.request.frame()._onAbortedNewDocumentNavigation(request._documentId, errorText); } this.emit(NetworkManagerEvents.RequestFailed, request.request); } diff --git a/test/navigation.spec.js b/test/navigation.spec.js index a5e7197fd5..8d736d0ff5 100644 --- a/test/navigation.spec.js +++ b/test/navigation.spec.js @@ -157,32 +157,14 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME page.on('requestfailed', request => expect(request).toBeTruthy()); let error = null; await page.goto(httpsServer.EMPTY_PAGE).catch(e => error = e); - if (CHROME) { - expect(error.message).toContain('net::ERR_CERT_AUTHORITY_INVALID'); - } else if (WEBKIT) { - if (process.platform === 'darwin') - expect(error.message).toContain('The certificate for this server is invalid'); - else - expect(error.message).toContain('Unacceptable TLS certificate'); - } else { - expect(error.message).toContain('SSL_ERROR_UNKNOWN'); - } + expectSSLError(error.message); }); it('should fail when navigating to bad SSL after redirects', async({page, server, httpsServer}) => { server.setRedirect('/redirect/1.html', '/redirect/2.html'); server.setRedirect('/redirect/2.html', '/empty.html'); let error = null; await page.goto(httpsServer.PREFIX + '/redirect/1.html').catch(e => error = e); - if (CHROME) { - expect(error.message).toContain('net::ERR_CERT_AUTHORITY_INVALID'); - } else if (WEBKIT) { - if (process.platform === 'darwin') - expect(error.message).toContain('The certificate for this server is invalid'); - else - expect(error.message).toContain('Unacceptable TLS certificate'); - } else { - expect(error.message).toContain('SSL_ERROR_UNKNOWN'); - } + expectSSLError(error.message); }); xit('should throw if networkidle is passed as an option', async({page, server}) => { let error = null; @@ -443,6 +425,15 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME expect(response).toBe(null); expect(page.url()).toBe(server.EMPTY_PAGE + '#foobar'); }); + it('should work with clicking on links which do not commit navigation', async({page, server, httpsServer}) => { + await page.goto(server.EMPTY_PAGE); + await page.setContent(`foobar`); + const [error] = await Promise.all([ + page.waitForNavigation().catch(e => e), + page.click('a'), + ]); + expectSSLError(error.message); + }); it('should work with history.pushState()', async({page, server}) => { await page.goto(server.EMPTY_PAGE); await page.setContent(` @@ -566,7 +557,7 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME await page.$eval('iframe', frame => frame.remove()); const error = await navigationPromise; - expect(error.message).toBe('Navigating frame was detached'); + expect(error.message).toContain('frame was detached'); }); it('should return matching responses', async({page, server}) => { // Disable cache: otherwise, chromium will cache similar requests. @@ -623,7 +614,7 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME ]); await page.$eval('iframe', frame => frame.remove()); await navigationPromise; - expect(error.message).toBe('Navigating frame was detached'); + expect(error.message).toContain('frame was detached'); }); }); @@ -635,4 +626,17 @@ module.exports.addTests = function({testRunner, expect, playwright, FFOX, CHROME expect(await page.evaluate(() => window._foo)).toBe(undefined); }); }); + + function expectSSLError(errorMessage) { + if (CHROME) { + expect(errorMessage).toContain('net::ERR_CERT_AUTHORITY_INVALID'); + } else if (WEBKIT) { + if (process.platform === 'darwin') + expect(errorMessage).toContain('The certificate for this server is invalid'); + else + expect(errorMessage).toContain('Unacceptable TLS certificate'); + } else { + expect(errorMessage).toContain('SSL_ERROR_UNKNOWN'); + } + } };