From c4c420e092b02da29768605aaaac514bede5d8ec Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Sat, 25 May 2019 10:37:17 -0500 Subject: [PATCH 1/4] Revert "misc(fix master): tmp revert of scroll change in #8625 (#9059)" This reverts commit 24ea72ac9d8b3340de28dd728440baf7968d8f31. --- lighthouse-core/config/default-config.js | 1 - lighthouse-core/gather/driver.js | 41 ++++++++++++++++++- lighthouse-core/gather/gather-runner.js | 5 +++ lighthouse-core/test/gather/driver-test.js | 13 ++++++ lighthouse-core/test/gather/fake-driver.js | 9 ++++ .../test/gather/gather-runner-test.js | 28 +++++++++++++ 6 files changed, 95 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js index 33ac027e8e01..fc5f152cbd30 100644 --- a/lighthouse-core/config/default-config.js +++ b/lighthouse-core/config/default-config.js @@ -135,7 +135,6 @@ const defaultConfig = { 'seo/embedded-content', 'seo/robots-txt', 'seo/tap-targets', - // Always run axe last because it scrolls the page down to the bottom 'accessibility', ], }, diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 93f267a8ae85..853cb328ea3b 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -86,6 +86,16 @@ class Driver { this._handleReceivedMessageFromTarget(event, []).catch(this._handleEventError); }); + // We use isolated execution contexts for `evaluateAsync` that can be destroyed through navigation + // and other page actions. Cleanup our relevant bookkeeping as we see those events. + this.on('Runtime.executionContextDestroyed', event => { + if (event.executionContextId === this._isolatedExecutionContextId) { + this._clearIsolatedContextId(); + } + }); + + this.on('Page.frameNavigated', () => this._clearIsolatedContextId()); + connection.on('protocolevent', this._handleProtocolEvent.bind(this)); /** @@ -464,7 +474,20 @@ class Driver { */ async evaluateAsync(expression, options = {}) { const contextId = options.useIsolation ? await this._getOrCreateIsolatedContextId() : undefined; - return this._evaluateInContext(expression, contextId); + + try { + // `await` is not redunant here because we want to `catch` the async errors + return await this._evaluateInContext(expression, contextId); + } catch (err) { + // If we were using isolation and the context disappeared on us, retry one more time. + if (contextId && err.message.includes('Cannot find context')) { + this._clearIsolatedContextId(); + const freshContextId = await this._getOrCreateIsolatedContextId(); + return this._evaluateInContext(expression, freshContextId); + } + + throw err; + } } /** @@ -1324,6 +1347,22 @@ class Driver { return flattenedDocument.nodes ? flattenedDocument.nodes : []; } + /** + * @param {{x: number, y: number}} position + * @return {Promise} + */ + scrollTo(position) { + const scrollExpression = `window.scrollTo(${position.x}, ${position.y})`; + return this.evaluateAsync(scrollExpression, {useIsolation: true}); + } + + /** + * @return {Promise<{x: number, y: number}>} + */ + getScrollPosition() { + return this.evaluateAsync(`({x: window.scrollX, y: window.scrollY})`, {useIsolation: true}); + } + /** * @param {{additionalTraceCategories?: string|null}=} settings * @return {Promise} diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 46388f345321..bc719adc58db 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -324,6 +324,10 @@ class GatherRunner { const apStatus = {msg: `Running afterPass methods`, id: `lh:gather:afterPass`}; log.time(apStatus, 'verbose'); + // Some gatherers scroll the page which can cause unexpected results for other gatherers. + // We reset the scroll position in between each gatherer. + const scrollPosition = pageLoadError ? null : await driver.getScrollPosition(); + for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; const status = { @@ -341,6 +345,7 @@ class GatherRunner { gathererResult.push(artifactPromise); gathererResults[gatherer.name] = gathererResult; await artifactPromise.catch(() => {}); + if (scrollPosition) await driver.scrollTo(scrollPosition); log.timeEnd(status); } log.timeEnd(apStatus); diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index d96ee191b1b9..8e3b0d069aee 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -317,6 +317,19 @@ describe('.evaluateAsync', () => { expect.anything() ); }); + + it('recovers from isolation failures', async () => { + connectionStub.sendCommand = createMockSendCommandFn() + .mockResponse('Page.getResourceTree', {frameTree: {frame: {id: 1337}}}) + .mockResponse('Page.createIsolatedWorld', {executionContextId: 9001}) + .mockResponse('Runtime.evaluate', Promise.reject(new Error('Cannot find context'))) + .mockResponse('Page.getResourceTree', {frameTree: {frame: {id: 1337}}}) + .mockResponse('Page.createIsolatedWorld', {executionContextId: 9002}) + .mockResponse('Runtime.evaluate', {result: {value: 'mocked value'}}); + + const value = await driver.evaluateAsync('"magic"', {useIsolation: true}); + expect(value).toEqual('mocked value'); + }); }); describe('.sendCommand', () => { diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js index 1db517772bea..108a40aa6d78 100644 --- a/lighthouse-core/test/gather/fake-driver.js +++ b/lighthouse-core/test/gather/fake-driver.js @@ -6,6 +6,8 @@ 'use strict'; function makeFakeDriver({protocolGetVersionResponse}) { + let scrollPosition = {x: 0, y: 0}; + return { getBrowserVersion() { return Promise.resolve(Object.assign({}, protocolGetVersionResponse, {milestone: 71})); @@ -57,6 +59,13 @@ function makeFakeDriver({protocolGetVersionResponse}) { evaluateAsync() { return Promise.resolve({}); }, + scrollTo(position) { + scrollPosition = position; + return Promise.resolve(); + }, + getScrollPosition() { + return Promise.resolve(scrollPosition); + }, registerPerformanceObserver() { return Promise.resolve(); }, diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 6dca2bffd0c3..2a3a0755c7a9 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -588,6 +588,34 @@ describe('GatherRunner', function() { }); }); + it('resets scroll position between every gatherer', async () => { + class ScrollMcScrollyGatherer extends TestGatherer { + afterPass(context) { + context.driver.scrollTo({x: 1000, y: 1000}); + } + } + + const url = 'https://example.com'; + const driver = Object.assign({}, fakeDriver); + const scrollToSpy = jest.spyOn(driver, 'scrollTo'); + + const passConfig = { + recordTrace: true, + gatherers: [ + {instance: new ScrollMcScrollyGatherer()}, + {instance: new TestGatherer()}, + ], + }; + + await GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}); + // One time for the afterPass of ScrollMcScrolly, two times for the resets of the two gatherers. + expect(scrollToSpy.mock.calls).toEqual([ + [{x: 1000, y: 1000}], + [{x: 0, y: 0}], + [{x: 0, y: 0}], + ]); + }); + it('does as many passes as are required', () => { const t1 = new TestGatherer(); const t2 = new TestGatherer(); From 24883e92f14e10e2f2413ba5a3523b052f4f3b7e Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 May 2019 09:46:02 -0500 Subject: [PATCH 2/4] fix merge conflicts --- lighthouse-core/gather/gather-runner.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index bc719adc58db..e39033b6fc7b 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -318,6 +318,7 @@ class GatherRunner { * @return {Promise} */ static async afterPass(passContext, loadData, gathererResults) { + const driver = passContext.driver; const config = passContext.passConfig; const gatherers = config.gatherers; @@ -326,7 +327,7 @@ class GatherRunner { // Some gatherers scroll the page which can cause unexpected results for other gatherers. // We reset the scroll position in between each gatherer. - const scrollPosition = pageLoadError ? null : await driver.getScrollPosition(); + const scrollPosition = await driver.getScrollPosition(); for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; From 3b1ec54d9e7b33a9be2e233f6a7313a8e42894d4 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 27 May 2019 09:55:34 -0500 Subject: [PATCH 3/4] fix testsg --- lighthouse-core/test/gather/gather-runner-test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 2a3a0755c7a9..af2f11ac1cec 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -374,6 +374,8 @@ describe('GatherRunner', function() { endTrace: asyncFunc, endDevtoolsLog: () => [], getBrowserVersion: async () => ({userAgent: ''}), + getScrollPosition: async () => 1, + scrollTo: async () => {}, }; const passConfig = { passName: 'default', @@ -607,7 +609,7 @@ describe('GatherRunner', function() { ], }; - await GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}); + await GatherRunner.afterPass({url, driver, passConfig}, {}, {TestGatherer: []}); // One time for the afterPass of ScrollMcScrolly, two times for the resets of the two gatherers. expect(scrollToSpy.mock.calls).toEqual([ [{x: 1000, y: 1000}], From add93d0d6688bf4e99f6b189a2ea60be5ab1550a Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 6 Jun 2019 17:26:15 -0500 Subject: [PATCH 4/4] Update lighthouse-core/gather/gather-runner.js --- lighthouse-core/gather/gather-runner.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index e39033b6fc7b..148c4fd33967 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -346,7 +346,7 @@ class GatherRunner { gathererResult.push(artifactPromise); gathererResults[gatherer.name] = gathererResult; await artifactPromise.catch(() => {}); - if (scrollPosition) await driver.scrollTo(scrollPosition); + await driver.scrollTo(scrollPosition); log.timeEnd(status); } log.timeEnd(apStatus);