Skip to content

Xml modify in place - #1124

Merged
xeioex merged 5 commits into
nginx:masterfrom
xeioex:xml-modify-in-place
Sep 11, 2026
Merged

Xml modify in place#1124
xeioex merged 5 commits into
nginx:masterfrom
xeioex:xml-modify-in-place

Conversation

@xeioex

@xeioex xeioex commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Previously, text and structural mutators copied and replaced the receiver's libxml2 node. Existing XMLNode wrappers continued to point at the detached original, while subsequent lookups returned the replacement. Wrappers for
children preserved by the operation likewise pointed into the detached old subtree. In QuickJS, mutating the detached receiver again could dereference the NULL result of xmlReplaceNode().

The fix is to modify the existing receiver's child list directly. Existing wrappers and subsequent lookups therefore continue to refer to the same libxml2 node. Removed child lists remain readable until the QuickJS document or njs VM is destroyed. A node is considered live only when following its parent links reaches its xmlDoc. Replacement children and cleanup records are prepared before the live tree is modified.

Changed behavior:

  • A removed child has an undefined $parent, while its descendants retain
    their internal parent links.
  • Mutating or canonicalizing a removed XMLNode throws TypeError.
  • Text and attribute values must be primitive strings; null and undefined
    retain their deletion semantics.
  • C14N rejects an excluding node from another document or a detached list.
  • addChild() and $tags reject namespace-bearing or unsupported source trees
    before mutation.
  • Named $tags assignments are consistently rejected by both engines.

While here, use an unsigned QuickJS $tags array length, support assigning $tags on an empty njs XMLNode, and make deleting $tags from an empty node a no-op.

Previously, text updates encoded content manually and then passed it to
libxml2. This duplicated libxml2 escaping rules and allocated the
encoded buffer before creating the text node.

The fix is to create a libxml2 text node from the unencoded value before
replacing the copied node's children. libxml2 now owns escaping, and a
failure leaves the original XML tree unchanged.
VadimZhestikov
VadimZhestikov previously approved these changes Sep 10, 2026

@VadimZhestikov VadimZhestikov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Notes (non-blocking)

  1. Memory tradeoff: removed subtrees are retained until the document/VM is destroyed (the retired list). This is a
    deliberate correctness-over-memory choice and is fine for nginx's request-scoped documents, but a script that
    repeatedly removes children from a long-lived document accumulates memory until teardown. Worth a one-line mention
    in docs; not a leak (it's freed at doc destruction).
  2. The two engines carry parallel implementations of the same redesign (+311/−233 njs, +319/−174 QuickJS); the test
    suite exercises both, and the behavior-parity items (e.g. named $tags rejection) are explicitly tested on each.
  3. The NULL-xmlReplaceNode crash is eliminated structurally (the function is gone), so I didn't need to reproduce the
    exact SEGV — the base's silent mishandling of the detached receiver in the negative control already shows the old
    model was unsafe.

@github-project-automation github-project-automation Bot moved this from New to In Review in NGINX OSS Unified Workspace Sep 10, 2026
Previously, text and structural mutators copied and replaced the receiver's
libxml2 node.  Existing XMLNode wrappers continued to point at the detached
original, while subsequent lookups returned the replacement.  Wrappers for
children preserved by the operation likewise pointed into the detached old
subtree.  In QuickJS, mutating the detached receiver again could dereference
the NULL result of xmlReplaceNode().

The fix is to modify the existing receiver's child list directly.  Existing
wrappers and subsequent lookups therefore continue to refer to the same
libxml2 node.  Removed child lists remain readable until the QuickJS
document or njs VM is destroyed.  A node is considered live only when
following its parent links reaches its xmlDoc.  Replacement children and
cleanup records are prepared before the live tree is modified.

Changed behaviour:
- A removed child has an undefined $parent, while its descendants retain
  their internal parent links.
- Mutating or canonicalizing a removed XMLNode throws TypeError.
- Text and attribute values must be primitive strings; null and undefined
  retain their deletion semantics.
- C14N rejects an excluding node from another document or a detached list.
- addChild() and $tags reject namespace-bearing or unsupported source trees
  before mutation.
- Named $tags assignments are consistently rejected by both engines.

While here, use an unsigned QuickJS $tags array length, support assigning
$tags on an empty njs XMLNode, and make deleting $tags from an empty node
a no-op.
Previously, assigning null or undefined to node.$tag$name removed
matching children in QuickJS, while the njs engine rejected every
assignment.

The fix is to distinguish property assignment from deletion in the
QuickJS exotic property handler. Assignments now throw TypeError in both
engines, while delete node.$tag$name remains supported.
Previously, XML declarations did not accurately model XML node property
values, mutation deletion semantics, or canonicalization aliases.

The fix is to align XMLDoc and XMLNode types with the current API and
add TypeScript coverage for the supported operations.
@xeioex
xeioex force-pushed the xml-modify-in-place branch from 15dccb6 to 1e59c36 Compare September 10, 2026 23:13

@VadimZhestikov VadimZhestikov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@xeioex
xeioex merged commit 7ea5e94 into nginx:master Sep 11, 2026
2 checks passed
@xeioex
xeioex deleted the xml-modify-in-place branch September 11, 2026 01:29
@github-project-automation github-project-automation Bot moved this from In Review to Done in NGINX OSS Unified Workspace Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants