JRuby-Nokogiri errors with “can't modify frozen string”

[denisdefreyne]

Nokogiri::XML.fragment cannot be used on frozen strings. (Nokogiri::XML.parse is not affected).

Steps to reproduce

Execute the following on JRuby and MRI:

input = <<-EOS
<?xml version="1.0" encoding="utf-8"?>
<library>
  <book title="I like turtles"/>
</library>
EOS
input.freeze

require 'nokogiri'
::Nokogiri::XML.fragment(input)

Expected output

No exception raised.

Actual output

RuntimeError: can't modify frozen string
    chomp! at org/jruby/RubyString.java:5761
       new at nokogiri/XmlDocumentFragment.java:93
     parse at /Users/ddfreyne/.gem/jruby/1.9.3/gems/nokogiri-1.6.1-java/lib/nokogiri/xml/document_fragment.rb:91
  fragment at /Users/ddfreyne/.gem/jruby/1.9.3/gems/nokogiri-1.6.1-java/lib/nokogiri/xml.rb:69
    (root) at test2.rb:10

Notes

In XmlDocumentFragment.java lines 115-119, the #chomp! method is used on the input argument, which fails for frozen strings:

// strips trailing \n off forcefully
// not to return new object in case of no chomp needed, chomp! is used here.
IRubyObject result;
if (context.getRuntime().is1_9()) result = str.chomp_bang19(context);
else result = str.chomp_bang(context);

[denisdefreyne]

Added example that fails, and clarified that this error only occurs for Nokogiri::XML.fragment but not for Nokogiri::XML.parse.

[headius]

The problem here is that DocumentFragment.new tries to "trim" the incoming string by using chomp! rather than dup'ing the string or using the non-mutative form of chomp. I removed the call to "trim" in XmlDocumentFragment::rbNew and no tests failed, so I'm not sure if this is needed.

Either way, it should be a simple fix: don't chomp!, dup and then chomp!, or chomp.

[denisdefreyne]

👍

[keltia]

@headius: rubinius has the exact same issue (tested on 2.2.6)

[flavorjones]

This behavior is a result of the fix written for #444, specifically commit 3f2e575.

No tests fail if I comment out the call to trim(). @yokolet, do you have an opinion on whether this code is even needed anymore? See the failing test on branch issue-1077-jruby-frozen-string.

[flavorjones]

Bump. @yokolet or @jvshahid, thoughts on the branch issue-1077-jruby-frozen-string ?

[yokolet]

Hi I just fixed this bug, also I added a test for issue#444. My apologies. I didn't add any test for issue#444, so just skipping trim method caused a regression. Now, both fixed.
Currently, my fix is in the issue-1077-jruby-frozen-string branch. Probably, the fix works. But, I appreciate if you guys look at the fix. I'll merge it to master later.

[flavorjones]

I'll take a look later today and merge if it looks good. Thank you!
On Jun 9, 2014 12:56 PM, "Yoko Harada" notifications@github.com wrote:

Hi I just fixed this bug, also I added a test for issue#444. My apologies.
I didn't add any test for issue#444, so just skipping trim method caused a
regression. Now, both fixed.
Currently, my fix is in the issue-1077-jruby-frozen-string branch.
Probably, the fix works. But, I appreciate if you guys look at the fix.
I'll merge it to master later.

—
Reply to this email directly or view it on GitHub
#1077 (comment)
.

[yokolet]

hmmm, it looks libxml version adds "\n" now. The parse result of <p>hi</p>\n got the same as JRuby of before this patch. issue#444
I don't know from what commit libxml version changed. Probably, I should skip that trim() method?

[flavorjones]

We never resolved this issue. #444 seems pretty unimportant relative to the frozen string issue described here, and I'm inclined to remove the trim call from XmlDocumentFragment.java.

@yokolet, @jvshahid - any thoughts here?

[jvshahid]

Merged the issue-1077-jruby-frozen-string branch, but changed the fix slightly to use chomp insntead of chomp! as was suggested in the previous comments.

[flavorjones]

LGTM, thanks!

[jvshahid]

The change in this pr caused the build to be red. it looks like the two implementations has changed their behavior regarding #444, jruby doesn't include the newline but MRI (as of master) includes a newline character. I'm not sure how to fix this issue other than removing the new test_444... that was added. I also want to remove the call to chomp from the JRuby implementation. I'm against these kinds of hacks that help fix minor edge cases/differences between the two implementation. Any objections ?

[flavorjones]

My vote was originally, and is still today, to remove the trim/chomp from
JRuby. I believe this will make it match MRI behavior, and if so,

On Wed, Feb 17, 2016 at 6:27 PM, John Shahid notifications@github.com
wrote:

The change in this pr caused the build to be red. it looks like the two
implementations has changed their behavior regarding #444
#444, jruby doesn't
include the newline but MRI (as of master) includes a newline character.
I'm not sure how to fix this issue other than removing the new test_444...
that was added. I also want to remove the call to chomp from the JRuby
implementation. I'm against these kinds of hacks that help fix minor edge
cases/differences between the two implementation. Any objections ?

—
Reply to this email directly or view it on GitHub
#1077 (comment)
.