From 50bb451e9cec275cfbc1efda3e1ced3f8a952e15 Mon Sep 17 00:00:00 2001
From: Moti Zilberman <moti@fb.com>
Date: Mon, 12 Sep 2022 03:45:43 -0700
Subject: [PATCH] Fix crash when GCing modules reachable via multiple paths
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Summary:
Changelog:

* **[Fix]**: Fix incremental build crashing when garbage collecting modules reachable via multiple paths in the graph.

Under the GC algorithm implemented in D36403390 (https://github.com/facebook/metro/commit/9065257b18a5951e6708d2d00221f7c5fa7b33b9), the same white ( = marked for deletion) node may be reached multiple times during the "CollectRoots" phase - specifically, if it is the child of two or more white nodes. We had an overly strict null check that caused a crash in this case.

In the test case added to `traverseDependencies-test`, the node that can be reached multiple times is `/baz`:

```
                    ┌─────────────────────────┐
                    │                         ▼
┌─────────┐   / ┌──────┐     ┌──────┐     ┌──────┐
│ /bundle │ ┈/▷ │ /foo │ ──▶ │ /bar │ ──▶ │ /baz │
└─────────┘ /   └──────┘     └──────┘     └──────┘
                    ▲            │
                    └────────────┘
```

A real-world instance of this crash can be seen by launching the default React Native starter app (created with `npx react-native init MyApp`) and deleting the contents of `index.js`. This causes a crash when Metro attempts to collect the `node_modules/babel/runtime/helpers/interopRequireDefault.js` dependency multiple times.

Reviewed By: huntie

Differential Revision: D39416657

fbshipit-source-id: 084b3a0f6363ffc5f51f8cdabc1fc7e81616f4db
---
 .../__tests__/traverseDependencies-test.js    | 36 +++++++++++++++++++
 .../metro/src/DeltaBundler/graphOperations.js |  9 ++---
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
index b8cf6a8ca2..82421d77e2 100644
--- a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
+++ b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
@@ -2188,6 +2188,42 @@ describe('edge cases', () => {
     expect(mockTransform).toHaveBeenCalledWith('/bar', undefined);
     expect(mockTransform).toHaveBeenCalledWith('/foo', undefined);
   });
+
+  it('removing a cycle with multiple outgoing edges to the same module', async () => {
+    /*
+                      ┌─────────────────────────┐
+                      │                         ▼
+    ┌─────────┐     ┌──────┐     ┌──────┐     ┌──────┐
+    │ /bundle │ ──▶ │ /foo │ ──▶ │ /bar │ ──▶ │ /baz │
+    └─────────┘     └──────┘     └──────┘     └──────┘
+                      ▲            │
+                      └────────────┘
+    */
+    Actions.addDependency('/bar', '/foo');
+    Actions.addDependency('/bar', '/baz');
+    files.clear();
+
+    await initialTraverseDependencies(graph, options);
+
+    /*
+                      ┌─────────────────────────┐
+                      │                         ▼
+    ┌─────────┐   / ┌──────┐     ┌──────┐     ┌──────┐
+    │ /bundle │ ┈/▷ │ /foo │ ──▶ │ /bar │ ──▶ │ /baz │
+    └─────────┘ /   └──────┘     └──────┘     └──────┘
+                      ▲            │
+                      └────────────┘
+    */
+    Actions.removeDependency('/bundle', '/foo');
+
+    expect(
+      getPaths(await traverseDependencies([...files], graph, options)),
+    ).toEqual({
+      added: new Set(),
+      modified: new Set(['/bundle']),
+      deleted: new Set(['/foo', '/bar', '/baz']),
+    });
+  });
 });
 
 describe('require.context', () => {
diff --git a/packages/metro/src/DeltaBundler/graphOperations.js b/packages/metro/src/DeltaBundler/graphOperations.js
index 063294d507..9ca16d674a 100644
--- a/packages/metro/src/DeltaBundler/graphOperations.js
+++ b/packages/metro/src/DeltaBundler/graphOperations.js
@@ -812,10 +812,11 @@ function collectWhite<T>(module: Module<T>, graph: Graph<T>, delta: Delta) {
   ) {
     graph.privateState.gc.color.set(module.path, 'black');
     for (const dependency of module.dependencies.values()) {
-      const childModule = nullthrows(
-        graph.dependencies.get(dependency.absolutePath),
-      );
-      collectWhite(childModule, graph, delta);
+      const childModule = graph.dependencies.get(dependency.absolutePath);
+      // The child may already have been collected.
+      if (childModule) {
+        collectWhite(childModule, graph, delta);
+      }
     }
     freeModule(module, graph, delta);
   }