Repository: GoogleChrome/lighthouse
Pull request #8727 (2019-04-30 01:42:42)

core(lightwallet): stricter verification of budget types
@khempenius @patrickhulce 

This is my proposal for slightly stricter checking of the budgets file, primarily by assuming nothing about the type when it comes into `budget.js` so that we can get a little more verification by the compiler that we're checking the types well.

There are also a few places where the type checks have been tightened and some more tests have been added.

Even though this now returns a totally new budget object instead of just validating in place, it does have the same issue as in #8675 where *in practice* it's just validating in place because it throws on any input that isn't valid. Renaming to reflect that as suggested in https://github.com/GoogleChrome/lighthouse/pull/8675#discussion_r279401765 makes a lot of sense to me, though not creating the object on the fly may reduce tsc's ability to verify the type. We could always create but not return? Or maybe it doesn't matter.

khempenius: LGTM.

Patch:
diff --git a/.travis.yml b/.travis.yml
index eb9afebd5..d99b65ea5 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -26,6 +26,7 @@ before_script:
   # see comment above about puppeteer
   - export CHROME_PATH="$(which google-chrome-stable)"
   - sh -e /etc/init.d/xvfb start
+  # Print out the Chrome version so we know what we're working with
   - google-chrome-stable --version
   - yarn build-all
 script:
diff --git a/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js b/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
index 0d4738055..79322f4d0 100644
--- a/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
+++ b/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
@@ -152,11 +152,12 @@ class CacheHeaders extends Audit {
       NetworkRequest.TYPES.Stylesheet,
     ]);
 
-    const resourceUrl = record.url;
+    // It's not a request loaded over the network, caching makes no sense
+    if (URL.NON_NETWORK_PROTOCOLS.includes(record.protocol)) return false;
+
     return (
       CACHEABLE_STATUS_CODES.has(record.statusCode) &&
-      STATIC_RESOURCE_TYPES.has(record.resourceType || 'Other') &&
-      !resourceUrl.includes('data:')
+      STATIC_RESOURCE_TYPES.has(record.resourceType || 'Other')
     );
   }
 
diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js
index f6460c28e..32a219b4a 100644
--- a/lighthouse-core/audits/font-display.js
+++ b/lighthouse-core/audits/font-display.js
@@ -116,6 +116,7 @@ class FontDisplay extends Audit {
       .filter(record => !passingFontURLs.has(record.url))
       // ...and that aren't data URLs, the blocking concern doesn't really apply
       .filter(record => !/^data:/.test(record.url))
+      .filter(record => !/^blob:/.test(record.url))
       .map(record => {
         // In reality the end time should be calculated with paint time included
         // all browsers wait 3000ms to block text so we make sure 3000 is our max wasted time
diff --git a/lighthouse-core/config/budget.js b/lighthouse-core/config/budget.js
index 820dda302..e788e8f49 100644
--- a/lighthouse-core/config/budget.js
+++ b/lighthouse-core/config/budget.js
@@ -5,10 +5,36 @@
  */
 'use strict';
 
+/**
+ * @param {unknown} arr
+ * @return {arr is Array<Record<string, unknown>>}
+ */
+function isArrayOfUnknownObjects(arr) {
+  return Array.isArray(arr) && arr.every(isObjectOfUnknownProperties);
+}
+
+/**
+ * @param {unknown} val
+ * @return {val is Record<string, unknown>}
+ */
+function isObjectOfUnknownProperties(val) {
+  return typeof val === 'object' && val !== null && !Array.isArray(val);
+}
+
+/**
+ * Returns whether `val` is numeric. Will not coerce to a number. `NaN` will
+ * return false, however ±Infinity will return true.
+ * @param {unknown} val
+ * @return {val is number}
+ */
+function isNumber(val) {
+  return typeof val === 'number' && !isNaN(val);
+}
+
 class Budget {
   /**
    * Asserts that obj has no own properties, throwing a nice error message if it does.
-   * Plugin and object name are included for nicer logging.
+   * `objectName` is included for nicer logging.
    * @param {Record<string, unknown>} obj
    * @param {string} objectName
    */
@@ -21,13 +47,14 @@ class Budget {
   }
 
   /**
-   * @param {LH.Budget.ResourceBudget} resourceBudget
+   * @param {Record<string, unknown>} resourceBudget
    * @return {LH.Budget.ResourceBudget}
    */
   static validateResourceBudget(resourceBudget) {
     const {resourceType, budget, ...invalidRest} = resourceBudget;
     Budget.assertNoExcessProperties(invalidRest, 'Resource Budget');
 
+    /** @type {Array<LH.Budget.ResourceType>} */
     const validResourceTypes = [
       'total',
       'document',
@@ -39,27 +66,29 @@ class Budget {
       'other',
       'third-party',
     ];
-    if (!validResourceTypes.includes(resourceBudget.resourceType)) {
-      throw new Error(`Invalid resource type: ${resourceBudget.resourceType}. \n` +
+    // Assume resourceType is an allowed string, throw if not.
+    if (!validResourceTypes.includes(/** @type {LH.Budget.ResourceType} */ (resourceType))) {
+      throw new Error(`Invalid resource type: ${resourceType}. \n` +
         `Valid resource types are: ${ validResourceTypes.join(', ') }`);
     }
-    if (isNaN(resourceBudget.budget)) {
-      throw new Error('Invalid budget: ${resourceBudget.budget}');
+    if (!isNumber(budget)) {
+      throw new Error(`Invalid budget: ${budget}`);
     }
     return {
-      resourceType,
+      resourceType: /** @type {LH.Budget.ResourceType} */ (resourceType),
       budget,
     };
   }
 
   /**
-   * @param {LH.Budget.TimingBudget} timingBudget
+   * @param {Record<string, unknown>} timingBudget
    * @return {LH.Budget.TimingBudget}
    */
   static validateTimingBudget(timingBudget) {
     const {metric, budget, tolerance, ...invalidRest} = timingBudget;
     Budget.assertNoExcessProperties(invalidRest, 'Timing Budget');
 
+    /** @type {Array<LH.Budget.TimingMetric>} */
     const validTimingMetrics = [
       'first-contentful-paint',
       'first-cpu-idle',
@@ -67,18 +96,19 @@ class Budget {
       'first-meaningful-paint',
       'estimated-input-latency',
     ];
-    if (!validTimingMetrics.includes(timingBudget.metric)) {
-      throw new Error(`Invalid timing metric: ${timingBudget.metric}. \n` +
+    // Assume metric is an allowed string, throw if not.
+    if (!validTimingMetrics.includes(/** @type {LH.Budget.TimingMetric} */ (metric))) {
+      throw new Error(`Invalid timing metric: ${metric}. \n` +
         `Valid timing metrics are: ${validTimingMetrics.join(', ')}`);
     }
-    if (isNaN(timingBudget.budget)) {
-      throw new Error('Invalid budget: ${timingBudget.budget}');
+    if (!isNumber(budget)) {
+      throw new Error(`Invalid budget: ${budget}`);
     }
-    if (timingBudget.tolerance !== undefined && isNaN(timingBudget.tolerance)) {
-      throw new Error('Invalid tolerance: ${timingBudget.tolerance}');
+    if (typeof tolerance !== 'undefined' && !isNumber(tolerance)) {
+      throw new Error(`Invalid tolerance: ${tolerance}`);
     }
     return {
-      metric,
+      metric: /** @type {LH.Budget.TimingMetric} */ (metric),
       budget,
       tolerance,
     };
@@ -87,43 +117,44 @@ class Budget {
   /**
    * More info on the Budget format:
    * https://github.com/GoogleChrome/lighthouse/issues/6053#issuecomment-428385930
-   * @param {Array<LH.Budget>} budgetArr
+   * @param {unknown} budgetJson
    * @return {Array<LH.Budget>}
    */
-  static initializeBudget(budgetArr) {
-    /** @type {Array<LH.Budget>} */
-    const budgets = [];
+  static initializeBudget(budgetJson) {
+    // Clone to prevent modifications of original and to deactivate any live properties.
+    budgetJson = JSON.parse(JSON.stringify(budgetJson));
+    if (!isArrayOfUnknownObjects(budgetJson)) {
+      throw new Error('Budget file is not defined as an array of budgets.');
+    }
 
-    budgetArr.forEach((b) => {
+    const budgets = budgetJson.map((b, index) => {
       /** @type {LH.Budget} */
       const budget = {};
 
       const {resourceSizes, resourceCounts, timings, ...invalidRest} = b;
       Budget.assertNoExcessProperties(invalidRest, 'Budget');
 
-      if (b.resourceSizes !== undefined) {
-        budget.resourceSizes = b.resourceSizes.map((r) => {
-          return Budget.validateResourceBudget(r);
-        });
+      if (isArrayOfUnknownObjects(resourceSizes)) {
+        budget.resourceSizes = resourceSizes.map(Budget.validateResourceBudget);
+      } else if (resourceSizes !== undefined) {
+        throw new Error(`Invalid resourceSizes entry in budget at index ${index}`);
       }
 
-      if (b.resourceCounts !== undefined) {
-        budget.resourceCounts = b.resourceCounts.map((r) => {
-          return Budget.validateResourceBudget(r);
-        });
+      if (isArrayOfUnknownObjects(resourceCounts)) {
+        budget.resourceCounts = resourceCounts.map(Budget.validateResourceBudget);
+      } else if (resourceCounts !== undefined) {
+        throw new Error(`Invalid resourceCounts entry in budget at index ${index}`);
       }
 
-      if (b.timings !== undefined) {
-        budget.timings = b.timings.map((t) => {
-          return Budget.validateTimingBudget(t);
-        });
+      if (isArrayOfUnknownObjects(timings)) {
+        budget.timings = timings.map(Budget.validateTimingBudget);
+      } else if (timings !== undefined) {
+        throw new Error(`Invalid timings entry in budget at index ${index}`);
       }
-      budgets.push({
-        resourceSizes,
-        resourceCounts,
-        timings,
-      });
+
+      return budget;
     });
+
     return budgets;
   }
 }
diff --git a/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js b/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
index d536ffefb..876b27945 100644
--- a/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
+++ b/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
@@ -56,6 +56,7 @@ function collectTagsThatBlockFirstPaint() {
               !scriptTag.hasAttribute('async') &&
               !scriptTag.hasAttribute('defer') &&
               !/^data:/.test(scriptTag.src) &&
+              !/^blob:/.test(scriptTag.src) &&
               scriptTag.getAttribute('type') !== 'module'
             );
           } else if (tag.tagName === 'LINK') {
diff --git a/proto/sample_v2_round_trip.json b/proto/sample_v2_round_trip.json
index 3212277ce..5c36f363d 100644
--- a/proto/sample_v2_round_trip.json
+++ b/proto/sample_v2_round_trip.json
@@ -2659,13 +2659,6 @@
                         "totalBytes": 144.0, 
                         "url": "http://localhost:10200/dobetterweb/empty_module.js?delay=500", 
                         "wastedBytes": 144.0
-                    }, 
-                    {
-                        "cacheHitProbability": 0.0, 
-                        "cacheLifetimeMs": 0.0, 
-                        "totalBytes": 0.0, 
-                        "url": "blob:http://localhost:10200/ae0eac03-ab9b-4a6a-b299-f5212153e277", 
-                        "wastedBytes": 0.0
                     }
                 ], 
                 "summary": {
@@ -2673,7 +2666,7 @@
                 }, 
                 "type": "table"
             }, 
-            "displayValue": "11 resources found", 
+            "displayValue": "10 resources found", 
             "id": "uses-long-cache-ttl", 
             "numericValue": 103455.0, 
             "score": 0.58, 


Test patch:
diff --git a/lighthouse-cli/test/smokehouse/perf/lantern-expectations.js b/lighthouse-cli/test/smokehouse/perf/lantern-expectations.js
index 5186beb4b..49a768160 100644
--- a/lighthouse-cli/test/smokehouse/perf/lantern-expectations.js
+++ b/lighthouse-cli/test/smokehouse/perf/lantern-expectations.js
@@ -26,4 +26,50 @@ module.exports = [
       },
     },
   },
+  {
+    lhr: {
+      requestedUrl: 'http://localhost:10200/tricky-main-thread.html?setTimeout',
+      finalUrl: 'http://localhost:10200/tricky-main-thread.html?setTimeout',
+      audits: {
+        'interactive': {
+          // Make sure all of the CPU time is reflected in the perf metrics as well.
+          score: '<.2',
+        },
+        'bootup-time': {
+          details: {
+            items: {
+              0: {
+              // FIXME: Appveyor finds this particular assertion very flaky for some reason :(
+                url: process.env.APPVEYOR ? /main/ : /main-thread-consumer/,
+                scripting: '>1000',
+              },
+            },
+          },
+        },
+      },
+    },
+  },
+  {
+    lhr: {
+      requestedUrl: 'http://localhost:10200/tricky-main-thread.html?fetch',
+      finalUrl: 'http://localhost:10200/tricky-main-thread.html?fetch',
+      audits: {
+        'interactive': {
+          // Make sure all of the CPU time is reflected in the perf metrics as well.
+          score: '<.2',
+        },
+        'bootup-time': {
+          details: {
+            items: {
+              0: {
+              // TODO: requires sampling profiler and async stacks, see https://github.com/GoogleChrome/lighthouse/issues/8526
+              // url: /main-thread-consumer/,
+                scripting: '>1000',
+              },
+            },
+          },
+        },
+      },
+    },
+  },
 ];
diff --git a/lighthouse-cli/test/smokehouse/tricky-metrics/expectations.js b/lighthouse-cli/test/smokehouse/tricky-metrics/expectations.js
index b7050ef4a..c94244363 100644
--- a/lighthouse-cli/test/smokehouse/tricky-metrics/expectations.js
+++ b/lighthouse-cli/test/smokehouse/tricky-metrics/expectations.js
@@ -36,42 +36,4 @@ module.exports = [
       },
     },
   },
-  {
-    lhr: {
-      requestedUrl: 'http://localhost:10200/tricky-main-thread.html?setTimeout',
-      finalUrl: 'http://localhost:10200/tricky-main-thread.html?setTimeout',
-      audits: {
-        'bootup-time': {
-          details: {
-            items: {
-              0: {
-              // FIXME: Appveyor finds this particular assertion very flaky for some reason :(
-                url: process.env.APPVEYOR ? /main/ : /main-thread-consumer/,
-                scripting: '>1000',
-              },
-            },
-          },
-        },
-      },
-    },
-  },
-  {
-    lhr: {
-      requestedUrl: 'http://localhost:10200/tricky-main-thread.html?fetch',
-      finalUrl: 'http://localhost:10200/tricky-main-thread.html?fetch',
-      audits: {
-        'bootup-time': {
-          details: {
-            items: {
-              0: {
-              // TODO: requires async stacks, https://github.com/GoogleChrome/lighthouse/pull/5504
-              // url: /main-thread-consumer/,
-                scripting: '>1000',
-              },
-            },
-          },
-        },
-      },
-    },
-  },
 ];
diff --git a/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js b/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
index aba0f68d0..4a3787183 100644
--- a/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
+++ b/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
@@ -24,6 +24,7 @@ function networkRecord(options = {}) {
     statusCode: options.statusCode || 200,
     resourceType: options.resourceType || NetworkRequest.TYPES.Script,
     transferSize: options.transferSize || 10000,
+    protocol: options.protocol || 'http/1.1',
     responseHeaders: headers,
   };
 }
@@ -207,7 +208,8 @@ describe('Cache headers audit', () => {
     const networkRecords = [
       networkRecord({statusCode: 500}),
       networkRecord({url: 'https://example.com/dynamic.js?userId=crazy', transferSize: 10}),
-      networkRecord({url: 'data:image/jpeg;base64,what'}),
+      networkRecord({url: 'data:image/jpeg;base64,what', protocol: 'data'}),
+      networkRecord({url: 'blob:http://example.com/ca6df701-9c67-49fd-a787', protocol: 'blob'}),
       networkRecord({resourceType: NetworkRequest.TYPES.XHR}),
     ];
 
diff --git a/lighthouse-core/test/config/budget-test.js b/lighthouse-core/test/config/budget-test.js
index 7a56d587b..19de4fb00 100644
--- a/lighthouse-core/test/config/budget-test.js
+++ b/lighthouse-core/test/config/budget-test.js
@@ -43,7 +43,7 @@ describe('Budget', () => {
           {
             metric: 'first-contentful-paint',
             budget: 1000,
-            tolerance: 500,
+            // tolerance is optional, so not defined here.
           },
         ],
       },
@@ -74,55 +74,103 @@ describe('Budget', () => {
 
     // Sets timings correctly
     assert.equal(budgets[0].timings.length, 2);
+    assert.deepStrictEqual(budgets[0].timings[0], {
+      metric: 'interactive',
+      budget: 2000,
+      tolerance: 1000,
+    });
     assert.equal(budgets[0].timings[1].metric, 'first-contentful-paint');
     assert.equal(budgets[0].timings[1].budget, 1000);
-    assert.equal(budgets[0].timings[1].tolerance, 500);
 
     // Does not set unsupplied budgets
     assert.equal(budgets[1].timings, null);
   });
 
+  it('accepts an empty array', () => {
+    const budgets = Budget.initializeBudget([]);
+    assert.deepStrictEqual(budgets, []);
+  });
+
   it('throws error if an unsupported budget property is used', () => {
     budget[0].sizes = [];
-    assert.throws(_ => Budget.initializeBudget(budget), /[sizes]/);
+    assert.throws(_ => Budget.initializeBudget(budget),
+      /Budget has unrecognized properties: \[sizes\]/);
+  });
+
+  describe('top-level validation', () => {
+    it('throws when provided an invalid budget array', () => {
+      assert.throws(_ => Budget.initializeBudget(55),
+        /Budget file is not defined as an array of budgets/);
+
+      assert.throws(_ => Budget.initializeBudget(['invalid123']),
+        /Budget file is not defined as an array of budgets/);
+
+      assert.throws(_ => Budget.initializeBudget([null]),
+        /Budget file is not defined as an array of budgets/);
+    });
+
+    it('throws when budget contains invalid resourceSizes entry', () => {
+      budget[0].resourceSizes = 55;
+      assert.throws(_ => Budget.initializeBudget(budget),
+        /^Error: Invalid resourceSizes entry in budget at index 0$/);
+    });
+
+    it('throws when budget contains invalid resourceCounts entry', () => {
+      budget[0].resourceCounts = 'A string';
+      assert.throws(_ => Budget.initializeBudget(budget),
+        /^Error: Invalid resourceCounts entry in budget at index 0$/);
+    });
+
+    it('throws when budget contains invalid timings entry', () => {
+      budget[1].timings = false;
+      assert.throws(_ => Budget.initializeBudget(budget),
+        /^Error: Invalid timings entry in budget at index 1$/);
+    });
   });
 
   describe('resource budget validation', () => {
     it('throws when an invalid resource type is supplied', () => {
       budget[0].resourceSizes[0].resourceType = 'movies';
-      assert.throws(_ => Budget.initializeBudget(budget), /Invalid resource type/);
+      assert.throws(_ => Budget.initializeBudget(budget),
+        // eslint-disable-next-line max-len
+        /Invalid resource type: movies. \nValid resource types are: total, document,/);
     });
 
     it('throws when an invalid budget is supplied', () => {
       budget[0].resourceSizes[0].budget = '100 MB';
-      assert.throws(_ => Budget.initializeBudget(budget), /Invalid budget/);
+      assert.throws(_ => Budget.initializeBudget(budget), /Invalid budget: 100 MB/);
     });
 
     it('throws when an invalid property is supplied', () => {
       budget[0].resourceSizes[0].browser = 'Chrome';
-      assert.throws(_ => Budget.initializeBudget(budget), /[browser]/);
+      assert.throws(_ => Budget.initializeBudget(budget),
+        /Resource Budget has unrecognized properties: \[browser\]/);
     });
   });
 
   describe('timing budget validation', () => {
     it('throws when an invalid metric is supplied', () => {
-      budget[0].timings[0].metric = 'lastMeaningfulPaint';
-      assert.throws(_ => Budget.initializeBudget(budget), /Invalid timing metric/);
+      budget[0].timings[0].metric = 'medianMeaningfulPaint';
+      assert.throws(_ => Budget.initializeBudget(budget),
+        // eslint-disable-next-line max-len
+        /Invalid timing metric: medianMeaningfulPaint. \nValid timing metrics are: first-contentful-paint, /);
     });
 
     it('throws when an invalid budget is supplied', () => {
       budget[0].timings[0].budget = '100KB';
-      assert.throws(_ => Budget.initializeBudget(budget), /Invalid budget/);
+      assert.throws(_ => Budget.initializeBudget(budget), /Invalid budget: 100KB/);
     });
 
     it('throws when an invalid tolerance is supplied', () => {
       budget[0].timings[0].tolerance = '100ms';
-      assert.throws(_ => Budget.initializeBudget(budget), /Invalid tolerance/);
+      assert.throws(_ => Budget.initializeBudget(budget), /Invalid tolerance: 100ms/);
     });
 
     it('throws when an invalid property is supplied', () => {
       budget[0].timings[0].device = 'Phone';
-      assert.throws(_ => Budget.initializeBudget(budget), /[device]/);
+      budget[0].timings[0].location = 'The middle somewhere, I don\'t know';
+      assert.throws(_ => Budget.initializeBudget(budget),
+        /Timing Budget has unrecognized properties: \[device, location\]/);
     });
   });
 });
diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js
index c3c29045b..4bbb22ea0 100644
--- a/lighthouse-core/test/config/config-test.js
+++ b/lighthouse-core/test/config/config-test.js
@@ -761,7 +761,7 @@ describe('Config', () => {
 
       it('should throw when provided an invalid budget', () => {
         assert.throws(() => new Config({settings: {budgets: ['invalid123']}}),
-          /Budget has unrecognized properties/);
+          /Budget file is not defined as an array of budgets/);
       });
     });
 
diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
index 3a83a0721..f96b06073 100644
--- a/lighthouse-core/test/results/sample_v2.json
+++ b/lighthouse-core/test/results/sample_v2.json
@@ -1910,7 +1910,7 @@
       "score": 0.58,
       "scoreDisplayMode": "numeric",
       "numericValue": 103455,
-      "displayValue": "11 resources found",
+      "displayValue": "10 resources found",
       "details": {
         "type": "table",
         "headings": [
@@ -2003,13 +2003,6 @@
             "cacheHitProbability": 0,
             "totalBytes": 144,
             "wastedBytes": 144
-          },
-          {
-            "url": "blob:http://localhost:10200/ae0eac03-ab9b-4a6a-b299-f5212153e277",
-            "cacheLifetimeMs": 0,
-            "cacheHitProbability": 0,
-            "totalBytes": 0,
-            "wastedBytes": 0
           }
         ],
         "summary": {
@@ -5093,7 +5086,7 @@
       "lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js | displayValue": [
         {
           "values": {
-            "itemCount": 11
+            "itemCount": 10
           },
           "path": "audits[uses-long-cache-ttl].displayValue"
         }