Skip to content

Fix StreamBuf parser hang on non-xdr:wsDr drawing root (#56)#58

Closed
senoff wants to merge 1 commit into
protobi:masterfrom
senoff:fix/streambuf-non-xdr-root
Closed

Fix StreamBuf parser hang on non-xdr:wsDr drawing root (#56)#58
senoff wants to merge 1 commit into
protobi:masterfrom
senoff:fix/streambuf-non-xdr-root

Conversation

@senoff
Copy link
Copy Markdown

@senoff senoff commented May 2, 2026

Summary

Fixes #56.

Root cause: DrawingXform.parseClose() compared the closing tag name against the hardcoded string "xdr:wsDr" and fell to default: return true for everything else. BaseXform.parse() treats false as the loop-exit signal for its for await loop. When a workbook's drawing file uses a non-standard namespace root (e.g. a proprietary or future application that doesn't use the xdr: spreadsheet drawing namespace), parseClose never returns false — so the loop exhausts all SAX events and then blocks waiting for stream data that StreamBuf never delivers, hanging wb.xlsx.load() indefinitely.

Fix (approach b — least invasive): In parseOpen(), when the opening element is not the expected xdr:wsDr, capture the actual tag name into this._rootTag instead of trying to look it up as a child anchor type. In parseClose(), the existing switch already exits on this.tag (xdr:wsDr); the default branch now also returns false when name === this._rootTag, providing the loop-exit signal regardless of what namespace the drawing root element uses.

Regression test: spec/integration/issues/issue-56-streambuf-non-xdr-root.spec.js takes the test-issue-1575.xlsx fixture, replaces xl/drawings/drawing1.xml's <xdr:wsDr .../> root with <some:other xmlns:some="http://example.com/unknown-drawing-ns"/> via JSZip, then asserts that wb.xlsx.load(buffer) completes within 5 seconds. Without the fix the test times out.

Also includes a one-line .prettierrc fix (trailingComma: "all""es5") to resolve the prettier↔eslint comma-dangle conflict on function arguments that was preventing the pre-commit hook from passing.

Test results

884 unit passing
201 integration passing  (+1 new regression test)
1 e2e passing

…t element (protobi#56)

DrawingXform.parseClose() compared the closing tag against the hardcoded
string "xdr:wsDr" and fell to `default: return true` for anything else.
Because BaseXform.parse() treats `false` as the loop-exit signal, a drawing
file whose root uses a non-standard namespace caused the for-await loop to
consume all SAX events and then block waiting for stream data that StreamBuf
never delivers — hanging the load indefinitely.

Fix approach (b): capture the actual root tag name into this._rootTag during
parseOpen() when the element is not the expected xdr:wsDr. parseClose() then
returns false when that captured name closes, giving the loop-exit signal
regardless of which namespace the drawing root uses.

Regression test: spec/integration/issues/issue-56-streambuf-non-xdr-root.spec.js
rewrites xl/drawings/drawing1.xml in the test-issue-1575.xlsx fixture to use
an unknown namespace root element and asserts that wb.xlsx.load() completes
within 5 seconds.

Also fixes .prettierrc trailingComma "all" → "es5" to resolve the
prettier↔eslint comma-dangle conflict on function arguments that was
blocking the pre-commit hook (same fix as fix/prettier-eslint-config-drift).
@senoff
Copy link
Copy Markdown
Author

senoff commented May 3, 2026

Local test run on this branch — npm install --legacy-peer-deps && npm run test:unit && npm run test:integration:

Tier This PR Baseline (master @ 4c9e73c) Δ
test:unit 884 passing, 1 pending, 0 failing 884 passing, 1 pending, 0 failing +1 int (this PR adds a regression test)
test:integration 201 passing, 0 failing 200 passing, 0 failing +1 int (this PR adds a regression test)

No regressions vs baseline. Skipped test:end-to-end + test:jasmine (browser/grunt tiers) — same regression-catching surface for unit + integration on a feature branch like this one.

Posted because the tests.yml workflow at this repo hasn't auto-fired on this PR (GitHub's first-time-contributor approval gate). Just want to make the merge feel less risky if/when you get to it. Happy to wait for the official CI run if you'd prefer to flip the approval gate; this is purely supplementary.

@senoff
Copy link
Copy Markdown
Author

senoff commented May 7, 2026

Regenerated per AGENTS.md guidance — see new PR #70. Closing this one in favor of the regenerated version.

@senoff senoff closed this May 7, 2026
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.

DrawingXform parser hangs on workbook drawings with non-xdr:wsDr root element

1 participant