8000 fix: report fatalError when doctype is inside elements by karfau · Pull Request #550 · xmldom/xmldom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: report fatalError when doctype is inside elements #550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Oct 4, 2023

Conversation

karfau
Copy link
Member
@karfau karfau commented Oct 4, 2023

which is a regression compared to 0.8.x, most likely introduced as part of #498

  • add check to parseDoctypeCommentOrCData
  • drop redundant and broken Element.appendChild implementation
  • hasInsertableNodeType now checks for CharacterData nodes instead of only text nodes
  • align ParseError and DOMException in how they are extending Error
  • wrap DOMExceptions in ParseError in sax parser
  • move custom errors to own module
    • and allow current and modern constructor arguments for DOMException

@codecov
Copy link
codecov bot commented Oct 4, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (99602b4) 92.77% compared to head (9665f1c) 93.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
+ Coverage   92.77%   93.88%   +1.10%     
==========================================
  Files           7        8       +1     
  Lines        2035     2044       +9     
  Branches      530      531       +1     
==========================================
+ Hits         1888     1919      +31     
+ Misses        147      125      -22     
Files Coverage Δ
lib/conventions.js 100.00% <ø> (ø)
lib/dom-parser.js 93.56% <100.00%> (+0.03%) ⬆️
lib/errors.js 100.00% <100.00%> (ø)
lib/index.js 100.00% <100.00%> (ø)
lib/sax.js 98.18% <100.00%> (+0.01%) ⬆️
lib/dom.js 90.06% <84.61%> (+1.57%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

karfau added 5 commits October 4, 2023 14:53
- `hasInsertableNodeType` now checks for `CharacterData` nodes instead of only text nodes
- wrap DOMExceptions in ParseError in sax parser
and allow current and modern constructor arguments for DOMException
and cover more DOMException cases
@karfau karfau marked this pull request as ready for review October 4, 2023 20:21
Copy link
Member Author
@karfau karfau left a comment

Choose a reason for hiding this comment

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

Self review completed

@karfau karfau merged commit 642c9e8 into master Oct 4, 2023
@karfau karfau deleted the fuzz/prevent-doctype-inside-elements branch October 4, 2023 20:27
@karfau karfau added this to the 0.9.0 milestone Oct 4, 2023
@karfau karfau added xml:not well-formed https://www.w3.org/TR/xml11/#dt-wellformed testing error handling fuzzer-finding labels Oct 4, 2023
karfau added a commit that referenced this pull request Oct 13, 2024
as part of #550
("drop redundant and broken `Element.appendChild` implementation")
the `_onUpdateChild` was not passed the added child,
so the `childNodes` were re-indexed from the `firstChild` each time a node was appended.

Now that the added child is being passed,
the performance in 0.9.0 is still not as good as in 0.8.*,
but this is because of the higher amount of checks being executed before adding a node.

fixes #748
karfau added a commit that referenced this pull request Oct 13, 2024
as part of #550 ("drop redundant
and broken `Element.appendChild` implementation")
the `_onUpdateChild` was not passed the added child, so the `childNodes`
were "re-indexed" from the `firstChild` each time a node was appended.
This is why the performance degraded significantly, when there are a lot
of childNodes to be appended.

fixes #748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant
0