[Flight] Walk parsed JSON instead of using reviver for parsing RSC payload#33
[Flight] Walk parsed JSON instead of using reviver for parsing RSC payload#33AbanoubGhadban wants to merge 1 commit intomainfrom
Conversation
WalkthroughThe pull request refactors the JSON parsing mechanism across multiple React Server DOM webpack client modules, replacing inline Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| for (var i = 0; i < value.length; i++) | ||
| value[i] = reviveModel(response, value[i], value, "" + i); | ||
| if (value[0] === REACT_ELEMENT_TYPE) { | ||
| if (value[0] === REACT_ELEMENT_TYPE) |
There was a problem hiding this comment.
Redundant check (compiler artifact) — the outer if at line 1461 already guarantees value[0] === REACT_ELEMENT_TYPE is true here, making this inner check unreachable-false. It's a no-op, but dead conditionals are confusing and could mislead future readers into thinking the else branch (line 1489 response = value) is reachable when the inner check fails — it isn't.
This appears identically in all 8 compiled files. It looks like a decompiler/compiler artifact in the upstream fork's output (rsc-patches/v19.0.3). Worth filing upstream or patching out of the compiled output to keep the code clean.
| if (value[0] === REACT_ELEMENT_TYPE) | |
| if (value[0] === REACT_ELEMENT_TYPE) | |
| b: { |
(i.e. remove the redundant outer if wrapper and fold the labeled block up one level)
| for (i in value) | ||
| "__proto__" === i | ||
| ? delete value[i] | ||
| : ((parentObject = reviveModel(response, value[i], value, i)), |
There was a problem hiding this comment.
parentObject parameter is clobbered — on the first loop iteration, parentObject is overwritten with the return value of reviveModel(), destroying the reference to the actual parent. Subsequent recursive calls still pass value (the current object) as parentObject correctly, so this doesn't affect correctness. But during debugging, inspecting parentObject mid-loop gives a misleading value.
Same pattern exists in all 8 compiled files — it's a minifier artifact (variable reuse). A simple rename to revived for the temp would make intent clear, but since this is compiled output that's a note for the upstream source rather than a direct edit here.
| if (file.endsWith(".hot-update.js")) break; | ||
| chunks.push(c.id, file); | ||
| break; | ||
| if ( |
There was a problem hiding this comment.
✅ Correct fix. The original code used three sequential break statements that exited the for loop unconditionally on the first file, even if it wasn't a .js file. The new if condition lets the loop continue past non-JS (e.g. CSS, image) assets until it finds a qualifying .js file. This was a real bug: in any chunk that emits a non-JS asset before its JS file, the manifest would silently record the wrong file or nothing at all.
Code ReviewOverviewThis PR backports facebook/react#35776 — replacing PerformanceThe benchmark numbers in the PR description are credible and match the upstream claim. The two-pass approach (full C++ parse → pure-JS walk) eliminates the per-node callback overhead that was the bottleneck. The only new allocation is the CorrectnessTraversal order is preserved. The original reviver ran bottom-up (children before parents). The new
void 0 !== parentObject ? (value[i] = parentObject) : delete value[i]correctly replicates the reviver's behavior of deleting properties whose reviver returned
Redundant double-check — Security
Issues
Plugin FixThe SummaryThe optimization is legitimate and the implementation is correct. The main concerns before merging are: (1) confirming the compiled output is reproducible from the stated fork commit, and (2) running the test plan — particularly round-trip parsing with lazy chunk references and the plugin fix with mixed-asset chunk groups. |
Greptile SummaryThis PR backports facebook/react#35776, replacing Confidence Score: 5/5Safe to merge — all findings are P2 style observations about transpilation artifacts with no functional impact. The core optimization is a faithful backport of a well-reviewed upstream React PR. The reviveModel traversal preserves the same initializingHandler push/pop ordering as the original reviver. The plugin fix correctly changes break-on-no-match to continue-on-no-match. The only observation is a redundant double REACT_ELEMENT_TYPE guard that is a transpilation artifact with no runtime effect. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["parseModel(response, json)"] --> B["JSON.parse(json)\nPure C++ — no JS callbacks"]
B --> C["reviveModel(response, parsedValue, root, '')"]
C --> D{typeof value?}
D -- string --> E{"starts with '$'?"}
E -- yes --> F["parseModelString()\nResolve RSC tokens\n($, $L, $@, $S, ...)"]
E -- no --> G["Return string as-is"]
D -- "non-object / null" --> H["Return primitive"]
D -- array --> I["Recursively reviveModel\neach element in order"]
I --> J{"value[0] ===\nREACT_ELEMENT_TYPE?"}
J -- yes --> K["Build React element object\nHandle initializingHandler push/pop"]
J -- no --> L["Return array"]
D -- object --> M["for (i in value)\nRecursively reviveModel each property"]
M --> N{"key === '__proto__'?"}
N -- yes --> O["delete value[i] (proto-pollution guard)"]
N -- no --> P{"result === undefined?"}
P -- yes --> Q["delete value[i]"]
P -- no --> R["value[i] = result"]
Reviews (1): Last reviewed commit: "[RSC-PATCH] Walk parsed JSON instead of ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (value[0] === REACT_ELEMENT_TYPE) { | ||
| if (value[0] === REACT_ELEMENT_TYPE) | ||
| b: { | ||
| value = { | ||
| $$typeof: REACT_ELEMENT_TYPE, | ||
| type: value[1], | ||
| key: value[2], | ||
| ref: null, | ||
| props: value[3] | ||
| }), | ||
| null !== initializingHandler) | ||
| ) | ||
| if ( | ||
| ((value = initializingHandler), | ||
| (initializingHandler = value.parent), | ||
| value.errored) | ||
| ) | ||
| (key = new ReactPromise("rejected", null, value.value, response)), | ||
| (key = createLazyChunkWrapper(key)); | ||
| else if (0 < value.deps) { | ||
| var blockedChunk = new ReactPromise( | ||
| "blocked", | ||
| null, | ||
| null, | ||
| response | ||
| ); | ||
| value.value = key; | ||
| value.chunk = blockedChunk; | ||
| key = createLazyChunkWrapper(blockedChunk); | ||
| }; | ||
| if (null !== initializingHandler) { | ||
| i = initializingHandler; | ||
| initializingHandler = i.parent; | ||
| if (i.errored) { | ||
| response = new ReactPromise("rejected", null, i.value, response); | ||
| response = createLazyChunkWrapper(response); | ||
| break b; | ||
| } | ||
| if (0 < i.deps) { | ||
| response = new ReactPromise("blocked", null, null, response); | ||
| i.value = value; | ||
| i.chunk = response; | ||
| response = createLazyChunkWrapper(response); | ||
| break b; | ||
| } | ||
| } | ||
| } else key = value; | ||
| return key; | ||
| response = value; | ||
| } | ||
| else response = value; |
There was a problem hiding this comment.
REACT_ELEMENT_TYPE double-check
The inner if (value[0] === REACT_ELEMENT_TYPE) guard (line 1462) is always true when reached — it is nested directly inside the identical outer check on line 1461. The outer check already guarantees the condition holds. The labeled break b escapes from the inner block but not any outer guard, so the else response = value branch on line 1489 is dead code. This appears to be a transpilation artifact from the upstream fork, but it may confuse future readers who try to understand when the else branch executes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js (1)
1671-1681:⚠️ Potential issue | 🔴 CriticalAdd the no-op setter for
$Yomitted-property descriptors.Line 1673 defines a getter-only, non-configurable property, then Line 2411 writes the revived
nullback to the same key. In strict mode this matches the failingCannot set property asyncQueue ... which has only a getterpipeline error. Add the no-op setter from the upstream follow-up so the explicit walk preserves the old reviver behavior: https://github.com/facebook/react/pull/35776/files🐛 Proposed fix
Object.defineProperty(parentObject, key, { get: function () { return "This object has been omitted by React in the console log to avoid sending too much data from the server. Try logging smaller or more specific objects."; }, + set: function () {}, enumerable: !0, configurable: !1 }),Also applies to: 2406-2412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js` around lines 1671 - 1681, The getter-only omitted-property descriptor in the switch case "Y" creates a non-configurable, getter-only property on parentObject[key], causing a strict-mode "Cannot set property ... which has only a getter" when code later writes to that key; fix it by adding a no-op setter (set: function() {}) to the Object.defineProperty call in the case "Y" branch so the property supports assignments while preserving enumerable and configurable values, and apply the identical change to the other occurrence around the revived null write (the block around the 2406–2412 region) to keep reviver behavior consistent.
♻️ Duplicate comments (1)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js (1)
2340-2412:⚠️ Potential issue | 🟠 MajorApply the same reviver-semantics guard in the edge dev bundle.
This manual walk has the same failure mode as the browser dev build:
$Ycan replace a property with a getter-only accessor before Lines 2341-2342 or Lines 2409-2412 assign back to it, throwing under"use strict". Thefor...inobject walk should also be own-property-only.Suggested guard for this generated variant
function parseModel(response, json) { json = JSON.parse(json); return reviveModel(response, json, { "": json }, ""); } + function setRevivedProperty(target, key, revivedValue) { + var descriptor = Object.getOwnPropertyDescriptor(target, key); + if (!descriptor || descriptor.writable || descriptor.set) + target[key] = revivedValue; + } function reviveModel(response, value, parentObject, key) { @@ if ("object" !== typeof value || null === value) return value; if (isArrayImpl(value)) { for (var i = 0; i < value.length; i++) - value[i] = reviveModel(response, value[i], value, "" + i); + setRevivedProperty( + value, + "" + i, + reviveModel(response, value[i], value, "" + i) + ); @@ - for (i in value) - "__proto__" === i - ? delete value[i] - : ((parentObject = reviveModel(response, value[i], value, i)), - void 0 !== parentObject - ? (value[i] = parentObject) - : delete value[i]); + for (i in value) + if (hasOwnProperty.call(value, i)) + "__proto__" === i + ? delete value[i] + : ((parentObject = reviveModel(response, value[i], value, i)), + void 0 !== parentObject + ? setRevivedProperty(value, i, parentObject) + : delete value[i]);Use the same read-only verification snippet from the browser development bundle comment; it reproduces both JavaScript behaviors independently of the repository files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js` around lines 2340 - 2412, The for...in walk in reviveModel can throw when a serialized $Y injects a getter-only accessor; update the loop that iterates properties of value (inside function reviveModel) to only operate on own, writable data properties: for each key i, first skip if !Object.prototype.hasOwnProperty.call(value, i); then read the descriptor via Object.getOwnPropertyDescriptor(value, i) and if it is an accessor without a setter or a non-writable data descriptor, do not attempt to assign back into value (delete or skip), otherwise proceed to call reviveModel and assign the result; apply this same read-only verification logic to both the array/object branches where properties are assigned so the edge dev bundle matches the browser dev guard.
🧹 Nitpick comments (1)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js (1)
2369-2376: Avoid shadowing theparentObjectparameter in the for-in loop.Line 2372 reuses
parentObject(a function parameter) as the loop variable to store revived property values, which shadows the parameter and reduces readability.♻️ Use a distinct variable name for clarity
for (i in value) "__proto__" === i ? delete value[i] - : ((parentObject = reviveModel(response, value[i], value, i)), - void 0 !== parentObject - ? (value[i] = parentObject) + : ((revivedValue = reviveModel(response, value[i], value, i)), + void 0 !== revivedValue + ? (value[i] = revivedValue) : delete value[i]);Declare
revivedValueat the top ofreviveModel:function reviveModel(response, value, parentObject, key) { + var i, revivedValue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js` around lines 2369 - 2376, The function reviveModel currently reassigns the parameter named parentObject inside its for-in loop, shadowing the parameter and harming clarity; fix it by introducing a new local variable (e.g., revivedValue) declared near the top of reviveModel and use that variable to store the result of reviveModel(response, value[i], value, i) and to decide whether to set or delete value[i], rather than assigning to the parentObject parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.js`:
- Around line 1660-1664: The loop that iterates properties in reviveModel
currently uses "for (i in value)" which can include inherited properties; update
that loop to skip non-own properties by checking hasOwnProperty.call(value, i)
before processing each key (use the existing module-level hasOwnProperty).
Ensure the existing logic (skipping "__proto__", calling reviveModel and
assigning or deleting parentObject) only runs when hasOwnProperty.call(value, i)
is true, so inherited prototype properties are not revived into the model.
In
`@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js`:
- Around line 2306-2365: Remove the redundant nested check "if (value[0] ===
REACT_ELEMENT_TYPE)" inside the outer guard and lift its block up so the
element-unwrapping logic executes directly when the outer condition is true;
delete the unreachable else branch that returns value (the branch around Line
2364), ensuring the existing behavior that prepares the React element object
(uses REACT_ELEMENT_TYPE, initializingHandler, ReactPromise,
createLazyChunkWrapper, getComponentNameFromType) and then returns response is
preserved exactly.
---
Outside diff comments:
In
`@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js`:
- Around line 1671-1681: The getter-only omitted-property descriptor in the
switch case "Y" creates a non-configurable, getter-only property on
parentObject[key], causing a strict-mode "Cannot set property ... which has only
a getter" when code later writes to that key; fix it by adding a no-op setter
(set: function() {}) to the Object.defineProperty call in the case "Y" branch so
the property supports assignments while preserving enumerable and configurable
values, and apply the identical change to the other occurrence around the
revived null write (the block around the 2406–2412 region) to keep reviver
behavior consistent.
---
Duplicate comments:
In
`@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js`:
- Around line 2340-2412: The for...in walk in reviveModel can throw when a
serialized $Y injects a getter-only accessor; update the loop that iterates
properties of value (inside function reviveModel) to only operate on own,
writable data properties: for each key i, first skip if
!Object.prototype.hasOwnProperty.call(value, i); then read the descriptor via
Object.getOwnPropertyDescriptor(value, i) and if it is an accessor without a
setter or a non-writable data descriptor, do not attempt to assign back into
value (delete or skip), otherwise proceed to call reviveModel and assign the
result; apply this same read-only verification logic to both the array/object
branches where properties are assigned so the edge dev bundle matches the
browser dev guard.
---
Nitpick comments:
In
`@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js`:
- Around line 2369-2376: The function reviveModel currently reassigns the
parameter named parentObject inside its for-in loop, shadowing the parameter and
harming clarity; fix it by introducing a new local variable (e.g., revivedValue)
declared near the top of reviveModel and use that variable to store the result
of reviveModel(response, value[i], value, i) and to decide whether to set or
delete value[i], rather than assigning to the parentObject parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b241740b-9ba0-4989-99c1-61b6bb04e6b6
📒 Files selected for processing (9)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.jssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.jssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.jssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.jssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.jssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.jssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.jssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.jssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js
| if (isArrayImpl(value)) { | ||
| for (var i = 0; i < value.length; i++) | ||
| value[i] = reviveModel(response, value[i], value, "" + i); | ||
| if (value[0] === REACT_ELEMENT_TYPE) { | ||
| if (value[0] === REACT_ELEMENT_TYPE) | ||
| if ( | ||
| ((key = value[4]), | ||
| (value = { | ||
| b: { | ||
| i = value[4]; | ||
| value = { | ||
| $$typeof: REACT_ELEMENT_TYPE, | ||
| type: value[1], | ||
| key: value[2], | ||
| props: value[3], | ||
| _owner: null === key ? response._debugRootOwner : key | ||
| }), | ||
| _owner: null === i ? response._debugRootOwner : i | ||
| }; | ||
| Object.defineProperty(value, "ref", { | ||
| enumerable: !1, | ||
| get: nullRefGetter | ||
| }), | ||
| (value._store = {}), | ||
| }); | ||
| value._store = {}; | ||
| Object.defineProperty(value._store, "validated", { | ||
| configurable: !1, | ||
| enumerable: !1, | ||
| writable: !0, | ||
| value: 1 | ||
| }), | ||
| }); | ||
| Object.defineProperty(value, "_debugInfo", { | ||
| configurable: !1, | ||
| enumerable: !1, | ||
| writable: !0, | ||
| value: null | ||
| }), | ||
| null !== initializingHandler) | ||
| ) { | ||
| var handler = initializingHandler; | ||
| initializingHandler = handler.parent; | ||
| handler.errored | ||
| ? ((key = new ReactPromise( | ||
| }); | ||
| if (null !== initializingHandler) { | ||
| i = initializingHandler; | ||
| initializingHandler = i.parent; | ||
| if (i.errored) { | ||
| response = new ReactPromise( | ||
| "rejected", | ||
| null, | ||
| handler.value, | ||
| i.value, | ||
| response | ||
| )), | ||
| (value = { | ||
| ); | ||
| value = { | ||
| name: getComponentNameFromType(value.type) || "", | ||
| owner: value._owner | ||
| }), | ||
| (key._debugInfo = [value]), | ||
| (value = createLazyChunkWrapper(key))) | ||
| : 0 < handler.deps && | ||
| ((key = new ReactPromise("blocked", null, null, response)), | ||
| (handler.value = value), | ||
| (handler.chunk = key), | ||
| (value = Object.freeze.bind(Object, value.props)), | ||
| key.then(value, value), | ||
| (value = createLazyChunkWrapper(key))); | ||
| } else Object.freeze(value.props); | ||
| return value; | ||
| }; | ||
| response._debugInfo = [value]; | ||
| response = createLazyChunkWrapper(response); | ||
| break b; | ||
| } | ||
| if (0 < i.deps) { | ||
| response = new ReactPromise("blocked", null, null, response); | ||
| i.value = value; | ||
| i.chunk = response; | ||
| value = Object.freeze.bind(Object, value.props); | ||
| response.then(value, value); | ||
| response = createLazyChunkWrapper(response); | ||
| break b; | ||
| } | ||
| } else Object.freeze(value.props); | ||
| response = value; | ||
| } | ||
| else response = value; | ||
| return response; | ||
| } | ||
| return value; | ||
| }; | ||
| } | ||
| for (i in value) | ||
| "__proto__" === i | ||
| ? delete value[i] | ||
| : ((parentObject = reviveModel(response, value[i], value, i)), | ||
| void 0 !== parentObject | ||
| ? (value[i] = parentObject) | ||
| : delete value[i]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrates the strict-mode accessor write and inherited-key hazards.
node <<'NODE'
"use strict";
function installOmittedAccessor(parentObject, key) {
Object.defineProperty(parentObject, key, {
get: function () {
return "omitted";
},
enumerable: true,
configurable: false
});
return null;
}
const payload = { omitted: "$Y" };
try {
payload.omitted = installOmittedAccessor(payload, "omitted");
console.log("assignment did not throw");
} catch (error) {
console.log("strict accessor assignment:", error.constructor.name);
}
Object.prototype.__rscInherited = "$undefined";
const model = JSON.parse('{"own":1}');
const keys = [];
for (const key in model) keys.push(key);
delete Object.prototype.__rscInherited;
console.log("for-in keys:", keys.join(","));
NODERepository: shakacode/react_on_rails_rsc
Length of output: 141
🏁 Script executed:
fd -t f "react-server-dom-webpack-client.browser.development.js"Repository: shakacode/react_on_rails_rsc
Length of output: 159
🏁 Script executed:
# Check file size first
wc -l src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.jsRepository: shakacode/react_on_rails_rsc
Length of output: 164
🏁 Script executed:
# Read the section mentioned in the review comment (lines 2131-2203)
sed -n '2131,2203p' src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.jsRepository: shakacode/react_on_rails_rsc
Length of output: 2720
🏁 Script executed:
# Check for "use strict" at the beginning of the file
head -20 src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.jsRepository: shakacode/react_on_rails_rsc
Length of output: 632
🏁 Script executed:
# Find the parseModelString function to see the "$Y" case
rg -n "parseModelString" src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js | head -20Repository: shakacode/react_on_rails_rsc
Length of output: 214
🏁 Script executed:
# Read the parseModelString function (starting at line 1352)
sed -n '1352,1500p' src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.jsRepository: shakacode/react_on_rails_rsc
Length of output: 4741
🏁 Script executed:
# Find the reviveModel function
rg -n "function reviveModel" src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.jsRepository: shakacode/react_on_rails_rsc
Length of output: 139
🏁 Script executed:
# Read the reviveModel function (starting at line 2125)
sed -n '2125,2210p' src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.jsRepository: shakacode/react_on_rails_rsc
Length of output: 3158
Prevent strict-mode TypeError when reviveModel assigns to a $Y accessor property.
When parseModelString("$Y") executes, it defines a non-configurable getter-only accessor on parentObject[key], then returns null. The reviveModel function subsequently attempts to assign back to that key at line 2133 (array case) and lines 2200–2203 (object case). In strict mode, assigning to a property with a getter but no setter throws TypeError. Additionally, the object iteration loop at line 2196 uses for...in without filtering inherited properties, which can traverse prototype chain properties not present in the original object.
Guard revived property assignments and use own-property-only iteration:
Suggested fix
function parseModel(response, json) {
json = JSON.parse(json);
return reviveModel(response, json, { "": json }, "");
}
+ function setRevivedProperty(target, key, revivedValue) {
+ var descriptor = Object.getOwnPropertyDescriptor(target, key);
+ if (!descriptor || descriptor.writable || descriptor.set)
+ target[key] = revivedValue;
+ }
function reviveModel(response, value, parentObject, key) {
if ("string" === typeof value)
return "$" === value[0]
? parseModelString(response, parentObject, key, value)
@@
if ("object" !== typeof value || null === value) return value;
if (isArrayImpl(value)) {
for (var i = 0; i < value.length; i++)
- value[i] = reviveModel(response, value[i], value, "" + i);
+ setRevivedProperty(
+ value,
+ "" + i,
+ reviveModel(response, value[i], value, "" + i)
+ );
@@
- for (i in value)
- "__proto__" === i
- ? delete value[i]
- : ((parentObject = reviveModel(response, value[i], value, i)),
- void 0 !== parentObject
- ? (value[i] = parentObject)
- : delete value[i]);
+ for (i in value)
+ if (hasOwnProperty.call(value, i))
+ "__proto__" === i
+ ? delete value[i]
+ : ((parentObject = reviveModel(response, value[i], value, i)),
+ void 0 !== parentObject
+ ? setRevivedProperty(value, i, parentObject)
+ : delete value[i]);| for (i in value) | ||
| "__proto__" === i | ||
| ? delete value[i] | ||
| : ((parentObject = reviveModel(response, value[i], value, i)), | ||
| void 0 !== parentObject ? (value[i] = parentObject) : delete value[i]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrates why `for...in` is not equivalent to JSON reviver/object-own traversal.
node <<'NODE'
"use strict";
Object.prototype.__rscInherited = "$undefined";
const model = JSON.parse('{"own":1}');
const keys = [];
for (const key in model) keys.push(key);
delete Object.prototype.__rscInherited;
console.log(keys.join(","));
// Current hazard: "own,__rscInherited"
// Expected after own-property guard: "own"
NODERepository: shakacode/react_on_rails_rsc
Length of output: 90
🏁 Script executed:
find . -name "react-server-dom-webpack-client.node.production.js" -type fRepository: shakacode/react_on_rails_rsc
Length of output: 157
🏁 Script executed:
sed -n '1655,1670p' ./src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.jsRepository: shakacode/react_on_rails_rsc
Length of output: 602
🏁 Script executed:
sed -n '1630,1670p' ./src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.jsRepository: shakacode/react_on_rails_rsc
Length of output: 1432
🏁 Script executed:
sed -n '1600,1670p' ./src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.jsRepository: shakacode/react_on_rails_rsc
Length of output: 2467
🏁 Script executed:
sed -n '1, 30p' ./src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.jsRepository: shakacode/react_on_rails_rsc
Length of output: 1104
🏁 Script executed:
node <<'NODE'
"use strict";
// Simulate the vulnerable code pattern from the file
const hasOwnProperty = Object.prototype.hasOwnProperty;
function vulnerableRevive(value) {
let result = {};
for (let i in value) {
if ("__proto__" === i) {
delete value[i];
} else {
result[i] = value[i];
}
}
return result;
}
function protectedRevive(value) {
let result = {};
for (let i in value) {
if (hasOwnProperty.call(value, i)) {
if ("__proto__" === i) {
delete value[i];
} else {
result[i] = value[i];
}
}
}
return result;
}
// Test: pollute Object.prototype
Object.prototype.__polluted = "evil";
const cleanJson = JSON.parse('{"own":"data"}');
console.log("Vulnerable revive picks up inherited:", Object.keys(vulnerableRevive(cleanJson)).length > 1);
console.log("Protected revive ignores inherited:", Object.keys(protectedRevive(cleanJson)).length === 1);
delete Object.prototype.__polluted;
NODERepository: shakacode/react_on_rails_rsc
Length of output: 155
Add hasOwnProperty guard to prevent inherited property enumeration.
Line 1660 uses for...in to traverse object properties, which enumerates inherited enumerable properties from the prototype chain. The previous JSON.parse reviver behavior only walked own properties. If Object.prototype is polluted before this code runs, those inherited keys will be copied into every revived model object.
Suggested guard
- for (i in value)
- "__proto__" === i
- ? delete value[i]
- : ((parentObject = reviveModel(response, value[i], value, i)),
- void 0 !== parentObject ? (value[i] = parentObject) : delete value[i]);
+ for (i in value)
+ if (hasOwnProperty.call(value, i))
+ "__proto__" === i
+ ? delete value[i]
+ : ((parentObject = reviveModel(response, value[i], value, i)),
+ void 0 !== parentObject
+ ? (value[i] = parentObject)
+ : delete value[i]);The hasOwnProperty variable is already available at the module level and the guard is straightforward to apply.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (i in value) | |
| "__proto__" === i | |
| ? delete value[i] | |
| : ((parentObject = reviveModel(response, value[i], value, i)), | |
| void 0 !== parentObject ? (value[i] = parentObject) : delete value[i]); | |
| for (i in value) | |
| if (hasOwnProperty.call(value, i)) | |
| "__proto__" === i | |
| ? delete value[i] | |
| : ((parentObject = reviveModel(response, value[i], value, i)), | |
| void 0 !== parentObject | |
| ? (value[i] = parentObject) | |
| : delete value[i]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.js`
around lines 1660 - 1664, The loop that iterates properties in reviveModel
currently uses "for (i in value)" which can include inherited properties; update
that loop to skip non-own properties by checking hasOwnProperty.call(value, i)
before processing each key (use the existing module-level hasOwnProperty).
Ensure the existing logic (skipping "__proto__", calling reviveModel and
assigning or deleting parentObject) only runs when hasOwnProperty.call(value, i)
is true, so inherited prototype properties are not revived into the model.
… payload Rebuild react-server-dom-webpack from React fork rsc-patches/v19.0.3 with the reviveModel optimization (backport of facebook/react#35776). Replaces JSON.parse reviver callback with plain JSON.parse() followed by a recursive reviveModel() walk in pure JS, yielding ~75% speedup in RSC chunk deserialization by eliminating C++→JS boundary crossings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AbanoubGhadban
force-pushed
the
perf/rsc-revive-model-walk
branch
from
aefcbd7 to
67b27f0
Compare
Code ReviewOverviewThis PR backports facebook/react#35776, replacing The optimization is legitimate and well-documented. A few issues are worth addressing before merge: Bug: Redundant duplicate check in
|
| if (value[0] === REACT_ELEMENT_TYPE) { | ||
| if (value[0] === REACT_ELEMENT_TYPE) |
There was a problem hiding this comment.
The outer if (value[0] === REACT_ELEMENT_TYPE) on line 1461 and this inner check are identical. Once the outer guard passes, this inner condition is always true, making the else response = value on line 1489 dead code that can never execute.
The correct structure (matching the upstream intent) should collapse these into a single check with the labeled block applied directly:
| if (value[0] === REACT_ELEMENT_TYPE) { | |
| if (value[0] === REACT_ELEMENT_TYPE) | |
| if (value[0] === REACT_ELEMENT_TYPE) |
And remove the outer if (value[0] === REACT_ELEMENT_TYPE) { wrapper (line 1461) so the else response = value; branch is actually reachable for non-React-element arrays. The same fix applies to all 8 react-server-dom-webpack-client.*.js files.
| try { | ||
| for (_iterator.s(); !(_step = _iterator.n()).done; ) { | ||
| const file = _step.value; | ||
| if (!file.endsWith(".js")) break; | ||
| if (file.endsWith(".hot-update.js")) break; | ||
| chunks.push(c.id, file); | ||
| break; | ||
| if ( | ||
| file.endsWith(".js") && | ||
| !file.endsWith(".hot-update.js") | ||
| ) { | ||
| chunks.push(c.id, file); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Good fix. The old code had three sequential break statements — the first two exited the loop early on non-JS or hot-update files, which meant a chunk whose file list started with a CSS/asset file would never have its JS file pushed to the manifest. The new condition correctly skips non-matching files and only breaks after a successful push.
| get: function () { | ||
| return "This object has been omitted by React in the console log to avoid sending too much data from the server. Try logging smaller or more specific objects."; | ||
| }, | ||
| set: function () {}, |
There was a problem hiding this comment.
The empty setter prevents a TypeError in strict mode when consuming code (e.g., a serializer or proxy) tries to assign to the omitted-object placeholder property. Without it, configurable: false + no setter = silent failure in sloppy mode but a throw in strict mode. Intentional and correct — just confirm this matches the upstream source in the React fork.
1 participant
Summary
Closes #34
Backport of facebook/react#35776 — ~75% speedup in RSC chunk deserialization.
Rebuilt
react-server-dom-webpackfrom React forkrsc-patches/v19.0.3with the optimization applied (fork PR).The optimization
Replaces
JSON.parse(json, reviver)with a two-step approach:JSON.parse(json)— runs entirely in V8's C++ engine, no callbacksreviveModel()— recursive pure-JS walk that applies RSC transformationsThe reviver callback was the bottleneck: V8's
JSON.parseis C++, and calling back into JS for every key-value pair incurs ~4x overhead even with a no-op reviver.Benchmark results (from upstream PR)
Additional changes in this rebuild
The plugin file also picks up the compiled form of
[RSC-PATCH] Fix client manifest chunk scan when non-JS assets precede JSfrom source (previously patched manually on main via #23).Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit