Fixed error in coercion to boolean for legacy versions (#1402) #1403

andreabergia · 2023-10-27T13:07:37Z

gbrail · 2023-10-31T00:08:40Z

Can you please add a comment on the PR or in the code that explains what you're trying to fix here, or what bug is being addressed? That'll help us remember why we did this. Thanks!

andreabergia · 2023-10-31T08:06:02Z

Can you please add a comment on the PR or in the code that explains what you're trying to fix here, or what bug is being addressed? That'll help us remember why we did this. Thanks!

I think I put a link to the detailed issue that I have opened in the commit message, but I forgot to put it in the PR 😅
It's intended to fix #1402

p-bakker · 2023-10-31T09:54:30Z

So in #1402 you provide 3 snippets and the last one is about stuff erroring out.

Am unsure if you're saying that that code errors out in the latest version of Rhino, or that you expect it to error out when running with a languageVersion < 1.3.

Regardless, I'm unsure why you made a change to ScriptableObject, because I haven't seen anything not working properly without that change in the current version of Rhino

p-bakker · 2023-10-31T09:54:52Z

src/org/mozilla/javascript/ScriptableObject.java

@@ -763,6 +763,9 @@ public Object[] getAllIds() {
     */
    @Override
    public Object getDefaultValue(Class<?> typeHint) {
+        if (typeHint == ScriptRuntime.BooleanClass) {


Why is this change required?

andreabergia · 2023-11-02T11:30:53Z

So in #1402 you provide 3 snippets and the last one is about stuff erroring out.

Am unsure if you're saying that that code errors out in the latest version of Rhino, or that you expect it to error out when running with a languageVersion < 1.3.

Regardless, I'm unsure why you made a change to ScriptableObject, because I haven't seen anything not working properly without that change in the current version of Rhino

So, the first two snippets are about "weird" behavior in language version < 1.3. Unfortunately, these are things that we still need to support in ServiceNow.

The change to ScriptableObject is required because of this snippet in ScriptRuntime::toBoolean:

                if (Context.getContext().isVersionECMA1()) {
                    // pure ECMA
                    return true;
                }
                // ECMA extension
                val = ((Scriptable) val).getDefaultValue(BooleanClass);
                if ((val instanceof Scriptable) && !isSymbol(val))
                    throw errorWithClassName("msg.primitive.expected", val);

It is invoking getDefaultValue with the hint boolean. My change simply makes this return true, because we were getting the error running this (admittedly weird) JS as detailed in #1402:

var Base = function() {};
var Extending = function() {};
Extending.prototype = Base;
var x = new Extending();
!!x

as getDefaultValue was trying to invoke valueOf and then toString, and weird things were happening. With this change, we short-circuit that problem, and also make it faster.

The end result is that:

for legacy version (< 1.3) we preserve the behavior that Rhino had for Boolean and arrays, but we fix the problem with the JS above
for all other versions, ScriptRuntime already skips the call to getDefaultValue, so nothing changes.

tonygermano · 2023-11-02T16:45:54Z

The Boolean case actually still works because of

rhino/src/org/mozilla/javascript/NativeBoolean.java

Lines 34 to 39 in f085d50

    
           public Object getDefaultValue(Class<?> typeHint) { 
        
               // This is actually non-ECMA, but will be proposed 
        
               // as a change in round 2. 
        
               if (typeHint == ScriptRuntime.BooleanClass) return ScriptRuntime.wrapBoolean(booleanValue); 
        
               return super.getDefaultValue(typeHint); 
        
           }

The [] case should be handled by your changes to NativeArray, which clearly limits its impact to those specific version levels. Though should you be using Context.getContext().isVersionECMA1() as in ScriptRuntime::toBoolean rather than testing the version numbers explicitly? Your code will affect version 0, and I don't think that it should.

Was there ever a time when the var x = new Extending() code wouldn't have thrown an error that would justify the change to ScriptableObject?

I mean, I don't see any evidence going back to the initial commit of this repo 23 years ago that the array case has ever worked as you're describing. I don't actually know how to build the project from back then to test it, though. It appears to be using Ant, but I think there is some magic that I do not know how to perform.

p-bakker · 2023-11-02T17:53:23Z

@andreabergia in which exact Rhino version did you see the different behavior compared to the latest version?

@tonygermano all binaries from Rhino 1.4R3 onwards are available from our docs: https://rhino.github.io/releases/

tonygermano · 2023-11-03T13:42:23Z

I was unable to get the shell to launch with 1.4R3.

In version 1.5R1 !![] evaluates to false for versions 100 and 110, but true for 120 and higher. This likely warrants more research.

This code

var Base = function() {};
var Extending = function() {};
Extending.prototype = Base;
var x = new Extending();
!!x

...behaves exactly as the current version and throws a TypeError for versions 100, 110, and 120. I think it would be incorrect to change this behavior now.

tonygermano · 2023-11-03T15:41:27Z

The most current snapshot behaves exactly the same as version 1.5R1. I do not think this PR is necessary.

!![] is false for versions 100 and 110 and true for all other versions.

The reason that version 120 differs from 100 and 110 is because

ScriptableObject::getDefaultValue tries calling valueOf on the array
This goes up the chain to Object.prototype.valueOf
This method simply returns the object on which it's called
ScriptableObject::getDefaultValue sees this is not a primitive value, so it tries calling toString on the array
Only in version 120 does Array.prototype.toString return "[]" rather than an empty string when the array is empty
ScriptRuntime::toBoolean then converts the empty string to false for versions 100 and 110 and "[]" to true for version 120

In addition to !![] the same behavior can be observed with Boolean([])

andreabergia · 2023-11-03T16:32:39Z

The [] case should be handled by your changes to NativeArray, which clearly limits its impact to those specific version levels. Though should you be using Context.getContext().isVersionECMA1() as in ScriptRuntime::toBoolean rather than testing the version numbers explicitly? Your code will affect version 0, and I don't think that it should.

Good point, I need to fix that.

In version 1.5R1 !![] evaluates to false for versions 100 and 110, but true for 120 and higher. This likely warrants more research.

Yes, this is what happens. We rely on that at ServiceNow.

This code
var Base = function() {};
var Extending = function() {};
Extending.prototype = Base;
var x = new Extending();
!!x
...behaves exactly as the current version and throws a TypeError for versions 100, 110, and 120. I think it would be incorrect to change this behavior now.

I disagree, without the change in ScriptableObject that does work for newer versions. This unit test passes with VERSION_ES6, but fails with VERSION_1_0:

    @Test
    public void codeCanBeRunWithoutRaisingErrorInModernMode() {
        Utils.runWithAllOptimizationLevels(
                cx -> {
                    cx.setLanguageVersion(Context.VERSION_ES6);

                    Scriptable scope = cx.initStandardObjects(null);
                    Object result =
                            cx.evaluateString(
                                    scope,
                                    "var Base = function() {};\n"
                                            + "var Extending = function() {};\n"
                                            + "Extending.prototype = Base;\n"
                                            + "var x = new Extending();\n"
                                            + "!!x\n",
                                    "test",
                                    1,
                                    null);
                    assertEquals(true, result);
                    return null;
                });
    }

tonygermano · 2023-11-03T16:47:46Z

I think your assumptions are wrong, and @p-bakker asked you to provide the exact version where rhino functioned the way you think it should.

When I pulled and tested on a version over 20 years old, it behaves in exactly the same way as a snapshot build from the master branch for all of your test cases.

I understand that you desire codeCanBeRunWithoutRaisingErrorInLegacyMode to pass, but it should fail or you could be introducing regression bugs for someone else.

Likewise emptyArraysCoerceToFalseInLegacyModealready passes without your changes. The key here is that if you want that behavior, you need to be using versions 100 or 110. That test should historically fail for version 120 for the reason I mentioned in my previous comment.

andreabergia · 2023-11-06T13:19:47Z

I think your assumptions are wrong, and @p-bakker asked you to provide the exact version where rhino functioned the way you think it should.

When I pulled and tested on a version over 20 years old, it behaves in exactly the same way as a snapshot build from the master branch for all of your test cases.

I understand that you desire codeCanBeRunWithoutRaisingErrorInLegacyMode to pass, but it should fail or you could be introducing regression bugs for someone else.

Likewise emptyArraysCoerceToFalseInLegacyModealready passes without your changes. The key here is that if you want that behavior, you need to be using versions 100 or 110. That test should historically fail for version 120 for the reason I mentioned in my previous comment.

I am doubtful there ever was a version of rhino where this worked. But, this is causing problems for us at ServiceNow, so we went and fixed it in our internal fork.
Since it looks like a generic bug that should not cause any regressions (it does not cause any problem in our big internal test suite), and since we don't really think many people other than us use version 1.0-1.2 of Rhino, we thought it might be good to get it fixed in upstream, as well.

p-bakker · 2023-11-06T17:11:04Z

So, which Rhino version does ServiceNow use? The 'our internal fork' link just links to documentation, not a forked repo. We'd like to know, because you seem to suggest behavior changed in the latest version compared to some earlier version of Rhino, but @tonygermano checked and wasnt able to find any Rhino version that behaved any different than the current master

Curious though: the docs you pointed to suggests your users can use ES2021 features that Rhino doesn't support. Are you cross-compiling or has ServiceNow implemented those features in their own Rhino fork?

andreabergia · 2023-11-06T17:32:12Z

So, which Rhino version does ServiceNow use? The 'our internal fork' link just links to documentation, not a forked repo. We'd like to know, because you seem to suggest behavior changed in the latest version compared to some earlier version of Rhino, but @tonygermano checked and wasnt able to find any Rhino version that behaved any different than the current master

Ok, I realize I have been confusing. I meant "engine version, as in 1.0 and so on", not "rhino version" as in release 1.7.11 or so.
So, I don't think that the snippet of code I put in the issue has ever worked in any release of Rhino. But we need to fix it when Rhino is executing code with the context version set to 1.0. Does that clarify?

Curious though: the docs you pointed to suggests your users can use ES2021 features that Rhino doesn't support. Are you cross-compiling or has ServiceNow implemented those features in their own Rhino fork?

I am unsure if my NDA allows me to tell you what we are doing... 😢 I'll try to find out what I can share.

p-bakker · 2023-11-06T18:44:26Z

I am unsure if my NDA allows me to tell you what we are doing... 😢 I'll try to find out what I can share.

K, as I said, just curious. If ServiceNow has implemented all those features in Rhino, I'd really like to know more about it, but am assuming cross-compiling is used.

p-bakker · 2023-11-06T19:13:41Z

Looks like !![] evaluating to false is the correct behavior, as JavaScript 1.2 is the Netscape JavaScript implementation that predates EcmaScript 1.0 and this particular behavior is something where Netscapse's JavaScript 1.2 implementation differs from EcmaScript 1.0 specification, see https://docstore.mik.ua/orelly/webprog/jscript/ch11_06.htm#jscript4-CHP-11-SECT-6

tonygermano · 2023-11-07T04:08:50Z

Looks like !![] evaluating to false is the correct behavior, as JavaScript 1.2 is the Netscape JavaScript implementation that predates EcmaScript 1.0 and this particular behavior is something where Netscapse's JavaScript 1.2 implementation differs from EcmaScript 1.0 specification, see https://docstore.mik.ua/orelly/webprog/jscript/ch11_06.htm#jscript4-CHP-11-SECT-6

That's an interesting document. I scoured the Netscape documentation (which was terrible) in archive.org and couldn't find anything to support the following. It was also missing from the MDN document in "what's new in 1.2." Yet we do follow this behavior, anyway, with regard to converting arrays to numbers in 1.2 (and only in 1.2.)

When an array object is used in a numeric context, it evaluates to its length. When used in a boolean context, it evaluates to false if its length is 0 and otherwise evaluates to true.

However, at least the MDN documentation did agree with both of these in "what's new in 1.2," which we do support correctly, also for only version 1.2.

The Array.toString( ) method separates array elements with a comma and a space, instead of just a comma, and returns the list of elements within square brackets. In addition, string elements of the array are quoted, so that the result is a string in legal array literal syntax.

The default Object.toString( ) method displays the values of all properties defined by the object, returning a string formatted using object literal syntax docstore.mik.ua/orelly/webprog/jscript/ch11_06.htm

If we label the boolean conversion of a zero length array to true in version 1.2 as a bug. Is it a behavior we want to change now, considering it has functioned this way since Rhino was released as open source? My understanding is that the intent is to minimize changes to the legacy versions to preserve backward compatibility. If this was truly a regression bug that was introduced in a more recent version, then surely we would address it, but I would think that the "bug" is the accepted behavior after 20+ years.

tonygermano · 2023-11-07T04:43:34Z

Here's more fun stuff to contemplate... I downloaded Netscape Navigator 7.2 (windows version) and installed it in Wine. For language version 1.2, !![] did evaluate to false. However, for language versions 1.0, 1.1, and 1.3 it evaluated to true, so apparently our 100 and 110 version behavior is wrong.

andreabergia · 2023-11-08T08:34:10Z

I am unsure if my NDA allows me to tell you what we are doing... 😢 I'll try to find out what I can share.

K, as I said, just curious. If ServiceNow has implemented all those features in Rhino, I'd really like to know more about it, but am assuming cross-compiling is used.

Ok, I got the green light - we are using a transpiler in front of Rhino. 🙂

If we label the boolean conversion of a zero length array to true in version 1.2 as a bug. Is it a behavior we want to change now, considering it has functioned this way since Rhino was released as open source? My understanding is that the intent is to minimize changes to the legacy versions to preserve backward compatibility. If this was truly a regression bug that was introduced in a more recent version, then surely we would address it, but I would think that the "bug" is the accepted behavior after 20+ years.

I don't think it's a bug - we need to preserve it, even if it is weird. But, from what you are saying, my change in NativeArray should not check for versionECMA1 but rather look something like:

    @Override
    public Object getDefaultValue(Class<?> hint) {
        if (hint == ScriptRuntime.NumberClass) {
            Context cx = Context.getContext();
            if (cx.getLanguageVersion() == Context.VERSION_1_2) return Long.valueOf(length);
        }
        if (hint == ScriptRuntime.BooleanClass) {
            Context cx = Context.getContext();
            if (cx.getLanguageVersion() == Context.VERSION_1_2) return length != 0;
        }
        return super.getDefaultValue(hint);
    }

I've updated the PR.

tonygermano · 2023-11-10T06:03:12Z

Rhino has not changed behavior in this regard since initial release. Now, if we compare Rhino to Netscape, Rhino differs from Netscape in 3 different language versions (1.0, 1.1, and 1.2.) However, in neither engine did all 3 of those versions evaluate to the same true or false value.

Maybe I can illustrate this with some code.

I ran this code in Netscape Navigator 7.2 (2004)

<html>
	<head></head>
	<body>
		<script language="JavaScript1.0">
			document.write('1.0: ' + !![])
		</script>
        	<br>
		<script language="JavaScript1.1">
			document.write('1.1: ' + !![])
		</script>
	        <br>
		<script language="JavaScript1.2">
			document.write('1.2: ' + !![])
		</script>
        	<br>
		<script language="JavaScript1.3">
			document.write('1.3: ' + !![])
		</script>
	</body>
</html>

and this was the output

1.0: true
1.1: true
1.2: false
1.3: true

This was in Rhino 1.5R1 (2000)

js> var v = [100,110,120,130]
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + !![]) }
100: false
110: false
120: true
130: true

Rhino 1.7 release 1 (2008)

js> var v = [100,110,120,130]
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + !![]) }
100: false
110: false
120: true
130: true

