core(gather-runner): always reset scroll position (#9060)

GoogleChrome · Jun 7, 2019 · 0a28612 · 0a28612
1 parent ddff3d6
commit 0a28612
Show file tree

Hide file tree

Showing 6 changed files with 98 additions and 2 deletions.
diff --git 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
@@ -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;
+    }
   }
 
   /**
@@ -1268,6 +1291,22 @@ class Driver {
     return flattenedDocument.nodes ? flattenedDocument.nodes : [];
   }
 
+  /**
+   * @param {{x: number, y: number}} position
+   * @return {Promise<void>}
+   */
+  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<void>}

diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
@@ -339,12 +339,17 @@ class GatherRunner {
    * @return {Promise<void>}
    */
   static async afterPass(passContext, loadData, gathererResults) {
+    const driver = passContext.driver;
     const config = passContext.passConfig;
     const gatherers = config.gatherers;
 
     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 = await driver.getScrollPosition();
+
     for (const gathererDefn of gatherers) {
       const gatherer = gathererDefn.instance;
       const status = {
@@ -362,6 +367,7 @@ class GatherRunner {
       gathererResult.push(artifactPromise);
       gathererResults[gatherer.name] = gathererResult;
       await artifactPromise.catch(() => {});
+      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
@@ -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
@@ -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
@@ -396,6 +396,8 @@ describe('GatherRunner', function() {
       endTrace: asyncFunc,
       endDevtoolsLog: () => [],
       getBrowserVersion: async () => ({userAgent: ''}),
+      getScrollPosition: async () => 1,
+      scrollTo: async () => {},
     };
     const passConfig = {
       passName: 'default',
@@ -677,6 +679,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();