Rhino and StringBuilder

[shelches]

I'm upgrading from Rhino 1.7R4 to 1.7.14, and I think I've stumbled across a change in behavior. I use this method:

Scriptable.put(String name, Scriptable start, Object value)

And supply a value whose type is StringBuilder. This causes a ClassCastException issue later, in ConsString.flatten(), when it tries to cast the StringBuilder to a String:

final String str = (String) next;

To temporarily fix this, I've added this block of code:

if (next instanceof StringBuilder) {
    final StringBuilder bldr = (StringBuilder) next;
    charPos -= bldr.length();
    bldr.getChars(0, bldr.length(), chars, charPos);
    next = stack.isEmpty() ? null : stack.removeFirst();
    continue;
}

Was I using Rhino incorrectly before, passing a value whose type is StringBuilder, and just getting lucky?

Or could this be an actual regression in Rhino?

Any idea if my temporary solution will cause problems in other parts of the engine?

Thanks very much for any assistance!

[p-bakker]

Think you're running into the same as #496

[shelches]

Oof. Okay, thanks.

I take it the only forward-looking solution is to never pass a StringBuilder to Scriptable.put(), and instead, always call .toString() on the StringBuilder before passing it to Scriptable.put()?

That's a bit of bad news for us, since it uses 2x as much CPU and 7x as much memory moving through the garbage collector in our tests, though admittedly our usage is probably not typical.

[tonygermano]

This seems like a bug in ConsString to me. It's set up to deal with CharSequences, but then it assumes all CharSequences are either instances of ConsString or String.

[p-bakker]

@shelches Any chance you can provide a reproducible sample where calling Scriptable.put() with a StringBuilder as value and things then crashing with a ClassCastException somewhere down the line?

@tonygermano it sounds like you're investigating whether this can be fixed, correct? Shall I reopen #496

[tonygermano]

If you want to use a StringBuilder as a StringBuilder, that should be possible if you wrap it in a NativeJavaObject. I know you can create StringBuilder instances from within javascript and they work correctly. I haven't fully traced it out, but I assume as soon as you try to concatenate the wrapped StringBuilder with a javascript string, it will coerce the StringBuilder to a String before adding it to the ConsString, so that may not gain you much over calling .toString() in the first place.

[shelches]

Thanks everyone for your input. Here is a simple program that reproduces the issue we are seeing:

package rhinotest;

import org.mozilla.javascript.Context;
import org.mozilla.javascript.Scriptable;

public class RhinoTest
{
	public static void main(String[] args)
	{
		Context ctx = Context.enter();
		Scriptable scriptable = ctx.initStandardObjects(null);
		StringBuilder bldr = new StringBuilder("hello");
		scriptable.put("var1", scriptable, bldr);
		String expression = "var1 = var1 + ' world'";
		ctx.evaluateString(scriptable, expression, "test", 1, null); // the issue happens here
	}
}

Running this test generates this exception and stack trace:

Exception in thread "main" java.lang.ClassCastException: java.lang.StringBuilder cannot be cast to java.lang.String
	at org.mozilla.javascript.ConsString.flatten(ConsString.java:74)
	at org.mozilla.javascript.ConsString.toString(ConsString.java:50)
	at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:381)
	at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3868)
	at org.mozilla.javascript.gen.test_1.call(test)
	at org.mozilla.javascript.gen.test_1.exec(test)
	at org.mozilla.javascript.Context.evaluateString(Context.java:1137)
	at rhinotest.RhinoTest.main(RhinoTest.java:15)

The ClassCastException happens on the 1st line of this code block in ConsString.flatten():

                final String str = (String) next;
                charPos -= str.length();
                str.getChars(0, str.length(), chars, charPos);
                next = stack.isEmpty() ? null : stack.removeFirst();

To temporarily work around the issue, I added this code block immediately before the code block above:

                if (next instanceof StringBuilder) {
                	final StringBuilder bldr = (StringBuilder) next;
                	charPos -= bldr.length();
                	bldr.getChars(0, bldr.length(), chars, charPos);
                	next = stack.isEmpty() ? null : stack.removeFirst();
                	continue;
                }

Any chance this is a regression? Does my fix look correct?

Thanks again for any input and assistance!

[p-bakker]

