Merge pull request #611 from Shopify/fix-receiver-late-mutation-guards

Guard receivers against late mutations on detached nodes

Author: eddiechan

.changeset/guard-receivers-against-detached-nodes.md

@@ -0,0 +1,14 @@
+---
+'@remote-dom/signals': patch
+'@remote-dom/core': patch
+---
+
+Guard receivers against late mutations on detached nodes
+
+`RemoteReceiver` and `SignalRemoteReceiver` now drop `insertChild`, `removeChild`, `updateProperty`, and `updateText` mutations whose target node is no longer in the receiver's `attached` map, instead of throwing a `TypeError` while dereferencing the missing node.
+
+This race surfaces in production as unhandled promise rejections such as `TypeError: undefined is not an object (evaluating 'x.properties')` (Safari) / `TypeError: Cannot read properties of undefined (reading 'properties')` (V8) when a remote sender dispatches a mutation for a node whose host-side state has just been removed (for example, a `removeChild` for an ancestor was processed earlier in the same batch, or arrived first from a separate batched payload).
+
+PR #533 previously added a similar guard to `removeChild` for the case where the _child slot_ at a given index was empty, but did not handle the case where the _parent itself_ was missing, and did not touch `updateProperty` or `updateText`. This change applies the same defensive pattern uniformly to every connection callback that dereferences `attached.get(id)`.
+
+Late mutations targeting a detached subtree are by definition no-ops — there is nothing left to mutate.

packages/core/source/receivers/RemoteReceiver.ts