Rhino 1.7.15-SNAPSHOT (2023)

js> var v = [100,110,120,130]
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + !![]) }
100: false
110: false
120: true
130: true

Your PR changes this table to the following, and I don't think there is justification for this change being "preservation" as this state has never existed anywhere in the past as far as I can tell.

100: false
110: false
120: false
130: true

I think either we call it a "bug" and make all language versions match Netscape or we say Rhino has been constant in this behavior since initial release and therefore the current behavior is now an immutable "feature." I think most people on this project lean towards the latter.

….2 and above

andreabergia · 2023-11-10T08:27:46Z

Your PR changes this table to the following, and I don't think there is justification for this change being "preservation" as this state has never existed anywhere in the past as far as I can tell.

I absolutely didn't mean to change that behavior, it is an oversight. I've pushed a fix, now it behaves in the "old" way.

This PR was only supposed to fix the initial problem with this javascript, not to touch arrays in any way. 🙂

var Base = function() {};
var Extending = function() {};
Extending.prototype = Base;
var x = new Extending();
!!x

tonygermano · 2023-11-10T16:50:30Z

That code is a similar situation. I don't see a need to "fix" anything. The only different between Netscape and current Rhino is that version 1.2 throws an error in Rhino, but doesn't in Netscape. Rhino has been constant in that differing behavior since initial release.

Netscape Navigator 7.2

<html>
	<head></head>
	<body>
		<script language="JavaScript1.0">
		    function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r;
		    try {r = !!x} catch(e) {r = e.toString()} return r;}
			document.write('1.0: ' + test())
		</script>
        <br>
		<script language="JavaScript1.1">
		    function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r;
		    try {r = !!x} catch(e) {r = e.toString()} return r;}
			document.write('1.1: ' + test())
		</script>
        <br>
		<script language="JavaScript1.2">
		    function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r;
		    try {r = !!x} catch(e) {r = e.toString()} return r;}
			document.write('1.2: ' + test())
		</script>
        <br>
		<script language="JavaScript1.3">
		    function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r;
		    try {r = !!x} catch(e) {r = e.toString()} return r;}
			document.write('1.3: ' + test())
		</script>
	</body>
</html>

1.0: TypeError: Function.prototype.toString called on incompatible object
1.1: TypeError: Function.prototype.toString called on incompatible object
1.2: true
1.3: true

Rhino 15R1

js> var v = [100,110,120,130]
js> function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r; try {r = !!x} catch(e) {r = e.toString()} return r;}
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + test()) }
100: TypeError: Method "toString" called on incompatible object.
110: TypeError: Method "toString" called on incompatible object.
120: TypeError: Method "toString" called on incompatible object.
130: true

