I added a test that asserts malformed profiling operations do not throw and that errors/warnings are logged#502
I added a test that asserts malformed profiling operations do not throw and that errors/warnings are logged#502everettbu wants to merge 6 commits into
Conversation
Greptile SummaryThis PR makes
Confidence Score: 1/5
Important Files Changed
Last reviewed commit: 5f8690e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| it('should tolerate malformed operations without throwing', () => { | ||
| const {getCommitTree, invalidateCommitTrees} = require('react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder'); |
There was a problem hiding this comment.
Test doesn’t hit all cases
This test claims to inject “duplicate add” and “remove root”, but ops1 only includes an ADD for id=2 once, and uses op code 6 (which is TREE_OPERATION_SET_SUBTREE_MODE), not TREE_OPERATION_REMOVE_ROOT. As written, it doesn’t actually verify the new non-throwing behavior for those scenarios.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js
Line: 337:339
Comment:
**Test doesn’t hit all cases**
This test claims to inject “duplicate add” and “remove root”, but `ops1` only includes an ADD for id=2 once, and uses op code `6` (which is `TREE_OPERATION_SET_SUBTREE_MODE`), not `TREE_OPERATION_REMOVE_ROOT`. As written, it doesn’t actually verify the new non-throwing behavior for those scenarios.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js
Line: 182:184
Comment:
**Cloning missing-node breaks**
`getClonedNode` no longer guards against `nodes.get(id)` being `null/undefined`. For malformed ops (e.g. ADD with a missing parent, REORDER on a missing id, UPDATE_TREE_BASE_DURATION on a missing id), this now does `Object.assign({}, undefined)` and returns a “node” without required fields. The next access (e.g. `parentNode.children.concat(...)`) will throw, defeating the goal of tolerating malformed operations. Consider restoring the previous null check (and logging/ignoring the op) rather than cloning an absent node.
How can I resolve this? If you propose a fix, please make it concise.
On duplicate Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js
Line: 214:216
Comment:
**Duplicate ADD desyncs parser**
On duplicate `TREE_OPERATION_ADD`, the code `break`s without advancing `i` to skip the remaining fields for that ADD op. That leaves `i` pointing at the add payload (parentID/ownerID/name/key/etc), and the loop will interpret those values as new operations, which can cause incorrect parsing and downstream errors. To safely ignore duplicate adds, `i` still needs to be incremented by the correct payload length for root vs non-root adds.
How can I resolve this? If you propose a fix, please make it concise. |
| it('should tolerate malformed operations without throwing', () => { | ||
| const {getCommitTree, invalidateCommitTrees} = require('react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder'); | ||
| const { | ||
| ElementTypeFunction, | ||
| ElementTypeRoot, | ||
| } = require('react-devtools-shared/src/frontend/types'); | ||
|
|
||
| const rootID = 1; | ||
| const snapshots = new Map(); | ||
| snapshots.set(rootID, { | ||
| id: rootID, | ||
| children: [2], | ||
| displayName: 'Root', | ||
| hocDisplayNames: null, | ||
| key: null, | ||
| type: ElementTypeRoot, | ||
| compiledWithForget: false, | ||
| }); | ||
| snapshots.set(2, { | ||
| id: 2, | ||
| children: [], | ||
| displayName: 'Child', | ||
| hocDisplayNames: null, | ||
| key: null, | ||
| type: ElementTypeFunction, | ||
| compiledWithForget: false, | ||
| }); | ||
| const initialTreeBaseDurations = new Map(); | ||
| initialTreeBaseDurations.set(rootID, 0); | ||
| initialTreeBaseDurations.set(2, 0); | ||
|
|
||
| const ops0 = [0, rootID, 0]; | ||
| const ops1 = [ | ||
| 0, rootID, 0, | ||
| 1, 2, ElementTypeFunction, rootID, 0, 0, 0, | ||
| 2, 1, 999, | ||
| 6, | ||
| 99, | ||
| ]; |
There was a problem hiding this comment.
Malformed ops array
ops1 starts with [0, rootID, 0], so updateTree() will treat rootID as the commit tree root and then 0 as stringTableSize; the next value (1) becomes the first operation. As constructed, the sequence 1, 2, ElementTypeFunction, rootID, 0, 0, 0, ... is not aligned to the TREE_OPERATION_ADD encoding expected by CommitTreeBuilder.updateTree and will cause the parser to read incorrect fields (e.g. parentID/ownerID/string IDs) and advance i incorrectly (packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js:295-302). This makes the test assertions unreliable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js
Line: 264:302
Comment:
**Malformed ops array**
`ops1` starts with `[0, rootID, 0]`, so `updateTree()` will treat `rootID` as the commit tree root and then `0` as `stringTableSize`; the next value (`1`) becomes the first operation. As constructed, the sequence `1, 2, ElementTypeFunction, rootID, 0, 0, 0, ...` is not aligned to the `TREE_OPERATION_ADD` encoding expected by `CommitTreeBuilder.updateTree` and will cause the parser to read incorrect fields (e.g. parentID/ownerID/string IDs) and advance `i` incorrectly (`packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js:295-302`). This makes the test assertions unreliable.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js
Line: 10:24
Comment:
**Undefined constant import**
`CommitTreeBuilder.js` imports `TREE_OPERATION_REMOVE_ROOT` (`packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js:10-24`), but `packages/react-devtools-shared/src/constants.js` explicitly notes it was removed and does not export it. This will make the module fail to load at runtime (and the new switch case can never work).
How can I resolve this? If you propose a fix, please make it concise.
In the Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js
Line: 345:353
Comment:
**Incorrect op parsing**
In the `TREE_OPERATION_SET_SUBTREE_MODE` case, `mode` is read from `operations[i + 1]` (same as `id`), and then `i += 3` even though only two operands appear to be consumed (`packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js:345-353`). This will desynchronize the parser and corrupt processing of subsequent operations.
How can I resolve this? If you propose a fix, please make it concise. |
| case TREE_OPERATION_REMOVE_ROOT: { | ||
| console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.'); | ||
| break; | ||
| } |
There was a problem hiding this comment.
TREE_OPERATION_REMOVE_ROOT is undefined — this case is dead code
TREE_OPERATION_REMOVE_ROOT was removed from constants.js (line 25: // Removed TREE_OPERATION_REMOVE_ROOT) and is not imported in this file. At runtime, this case matches against undefined, which will never equal any numeric operation code. This means the handler is unreachable dead code, and any actual operation code 6 (the old value of TREE_OPERATION_REMOVE_ROOT) will fall through to the default branch and halt parsing instead.
Either import or define a constant for the old value (e.g. const TREE_OPERATION_REMOVE_ROOT = 6;) or remove this case entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js
Line: 320-323
Comment:
**`TREE_OPERATION_REMOVE_ROOT` is undefined — this case is dead code**
`TREE_OPERATION_REMOVE_ROOT` was removed from `constants.js` (line 25: `// Removed TREE_OPERATION_REMOVE_ROOT`) and is not imported in this file. At runtime, this `case` matches against `undefined`, which will never equal any numeric operation code. This means the handler is unreachable dead code, and any actual operation code `6` (the old value of `TREE_OPERATION_REMOVE_ROOT`) will fall through to the `default` branch and halt parsing instead.
Either import or define a constant for the old value (e.g. `const TREE_OPERATION_REMOVE_ROOT = 6;`) or remove this case entirely.
How can I resolve this? If you propose a fix, please make it concise.
| if (nodes.has(id)) { | ||
| throw new Error( | ||
| `Commit tree already contains fiber "${id}". This is a bug in React DevTools.`, | ||
| // Duplicate add detected; log and skip instead of throwing to avoid crashing the UI. | ||
| console.error( | ||
| `Commit tree already contains fiber "${id}". Skipping duplicate add.`, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Duplicate-add break corrupts the operation parser index
When a duplicate add is detected, the code breaks out of the switch after only advancing i by 3 (past opcode, id, type). However, a non-root TREE_OPERATION_ADD consumes 5 additional fields (parentID, ownerID, displayNameStringID, keyStringID, namePropStringID), and a root-type add consumes 4 additional fields. By breaking early without advancing i past these fields, the next loop iteration reads from the middle of the current operation's payload, misinterpreting data values as operation codes. This will cause cascading parse errors for the remainder of the operations array.
The fix should skip the correct number of remaining fields based on type before breaking, for example:
| if (nodes.has(id)) { | |
| throw new Error( | |
| `Commit tree already contains fiber "${id}". This is a bug in React DevTools.`, | |
| // Duplicate add detected; log and skip instead of throwing to avoid crashing the UI. | |
| console.error( | |
| `Commit tree already contains fiber "${id}". Skipping duplicate add.`, | |
| ); | |
| break; | |
| } | |
| if (nodes.has(id)) { | |
| // Duplicate add detected; log and skip instead of throwing to avoid crashing the UI. | |
| console.error( | |
| `Commit tree already contains fiber "${id}". Skipping duplicate add.`, | |
| ); | |
| if (type === ElementTypeRoot) { | |
| i += 4; // isStrictModeCompliant, Profiling flag, supportsStrictMode, hasOwnerMetadata | |
| } else { | |
| i += 5; // parentID, ownerID, displayNameStringID, keyStringID, nameProp | |
| } | |
| break; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js
Line: 204-210
Comment:
**Duplicate-add `break` corrupts the operation parser index**
When a duplicate add is detected, the code `break`s out of the switch after only advancing `i` by 3 (past opcode, id, type). However, a non-root `TREE_OPERATION_ADD` consumes 5 additional fields (parentID, ownerID, displayNameStringID, keyStringID, namePropStringID), and a root-type add consumes 4 additional fields. By breaking early without advancing `i` past these fields, the next loop iteration reads from the middle of the current operation's payload, misinterpreting data values as operation codes. This will cause cascading parse errors for the remainder of the operations array.
The fix should skip the correct number of remaining fields based on `type` before breaking, for example:
```suggestion
if (nodes.has(id)) {
// Duplicate add detected; log and skip instead of throwing to avoid crashing the UI.
console.error(
`Commit tree already contains fiber "${id}". Skipping duplicate add.`,
);
if (type === ElementTypeRoot) {
i += 4; // isStrictModeCompliant, Profiling flag, supportsStrictMode, hasOwnerMetadata
} else {
i += 5; // parentID, ownerID, displayNameStringID, keyStringID, nameProp
}
break;
}
```
How can I resolve this? If you propose a fix, please make it concise.2 participants
Mirror of facebook/react#35674
Original author: apoorvgarewal07