Not having analysed this much, I's say that instead of adding the if (next instanceof StringBuilder) {...} part, instead the existing final String str = (String) next; part ought to be changed to something along the lines of final CharSequence str = (CharSequence) next; ..., so it can handle StringBuilder, StringBuffer and String all at the same time.

[p-bakker]

Do wonder if there's a change of things crashing if after performing a concat of a string with a StringBuilder instance in Javascript (like your "var1 = var1 + ' world'"; expression) the StringBuilder instance gets appended to: the ConString constructor gets the lengths of both CharSequences and stores them. The StringBuilder is mutable, so its length can change over time, making the stored length value in the conStirng instance invalid, resulting in an invalid length for the chars array in the flatten method (I think)

[shelches]

Unfortunately, CharSequence does not have getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin), so I'm not sure if a common code block is a possibility.

[p-bakker]

So the code would need to use some other means to put the chars from the next' charSequence into the chars` char[]

[p-bakker]

I'm gonna reopen #496 with a pointer to this discussion.

@shelches if you fancy creating a PR for this, that would be great

[shelches]

Thank you very much for all your assistance. I think I successfully created my 1st GitHub pull request, which can be found here:

#1210

You are right that if the length of the StringBuilder or StringBuffer changes, an IndexOutOfBroundsException can be generated in ConsString.flatten() -- the developer must take care to synchronize the JS variable and Java variable, if that scenario is possible.

However, I think this is a step in the right direction, as it removes the ClassCastException, so that StringBuilder and StringBuffer can once again be used to initialize JS variables, even if some care must be taken in their use. It also restores pre-1.7.12 behavior.

Thank you everyone!

[shelches]

I tried the solution suggested by @tonygermano (thanks, Tony!) by changing this:

final String str = (String) next;

To this:

final String str = next.toString();

Are there any objections to this solution, which changes a single line of code? If not, I can put together a PR.

Thanks to everyone for your assistance!

[shelches]

I should additionally mention that I executed a suite of performance tests with this change and didn't notice any performance difference.

[p-bakker]

If that works from a performance perspective, that might work as well.

I am wondering though whether doing string concatenation in js with a StringBuilder/Buffer instance, that gets mutated after the concat, but before the ConString.flatten() method is invoked, will not throw an ArrayIndexOutOfBounds exception when the flatten method gets invoked

[shelches]

Good thought. I tested this scenario, and it seems to work OK (no exception, results as expected), though I can't promise my tests were exhaustive...

That being said, I think changing (String) next to next.toString() in flatten() should be OK either way... performance seems identical (perhaps even a very, very small improvement), and results in the generated class file being 1 byte smaller -- and results in StringBuilder and StringBuffer not causing ClassCastExceptions.

[p-bakker]

This example throws a java.lang.StringIndexOutOfBoundsException:

		Context ctx = Context.enter();
		Scriptable scriptable = ctx.initStandardObjects(null);
		StringBuilder bldr = new StringBuilder("worl");
		scriptable.put("var1", scriptable, bldr);
		String expression = "var one = 'hello' + var1";
		ctx.evaluateString(scriptable, expression, "test", 1, null);
		bldr.append("d");
		Object result = ctx.evaluateString(scriptable, "one", "test", 1, null);
		
		System.out.println(result);
	```

[shelches]

Do you think the potential for a java.lang.StringIndexOutOfBoundsException means the proposed patch (changing from casting to String to calling .toString()) is a bad idea/change?

[shelches]

I spent some time today setting up GitHub (keys, PAT, etc.) but it does not appear I can push to a (non-master/main) branch on the Mozilla Rhino project (a prerequisite to creating a pull request?).

I've never used GitHub before, so I'm not sure if I'm simply not allowed access, or if I've done something wrong. Can anyone point me in the right direction? Any assistance is appreciated!

[p-bakker]

In order to provide PRs, you need to fork the Rhino repo to your own account, create a branch there with your changes and then you can put up that branch as a PR in this repo

[p-bakker]

Also noticed that ScriptRuntime.add(Object val1, Object val2, Context cx) doesn't hit the constring optimize when a StringBuilder/Buffer is instantiated in JavaScript (var x = 'hello' + new java.lang.StringBuilder('world')), because the new java.lang.StringBuilder('world') part becomes an instance of NativeJavaObject, thus the check whether both sides of the add operator are instances of CharSequence in ScriptRuntime.add(...) eveluates to false, so the ConString optimizeis skipped.

[p-bakker]

This got resolved via #1210