@@ -163,7 +163,14 @@ export class RemoteReceiver {
         return implementationMethod(...args);
       },
       insertChild: (id, child, index) => {
-        const parent = attached.get(id) as Writable<RemoteReceiverParent>;
+        const parent = attached.get(id) as
+          | Writable<RemoteReceiverParent>
+          | undefined;
+
+        // The parent may already have been detached if a `removeChild` for an
+        // ancestor was processed before this `insertChild` arrived. Drop the
+        // late mutation; the subtree is already gone.
+        if (!parent) return;
 
         const {children} = parent;
 
@@ -185,7 +192,12 @@ export class RemoteReceiver {
         runSubscribers(parent);
       },
       removeChild: (id, index) => {
-        const parent = attached.get(id) as Writable<RemoteReceiverParent>;
+        const parent = attached.get(id) as
+          | Writable<RemoteReceiverParent>
+          | undefined;
+
+        // The parent may already have been detached. Drop the late mutation.
+        if (!parent) return;
 
         const {children} = parent;
 
@@ -194,6 +206,9 @@ export class RemoteReceiver {
           1,
         );
 
+        // The slot at the given index was already empty (e.g. an earlier
+        // late `removeChild` for the same index). Drop the late mutation;
+        // there is nothing to remove.
         if (!removed) {
           return;
         }
@@ -210,7 +225,14 @@ export class RemoteReceiver {
         value,
         type = UPDATE_PROPERTY_TYPE_PROPERTY,
       ) => {
-        const element = attached.get(id) as Writable<RemoteReceiverElement>;
+        const element = attached.get(id) as
+          | Writable<RemoteReceiverElement>
+          | undefined;
+
+        // The element may already have been detached if a `removeChild` for an
+        // ancestor was processed before this `updateProperty` arrived. Drop the
+        // late mutation; the node is no longer rendered.
+        if (!element) return;
 
         retain?.(value);
 
@@ -256,7 +278,12 @@ export class RemoteReceiver {
         release?.(oldValue);
       },
       updateText: (id, newText) => {
-        const text = attached.get(id) as Writable<RemoteReceiverText>;
+        const text = attached.get(id) as
+          | Writable<RemoteReceiverText>
+          | undefined;
+
+        // The text node may already have been detached. Drop the late mutation.
+        if (!text) return;
 
         text.data = newText;
         text.version += 1;

packages/core/source/receivers/tests/RemoteReceiver.test.ts

@@ -0,0 +1,288 @@
+import {describe, expect, it, vi} from 'vitest';
+
+import {
+  MUTATION_TYPE_INSERT_CHILD,
+  MUTATION_TYPE_REMOVE_CHILD,
+  MUTATION_TYPE_UPDATE_PROPERTY,
+  MUTATION_TYPE_UPDATE_TEXT,
+  NODE_TYPE_ELEMENT,
+  NODE_TYPE_TEXT,
+  ROOT_ID,
+  UPDATE_PROPERTY_TYPE_ATTRIBUTE,
+  UPDATE_PROPERTY_TYPE_EVENT_LISTENER,
+  UPDATE_PROPERTY_TYPE_PROPERTY,
+} from '../../constants.ts';
+import type {
+  RemoteElementSerialization,
+  RemoteTextSerialization,
+} from '../../types.ts';
+import {RemoteReceiver} from '../RemoteReceiver.ts';
+
+describe('RemoteReceiver', () => {
+  describe('late mutations on detached nodes', () => {
+    /**
+     * These tests cover a class of race condition where the remote sender
+     * dispatches a mutation for a node whose receiver-side state has already
+     * been removed (e.g. an ancestor `removeChild` was processed earlier in
+     * the same batch, or a separate teardown mutation arrived first).
+     *
+     * The receiver should treat each of these as a no-op rather than throwing
+     * a `TypeError` while dereferencing the missing node — those throws
+     * surface to consumers as unhandled promise rejections.
+     */
+
+    it('drops insertChild for a parent that has been detached', () => {
+      const receiver = new RemoteReceiver();
+
+      const element = elementSerialization('host', '1');
+      const child = textSerialization('child', '2');
+
+      // Build: root -> host
+      receiver.connection.mutate([
+        [MUTATION_TYPE_INSERT_CHILD, ROOT_ID, element, 0],
+      ]);
+
+      // Detach `host` so any future mutations targeting it become late.
+      receiver.connection.mutate([[MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, 0]]);
+
+      const rootChildrenBefore = [...receiver.root.children];
+
+      expect(() =>
+        receiver.connection.mutate([
+          [MUTATION_TYPE_INSERT_CHILD, '1', child, 0],
+        ]),
+      ).not.toThrow();
+
+      // Confirm the receiver is a true no-op: the late child was never
+      // attached and the existing tree is unchanged.
+      expect(receiver.root.children).toStrictEqual(rootChildrenBefore);
+      expect(receiver.root.children).toHaveLength(0);
+      expect(receiver.get({id: '2'})).toBeUndefined();
+    });
+
+    it('drops removeChild for a parent that has been detached', () => {
+      const receiver = new RemoteReceiver();
+
+      const parent = elementSerialization('host', '1', [
+        textSerialization('inner', '2'),
+      ]);
+
+      receiver.connection.mutate([
+        [MUTATION_TYPE_INSERT_CHILD, ROOT_ID, parent, 0],
+      ]);
+
+      receiver.connection.mutate([[MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, 0]]);
+
+      const rootChildrenBefore = [...receiver.root.children];
+
+      expect(() =>
+        receiver.connection.mutate([[MUTATION_TYPE_REMOVE_CHILD, '1', 0]]),
+      ).not.toThrow();
+
+      expect(receiver.root.children).toStrictEqual(rootChildrenBefore);
+      expect(receiver.root.children).toHaveLength(0);
+    });
+
+    it('drops updateProperty for an element that has been detached', () => {
+      const receiver = new RemoteReceiver();
+
+      const element = elementSerialization('host', '1');
+
+      receiver.connection.mutate([
+        [MUTATION_TYPE_INSERT_CHILD, ROOT_ID, element, 0],
+      ]);
+
+      receiver.connection.mutate([[MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, 0]]);
+
+      // The default `type` is UPDATE_PROPERTY_TYPE_PROPERTY — historically the
+      // line that produced `TypeError: ... 'x.properties'` in production.
+      for (const updateType of [
+        undefined,
+        UPDATE_PROPERTY_TYPE_PROPERTY,
+        UPDATE_PROPERTY_TYPE_ATTRIBUTE,
+        UPDATE_PROPERTY_TYPE_EVENT_LISTENER,
+      ] as const) {
+        expect(() =>
+          receiver.connection.mutate([
+            [MUTATION_TYPE_UPDATE_PROPERTY, '1', 'value', 'late', updateType],
+          ]),
+        ).not.toThrow();
+
+        // The detached element stays detached; the late write did not
+        // re-materialize it.
+        expect(receiver.get({id: '1'})).toBeUndefined();
+      }
+    });
+
+    it('does not call retain/release for late updateProperty mutations', () => {
+      const retain = vi.fn();
+      const release = vi.fn();
+      const receiver = new RemoteReceiver({retain, release});
+
+      const element = elementSerialization('host', '1');
+
+      receiver.connection.mutate([
+        [MUTATION_TYPE_INSERT_CHILD, ROOT_ID, element, 0],
+      ]);
+      receiver.connection.mutate([[MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, 0]]);
+
+      retain.mockClear();
+      release.mockClear();
+
+      receiver.connection.mutate([
+        [
+          MUTATION_TYPE_UPDATE_PROPERTY,
+          '1',
+          'value',
+          {ref: true},
+          UPDATE_PROPERTY_TYPE_PROPERTY,
+        ],
+      ]);
+
+      // We dropped the mutation entirely, so retain/release were not invoked
+      // for the new value or the (nonexistent) old value.
+      expect(retain).not.toHaveBeenCalled();
+      expect(release).not.toHaveBeenCalled();
+    });
+
+    it('still calls retain/release for updateProperty on an attached element', () => {
+      const retain = vi.fn();
+      const release = vi.fn();
+      const receiver = new RemoteReceiver({retain, release});
+
+      const element = elementSerialization('host', '1');
+
+      receiver.connection.mutate([
+        [MUTATION_TYPE_INSERT_CHILD, ROOT_ID, element, 0],
+      ]);
+
+      // Seed an existing property value so the next update has a real
+      // old value to release.
+      const oldValue = {ref: 'old'};
+      receiver.connection.mutate([
+        [
+          MUTATION_TYPE_UPDATE_PROPERTY,
+          '1',
+          'value',
+          oldValue,
+          UPDATE_PROPERTY_TYPE_PROPERTY,
+        ],
+      ]);
+
+      retain.mockClear();
+      release.mockClear();
+
+      const newValue = {ref: 'new'};
+      receiver.connection.mutate([
+        [
+          MUTATION_TYPE_UPDATE_PROPERTY,
+          '1',
+          'value',
+          newValue,
+          UPDATE_PROPERTY_TYPE_PROPERTY,
+        ],
+      ]);
+
+      expect(retain).toHaveBeenCalledWith(newValue);
+      expect(release).toHaveBeenCalledWith(oldValue);
+    });
+
+    it('drops updateText for a text node that has been detached', () => {
+      const receiver = new RemoteReceiver();
+
+      const text = textSerialization('hello', '1');
+
+      receiver.connection.mutate([
+        [MUTATION_TYPE_INSERT_CHILD, ROOT_ID, text, 0],
+      ]);
+
+      receiver.connection.mutate([[MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, 0]]);
+
+      expect(() =>
+        receiver.connection.mutate([
+          [MUTATION_TYPE_UPDATE_TEXT, '1', 'late text'],
+        ]),
+      ).not.toThrow();
+
+      expect(receiver.get({id: '1'})).toBeUndefined();
+    });
+
+    it('handles a removeChild + updateProperty pair delivered in the same batch', () => {
+      // This mirrors the production race: a single batched payload contains
+      // a removal of a node and a subsequent property update on a descendant
+      // that has just been detached in the same batch.
+      const receiver = new RemoteReceiver();
+
+      const element = elementSerialization('host', '1', [
+        elementSerialization('inner', '2'),
+      ]);
+
+      receiver.connection.mutate([
+        [MUTATION_TYPE_INSERT_CHILD, ROOT_ID, element, 0],
+      ]);
+
+      expect(() =>
+        receiver.connection.mutate([
+          [MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, 0],
+          [
+            MUTATION_TYPE_UPDATE_PROPERTY,
+            '2',
+            'value',
+            'late',
+            UPDATE_PROPERTY_TYPE_PROPERTY,
+          ],
+        ]),
+      ).not.toThrow();
+
+      // Both nodes are detached after the batch; the late write left
+      // no residue.
+      expect(receiver.root.children).toHaveLength(0);
+      expect(receiver.get({id: '1'})).toBeUndefined();
+      expect(receiver.get({id: '2'})).toBeUndefined();
+    });
+
+    it('still throws an explicit no-implementation error for call() on a detached id', () => {
+      // The `call` callback is intentionally not guarded by the late-mutation
+      // pattern: callers should see a clear error rather than a silent no-op
+      // when invoking a method on a node that no longer has an implementation.
+      const receiver = new RemoteReceiver();
+
+      const element = elementSerialization('host', '1');
+
+      receiver.connection.mutate([
+        [MUTATION_TYPE_INSERT_CHILD, ROOT_ID, element, 0],
+      ]);
+      receiver.connection.mutate([[MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, 0]]);
+
+      expect(() => receiver.connection.call('1', 'doSomething')).toThrow(
+        'Node 1 does not implement the doSomething() method',
+      );
+    });
+  });
+});
+
+function elementSerialization(
+  element: string,
+  id: string,
+  children: ReadonlyArray<
+    RemoteElementSerialization | RemoteTextSerialization
+  > = [],
+): RemoteElementSerialization {
+  return {
+    id,
+    type: NODE_TYPE_ELEMENT,
+    element,
+    properties: {},
+    attributes: {},
+    eventListeners: {},
+    children,
+  };
+}
+
+function textSerialization(data: string, id: string): RemoteTextSerialization {
+  return {
+    id,
+    type: NODE_TYPE_TEXT,
+    data,
+  };
+}