From 5cfd6d9fe92ca32e11e09a08ea0af101131611f4 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 8 Jun 2023 10:33:28 -0700 Subject: [PATCH] fix(cr network): emit sw-handled requests when routing is enabled (#23589) Previously, such requests were skipped because they never receive `Fetch.requestPaused` as there was no real network. Also cleanup some redundant tests and move them from chromium-only file. Fixes #23424. --- .../src/server/chromium/crNetworkManager.ts | 12 +- tests/assets/serviceworkers/stub/sw.html | 4 + tests/assets/serviceworkers/stub/sw.js | 29 ++++ tests/library/chromium/chromium.spec.ts | 164 ------------------ tests/page/page-event-request.spec.ts | 98 +++++++++++ 5 files changed, 135 insertions(+), 172 deletions(-) create mode 100644 tests/assets/serviceworkers/stub/sw.html create mode 100644 tests/assets/serviceworkers/stub/sw.js diff --git a/packages/playwright-core/src/server/chromium/crNetworkManager.ts b/packages/playwright-core/src/server/chromium/crNetworkManager.ts index 8a31b59262..caa07046db 100644 --- a/packages/playwright-core/src/server/chromium/crNetworkManager.ts +++ b/packages/playwright-core/src/server/chromium/crNetworkManager.ts @@ -384,16 +384,12 @@ export class CRNetworkManager { // For frame-level Requests that are handled by a Service Worker's fetch handler, we'll never get a requestPaused event, so we need to // manually create the request. In an ideal world, crNetworkManager would be able to know this on Network.requestWillBeSent, but there // is not enough metadata there. - // - // PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS we guard with, since this would fix an old bug where, when using routing, - // request would not be emitted to the user for requests made by a page with a SW (and fetch handler) registered - if (!!process.env.PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS && !request && event.response.fromServiceWorker) { + if (!request && event.response.fromServiceWorker) { const requestWillBeSentEvent = this._requestIdToRequestWillBeSentEvent.get(event.requestId); - const frame = requestWillBeSentEvent?.frameId ? this._page?._frameManager.frame(requestWillBeSentEvent.frameId) : null; - if (requestWillBeSentEvent && frame) { - this._onRequest(frame, requestWillBeSentEvent, null /* requestPausedPayload */); - request = this._requestIdToRequest.get(event.requestId); + if (requestWillBeSentEvent) { this._requestIdToRequestWillBeSentEvent.delete(event.requestId); + this._onRequest(undefined, requestWillBeSentEvent, null /* requestPausedPayload */); + request = this._requestIdToRequest.get(event.requestId); } } // FileUpload sends a response without a matching request. diff --git a/tests/assets/serviceworkers/stub/sw.html b/tests/assets/serviceworkers/stub/sw.html new file mode 100644 index 0000000000..b29d3f2e34 --- /dev/null +++ b/tests/assets/serviceworkers/stub/sw.html @@ -0,0 +1,4 @@ + diff --git a/tests/assets/serviceworkers/stub/sw.js b/tests/assets/serviceworkers/stub/sw.js new file mode 100644 index 0000000000..7485dd4290 --- /dev/null +++ b/tests/assets/serviceworkers/stub/sw.js @@ -0,0 +1,29 @@ +const kSwHtml = ` + +`; + +self.addEventListener('fetch', event => { + if (event.request.url.endsWith('sw.html')) { + const blob = new Blob([kSwHtml], { type: 'text/html' }); + const response = new Response(blob, { status: 200 , statusText: 'OK' }); + event.respondWith(response); + return; + } + if (event.request.url.includes('error')) { + event.respondWith(Promise.reject(new Error('uh oh'))); + return; + } + const slash = event.request.url.lastIndexOf('/'); + const name = event.request.url.substring(slash + 1); + const blob = new Blob(['responseFromServiceWorker:' + name], { type: name.endsWith('.css') ? 'text/css' : 'application/javascript' }); + const response = new Response(blob, {status: 200 , statusText: 'OK' }); + event.respondWith(response); +}); + +self.addEventListener('activate', event => { + event.waitUntil(clients.claim()); +}); diff --git a/tests/library/chromium/chromium.spec.ts b/tests/library/chromium/chromium.spec.ts index 8024706518..8702a705c7 100644 --- a/tests/library/chromium/chromium.spec.ts +++ b/tests/library/chromium/chromium.spec.ts @@ -174,82 +174,6 @@ playwrightTest('should pass args with spaces', async ({ browserType, createUserD expect(userAgent).toBe('I am Foo'); }); -test.describe('should emit page-level network events with service worker fetch handler', () => { - test.describe('when not using routing', () => { - test('successful request', async ({ page, server }) => { - await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); - await page.evaluate(() => window['activationPromise']); - - const [pageReq, pageResp, /* pageFinished */, swResponse] = await Promise.all([ - page.waitForEvent('request'), - page.waitForEvent('response'), - page.waitForEvent('requestfinished'), - page.evaluate(() => window['fetchDummy']('foo')), - ]); - expect(swResponse).toBe('responseFromServiceWorker:foo'); - expect(pageReq.url()).toMatch(/fetchdummy\/foo$/); - expect(pageReq.serviceWorker()).toBe(null); - expect(pageResp.fromServiceWorker()).toBe(true); - expect(pageResp).toBe(await pageReq.response()); - expect((await pageReq.response()).fromServiceWorker()).toBe(true); - }); - - test('failed request', async ({ page, server }) => { - await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); - await page.evaluate(() => window['activationPromise']); - - const [pageReq] = await Promise.all([ - page.waitForEvent('request'), - page.waitForEvent('requestfailed'), - page.evaluate(() => window['fetchDummy']('error')).catch(e => e), - ]); - expect(pageReq.url()).toMatch(/fetchdummy\/error$/); - expect(pageReq.failure().errorText).toMatch(/net::ERR_FAILED/); - expect(pageReq.serviceWorker()).toBe(null); - expect(await pageReq.response()).toBe(null); - }); - }); - - test.describe('when routing', () => { - test('successful request', async ({ page, server, context }) => { - await context.route('**', route => route.continue()); - await page.route('**', route => route.continue()); - await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); - await page.evaluate(() => window['activationPromise']); - - const [result, pageResp] = await Promise.all([ - page.waitForEvent('request', { timeout: 750 }).catch(e => 'timeout'), - page.evaluate(() => window['fetchDummy']('foo')), - ]); - expect(result).toBe('timeout'); - expect(pageResp).toBeTruthy(); - }); - - test('failed request', async ({ page, server, context }) => { - await context.route('**', route => route.continue()); - let markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker = false; - await page.route('**', route => { - if (route.request().url().endsWith('foo')) - markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker = true; - void route.continue(); - }); - await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); - await page.evaluate(() => window['activationPromise']); - - const [pageReq] = await Promise.all([ - page.waitForEvent('request'), - page.waitForEvent('requestfailed'), - page.evaluate(() => window['fetchDummy']('error')).catch(e => e), - ]); - expect(pageReq.url()).toMatch(/fetchdummy\/error$/); - expect(pageReq.failure().errorText).toMatch(/net::ERR_FAILED/); - expect(pageReq.serviceWorker()).toBe(null); - expect(await pageReq.response()).toBe(null); - expect(markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker).toBe(false); - }); - }); -}); - test.describe('PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1', () => { test.skip(({ mode }) => mode !== 'default', 'Cannot set env variables in non-default'); test.beforeAll(() => process.env.PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS = '1'); @@ -692,94 +616,6 @@ test.describe('PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1', () => { expect(error).toMatch(/REJECTED.*Failed to fetch/); }); - test.describe('should emit page-level network events with service worker fetch handler', () => { - test.describe('when not using routing', () => { - test('successful request', async ({ page, server }) => { - await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); - await page.evaluate(() => window['activationPromise']); - - const [pageReq, pageResp, /* pageFinished */, swResponse] = await Promise.all([ - page.waitForEvent('request'), - page.waitForEvent('response'), - page.waitForEvent('requestfinished'), - page.evaluate(() => window['fetchDummy']('foo')), - ]); - expect(swResponse).toBe('responseFromServiceWorker:foo'); - expect(pageReq.url()).toMatch(/fetchdummy\/foo$/); - expect(pageReq.serviceWorker()).toBe(null); - expect(pageResp.fromServiceWorker()).toBe(true); - expect(pageResp).toBe(await pageReq.response()); - expect((await pageReq.response()).fromServiceWorker()).toBe(true); - }); - - test('failed request', async ({ page, server }) => { - await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); - await page.evaluate(() => window['activationPromise']); - - const [pageReq] = await Promise.all([ - page.waitForEvent('request'), - page.waitForEvent('requestfailed'), - page.evaluate(() => window['fetchDummy']('error')).catch(e => e), - ]); - expect(pageReq.url()).toMatch(/fetchdummy\/error$/); - expect(pageReq.failure().errorText).toMatch(/net::ERR_FAILED/); - expect(pageReq.serviceWorker()).toBe(null); - expect(await pageReq.response()).toBe(null); - }); - }); - - test.describe('when routing', () => { - test('successful request', async ({ page, server, context }) => { - await context.route('**', route => route.continue()); - let markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker = false; - await page.route('**', route => { - if (route.request().url().endsWith('foo')) - markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker = true; - void route.continue(); - }); - await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); - await page.evaluate(() => window['activationPromise']); - - const [pageReq, pageResp, /* pageFinished */, swResponse] = await Promise.all([ - page.waitForEvent('request'), - page.waitForEvent('response'), - page.waitForEvent('requestfinished'), - page.evaluate(() => window['fetchDummy']('foo')), - ]); - expect(swResponse).toBe('responseFromServiceWorker:foo'); - expect(pageReq.url()).toMatch(/fetchdummy\/foo$/); - expect(pageReq.serviceWorker()).toBe(null); - expect(pageResp.fromServiceWorker()).toBe(true); - expect(pageResp).toBe(await pageReq.response()); - expect((await pageReq.response()).fromServiceWorker()).toBe(true); - expect(markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker).toBe(false); - }); - - test('failed request', async ({ page, server, context }) => { - await context.route('**', route => route.continue()); - let markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker = false; - await page.route('**', route => { - if (route.request().url().endsWith('foo')) - markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker = true; - void route.continue(); - }); - await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); - await page.evaluate(() => window['activationPromise']); - - const [pageReq] = await Promise.all([ - page.waitForEvent('request'), - page.waitForEvent('requestfailed'), - page.evaluate(() => window['fetchDummy']('error')).catch(e => e), - ]); - expect(pageReq.url()).toMatch(/fetchdummy\/error$/); - expect(pageReq.failure().errorText).toMatch(/net::ERR_FAILED/); - expect(pageReq.serviceWorker()).toBe(null); - expect(await pageReq.response()).toBe(null); - expect(markFailureIfPageRoutesARequestAlreadyHandledByServiceWorker).toBe(false); - }); - }); - }); - test('setExtraHTTPHeaders', async ({ context, page, server }) => { const [worker] = await Promise.all([ context.waitForEvent('serviceworker'), diff --git a/tests/page/page-event-request.spec.ts b/tests/page/page-event-request.spec.ts index cf56574c73..89c971c971 100644 --- a/tests/page/page-event-request.spec.ts +++ b/tests/page/page-event-request.spec.ts @@ -53,9 +53,107 @@ it('should report requests and responses handled by service worker', async ({ pa ]); expect(swResponse).toBe('responseFromServiceWorker:foo'); expect(request.url()).toBe(server.PREFIX + '/serviceworkers/fetchdummy/foo'); + expect(request.serviceWorker()).toBe(null); const response = await request.response(); expect(response.url()).toBe(server.PREFIX + '/serviceworkers/fetchdummy/foo'); expect(await response.text()).toBe('responseFromServiceWorker:foo'); + expect(response.fromServiceWorker()).toBe(true); + + const [failedRequest] = await Promise.all([ + page.waitForEvent('requestfailed'), + page.evaluate(() => window['fetchDummy']('error')).catch(e => e), + ]); + expect(failedRequest.url()).toBe(server.PREFIX + '/serviceworkers/fetchdummy/error'); + expect(failedRequest.failure()).not.toBe(null); + expect(failedRequest.serviceWorker()).toBe(null); + expect(await failedRequest.response()).toBe(null); +}); + +it('should report requests and responses handled by service worker with routing', async ({ page, server, isAndroid, isElectron }) => { + it.fixme(isAndroid); + it.fixme(isElectron); + + await page.route('**/*', route => route.continue()); + await page.goto(server.PREFIX + '/serviceworkers/fetchdummy/sw.html'); + await page.evaluate(() => window['activationPromise']); + const [swResponse, request] = await Promise.all([ + page.evaluate(() => window['fetchDummy']('foo')), + page.waitForEvent('request'), + ]); + expect(swResponse).toBe('responseFromServiceWorker:foo'); + expect(request.url()).toBe(server.PREFIX + '/serviceworkers/fetchdummy/foo'); + expect(request.serviceWorker()).toBe(null); + const response = await request.response(); + expect(response.url()).toBe(server.PREFIX + '/serviceworkers/fetchdummy/foo'); + expect(await response.text()).toBe('responseFromServiceWorker:foo'); + + const [failedRequest] = await Promise.all([ + page.waitForEvent('requestfailed'), + page.evaluate(() => window['fetchDummy']('error')).catch(e => e), + ]); + expect(failedRequest.url()).toBe(server.PREFIX + '/serviceworkers/fetchdummy/error'); + expect(failedRequest.failure()).not.toBe(null); + expect(failedRequest.serviceWorker()).toBe(null); + expect(await failedRequest.response()).toBe(null); +}); + +it('should report navigation requests and responses handled by service worker', async ({ page, server, isAndroid, isElectron, browserName }) => { + it.fixme(isAndroid); + it.fixme(isElectron); + + await page.goto(server.PREFIX + '/serviceworkers/stub/sw.html'); + await page.evaluate(() => window['activationPromise']); + + const reloadResponse = await page.reload(); + expect(await page.evaluate('window.fromSW')).toBe(true); + expect(reloadResponse.url()).toBe(server.PREFIX + '/serviceworkers/stub/sw.html'); + await page.evaluate(() => window['activationPromise']); + + if (browserName !== 'firefox') { + // When SW fetch throws, Firefox does not fail the navigation, + // but rather falls back to the real network. + + const [, failedRequest] = await Promise.all([ + page.evaluate(() => { + window.location.href = '/serviceworkers/stub/error.html'; + }), + page.waitForEvent('requestfailed'), + ]); + expect(failedRequest.url()).toBe(server.PREFIX + '/serviceworkers/stub/error.html'); + expect(failedRequest.failure().errorText).toContain(browserName === 'chromium' ? 'net::ERR_FAILED' : 'uh oh'); + expect(failedRequest.serviceWorker()).toBe(null); + expect(await failedRequest.response()).toBe(null); + } +}); + +it('should report navigation requests and responses handled by service worker with routing', async ({ page, server, isAndroid, isElectron, browserName }) => { + it.fixme(isAndroid); + it.fixme(isElectron); + + await page.route('**/*', route => route.continue()); + await page.goto(server.PREFIX + '/serviceworkers/stub/sw.html'); + await page.evaluate(() => window['activationPromise']); + + const reloadResponse = await page.reload(); + expect(await page.evaluate('window.fromSW')).toBe(true); + expect(reloadResponse.url()).toBe(server.PREFIX + '/serviceworkers/stub/sw.html'); + await page.evaluate(() => window['activationPromise']); + + if (browserName !== 'firefox') { + // When SW fetch throws, Firefox does not fail the navigation, + // but rather falls back to the real network. + + const [, failedRequest] = await Promise.all([ + page.evaluate(() => { + window.location.href = '/serviceworkers/stub/error.html'; + }), + page.waitForEvent('requestfailed'), + ]); + expect(failedRequest.url()).toBe(server.PREFIX + '/serviceworkers/stub/error.html'); + expect(failedRequest.failure().errorText).toContain(browserName === 'chromium' ? 'net::ERR_FAILED' : 'uh oh'); + expect(failedRequest.serviceWorker()).toBe(null); + expect(await failedRequest.response()).toBe(null); + } }); it('should return response body when Cross-Origin-Opener-Policy is set', async ({ page, server, browserName }) => {