Skip to content

Commit

Permalink
Partial fix DevTools Profiler ''Could not find node…'' error (#17759)
Browse files Browse the repository at this point in the history
The Profiler stores:

1. A snapshot of the React tree when profiling started
2. The operations array for each commit
3. Profiling metadata (e.g. durations, what changed, etc) for each commit

It uses this information (snapshot + operations diff) to reconstruct the state of the application for a given commit as it's viewed in the Profiler UI. Because of this, it's very important that the operations and metadata arrays align. If they don't align, the profiler will be unable to correctly reconstruct the tree, and it will likely throw errors (like 'Could not find node…')

#16446 tracks a long-standing bug where these two arrays get misaligned. I am still not entirely sure what causes this bug, but with PR #17253, I exacerbated things by introducing another potential way for it to happen. This PR addresses the regression at least (and adds test coverage for it).

I will follow up this afternoon on the original #16446 issue. I think I may have a lead on what's happening at least, if not exactly an idea of how to reproduce it.
  • Loading branch information
Brian Vaughn authored Jan 3, 2020
1 parent cca994c commit 5d3d71b
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -118,5 +118,6 @@ describe('ProfilerStore', () => {
const root = store.roots[0];
const data = store.profilerStore.getDataForRoot(root);
expect(data.commitData).toHaveLength(1);
expect(data.operations).toHaveLength(1);
});
});
27 changes: 24 additions & 3 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1021,12 +1021,32 @@ export function attach(
pendingSimulatedUnmountedIDs.length === 0 &&
pendingUnmountedRootID === null
) {
// If we're currently profiling, send an "operations" method even if there are no mutations to the tree.
// The frontend needs this no-op info to know how to reconstruct the tree for each commit,
// even if a particular commit didn't change the shape of the tree.
// If we aren't profiling, we can just bail out here.
// No use sending an empty update over the bridge.
if (!isProfiling) {
return;
}

const current = root.current;
const alternate = current.alternate;

// Certain types of updates bail out at the root without doing any actual render work.
// React should probably not call the DevTools commit hook in this case,
// but if it does- we can detect it and filter them out from the profiler.
// NOTE: Keep this logic in sync with the one in handleCommitFiberRoot()
const didBailoutAtRoot =
alternate !== null &&
alternate.expirationTime === 0 &&
alternate.childExpirationTime === 0;

// The Profiler stores metadata for each commit and reconstructs the app tree per commit using:
// (1) an initial tree snapshot and
// (2) the operations array for each commit
// Because of this, it's important that the operations and metadata arrays align,
// So the logic that skips metadata for bailout commits should also apply to filter operations.
if (didBailoutAtRoot) {
return;
}
}

const numUnmountIDs =
Expand Down Expand Up @@ -1758,6 +1778,7 @@ export function attach(
// Certain types of updates bail out at the root without doing any actual render work.
// React should probably not call the DevTools commit hook in this case,
// but if it does- we can detect it and filter them out from the profiler.
// NOTE: Keep this logic in sync with the one in flushPendingEvents()
const didBailoutAtRoot =
alternate !== null &&
alternate.expirationTime === 0 &&
Expand Down

0 comments on commit 5d3d71b

Please sign in to comment.