Rhino 1.7 release 1 2008 03 06

js> var v = [100,110,120,130]
js> function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r; try {r = !!x} catch(e) {r = e.toString()} return r;}
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + test()) }
100: TypeError: Method "toString" called on incompatible object.
110: TypeError: Method "toString" called on incompatible object.
120: TypeError: Method "toString" called on incompatible object.
130: true

Rhino 1.7.15-SNAPSHOT

js> var v = [100,110,120,130]
js> function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r; try {r = !!x} catch(e) {r = e.toString()} return r;}
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + test()) }
100: TypeError: Method "toString" called on incompatible object (org.mozilla.javascript.NativeObject is not an instance of org.mozilla.javascript.BaseFunction).
110: TypeError: Method "toString" called on incompatible object (org.mozilla.javascript.NativeObject is not an instance of org.mozilla.javascript.BaseFunction).
120: TypeError: Method "toString" called on incompatible object (org.mozilla.javascript.NativeObject is not an instance of org.mozilla.javascript.BaseFunction).
130: true

p-bakker · 2023-11-12T09:20:20Z

I'm also quite hesitant to start changing behavior that has existed in these old language versions van 20 years...

Thoughts @gbrail @rbri?

andreabergia · 2024-02-21T09:52:12Z

Well, seems this won't get merged. Closing it.

