Skip to content

fix(json-crdt): guard ObjNode.children against missing nodes#1047

Open
privateJiangyaokai wants to merge 1 commit into
streamich:masterfrom
privateJiangyaokai:fix/objnode-children-missing-node-guard
Open

fix(json-crdt): guard ObjNode.children against missing nodes#1047
privateJiangyaokai wants to merge 1 commit into
streamich:masterfrom
privateJiangyaokai:fix/objnode-children-missing-node-guard

Conversation

@privateJiangyaokai

Copy link
Copy Markdown

Problem

ObjNode.children() passes the result of index.get(id) straight to the callback with a non-null assertion:

public children(callback: (node: JsonNode) => void) {
  const index = this.doc.index;
  this.keys.forEach((id, key) => callback(index.get(id)!));
}

index.get(id) can legitimately return undefined when a key references a node id that is not present in the model index (e.g. a dangling reference encountered while applying a patch). The ! assertion hides this, so undefined reaches the callback.

The most visible symptom is a crash in Model._gcTree (formerly deleteNodeTree), which reads value.sid on its argument before the index.get existence check:

_gcTree(value: ITimestampStruct) {
  const isSystemNode = value.sid === 0;  // TypeError: Cannot read properties of undefined (reading 'sid')
  ...
  node.children((child) => this._gcTree(child.id));
}

When an ObjNode has a child key pointing at a missing node, children() invokes the callback with undefined, and _gcTree(undefined) throws Cannot read properties of undefined (reading 'sid'), aborting the whole patch/GC pass.

Fix

Guard against the missing node before invoking the callback — exactly what ArrNode.children() and VecNode.children() already do:

public children(callback: (node: JsonNode) => void) {
  const index = this.doc.index;
  this.keys.forEach((id) => {
    const node = index.get(id);
    if (node) callback(node);
  });
}

This makes ObjNode consistent with the other collection nodes:

  • ArrNode.children: const node = index.get(data[i]); if (node) callback(node);
  • VecNode.children: const node = index.get(id); if (node) callback(node);

Only ObjNode was missing the guard (and additionally used an unsound !). No behavior change for well-formed models; it only prevents a hard crash when a dangling reference is present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant