Stop plugin content scripts from crashing the app on remove - #1456
Conversation
A content script that wraps a React-owned button or link moves that node out of the tree React expects. The next list remove or reorder then throws NotFoundError during commit. Keep the existing removeChild/insertBefore guard, extend it to replaceChild, and refuse those reparents while a plugin script or its observer runs. The plugin can still insert sibling controls.
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request now. I will check security, code quality, performance, architecture, and product behavior. |
| native.apply(this, kept); | ||
| }; | ||
|
|
||
| Node.prototype.removeChild = guardedRemoveChild; |
There was a problem hiding this comment.
🚨 slopcop/review — Common DOM move operations bypass this wrapper list.
replaceChildren, insertAdjacentElement, and Range.insertNode remain native. A plugin can also call host.replaceWith(group) and then append the detached host node.
These paths move a React-owned node while isolation is active. Please cover each production move path with Chromium tests. Consider an architecture that does not depend on a partial method list.
| nativeFragmentPrepend, | ||
| ); | ||
|
|
||
| if (NativeMutationObserver !== null) { |
There was a problem hiding this comment.
🚨 slopcop/review — The MutationObserver replacement breaks native constructor behavior.
This function always returns a native observer and ignores new.target. Therefore, an observer subclass does not create an instance of that subclass.
The replacement also works without new. Please preserve both native constructor rules and subclass identity. Add a real browser test for both cases.
|
|
||
| act(() => root.unmount()); | ||
| container.remove(); | ||
| uninstallForeignDomMutationGuardForTest(); |
There was a problem hiding this comment.
🚨 slopcop/review — A failed assertion can leave global DOM prototypes changed.
This cleanup runs only after all earlier assertions pass. A failure will affect later tests in the same file.
Please move the uninstall call to afterEach or use a finally block.
| @@ -506,38 +507,45 @@ async function mountWithTimeout( | |||
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | |||
| let timedOut = false; | |||
| const mountPromise = Promise.resolve().then(() => | |||
There was a problem hiding this comment.
🚨 slopcop/review — Supported asynchronous mounts escape the DOM isolation.
The SDK permits mount to return a Promise. This wrapper clears its state before the first awaited continuation runs.
I reproduced this path in Chromium. A mount used await Promise.resolve() and then moved the React-owned Settings link into a new span. The guard logged no refusal.
Please apply the same policy to supported asynchronous work and later callbacks. Add production-path tests for an async mount, an async disposer, and an event callback.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: React owns pieces on a board. This patch stops one plugin move, but other supported moves still get through.
I found four reliability defects that should block the merge. I left a comment-only review, as required.
-
Async plugin work escapes the guard. The SDK permits async mounts and cleanup functions.
A Chromium test confirmed the gap. The synchronous plugin move failed, and the Settings link stayed with React.
The same plugin used one
await. It then moved the Settings link into its wrapper, and the guard logged no refusal. -
Common DOM move APIs bypass the wrapper list. These APIs include
replaceChildren,insertAdjacentElement, andRange.insertNode.A plugin can also detach a React node first. The guard then permits the later append because the node has no parent.
-
The MutationObserver replacement breaks native constructor rules. An observer subclass no longer creates an instance of that subclass.
-
The SDK test host and plugin documentation do not match the new production restriction.
The new integration test also needs reliable cleanup. An early assertion failure currently leaves global DOM prototypes changed.
The narrow replaceChild fallback looks useful. The broader isolation layer needs a clear contract and one shared conformance test set.
I found no separate security defect. Content scripts already run as trusted same-origin code, so this guard is a reliability control.
I found no duplicate DOM guard. The architectural risk comes from a partial global method list and different production and test-host behavior.
The performance worker measured a 14 percent median increase in a raw append-and-remove benchmark. This result does not prove user-visible delay.
Validation passed for the focused 30 tests and the @bb/app Turbo typecheck. All current GitHub checks also pass.
The final GPT-5.6 review gate returned REQUEST CHANGES. I did not use the GitHub request-changes option.
Hold isolation across async mount and dispose, and drop it if the host aborts a stuck mount. Cover replaceChildren, insertAdjacentElement, and Range.insertNode. Keep MutationObserver subclass identity. Restore the guard after every plugin-frontend test.
|
Addressed the SlopCop review on this head:
I am not copying this fence into
|
1 participant
Why
A user plugin (File Reveal) wraps host links and buttons in a new span. React still treats the old parent as the owner. The next remove or reorder of that same node throws
NotFoundErrorduring commit and can blank the window.PR #1418 already made
removeChildandinsertBeforenon-fatal. That was not enough. The plugin still stole React-owned nodes, leftover wrappers stayed in the tree, andreplaceChildcould still throw.What
replaceChildand swallowNotFoundErroron the existing paths.Test plan
pnpm exec turbo run typecheck --filter=@bb/apppnpm exec vitest run --config vitest.config.ts src/lib/foreign-dom-mutation-guard.test.tsx src/lib/plugin-frontend-reload.test.ts(30 passed)Risks
The expensive fiber check runs only while a plugin content script (or its observer) is moving a node. React commits hit one integer check and then the native method. Same cost class as the #1418 guard.