Skip to content
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

fix(cruby): SAX and Push parser error handling in the presence of foreign handlers #2169

Merged
merged 5 commits into from Jan 6, 2021

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

When libxml-ruby is loaded before Nokogiri, it sets its own global libxml2 error handlers. There are a few places where Nokogiri does not defensively remove that handler or use its own handlers. Commit 771164d (shipped in v1.11.0) removed one instance of a defensive removal, and that combined with the presence of libxml-ruby caused some tests to fail in ActiveSupport's test suite.

This changeset introduces a systemic approach to testing handlers, setting a foreign handlers before every test in the suite and asserting that it's never called. It then introduces defensive calls to xmlSetStructuredErrorFunc where necessary.

Notably, it also introduces a new pattern of save-and-restore the previous handler in one place where we're recursively calling HTML parsers (see EncodingReader for details). This is a pattern we should extend to all other places in the code, but in the interest of time I'm not attempting that in this changeset (I'll open a new issue to track that work and backlink to this).

Have you included adequate test coverage?

Yes, a significant part of this changeset is introducing additional test coverage.

Does this change affect the behavior of either the C or the Java implementations?

This changes the behavior of the C extension only, as it relates to libxml2 behavior.

to check that we're setting error handlers everywhere we need to.

Related to #2168
Note that this change regresses the behavior changed in 771164d which
we'll have to fix in an upcoming commit.

Related to #2168, #87
originally introduced in 771164d but broken in the recent commits.

This is an incomplete fix. We should adopt this same strategy of
save-and-restore everywhere we set the error handlers.
@codeclimate
Copy link

codeclimate bot commented Jan 6, 2021

Code Climate has analyzed commit f9a2c4e and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 94.3%.

View more on Code Climate.

@flavorjones flavorjones merged commit 3d90c6d into master Jan 6, 2021
@flavorjones flavorjones deleted the 2168-active-support-test-failure branch January 6, 2021 03:47
@flavorjones flavorjones added this to the v1.11.1 milestone Jan 6, 2021
This was referenced Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant