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

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',
     ],
   },

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<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>}

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);

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', () => {

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();
     },

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();