fix(css): release the printed-text store's text when it is dropped - #17
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: webpack#21958 Source head: 9f3fa23
⛔ Shipwright · BlockedRecommendation: do not merge PR #17 · Tier
Findings (6)
Fireworks usage: 8,781 input · 1,303 output · 10,084 total tokens · $0.0028 · 20s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| * Move the epoch on and release the text every slot written under the old one | ||
| * holds. Only the range actually written is walked, so the drop stays O(1) | ||
| * per node. | ||
| * @returns {void} |
There was a problem hiding this comment.
Shipwright · CRITICAL
The '_moveEpoch' loop clears slots from '_storeLow' to '_storeHigh', but '_storeLow' is initialized to '0x7fffffff' and only updated in '_store' when 'n < _storeLow'.
Impact: The '_moveEpoch' loop clears slots from '_storeLow' to '_storeHigh', but '_storeLow' is initialized to '0x7fffffff' and only updated in '_store' when 'n < _storeLow'. If '_store' is never called before '_moveEpoch' (e.g., 'dropStore()' invoked on a fresh context), '_storeHigh' is '-1' so the loop is skipped — that path is safe. However, if '_store' is called with a node index 'n' that is less than the current '_s…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| // Dropping the store has to let go of the text, not just stop answering for | ||
| // it: a stylesheet prints ~300k nodes, which is the output over again. | ||
| describe("printed-text store", () => { | ||
| /** |
There was a problem hiding this comment.
Shipwright · HIGH
The test helper 'held' directly accesses 'context._storeText' and sums '.length' of each entry, but it does not account for the fact that '_storeText' may contain 'undefined' holes
Impact: The test helper 'held' directly accesses 'context._storeText' and sums '.length' of each entry, but it does not account for the fact that '_storeText' may contain 'undefined' holes if '_growStore' allocates a larger array without filling all slots. The test 'lets go of the text when the epoch moves' asserts 'held(context)' is exactly '500 * 64' after printing 500 nodes, which passes only because 'printNode' fills ev…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| * Move the epoch on and release the text every slot written under the old one | ||
| * holds. Only the range actually written is walked, so the drop stays O(1) | ||
| * per node. | ||
| * @returns {void} |
There was a problem hiding this comment.
Shipwright · HIGH
The '_moveEpoch' loop writes '""' to every slot in '[_storeLow, _storeHigh]', but '_storeGen' is not cleared for those slots.
Impact: The '_moveEpoch' loop writes '""' to every slot in '[_storeLow, _storeHigh]', but '_storeGen' is not cleared for those slots. A subsequent 'get(node)' checks '_storeGen[n] === this._gen'; after '_gen++', old slots read as absent, so this is correct for the epoch check. However, '_storeText' is a dense array and the loop is O(range) per drop. The comment claims 'the drop stays O(1) per node', but if a single node wit…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| NodeType: HtmlNodeType, | ||
| SourceProcessor: HtmlSourceProcessor | ||
| } = require("../lib/html/syntax"); | ||
| const { PrintContext } = require("../lib/util/SourceProcessor"); |
There was a problem hiding this comment.
Shipwright · HIGH
The change introduces a manual memory-management loop that clears string references to release memory.
Impact: The change introduces a manual memory-management loop that clears string references to release memory. While not a direct security vulnerability, the pattern of manually nulling array slots to free memory is error-prone: if '_storeText' is ever aliased or exposed (e.g., via a getter or debug path), the clearing loop could mutate a shared reference and cause use-after-free-style logic bugs where one consumer sees '""…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| * Move the epoch on and release the text every slot written under the old one | ||
| * holds. Only the range actually written is walked, so the drop stays O(1) | ||
| * per node. | ||
| * @returns {void} |
There was a problem hiding this comment.
Shipwright · HIGH
The '_moveEpoch' method is called from 'dropStore', 'take', and 'takeRetractable', but there is no synchronization or guard against reentrancy.
Impact: The '_moveEpoch' method is called from 'dropStore', 'take', and 'takeRetractable', but there is no synchronization or guard against reentrancy. If a printer callback (the 'printer' function passed to 'PrintContext') triggers a nested 'dropStore' or 'take' during the '_moveEpoch' loop, the epoch counter and store bounds could be left in an inconsistent state, causing text to be released prematurely or retained indefi…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Summary
PrintContext.dropStore()moved the epoch on so the store stopped answering for a node, but left the node's text referenced in the column — so text the method documents as dropped stayed reachable until that slot happened to be reused. The epoch move now clears the range written since the last one, which keeps the drop O(1) per node and caps store occupancy at 46 KB rather than 328 KB on a 30 MB stylesheet.Output is byte-identical on all 20
benchmark:css-minifiersfixtures, and neither CPU nor peak RSS moves measurably — this is a correctness fix to the store, not a performance claim.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes — three cases under
printed-text storeintest/SourceProcessor.unittest.js; two of them fail without the change.Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Yes — Claude Code was used to investigate the store's retention behaviour, write the change and the tests, and run the measurements (exact store-occupancy accounting, CPU and peak-RSS comparison over 11 stylesheets, and
yarn benchmark:css-minifiers). An earlier framing of this as a performance improvement was dropped because the CPU and RSS deltas did not reproduce across independent runs; only the counted occupancy result is claimed above.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Source merge-base:
7cb6dd49a74909407d55faad27f75a3d8542cf7dSource head:
9f3fa235b42ec449f0d16c2fa7274f782ca4336e