Skip to content

Commit

Permalink
Fix crash when GCing modules reachable via multiple paths
Browse files Browse the repository at this point in the history
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 (9065257), 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
  • Loading branch information
motiz88 authored and facebook-github-bot committed Sep 12, 2022
1 parent e9ee037 commit 50bb451
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
9 changes: 5 additions & 4 deletions packages/metro/src/DeltaBundler/graphOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

0 comments on commit 50bb451

Please sign in to comment.