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 ClassCastException when using StringBuilder/Buffer #496 #1210

Merged
merged 7 commits into from
May 27, 2022

Conversation

shelches
Copy link
Contributor

Fixes a ClassCastException in the flatten() method, if a StringBuilder or StringBuffer was used to initialize a JavaScript variable.

@shelches
Copy link
Contributor Author

I should add that compilation was successful, one unrelated test failed (the same test fails before I made my changes).

@rbri
Copy link
Collaborator

rbri commented Apr 20, 2022

Thanks for your conyribution, can you please also add a test case.

@p-bakker p-bakker linked an issue Apr 21, 2022 that may be closed by this pull request
@p-bakker p-bakker changed the title Fix ClassCastException #1206 Fix ClassCastException when using StringBuilder/Buffer #496 Apr 21, 2022
@p-bakker
Copy link
Collaborator

p-bakker commented Apr 21, 2022

Note that this PR fixes issue #496. Also see discussion #1206

Note that while this PR fixes a ClassCastException when concatenating strings and StringBuilder/Buffers together in JavaScript,
if the StringBuilder/Buffer gets modified after the concatenation in Javascript, an ArrayIndexOutOfBounds exception will be thrown, see #1206 (reply in thread). @rbri / @gbrail Any thoughs on this?

And on a sidenote: when doing string concatenation inside JS with StringBuilder/Buffers instantiated inside JS as well, the optimized path that uses ConString isn't getting hit, see #1206 (comment)

@shelches
Copy link
Contributor Author

test

@shelches shelches closed this Apr 25, 2022
@shelches shelches reopened this Apr 25, 2022
@shelches
Copy link
Contributor Author

@rbri I created some unit tests in testsrc/org/mozilla/javascript/tests/Issue1206Test.java for testing String, StringBuffer, and StringBuilder. Without the patch, the tests fail where they should, with a java.lang.ClassCastException. With the patch, they all succeed.

The new unit test file (Issue1206Test.java) is in the fix-classcastexception branch on the Mozilla Rhino fork of my GitHub account. Hope I handled this correctly! (It's my first time, but hopefully not nearly my last, contributing to open source!)

Please let me know if everything looks OK. Thanks!

@p-bakker p-bakker requested review from gbrail and rbri April 28, 2022 12:52
@gbrail
Copy link
Collaborator

gbrail commented Apr 30, 2022

I think that it's good to have this flexibility, but the fact that the contract for this class is confusing in that it will crash if you change a StringBuilder after putting it into this Object is not a great thing. I think that we should fix that but think carefully about where this class is actually used. Looks like there are two fixes:

  1. Call "toString" in the constructor, rather than in the "flatten" method. That way, the whole thing is immutable, which is consistent with how strings work in JavaScript in general.

  2. Take out the premature optimization of setting "length" in the constructor, and calculate in "flatten" instead. That means that a StringBuffer/Builder could be modified after this object is created.

I'd suggest that 2. is not intuitive to a typical JavaScript programmer who expects strings to be immutable, so I'd suggest thinking about 1. instead.

@gbrail
Copy link
Collaborator

gbrail commented Apr 30, 2022

In fact, based on how this object is probably typically used, we could even less-prematurely optimize with something that tests if either "str1" or "str2" is NOT just a String or ConsString in the constructor and then just flatten it right there, presuming that "toString" on anything that's not already a String is going to have some cost anyway. At any rate this is a complex set of operations and it's worth thinking more carefully about.

@shelches
Copy link
Contributor Author

I think that it's good to have this flexibility, but the fact that the contract for this class is confusing in that it will crash if you change a StringBuilder after putting it into this Object is not a great thing. I think that we should fix that but think carefully about where this class is actually used. Looks like there are two fixes:

  1. Call "toString" in the constructor, rather than in the "flatten" method. That way, the whole thing is immutable, which is consistent with how strings work in JavaScript in general.
  2. Take out the premature optimization of setting "length" in the constructor, and calculate in "flatten" instead. That means that a StringBuffer/Builder could be modified after this object is created.

I'd suggest that #2 is not intuitive to a typical JavaScript programmer who expects strings to be immutable, so I'd suggest thinking about #1 instead.

The issue we have at $work is that not using StringBuilder results in 2x more CPU use and 7x more garbage collection, which unfortunately is a "show stopper" for us, although we recognize this is our problem, not Rhino's problem. We are still in the process of evaluating work arounds (a temporary solution to give us some breathing room is to supply our own mildly customized Rhino jar, but that's not a good long-term solution).

@gbrail
Copy link
Collaborator

gbrail commented May 2, 2022

I'd be OK with taking out the optimization that ConsString has now of pre-computing the length. I think that would fix the crash that @p-bakker identified while still giving you the performance benefit. That's what I was getting to in #1210 (comment) above.

@shelches
Copy link
Contributor Author

