Do not throw an error when browsertime provides null timestamps incor…

…rectly (Merge PR #4399)

src/components/timeline/TrackVisualProgressGraph.js

@@ -102,7 +102,7 @@ class TrackVisualProgressCanvas extends React.PureComponent<CanvasProps> {
         // Create a path for the top of the chart. This is the line that will have
         // a stroke applied to it.
         x =
-          (deviceWidth * (progressGraphData[i].timestamp - rangeStart)) /
+          (deviceWidth * ((progressGraphData[i].timestamp ?? 0) - rangeStart)) /
           rangeLength;
         // Add on half the stroke's line width so that it won't be cut off the edge
         // of the graph.
@@ -133,7 +133,7 @@ class TrackVisualProgressCanvas extends React.PureComponent<CanvasProps> {
       // of the canvas.
       ctx.lineTo(x + interval, deviceHeight);
       ctx.lineTo(
-        (deviceWidth * (progressGraphData[0].timestamp - rangeStart)) /
+        (deviceWidth * ((progressGraphData[0].timestamp ?? 0) - rangeStart)) /
           rangeLength +
           interval,
         deviceHeight
@@ -221,15 +221,16 @@ class TrackVisualProgressGraphImpl extends React.PureComponent<Props, State> {
     const rangeLength = rangeEnd - rangeStart;
     const timeAtMouse = rangeStart + ((mouseX - left) / width) * rangeLength;
     if (
-      timeAtMouse < progressGraphData[0].timestamp ||
+      timeAtMouse < (progressGraphData[0].timestamp ?? 0) ||
       timeAtMouse >
-        progressGraphData[progressGraphData.length - 1].timestamp + interval
+        (progressGraphData[progressGraphData.length - 1].timestamp ?? 0) +
+          interval
     ) {
       // We are outside the range of the samples, do not display hover information.
       this.setState({ hoveredVisualProgress: null });
     } else {
       const graphTimestamps = progressGraphData.map(
-        ({ timestamp }) => timestamp
+        ({ timestamp }) => timestamp ?? 0
       );
       let hoveredVisualProgress = bisectionRight(graphTimestamps, timeAtMouse);
       if (hoveredVisualProgress === progressGraphData.length) {
@@ -277,7 +278,8 @@ class TrackVisualProgressGraphImpl extends React.PureComponent<Props, State> {
     } = this.props;
     const rangeLength = rangeEnd - rangeStart;
     const left =
-      (width * (progressGraphData[graphDataIndex].timestamp - rangeStart)) /
+      (width *
+        ((progressGraphData[graphDataIndex].timestamp ?? 0) - rangeStart)) /
       rangeLength;
 
     const unitSampleCount = progressGraphData[graphDataIndex].percent / 100;

src/profile-logic/process-profile.js

@@ -28,6 +28,7 @@ import { isArtTraceFormat, convertArtTraceProfile } from './import/art-trace';
 import {
   PROCESSED_PROFILE_VERSION,
   INTERVAL,
+  INTERVAL_END,
   INSTANT,
 } from '../app-logic/constants';
 import {
@@ -87,6 +88,7 @@ import type {
   PageList,
   ThreadIndex,
   BrowsertimeMarkerPayload,
+  MarkerPhase,
 } from 'firefox-profiler/types';
 
 type RegExpResult = null | string[];
@@ -1736,18 +1738,30 @@ export function processVisualMetrics(
     (cat) => cat.name === 'Test'
   );
 
-  function addMetricMarker(
+  function maybeAddMetricMarker(
     thread: Thread,
     name: string,
-    startTime: number,
-    endTime?: number,
+    phase: MarkerPhase,
+    startTime: number | null,
+    endTime: number | null,
     payload?: BrowsertimeMarkerPayload
   ) {
+    if (
+      // All phases except INTERVAL_END should have a start time.
+      (phase !== INTERVAL_END && startTime === null) ||
+      // Only INTERVAL and INTERVAL_END should have an end time.
+      ((phase === INTERVAL || phase === INTERVAL_END) && endTime === null)
+    ) {
+      // Do not add if some timestamps we expect are missing.
+      // This should ideally never happen but timestamps could be null due to
+      // browsertime bug here: https://github.com/sitespeedio/browsertime/issues/1746.
+      return;
+    }
     // Add the marker to the given thread.
     thread.markers.name.push(thread.stringTable.indexForString(name));
     thread.markers.startTime.push(startTime);
-    thread.markers.endTime.push(endTime ? endTime : 0);
-    thread.markers.phase.push(endTime ? INTERVAL : INSTANT);
+    thread.markers.endTime.push(endTime);
+    thread.markers.phase.push(phase);
     thread.markers.category.push(testingCategoryIdx);
     thread.markers.data.push(payload ?? null);
     thread.markers.length++;
@@ -1783,9 +1797,9 @@ export function processVisualMetrics(
 
     // Add the progress marker to the parent process main thread.
     const markerName = `${metricName} Progress`;
-    addMetricMarker(mainThread, markerName, startTime, endTime);
+    maybeAddMetricMarker(mainThread, markerName, INTERVAL, startTime, endTime);
     // Add the progress marker to the tab process main thread.
-    addMetricMarker(tabThread, markerName, startTime, endTime);
+    maybeAddMetricMarker(tabThread, markerName, INTERVAL, startTime, endTime);
 
     // Add progress markers for every visual progress change for more fine grained information.
     const progressMarkerSchema = {
@@ -1805,19 +1819,21 @@ export function processVisualMetrics(
       };
 
       // Add it to the parent process main thread.
-      addMetricMarker(
+      maybeAddMetricMarker(
         mainThread,
         changeMarkerName,
+        INSTANT,
         timestamp,
-        undefined, // endTime
+        null, // endTime
         payload
       );
       // Add it to the tab process main thread.
-      addMetricMarker(
+      maybeAddMetricMarker(
         tabThread,
         changeMarkerName,
+        INSTANT,
         timestamp,
-        undefined, // endTime
+        null, // endTime
         payload
       );
     }

src/test/unit/process-profile.test.js

@@ -664,22 +664,28 @@ describe('profile meta processing', function () {
 });
 
 describe('visualMetrics processing', function () {
-  function checkVisualMetricsForThread(thread: Thread) {
-    const metrics = [
-      { name: 'Visual', changeMarkerLength: 7 },
-      { name: 'ContentfulSpeedIndex', changeMarkerLength: 6 },
-      { name: 'PerceptualSpeedIndex', changeMarkerLength: 6 },
-    ];
-
-    for (const { name, changeMarkerLength } of metrics) {
+  function checkVisualMetricsForThread(
+    thread: Thread,
+    metrics: Array<{|
+      name: string,
+      hasProgressMarker: boolean,
+      changeMarkerLength: number,
+    |}>
+  ) {
+    for (const { name, hasProgressMarker, changeMarkerLength } of metrics) {
       // Check the visual metric progress markers.
       const metricProgressMarkerIndex = thread.stringTable.indexForString(
         `${name} Progress`
       );
       const metricProgressMarker = thread.markers.name.find(
         (name) => name === metricProgressMarkerIndex
       );
-      expect(metricProgressMarker).toBeTruthy();
+
+      if (hasProgressMarker) {
+        expect(metricProgressMarker).toBeTruthy();
+      } else {
+        expect(metricProgressMarker).toBeFalsy();
+      }
 
       // Check the visual metric change markers.
       const metricChangeMarkerIndex = thread.stringTable.indexForString(
@@ -712,7 +718,19 @@ describe('visualMetrics processing', function () {
       throw new Error('Could not find the parent process main thread.');
     }
 
-    checkVisualMetricsForThread(parentProcessMainThread);
+    checkVisualMetricsForThread(parentProcessMainThread, [
+      { name: 'Visual', hasProgressMarker: true, changeMarkerLength: 7 },
+      {
+        name: 'ContentfulSpeedIndex',
+        hasProgressMarker: true,
+        changeMarkerLength: 6,
+      },
+      {
+        name: 'PerceptualSpeedIndex',
+        hasProgressMarker: true,
+        changeMarkerLength: 6,
+      },
+    ]);
   });
 
   it('adds markers to the tab process', function () {
@@ -734,6 +752,67 @@ describe('visualMetrics processing', function () {
       throw new Error('Could not find the tab process main thread.');
     }
 
-    checkVisualMetricsForThread(tabProcessMainThread);
+    checkVisualMetricsForThread(tabProcessMainThread, [
+      { name: 'Visual', hasProgressMarker: true, changeMarkerLength: 7 },
+      {
+        name: 'ContentfulSpeedIndex',
+        hasProgressMarker: true,
+        changeMarkerLength: 6,
+      },
+      {
+        name: 'PerceptualSpeedIndex',
+        hasProgressMarker: true,
+        changeMarkerLength: 6,
+      },
+    ]);
+  });
+
+  it('does not add markers to the parent process if metric timestamps are null', function () {
+    const geckoProfile = createGeckoProfile();
+    const visualMetrics = getVisualMetrics();
+
+    // Make all the VisualProgress timestamps null on purpose.
+    visualMetrics.VisualProgress = visualMetrics.VisualProgress.map(
+      (progress) => ({ ...progress, timestamp: null })
+    );
+    // Make only one ContentfulSpeedIndexProgress timestamp null on purpose.
+    ensureExists(visualMetrics.ContentfulSpeedIndexProgress)[0].timestamp =
+      null;
+
+    // Add the visual metrics to the profile.
+    geckoProfile.meta.visualMetrics = visualMetrics;
+    // Make sure that the visual metrics are not changed.
+    expect(visualMetrics.VisualProgress).toHaveLength(7);
+    expect(visualMetrics.ContentfulSpeedIndexProgress).toHaveLength(6);
+    expect(visualMetrics.PerceptualSpeedIndexProgress).toHaveLength(6);
+
+    // Processing the profile.
+    const processedProfile = processGeckoProfile(geckoProfile);
+    const parentProcessMainThread = processedProfile.threads.find(
+      (thread) =>
+        thread.name === 'GeckoMain' && thread.processType === 'default'
+    );
+
+    if (!parentProcessMainThread) {
+      throw new Error('Could not find the parent process main thread.');
+    }
+
+    checkVisualMetricsForThread(parentProcessMainThread, [
+      // Instead of 7, we should have 0 markers for Visual because we made all
+      // the timestamps null.
+      { name: 'Visual', hasProgressMarker: false, changeMarkerLength: 0 },
+      // Instead of 6, we should have 5 markers for ContentfulSpeedIndex.
+      {
+        name: 'ContentfulSpeedIndex',
+        hasProgressMarker: true,
+        changeMarkerLength: 5,
+      },
+      // We didn't change the PerceptualSpeedIndexProgress, so we should have 6.
+      {
+        name: 'PerceptualSpeedIndex',
+        hasProgressMarker: true,
+        changeMarkerLength: 6,
+      },
+    ]);
   });
 });

src/types/profile.js

@@ -681,7 +681,8 @@ export type ProgressGraphData = {|
   // A percentage that describes the visual completeness of the webpage, ranging from 0% - 100%
   percent: number,
   // The time in milliseconds which the sample was taken.
-  timestamp: Milliseconds,
+  // This can be null due to https://github.com/sitespeedio/browsertime/issues/1746.
+  timestamp: Milliseconds | null,
 |};
 
 /**