Repository: GoogleChrome/lighthouse
Pull request #6099 (2018-09-24 18:48:28)

core: replace WebInspector traceparser with native JSON.parse
possibly more controversial than #6090 and #6091

The reason we [introduced a streaming trace parser](https://github.com/GoogleChrome/lighthouse/pull/2593) was because we would get `RangeError: Invalid string length` with [traces that were larger than 256MB](https://github.com/GoogleChrome/lighthouse/issues/1685#issuecomment-279115446), the previous maximum string size in V8.

This changed in V8 6.2, which increased the max size to 1GB. This landed in [Node 8.10.0](https://nodejs.org/en/blog/release/v8.10.0/#header-notable-changes) in March, which is after the original LTS release of 8.9. However:
- the LTS branch expects users to keep up with MINOR updates. [8.9.4](https://nodejs.org/en/blog/release/v8.9.4/) was the last release in the 8.9.x branch in January, and major security updates have happened in [8.11.0](https://nodejs.org/en/blog/release/v8.11.0/), [8.11.3](https://nodejs.org/en/blog/release/v8.11.3/), [8.11.4](https://nodejs.org/en/blog/release/v8.11.4/)
- trace parser is now only used for `-A` mode after #6056
- since 3.0, our traces have been significantly smaller (e.g. a full time-out verge run is more like 150MB)

It thus seems reasonable to expect at least Node 8.10 to run Lighthouse if we're supporting at least the LTS branch. I've updated the `package.json` `engines` entry to give a warning on install in `npm` (not sure what the yarn `engines` behavior is these days).

brendankenny: removes another 6KB off our gzipped bundle (508.62KB -> 502.56KB) since we can remove some of the other `common/` and `sdk/` files in addition to the parser itself.
brendankenny: oh and
- it's faster to just `JSON.parse` the string. Loading that 150MB trace takes about 2760ms on master for me, 1618ms after this PR

Patch:
diff --git a/lighthouse-core/lib/asset-saver.js b/lighthouse-core/lib/asset-saver.js
index d3ae67d07..ceda847fd 100644
--- a/lighthouse-core/lib/asset-saver.js
+++ b/lighthouse-core/lib/asset-saver.js
@@ -12,7 +12,6 @@ const stream = require('stream');
 const Simulator = require('./dependency-graph/simulator/simulator');
 const lanternTraceSaver = require('./lantern-trace-saver');
 const Metrics = require('./traces/pwmetrics-events');
-const TraceParser = require('./traces/trace-parser');
 const rimraf = require('rimraf');
 const mkdirp = require('mkdirp');
 
@@ -94,7 +93,7 @@ async function loadArtifacts(basePath) {
 
   // load devtoolsLogs
   artifacts.devtoolsLogs = {};
-  filenames.filter(f => f.endsWith(devtoolsLogSuffix)).map(filename => {
+  filenames.filter(f => f.endsWith(devtoolsLogSuffix)).forEach(filename => {
     const passName = filename.replace(devtoolsLogSuffix, '');
     const devtoolsLog = JSON.parse(fs.readFileSync(path.join(basePath, filename), 'utf8'));
     artifacts.devtoolsLogs[passName] = devtoolsLog;
@@ -102,22 +101,12 @@ async function loadArtifacts(basePath) {
 
   // load traces
   artifacts.traces = {};
-  const promises = filenames.filter(f => f.endsWith(traceSuffix)).map(filename => {
-    return new Promise(resolve => {
-      const passName = filename.replace(traceSuffix, '');
-      const readStream = fs.createReadStream(path.join(basePath, filename), {
-        encoding: 'utf-8',
-        highWaterMark: 4 * 1024 * 1024, // TODO benchmark to find the best buffer size here
-      });
-      const parser = new TraceParser();
-      readStream.on('data', chunk => parser.parseChunk(chunk));
-      readStream.on('end', _ => {
-        artifacts.traces[passName] = parser.getTrace();
-        resolve();
-      });
-    });
+  filenames.filter(f => f.endsWith(traceSuffix)).forEach(filename => {
+    const file = fs.readFileSync(path.join(basePath, filename), {encoding: 'utf-8'});
+    const trace = JSON.parse(file);
+    const passName = filename.replace(traceSuffix, '');
+    artifacts.traces[passName] = Array.isArray(trace) ? {traceEvents: trace} : trace;
   });
-  await Promise.all(promises);
 
   return artifacts;
 }
diff --git a/lighthouse-core/lib/traces/trace-parser.js b/lighthouse-core/lib/traces/trace-parser.js
deleted file mode 100644
index d5a3f0dfd..000000000
--- a/lighthouse-core/lib/traces/trace-parser.js
+++ /dev/null
@@ -1,74 +0,0 @@
-/**
- * @license Copyright 2017 Google Inc. All Rights Reserved.
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
- * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
- */
-'use strict';
-
-const WebInspector = require('../web-inspector');
-
-/**
- * Traces > 256MB hit limits in V8, so TraceParser will parse the trace events stream as it's
- * received. We use DevTools' TimelineLoader for the heavy lifting, as it has a fast trace-specific
- * streaming JSON parser.
- * The resulting trace doesn't include the "metadata" property, as it's excluded via DevTools'
- * implementation.
- * FIXME: This can be removed once Node 8 support is dropped. https://stackoverflow.com/a/47781288/89484
- */
-class TraceParser {
-  constructor() {
-    /** @type {Array<LH.TraceEvent>} */
-    this.traceEvents = [];
-
-    this.tracingModel = {
-      reset: () => this._reset(),
-      /** @param {Array<LH.TraceEvent>} evts */
-      addEvents: evts => this._addEvents(evts),
-    };
-
-    const delegateMock = {
-      loadingProgress: () => {},
-      loadingStarted: () => {},
-      /** @param {boolean} success */
-      loadingComplete: success => {
-        if (!success) throw new Error('Parsing problem');
-      },
-    };
-    this.loader = new WebInspector.TimelineLoader(this.tracingModel, delegateMock);
-  }
-
-  /**
-   * Reset the trace events array
-   */
-  _reset() {
-    this.traceEvents = [];
-  }
-
-  /**
-   * Adds parsed trace events to array
-   * @param {Array<LH.TraceEvent>} evts
-   */
-  _addEvents(evts) {
-    this.traceEvents.push(...evts);
-  }
-
-  /**
-   * Receive chunk of streamed trace
-   * @param {string} data
-   */
-  parseChunk(data) {
-    this.loader.write(data);
-  }
-
-  /**
-   * Returns entire trace
-   * @return {{traceEvents: Array<LH.TraceEvent>}}
-   */
-  getTrace() {
-    return {
-      traceEvents: this.traceEvents,
-    };
-  }
-}
-
-module.exports = TraceParser;
diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js
index 6d7594d7c..b9817cf58 100644
--- a/lighthouse-core/lib/web-inspector.js
+++ b/lighthouse-core/lib/web-inspector.js
@@ -21,10 +21,6 @@ module.exports = (function() {
     global.self = global;
   }
 
-  if (typeof global.window === 'undefined') {
-    global.window = global;
-  }
-
   global.Node = {
     ELEMENT_NODE: 1,
     TEXT_NODE: 3,
@@ -45,28 +41,11 @@ module.exports = (function() {
   // See https://github.com/GoogleChrome/lighthouse/issues/73
   const _setImmediate = global.setImmediate;
 
-  global.Protocol = {
-    Agents() {},
-  };
-
   global.WebInspector = {};
   const WebInspector = global.WebInspector;
 
   // Shared Dependencies
-  require('chrome-devtools-frontend/front_end/common/Object.js');
-  require('chrome-devtools-frontend/front_end/common/UIString.js');
   require('chrome-devtools-frontend/front_end/platform/utilities.js');
-  require('chrome-devtools-frontend/front_end/sdk/Target.js');
-  require('chrome-devtools-frontend/front_end/sdk/TargetManager.js');
-
-  // Dependencies for timeline-model
-  WebInspector.console = {
-    error() {},
-  };
-
-  // used for streaming json parsing
-  require('chrome-devtools-frontend/front_end/common/TextUtils.js');
-  require('chrome-devtools-frontend/front_end/timeline/TimelineLoader.js');
 
   // Dependencies for effective CSS rule calculation.
   require('chrome-devtools-frontend/front_end/common/TextRange.js');
diff --git a/lighthouse-extension/app/src/lighthouse-ext-background.js b/lighthouse-extension/app/src/lighthouse-ext-background.js
index fcb92a09a..9041b9ae1 100644
--- a/lighthouse-extension/app/src/lighthouse-ext-background.js
+++ b/lighthouse-extension/app/src/lighthouse-ext-background.js
@@ -231,8 +231,8 @@ function isRunning() {
   return lighthouseIsRunning;
 }
 
-// Run when in extension context, but not in devtools.
-if ('chrome' in window && chrome.runtime) {
+// Run when in extension context, but not in devtools or unit tests.
+if (typeof window !== 'undefined' && 'chrome' in window && chrome.runtime) {
   chrome.runtime.onInstalled.addListener(details => {
     if (details.previousVersion) {
       // eslint-disable-next-line no-console
diff --git a/package.json b/package.json
index a97a99ce9..76574c69c 100644
--- a/package.json
+++ b/package.json
@@ -8,7 +8,7 @@
     "chrome-debug": "./lighthouse-core/scripts/manual-chrome-launcher.js"
   },
   "engines": {
-    "node": ">=8.9"
+    "node": ">=8.10"
   },
   "scripts": {
     "install-all": "npm-run-posix-or-windows install-all:task",


Test patch:
diff --git a/lighthouse-core/test/lib/asset-saver-test.js b/lighthouse-core/test/lib/asset-saver-test.js
index 014ec4490..92242f4a2 100644
--- a/lighthouse-core/test/lib/asset-saver-test.js
+++ b/lighthouse-core/test/lib/asset-saver-test.js
@@ -170,4 +170,15 @@ describe('asset-saver helper', () => {
         });
     }, 40 * 1000);
   });
+
+  describe('loadArtifacts', () => {
+    it('loads artifacts from disk', async () => {
+      const artifactsPath = __dirname + '/../fixtures/artifacts/perflog/';
+      const artifacts = await assetSaver.loadArtifacts(artifactsPath);
+      assert.strictEqual(artifacts.LighthouseRunWarnings.length, 2);
+      assert.strictEqual(artifacts.URL.requestedUrl, 'https://www.reddit.com/r/nba');
+      assert.strictEqual(artifacts.devtoolsLogs.defaultPass.length, 555);
+      assert.strictEqual(artifacts.traces.defaultPass.traceEvents.length, 12);
+    });
+  });
 });
diff --git a/lighthouse-core/test/lib/traces/trace-parser-test.js b/lighthouse-core/test/lib/traces/trace-parser-test.js
deleted file mode 100644
index 4e770e4b2..000000000
--- a/lighthouse-core/test/lib/traces/trace-parser-test.js
+++ /dev/null
@@ -1,93 +0,0 @@
-/**
- * @license Copyright 2017 Google Inc. All Rights Reserved.
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
- * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
- */
-'use strict';
-
-const TraceParser = require('../../../lib/traces/trace-parser');
-const fs = require('fs');
-const assert = require('assert');
-
-
-/* eslint-env jest */
-describe('traceParser parser', () => {
-  it('returns preact trace data the same as JSON.parse', (done) => {
-    const filename = `${__dirname}/../../fixtures/traces/progressive-app-m60.json`;
-    const readStream = fs.createReadStream(filename, {
-      encoding: 'utf-8',
-      // devtools sends traces in 10mb chunks, but this trace is 12MB so we'll do a few chunks
-      highWaterMark: 4 * 1024 * 1024,
-    });
-    const parser = new TraceParser();
-
-    readStream.on('data', (chunk) => {
-      parser.parseChunk(chunk);
-    });
-    readStream.on('end', () => {
-      const streamedTrace = parser.getTrace();
-      const readTrace = JSON.parse(fs.readFileSync(filename));
-
-      assert.equal(streamedTrace.traceEvents.length, readTrace.traceEvents.length);
-      streamedTrace.traceEvents.forEach((evt, i) => {
-        assert.deepStrictEqual(evt, readTrace.traceEvents[i]);
-      });
-
-      done();
-    });
-  }, 10000);
-
-
-  it('parses a trace > 256mb (slow)', () => {
-    const parser = new TraceParser();
-    let bytesRead = 0;
-    // FYI: this trace doesn't have a traceEvents property ;)
-    const filename = '/../../fixtures/traces/devtools-homepage-w-screenshots-trace.json';
-    const events = JSON.parse(fs.readFileSync(__dirname + filename));
-
-    /**
-     * This function will synthesize a trace that's over 256 MB. To do that, we'll take an existing
-     * trace and repeat the same events again and again until we've gone over 256 MB.
-     * Note: We repeat all but the last event, as it's the CpuProfile event, and it triggers
-     * specific handling in the devtools streaming parser.
-     * Once we reach > 256 MB, we add in the CpuProfile event.
-     */
-    function buildAndParse256mbTrace() {
-      const stripOuterBrackets = str => str.replace(/^\[/, '').replace(/\]$/, '');
-      const partialEventsStr = events => stripOuterBrackets(JSON.stringify(events));
-      const traceEventsStr = partialEventsStr(events.slice(0, events.length-2)) + ',';
-
-      // read the trace intro
-      parser.parseChunk(`{"traceEvents": [${traceEventsStr}`);
-      bytesRead += traceEventsStr.length;
-
-      // just keep reading until we've gone over 256 MB
-      // 256 MB is hard limit of a string in v8
-      // https://mobile.twitter.com/bmeurer/status/879276976523157505
-      while (bytesRead <= (Math.pow(2, 28)) - 16) {
-        parser.parseChunk(traceEventsStr);
-        bytesRead += traceEventsStr.length;
-      }
-
-      // the CPU Profiler event is last (and big), inject it just once
-      const lastEventStr = partialEventsStr(events.slice(-1));
-      parser.parseChunk(lastEventStr + ']}');
-      bytesRead += lastEventStr.length;
-    }
-
-    buildAndParse256mbTrace();
-    const streamedTrace = parser.getTrace();
-
-    assert.ok(bytesRead > 256 * 1024 * 1024, `${bytesRead} bytes read`);
-    assert.strictEqual(bytesRead, 270128965, `${bytesRead} bytes read`);
-
-    // if > 256 MB are read we should have ~480,000 trace events
-    assert.ok(streamedTrace.traceEvents.length > 400 * 1000, 'not >400,000 trace events');
-    assert.ok(streamedTrace.traceEvents.length > events.length * 5, 'not way more trace events');
-    assert.strictEqual(streamedTrace.traceEvents.length, 480151);
-
-    assert.deepStrictEqual(
-        streamedTrace.traceEvents[events.length - 2],
-        events[0]);
-  }, 40 * 1000);
-});