I'd be OK with taking out the optimization that ConsString has now of pre-computing the length. I think that would fix the crash that @p-bakker identified while still giving you the performance benefit. That's what I was getting to in #1210 (comment) above.

Just a quick update: I have been looking into removing the code that pre-computes the length. A quick, minimal test was to change the length() method to this:

    public int length() {
        return isFlat ? left.length() : left.length() + right.length();
    }

Alas, this causes test failures. For example, testAppendManyStrings() in ConsStringTest.java fails with a java.lang.StackOverflowError. (Several other tests fail with the same exception.)

I'm digging further... but I think the ConsString.java refactoring (from years back) may have left it fundamentally incompatible with supporting java.lang.StringBuffer and java.lang.StringBuilder. I think it only truly supports java.lang.String.

However, if I come up with an unexpected solution, I'll be sure to comment here. My guess, though, is we'll need to accept that java.lang.StringBuffer and java.lang.StringBuilder are no longer supported.

@p-bakker
Copy link
Collaborator

Tnx for your continued investigation into this.

As for your minimal test: @gbrail suggested to call .toString() on str1/str2 if they are not instances of String or ConString (see comment #1210 (comment))

When computing the length inside the .length() method, the returned length could change over time if either of the values is a StringBuilder that gets modified.

@shelches
Copy link
Contributor Author

It sounds like the change that would result in the most intuitive behavior for a JavaScript developer would be:

    public ConsString(CharSequence str1, CharSequence str2) {
        if (!(str1 instanceof String) && !(str1 instanceof ConsString)) {
            str1 = str1.toString();
        }
        if (!(str2 instanceof String) && !(str2 instanceof ConsString)) {
            str2 = str2.toString();
        }
        left = str1;
        right = str2;
        length = left.length() + right.length();
        isFlat = false;
    }

That way, if someone passes in a StringBuffer, StringBuilder, or any other kind of mutable CharSequence, and mutates it in Java code later, there would be no failures.

This means the original patch could be reverted, because the (String) cast would always work.

How does this look? @gbrail @p-bakker (with much thanks for all the help!)

@p-bakker
Copy link
Collaborator

Yeah, I think that is along the lines that @gbrail suggested and imho the best approach as well, as it won't cause any exceptions and gives predictable behavior.

@shelches
Copy link
Contributor Author

Yeah, I think that is along the lines that @gbrail suggested and imho the best approach as well, as it won't cause any exceptions and gives predictable behavior.

Thank you! I believe I have successfully pushed the correct changes to my fork, and I think the pull request is still open for everyone's consideration.

All tests passed on my local system as well, and the original test I created also passes (and is in my fork as well).

/**
* Tests the ConsString class to ensure it properly supports String, StringBuffer and StringBuilder.
*/
public class Issue1206Test {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think all the tests currently just check whether the code doesn't throw an exception

Think it would be better if besides that, they'd also assert the result of cx.evaluateString to be as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Those tests have been updated to assert the results of those cx.evaluateString calls.

The fork/branch has been updated. I think this might be ready for review by the Rhino team! Thanks!

@p-bakker
Copy link
Collaborator

Looks like there are some formatting issues in your PR, as the build fails on Spotless

@shelches
Copy link
Contributor Author

Looks like there are some formatting issues in your PR, as the build fails on Spotless

Thank you, I believe I have resolved the code formatting issue.

@p-bakker
Copy link
Collaborator

LGTM

@gbrail
Copy link
Collaborator

gbrail commented May 24, 2022

This ends up being a very simple fix, which I like. Does it resolve your performance issues? If so I'll merge it!

@shelches
Copy link
Contributor Author

This ends up being a very simple fix, which I like. Does it resolve your performance issues? If so I'll merge it!

Yes, it resolves our performance issues as well. Thank you!

@gbrail
Copy link
Collaborator

gbrail commented May 27, 2022

Great -- thanks for fixing this!

@gbrail gbrail merged commit 50ae46c into mozilla:master May 27, 2022
@p-bakker p-bakker added this to the Release 1.7.15 milestone May 28, 2022
@shelches
Copy link
Contributor Author

Great -- thanks for fixing this!

Thank you very much, and thanks to everyone for the help and input!

Does the Mozilla Rhino project have a release date for the next version? Just wondering if, at work, I should deliver a custom jar (with this change), or perhaps wait until the next Rhino release (depending on the schedule). Thanks!

@p-bakker
Copy link
Collaborator

p-bakker commented May 31, 2022

There's no tentative date for the next Rhino release, so i'd go with a custom jar

jcompagner pushed a commit to Servoy/rhino that referenced this pull request Aug 10, 2023
…ozilla#1210)

Convert non-String, non-ConsString objects to a string when creating a ConsString so that StringBuilders and other CharSequence objects can be used natively more easily.

Fixes mozilla#1206 and mozilla#496

Co-authored-by: Edward Jensen <[email protected]>
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.

StringBuilder/Buffer support in scripting broken
4 participants