From b8c2cadb72c69672da260a3271941624203b6f3f Mon Sep 17 00:00:00 2001
From: Rob Hogan <robhogan@fb.com>
Date: Thu, 14 Sep 2023 07:43:58 -0700
Subject: [PATCH] Fix "unexpected null" handling overlapping file changes

Summary:
Fixes a crash where a race in overlapping file change processing could occasionally throw "unexpected null".

Fixes https://github.com/facebook/metro/issues/1015

## Root cause
The problem logic is in some instrumentation code introduced in D40346848.

`metro-file-map` batches changes using `setInterval(emitChange, 30)`. The instrumentation attempted to record the time of the first event in a given batch, and then "reset the clock" when the batch is emitted to the consumer.

The problem occurred when an emit interval fell such that a non-empty `emitChange` occurred *during* the async `_processFile` step of a subsequent change. The first emit would reset the "start time" to `null`, and the second `emitChange` would fail at `nullthrows(eventStartTimestamp)`, because `eventStartTimestamp` is only set *before* `_processFile` starts (and only if already `null`).

The (over)writing of `eventStartTimestamp` was flawed, because we don't know at the start of change processing that this event is going to be the start of a batch - this only becomes clear when an event is subsequently enqueued for emit after file processing.

## This diff
This changes the recording of the start time such that we take a timestamp on every change, and record it as the batch start time on the creation of a new batch. We rearrange the various moving parts into a nullable object, so that "first event time" is tightly coupled to the batch it describes, and can never be `null` for a non-empty batch.

## Changelog
```
 * **[Fix]:** Fix "unexpected null" crash when handling a batch of file changes.
```

Differential Revision: D49272782
---
 packages/metro-file-map/src/index.js | 73 +++++++++++++++++-----------
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js
index d42e6257ca..eb9dcd84a1 100644
--- a/packages/metro-file-map/src/index.js
+++ b/packages/metro-file-map/src/index.js
@@ -878,33 +878,39 @@ export default class FileMap extends EventEmitter {
     const rootDir = this._options.rootDir;
 
     let changeQueue: Promise<null | void> = Promise.resolve();
-    let eventsQueue: EventsQueue = [];
-    let eventStartTimestamp = null;
+    let nextEmit: ?{
+      eventsQueue: EventsQueue,
+      firstEventTimestamp: number,
+      firstEnqueuedTimestamp: number,
+    } = null;
 
     const emitChange = () => {
-      if (eventsQueue.length) {
-        const hmrPerfLogger = this._options.perfLoggerFactory?.('HMR', {
-          key: this._getNextChangeID(),
+      if (nextEmit == null || nextEmit.eventsQueue.length === 0) {
+        // Nothing to emit
+        return;
+      }
+      const {eventsQueue, firstEventTimestamp, firstEnqueuedTimestamp} =
+        nextEmit;
+      const hmrPerfLogger = this._options.perfLoggerFactory?.('HMR', {
+        key: this._getNextChangeID(),
+      });
+      if (hmrPerfLogger != null) {
+        hmrPerfLogger.start({timestamp: firstEventTimestamp});
+        hmrPerfLogger.point('waitingForChangeInterval_start', {
+          timestamp: firstEnqueuedTimestamp,
         });
-        if (hmrPerfLogger != null) {
-          hmrPerfLogger.start({timestamp: nullthrows(eventStartTimestamp)});
-          hmrPerfLogger.point('waitingForChangeInterval_start', {
-            timestamp: nullthrows(eventStartTimestamp),
-          });
-          hmrPerfLogger.point('waitingForChangeInterval_end');
-          hmrPerfLogger.annotate({
-            int: {eventsQueueLength: eventsQueue.length},
-          });
-          hmrPerfLogger.point('fileChange_start');
-        }
-        const changeEvent: ChangeEvent = {
-          logger: hmrPerfLogger,
-          eventsQueue,
-        };
-        this.emit('change', changeEvent);
-        eventsQueue = [];
-        eventStartTimestamp = null;
+        hmrPerfLogger.point('waitingForChangeInterval_end');
+        hmrPerfLogger.annotate({
+          int: {eventsQueueLength: eventsQueue.length},
+        });
+        hmrPerfLogger.point('fileChange_start');
       }
+      const changeEvent: ChangeEvent = {
+        logger: hmrPerfLogger,
+        eventsQueue,
+      };
+      this.emit('change', changeEvent);
+      nextEmit = null;
     };
 
     const onChange = (
@@ -952,15 +958,14 @@ export default class FileMap extends EventEmitter {
         return;
       }
 
-      if (eventStartTimestamp == null) {
-        eventStartTimestamp = performance.timeOrigin + performance.now();
-      }
+      const onChangeStartTime = performance.timeOrigin + performance.now();
 
       changeQueue = changeQueue
         .then(async () => {
           // If we get duplicate events for the same file, ignore them.
           if (
-            eventsQueue.find(
+            nextEmit != null &&
+            nextEmit.eventsQueue.find(
               event =>
                 event.type === type &&
                 event.filePath === absoluteFilePath &&
@@ -978,11 +983,21 @@ export default class FileMap extends EventEmitter {
           const linkStats = fileSystem.linkStats(relativeFilePath);
 
           const enqueueEvent = (metadata: ChangeEventMetadata) => {
-            eventsQueue.push({
+            const event = {
               filePath: absoluteFilePath,
               metadata,
               type,
-            });
+            };
+            if (nextEmit == null) {
+              nextEmit = {
+                eventsQueue: [event],
+                firstEventTimestamp: onChangeStartTime,
+                firstEnqueuedTimestamp:
+                  performance.timeOrigin + performance.now(),
+              };
+            } else {
+              nextEmit.eventsQueue.push(event);
+            }
             return null;
           };