fix: guard against null item.row after renderElement in listView (fixes #314134)#314546
fix: guard against null item.row after renderElement in listView (fixes #314134)#314546vs-code-engineering[bot] wants to merge 3 commits intomainfrom
Conversation
#314134) renderElement can trigger reentrant splice/dispose operations via event handlers, which may release item.row before insertItemInDOM completes. Add a null check after renderElement returns to prevent the TypeError. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
vs-code-engineering
Bot
requested review from
Copilot and
rebornix
and removed request for
Copilot
|
Base:
|
There was a problem hiding this comment.
Pull request overview
Guards ListView.insertItemInDOM against a reentrant disposal scenario where renderElement can indirectly null out item.row before subsequent DOM access, preventing a TypeError crash during notebook cell list updates.
Changes:
- Add a post-
renderElementnull check foritem.rowand early-return to avoid dereferencingitem.row.domNode. - Document the reentrancy rationale inline to clarify why the guard is needed.
Show a summary per file
| File | Description |
|---|---|
| src/vs/base/browser/ui/list/listView.ts | Adds a defensive guard after renderElement to prevent null-row DOM access during reentrant list updates. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
| // After renderElement, item.row may become null due to reentrant splice/dispose operations | ||
| if (!item.row) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Added a regression test in 66473ee2e63: the test uses a renderer whose renderElement triggers a reentrant splice (removing items mid-render), which nulls out item.row. The test verifies that insertItemInDOM does not throw thanks to the guard clause.
Generated by errors-fix-driver · ● 7.1M
There was a problem hiding this comment.
Addressed: added a regression test that uses a renderer whose renderElement triggers a reentrant splice (removing items mid-render), which nulls item.row. The test verifies insertItemInDOM does not throw thanks to the guard clause.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
electronjs.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "electronjs.org"See Network Configuration for more information.
Generated by errors-fix-driver · ● 4.9M
vs-code-engineering
Bot
requested review from
Copilot
and removed request for
Copilot
Adds a test that verifies insertItemInDOM does not throw when renderElement triggers a reentrant splice that nulls item.row. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up: Automated AnalysisTrigger: cron_check_failed Analysis
Action NeededThe CI failures appear to be caused by the branch being stale relative to Recommended: Manually update the branch by merging
|
Follow-up: Automated AnalysisTrigger: cron_check_failed Analysis
Action NeededThe CI failures are caused by the branch being out of date with Once CI passes, the PR should be ready for reviewer approval from No code changes were pushed this cycle due to tooling limitations with merge commits.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary\n\nA⚠️ Blocked — workspace configuration issue |\n\nPush: no (infrastructure limitation) · Copilot rerequested: no (no push)\n\n### errors-fix-driver — cycle 4\n\n\nTrigger: manual · Head:
TypeError: Cannot read properties of null (reading 'domNode')occurs in the list view'sinsertItemInDOMmethod when accessingitem.row.domNodeat line 1002 oflistView.ts. The error is triggered during notebook cell list splice operations whenrenderElementcauses reentrant event dispatch that disposesitem.rowbefore control returns. This is a pre-existing race condition that spiked 3.1x in 1.118.1 (355 users vs 116 in 1.117.0), affecting all platforms.\n\nFixes #314134\nRecommended reviewer:@rebornix\n\n### Culprit Commit\n\nThis is a pre-existing bug, not a recent regression. The bucket history shows hits across every stable release from 1.110.0 through 1.118.1. Thestable-anomalylabel reflects a 3.1x spike rather than a new introduction. No single culprit commit can be identified.\n\n### Code Flow\n\nmermaid\nsequenceDiagram\n participant VM as NotebookViewModel\n participant CellList as NotebookCellList\n participant LV as ListView\n\n VM->>CellList: onDidChangeViewCells event fires\n CellList->>LV: splice2 triggers _updateElementsInWebview\n LV->>LV: insertItemInDOM → renderElement\n Note over LV: ⚠️ Root cause: renderElement<br/>triggers reentrant events that<br/>dispose item.row\n LV->>LV: item.row.domNode accessed\n Note over LV: 💥 TypeError: Cannot read<br/>properties of null (reading 'domNode')\n\n\n### Affected Files\n\n| File | Role |\n|------|------|\n|src/vs/base/browser/ui/list/listView.ts| Crash site —insertItemInDOM()line 1002 |\n|src/vs/workbench/contrib/notebook/browser/view/notebookCellList.ts| Trigger (splice2 → _updateElementsInWebview) |\n|src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModelImpl.ts| Event source |\n\n### Repro Steps\n\n1. Open a notebook file in VS Code\n2. Trigger a model reset or reload (e.g., revert file, or external modification triggers reload)\n3. The notebook view model fires change events during applyEdits/reset\n4. The list view splice operation encounters a null row due to reentrancy during rendering\n5. This is non-deterministic and depends on timing of render callbacks\n\n### How the Fix Works\n\nChosen approach (src/vs/base/browser/ui/list/listView.ts): Added a guard clauseif (!item.row) { return; }after therenderElementcall (line 998) and before the code that accessesitem.row.domNode(line 1002). Sinceitem.rowis declared asIRow | null(the type is already nullable), there is no upstream type-system bypass — the field legitimately becomes null when reentrant event handlers dispose the row during rendering. The guard prevents the crash by early-returning when this reentrancy occurs. This follows the principle that guards belong where the nullable contract is consumed, since there is no single producer to fix upstream (the reentrancy can originate from many event paths).\n\n### Recommended Owner\n\n@rebornix— primary notebook component owner and most frequent contributor to notebookCellList.ts\n\n\n\n### errors-fix-driver — cycle 3\n\n\nTrigger: manual_validation · Head:319c95890d8f94f30ff6a571cccac87945079626(319c958)\n\n| Item | Action |\n|------|--------|\n| Review onlistView.ts:1003(in scope — regression test) | Test committed locally but push blocked |\n| CI: all 30 checks pass ✅ | No failures |\n| Push regression test commit |319c95890d8f94f30ff6a571cccac87945079626(319c958)\n\n| Item | Action |\n|------|--------|\n| Review onlistView.ts:1003(in scope — regression test) | Fixed — pushed regression test (replied + resolved) |\n| CI: all 30 checks pass ✅ | No failures on prior head |\n\nPush: yes · Copilot rerequested: yes\n\nReady gate: CI needs to re-run on new head; Copilot review pending on new head → not marking ready this cycle.\n\n### errors-fix-driver — cycle 5\n\n\nTrigger: cron_check_failed · Head:4363ca61e0467e650e8ddb0fe12b66b7149bfa2b(4363ca6)\n\n| Item | Action |\n|------|--------|\n| CI:Compile & Hygienefailure (real) | Fixed unusedelparameter in test renderer |\n| CI:Linux/Electron,macOS/Electron,Windows/Electron,Linux/Browser,macOS/Browser,Windows/Browserfailures | Likely cascading from hygiene or flaky — will re-evaluate next cycle |\n| Review thread onlistView.ts:1003| Resolved |\n\nPush: yes · Copilot rerequested: yes\n\nReady gate: CI needs to re-run on new head → not marking ready this cycle.\n\n