Fixed error in coercion to boolean for legacy versions (mozilla#1402)

49500fb

p-bakker reviewed Oct 31, 2023

View reviewed changes

tonygermano mentioned this pull request Nov 3, 2023

Problem with coercion to boolean in legacy versions #1402

Closed

Fixed version check in NativeArray

8cd1478

Fixed again version check in NativeArray - it only applies to 1.2

82a530d

Fixed once more NativeArray - it should work for 1.0 and 1.1, not 1…

1388400

….2 and above

andreabergia closed this Feb 21, 2024

andreabergia deleted the legacy-versions-and-bool-coercion branch February 21, 2024 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed error in coercion to boolean for legacy versions (#1402) #1403

Fixed error in coercion to boolean for legacy versions (#1402) #1403

andreabergia commented Oct 27, 2023 •

edited

Loading

gbrail commented Oct 31, 2023

andreabergia commented Oct 31, 2023

p-bakker commented Oct 31, 2023

p-bakker Oct 31, 2023

andreabergia commented Nov 2, 2023

tonygermano commented Nov 2, 2023

p-bakker commented Nov 2, 2023

tonygermano commented Nov 3, 2023

tonygermano commented Nov 3, 2023 •

edited

Loading

andreabergia commented Nov 3, 2023

tonygermano commented Nov 3, 2023

andreabergia commented Nov 6, 2023

p-bakker commented Nov 6, 2023

andreabergia commented Nov 6, 2023

p-bakker commented Nov 6, 2023

p-bakker commented Nov 6, 2023

tonygermano commented Nov 7, 2023

tonygermano commented Nov 7, 2023

andreabergia commented Nov 8, 2023

tonygermano commented Nov 10, 2023 •

edited

Loading

andreabergia commented Nov 10, 2023

tonygermano commented Nov 10, 2023

p-bakker commented Nov 12, 2023

andreabergia commented Feb 21, 2024

Fixed error in coercion to boolean for legacy versions (#1402) #1403

Fixed error in coercion to boolean for legacy versions (#1402) #1403

Conversation

andreabergia commented Oct 27, 2023 • edited Loading

gbrail commented Oct 31, 2023

andreabergia commented Oct 31, 2023

p-bakker commented Oct 31, 2023

p-bakker Oct 31, 2023

Choose a reason for hiding this comment

andreabergia commented Nov 2, 2023

tonygermano commented Nov 2, 2023

p-bakker commented Nov 2, 2023

tonygermano commented Nov 3, 2023

tonygermano commented Nov 3, 2023 • edited Loading

andreabergia commented Nov 3, 2023

tonygermano commented Nov 3, 2023

andreabergia commented Nov 6, 2023

p-bakker commented Nov 6, 2023

andreabergia commented Nov 6, 2023

p-bakker commented Nov 6, 2023

p-bakker commented Nov 6, 2023

tonygermano commented Nov 7, 2023

tonygermano commented Nov 7, 2023

andreabergia commented Nov 8, 2023

tonygermano commented Nov 10, 2023 • edited Loading

andreabergia commented Nov 10, 2023

tonygermano commented Nov 10, 2023

p-bakker commented Nov 12, 2023

andreabergia commented Feb 21, 2024

andreabergia commented Oct 27, 2023 •

edited

Loading

tonygermano commented Nov 3, 2023 •

edited

Loading

tonygermano commented Nov 10, 2023 •

edited

Loading