Shopify / Shopify/remote-dom

Incorrect Node Index Calculation and Duplicate Insertions in `RemoteMutationObserver`

Open
#560 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
1.3k
Forks
91
Avg merge
9d 14h
Merged PRs (30d)
38

Description

Description

We have observed persistent issues with RemoteMutationObserver when used in combination with the iframe sandbox. Specifically, problems arise when mutation records contain interleaving removals and additions of nodes, leading to incorrect behavior where nodes are not properly added or removed. Additionally, under certain conditions, nodes may be inserted multiple times on the host side.

Steps to Reproduce

A reproducible scenario for the index issue can be found in the kitchen-sink demo app of this fork, where a mutation replaces two child nodes with two new child nodes.

Initial DOM State
<ui-stack>
  <ui-text>Loading 1</ui-text>
  <ui-text>Loading 2</ui-text>
</ui-stack>
After State Update
<ui-stack>
  <ui-button>Button 1</ui-button>
  <ui-button>Button 2</ui-button>
</ui-stack>
Detected Mutations by MutationObserver
Action Previous Sibling Node Next Sibling
Remove null <ui-text>Loading 1</ui-text> <ui-text>Loading 2</ui-text>
Insert <ui-text>Loading 2</ui-text> <ui-button>Button 1</ui-button> null
Remove null <ui-text>Loading 2</ui-text> <ui-button>Button 1</ui-button>
Insert <ui-button>Button 1</ui-button> <ui-button>Button 2</ui-button> null
Mutation Records Generated by RemoteMutationObserver
Action Node Index
Remove null 0
Insert <ui-button>Button 1</ui-button> 0
Remove null 0
Insert <ui-button>Button 2</ui-button> 1
Incorrect Final DOM State
<ui-stack>
  <ui-button>Loading 2</ui-button>
  <ui-button>Button 2</ui-button>
</ui-stack>

In this state, <ui-text>Loading 2</ui-text> was incorrectly retained instead of being removed, and <ui-button>Button 1</ui-button> is missing.

A small side node

Prior to this bugfix, the second example here even caused a crash, as described in this issue. However, this fix merely acts as a temporary patch rather than addressing the root problem. I even would suggest to be more strict (throwing errors) when invariants are not met, like a node that could not be found in the receivers lookup registries. This is how errors in the algorithm or logic are first noticed and subsequent errors are avoided.

Root Cause Analysis

1. Unreliable Index Calculation
  • The issue stems from how RemoteMutationObserver determines the index of inserted and removed nodes.
  • When processing multiple mutation records within the same mutation event, the index calculation is unreliable.
  • This is because the mutation event occurs after the mutation has already been applied.
  • The removed node <ui-text>Loading 2</ui-text> no longer exists in the parent’s child list at the time of index determination.
  • As a result, the algorithm incorrectly assumes an index of 0, leading to incorrect mutations.
2. Duplicate Insertions
  • In rare cases, nodes are inserted multiple times on the host side.
  • This typically occurs when replacing a node with a subtree of deeply nested nodes.
  • Although RemoteMutationObserver tries to avoid duplicate insertions using addedNode.contains(node), this check is insufficient because it only detects direct children, not deep descendants.

Expected Behavior

RemoteMutationObserver should ensure correct node insertions and removals, even when multiple mutation records are involved or deeply nested structures are inserted. It should also prevent duplicate insertions of the same node.

Proposed Solution

1. Use Reference Nodes Instead of Indices

A more reliable approach is to replace index-based positioning with remote IDs of reference nodes:

  • For removals: The removed node itself serves as the reference.
  • For insertions: The reference should be the next or previous sibling node, with the next sibling recommended, as this simplifies processing on the receiving end:
    • If the successor is null, the node should be appended.
    • Otherwise, the node should be inserted before the reference.
2. Prevent Duplicate Insertions on the Host Side
  • Rather than implementing recursive descendant checks in RemoteMutationObserver (which would be costly), the host/receiver now checks before inserting a node whether it already exists under the expected parent.
  • Since the parent is known, this check is efficient and avoids unnecessary overhead.

This combination ensures both correctness and performance across a range of complex mutation scenarios.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with RemoteMutationObserver and the reproduction in examples/kitchen-sink/app/remote/examples/react-mutations.tsx. Trace how records are converted into remote mutations and how the host/receiver inserts nodes, paying particular attention to interleaved removals, additions, and nested subtrees. Done means replacement sequences preserve the expected DOM and no node is inserted more than once.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend, web-dev
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.