core(image-elements): cap natural size fetch time

lighthouse-core/gather/driver.js

@@ -353,6 +353,10 @@ class Driver {
    * @return {Promise<*>}
    */
   async _evaluateInContext(expression, contextId) {
+    // Use a higher than default timeout if the user hasn't specified a specific timeout.
+    // Otherwise, use whatever was requested.
+    const timeout = this._nextProtocolTimeout === DEFAULT_PROTOCOL_TIMEOUT ?
+      60000 : this._nextProtocolTimeout;
     const evaluationParams = {
       // We need to explicitly wrap the raw expression for several purposes:
       // 1. Ensure that the expression will be a native Promise and not a polyfill/non-Promise.
@@ -372,11 +376,11 @@ class Driver {
       includeCommandLineAPI: true,
       awaitPromise: true,
       returnByValue: true,
-      timeout: 60000,
+      timeout,
       contextId,
     };
 
-    this.setNextProtocolTimeout(60000);
+    this.setNextProtocolTimeout(timeout);
     const response = await this.sendCommand('Runtime.evaluate', evaluationParams);
     if (response.exceptionDetails) {
       // An error occurred before we could even create a Promise, should be *very* rare

lighthouse-core/gather/gatherers/image-elements.js

@@ -87,8 +87,8 @@ function collectImageElementInfo() {
       displayedHeight: element.clientHeight,
       clientRect: getClientRect(element),
       // CSS Images do not expose natural size, we'll determine the size later
-      naturalWidth: Number.MAX_VALUE,
-      naturalHeight: Number.MAX_VALUE,
+      naturalWidth: 0,
+      naturalHeight: 0,
       isCss: true,
       isPicture: false,
       usesObjectFit: false,
@@ -130,12 +130,14 @@ class ImageElements extends Gatherer {
   async fetchElementWithSizeInformation(driver, element) {
     const url = JSON.stringify(element.src);
     try {
+      // We don't want this to take forever, 250ms should be enough for images that are cached
+      driver.setNextProtocolTimeout(250);
       /** @type {{naturalWidth: number, naturalHeight: number}} */
       const size = await driver.evaluateAsync(`(${determineNaturalSize.toString()})(${url})`);
       return Object.assign(element, size);
     } catch (_) {
       // determineNaturalSize fails on invalid images, which we treat as non-visible
-      return Object.assign(element, {naturalWidth: 0, naturalHeight: 0});
+      return element;
     }
   }
 
@@ -147,7 +149,9 @@ class ImageElements extends Gatherer {
   async afterPass(passContext, loadData) {
     const driver = passContext.driver;
     const indexedNetworkRecords = loadData.networkRecords.reduce((map, record) => {
-      if (/^image/.test(record.mimeType) && record.finished) {
+      // The network record is only valid for size information if it finished with a successful status
+      // code that indicates a complete resource response.
+      if (/^image/.test(record.mimeType) && record.finished && record.statusCode === 200) {
         map[record.url] = record;
       }
 
@@ -163,6 +167,9 @@ class ImageElements extends Gatherer {
     const elements = await driver.evaluateAsync(expression);
 
     const imageUsage = [];
+    const top50Images = Object.values(indexedNetworkRecords)
+      .sort((a, b) => b.resourceSize - a.resourceSize)
+      .slice(0, 50);
     for (let element of elements) {
       // Pull some of our information directly off the network record.
       const networkRecord = indexedNetworkRecords[element.src] || {};
@@ -177,8 +184,13 @@ class ImageElements extends Gatherer {
 
       // Images within `picture` behave strangely and natural size information isn't accurate,
       // CSS images have no natural size information at all. Try to get the actual size if we can.
-      // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image.
-      if ((element.isPicture || element.isCss) && networkRecord) {
+      // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image,
+      // or it's not in the top 50 largest images.
+      if (
+        (element.isPicture || element.isCss) &&
+        networkRecord &&
+        top50Images.includes(networkRecord)
+      ) {
         element = await this.fetchElementWithSizeInformation(driver, element);
       }
 

lighthouse-core/test/gather/driver-test.js

@@ -31,13 +31,14 @@ function createMockSendCommandFn() {
   const mockFn = jest.fn().mockImplementation(command => {
     const indexOfResponse = mockResponses.findIndex(entry => entry.command === command);
     if (indexOfResponse === -1) throw new Error(`${command} unimplemented`);
-    const {response} = mockResponses[indexOfResponse];
+    const {response, delay} = mockResponses[indexOfResponse];
     mockResponses.splice(indexOfResponse, 1);
+    if (delay) return new Promise(resolve => setTimeout(() => resolve(response), delay));
     return Promise.resolve(response);
   });
 
-  mockFn.mockResponse = (command, response) => {
-    mockResponses.push({command, response});
+  mockFn.mockResponse = (command, response, delay) => {
+    mockResponses.push({command, response, delay});
     return mockFn;
   };
 
@@ -260,6 +261,34 @@ describe('.evaluateAsync', () => {
     connectionStub.sendCommand.findInvocation('Runtime.evaluate');
   });
 
+  it('uses a high default timeout', async () => {
+    connectionStub.sendCommand = createMockSendCommandFn()
+    .mockResponse('Runtime.evaluate', {result: {value: 2}}, 65000);
+
+    const evaluatePromise = makePromiseInspectable(driver.evaluateAsync('1 + 1'));
+    jest.advanceTimersByTime(30000);
+    await flushAllTimersAndMicrotasks();
+    expect(evaluatePromise).not.toBeDone();
+
+    jest.advanceTimersByTime(30000);
+    await flushAllTimersAndMicrotasks();
+    expect(evaluatePromise).toBeDone();
+    await expect(evaluatePromise).rejects.toBeTruthy();
+  });
+
+  it('uses the specific timeout given', async () => {
+    connectionStub.sendCommand = createMockSendCommandFn()
+    .mockResponse('Runtime.evaluate', {result: {value: 2}}, 10000);
+
+    driver.setNextProtocolTimeout(5000);
+    const evaluatePromise = makePromiseInspectable(driver.evaluateAsync('1 + 1'));
+
+    jest.advanceTimersByTime(5001);
+    await flushAllTimersAndMicrotasks();
+    expect(evaluatePromise).toBeDone();
+    await expect(evaluatePromise).rejects.toBeTruthy();
+  });
+
   it('evaluates an expression in isolation', async () => {
     connectionStub.sendCommand = createMockSendCommandFn()
       .mockResponse('Page.getResourceTree', {frameTree: {frame: {id: 1337}}})

types/artifacts.d.ts

@@ -81,7 +81,7 @@ declare global {
       HTMLWithoutJavaScript: {bodyText: string, hasNoScript: boolean};
       /** Whether the page ended up on an HTTPS page after attempting to load the HTTP version. */
       HTTPRedirect: {value: boolean};
-      /** Information on size and loading for all the images in the page. */
+      /** Information on size and loading for all the images in the page. Natural size information for `picture` and CSS images is only available if the image was one of the largest 50 images. */
       ImageElements: Artifacts.ImageElement[];
       /** Information on JS libraries and versions used by the page. */
       JSLibraries: {name: string, version: string, npmPkgName: string}[];