importClass usage that worked in 1.7.12 now fails in 1.7.14

[Easy65]

Hi,

we recently upgraded from version 1.7.12 to 1.7.14 and now one of our test fails.
The test uses importClass with a class in our JUnit tests and run the same script multiple times. We get this exception in 1.7.14:

org.mozilla.javascript.EvaluatorException: Cannot import "JSLongTest" since a property by that name is already defined. (TestJSScriptCode2#2)
at org.mozilla.javascript.DefaultErrorReporter.runtimeError(DefaultErrorReporter.java:79)
at org.mozilla.javascript.Context.reportRuntimeError(Context.java:907)
at org.mozilla.javascript.Context.reportRuntimeError(Context.java:963)
at org.mozilla.javascript.Context.reportRuntimeErrorById(Context.java:914)
at org.mozilla.javascript.ImporterTopLevel.importClass(ImporterTopLevel.java:216)
at org.mozilla.javascript.ImporterTopLevel.js_importClass(ImporterTopLevel.java:176)
at org.mozilla.javascript.ImporterTopLevel.execIdCall(ImporterTopLevel.java:257)
at org.mozilla.javascript.IdFunctionObject.call(IdFunctionObject.java:100)
at org.mozilla.javascript.optimizer.OptRuntime.callName(OptRuntime.java:59)
at org.mozilla.javascript.gen.TestJSScriptCode2_1._c_script_0(TestJSScriptCode2:2)
at org.mozilla.javascript.gen.TestJSScriptCode2_1.call(TestJSScriptCode2)
at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:380)
at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3868)
at org.mozilla.javascript.gen.TestJSScriptCode2_1.call(TestJSScriptCode2)
at org.mozilla.javascript.gen.TestJSScriptCode2_1.exec(TestJSScriptCode2)

I tried to debug org.mozilla.javascript.ImporterTopLevel.importClass and compared the sources. There is now a scope object that is used instead of this, and there are 2 differences that I see:

In 1.7.12, the NativeJavaClass cl is always the same Java object. And we always have a new ImporterTopLevel object in each run, so cl is not found:

private void importClass(NativeJavaClass cl)
{
    String s = cl.getClassObject().getName();
    String n = s.substring(s.lastIndexOf('.')+1);
    Object val = get(n, this);
    if (**val != NOT_FOUND** && val != cl) {
        throw Context.reportRuntimeError1("msg.prop.defined", n);
    }
    //defineProperty(n, cl, DONTENUM);
    put(n, this, cl);
}

In 1.7.14, the NativeJavaClass cl is a different Java object (but equal) in each run. And the ImporterTopLevel object is the same on the second run, so the object is found, but not the same object, so the error occurs:

private static void importClass(Scriptable scope, NativeJavaClass cl) {
    String s = cl.getClassObject().getName();
    String n = s.substring(s.lastIndexOf('.') + 1);
    Object val = scope.get(n, scope);
    if (val != NOT_FOUND && **val != cl**) {
        **throw Context.reportRuntimeErrorById("msg.prop.defined", n);**
    }
    // defineProperty(n, cl, DONTENUM);
    scope.put(n, scope, cl);
}

The code would probably work as expected, if the cl object would be the same Java object as it is in 1.7.12.

Any idea why this happens in 1.7.14?

Regards,
Andreas Mueller

[p-bakker]

Based on the lack of answers, it seems to me no one has an idea why this happens 🙄

You wouldn't have happened to make progress yourself figuring this out?

My only suggestion at this point would to look at whether this behavior change happened in 1.7.13 or 1.7.14 and then to try to figure out which commit/merge caused the behaviour change...

[tonygermano]

See #826

[p-bakker]

@tonygermano have you definitively determined that #826 indeed is the cause of the breakage?

@Easy65 Any change to provide at least a testcase that reproduces the issue?

[Easy65]

@p-bakker: Sorry, I cannot provide a test case. This is a regression in a rather huge project were I am not familiar with the details. The people who implemented the functionality and the tests are no longer here. What I could do is running the tests with a patched version to check that it works again. Or to run the test case with an instrumented version of rhino that does some useful println to get an understanding of the situation.

[tonygermano]

It is hard to definitively determine if that's the cause without a test case, but that seems like the likely culprit, as it is what introduced the changes to how the imports work between versions 1.7.13 and 1.7.14.

[p-bakker]

@rPraml does this ring any bells for you, given that you authored the #826 PR?

[tonygermano]

I would be interested in seeing a test case that causes the NativeJavaClass in the second parameter to be the same object in 1.7.12, but different (but equal) objects in 1.7.14. If that is what is happening, would the fix be as simple as changing line 215 to if (val != NOT_FOUND && !cl.equals(val)) {?

rhino/rhino/src/main/java/org/mozilla/javascript/ImporterTopLevel.java

Lines 211 to 220 in 7feaad0

	private static void importClass(Scriptable scope, NativeJavaClass cl) {
	String s = cl.getClassObject().getName();
	String n = s.substring(s.lastIndexOf('.') + 1);
	Object val = scope.get(n, scope);
	if (val != NOT_FOUND && val != cl) {
	throw Context.reportRuntimeErrorById("msg.prop.defined", n);
	}
	// defineProperty(n, cl, DONTENUM);
	scope.put(n, scope, cl);
	}

The somewhat strange thing about how this method currently works is that if the same NativeJavaClass is found to have been already imported, we still do the put and "import" it over itself.

I do a decent amount of Java interop, and I never use importClass.

// I find this
const JSLongTest = Packages.path.to.JSLongTest
// preferable to
importClass(Packages.path.to.JSLongTest)

You can even use destructuring to import and alias multiple classes from the same package at once

const {Integer, String:JString} = java.lang
var s1 = String('this is a javascript string)
var s2 = new JString('this is a java.lang.String')
var i = Integer.valueOf(2)

[anonyein]

I would be interested in seeing a test case that causes the NativeJavaClass in the second parameter to be the same object in 1.7.12, but different (but equal) objects in 1.7.14. If that is what is happening, would the fix be as simple as changing line 215 to if (val != NOT_FOUND && !cl.equals(val)) {?

rhino/rhino/src/main/java/org/mozilla/javascript/ImporterTopLevel.java

Lines 211 to 220 in 7feaad0

	private static void importClass(Scriptable scope, NativeJavaClass cl) {
	String s = cl.getClassObject().getName();
	String n = s.substring(s.lastIndexOf('.') + 1);
	Object val = scope.get(n, scope);
	if (val != NOT_FOUND && val != cl) {
	throw Context.reportRuntimeErrorById("msg.prop.defined", n);
	}
	// defineProperty(n, cl, DONTENUM);
	scope.put(n, scope, cl);
	}

The somewhat strange thing about how this method currently works is that if the same NativeJavaClass is found to have been already imported, we still do the put and "import" it over itself.

I do a decent amount of Java interop, and I never use importClass.

// I find this
const JSLongTest = Packages.path.to.JSLongTest
// preferable to
importClass(Packages.path.to.JSLongTest)

You can even use destructuring to import and alias multiple classes from the same package at once

const {Integer, String:JString} = java.lang
var s1 = String('this is a javascript string)
var s2 = new JString('this is a java.lang.String')
var i = Integer.valueOf(2)

You are right!

const {Integer, String:JString} = Packages.java.lang;
var s1 = String('this is a javascript string')
var s2 = new JString('this is a java.lang.String')
var i = Integer.valueOf(2)

this code cannot run, with error "Integer and JString are undefined"

The bug has been verified, the "const" has too much bugs in many cases!

var {Integer, String:JString} = Packages.java.lang;
var s1 = String('this is a javascript string')
var s2 = new JString('this is a java.lang.String')
var i = Integer.valueOf(2)

All right

[p-bakker]

@rPraml any chance you could have a look to see if your #826 PR might be the culprit of this regression

[rPraml]

@p-bakker Sorry, I missed the mention. Can take a look next week

[rPraml]

As far as I found out, the following script fails

importClass(java.util.Date)
importClass(java.util.Date)

... but this fails also on 1.7.12. (It would be really good, if we'll get more input here)
I would say, we can check for equals instead of == here, but then, I would say, first class wins.

[rPraml]

Edit: sorry it is clear, that importClass(java.util.Date) will not work. Nevertheless I do not understand yet, how to run into the situation, that you import different classes twice

[Easy65]

TestImportClass.zip

Hi, I finally had the time to create a test that is similar to the code that fails in our tests.
Hopefully this helps to analyze the situation.

[rPraml]

I was able to identify the change in #826 as the cause of this issue.

(You can find the tescase here:
https://github.com/mozilla/rhino/pull/1676/files#diff-ef5a3ed83642c6f8a4fcfd5be8e8944f9349dc1e4a7a6629394b13960571ed0cR79)

I'm happy to revise the change I made, but I think we need to first clarify what the correct or desired behavior is.

Both @Eaysy65 and I want to use a SharedScope.

While I do the following in #826

sharedScope = new ImporterTopLevel(tmpCtx, true); // define ImporterTopLevel as shared scope
sharedScope.sealObject();

scope1 = new NativeObject(); // use simple objects as context-scope
scope1.setPrototype(sharedScope); // and set the shared scope as prototype
script.exec(cx, scope1);

is the initialization in the sample that @Easy65 provided as following

sharedScope = cx.initStandardObjects(); // this is a NativeObject

Scriptable scope1 = new ImporterTopLevel(cx); // this would redefine standardObjects again
scope1.setParentScope(sharedScope);
script.exec(cx, scope1);

As you see, this is the other way round and we have multiple TopLevelScopes, where the object is also the parentScope, which was not intended by me in #826. (One TopLevel scope and many small objects, that have the toplevel as prototype)

I could of course say that Easy65's version is wrong, but I would like to hear a second opinion here.

(I would also like to take a look at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Rhino/Scopes_and_Contexts#Sharing_Scopes, but the doc is gone and web.archive.org is currently offline)

@Easy65 can you try to change your code as following:

ScriptableObject init_scope = new ImporterTopLevel(js_ctx); // was:  = js_ctx.initStandardObjects();
init_scope.sealObject(); // it is also a good idea to seal this

Scriptable scope = new NativeObject; // was: = new ImporterTopLevel(js_ctx);
scope.setPrototype(init_scope); // was: scope.setParentScope(init_scope);

[Easy65]

I changed this in the test class, but it fails if I add the sealObject() call, because of this line:
init_scope.put(Variable.INIT_ENV, init_scope, env);

Even if I changed this to:
scope.put(Variable.INIT_ENV, init_scope, env);
and move it behind the scope.setPrototype() call, I still get the error:

org.mozilla.javascript.EvaluatorException: Cannot modify a property of a sealed object: j_init_env.
at org.mozilla.javascript.DefaultErrorReporter.runtimeError(DefaultErrorReporter.java:79)

I try to change it also in our real code, but this is rather complex and I got it not working yet.
But I probably will run into the same issue above.

[p-bakker]

@rPraml we host the docs ourselves: https://rhino.github.io/docs/scopes_and_contexts/#sharing-scopes

[Easy65]

I finally got it running in the real code, but also only without the sealObject() call.
My problem is that I only inherited this code and tests and I have only a vague understanding of all this.
A comment suggests that we set the variables on the init_scope so that we have it on multiple exec calls.

After thinking a while about it, the correct way seems to seal the object after the variables are set.
This may break existing code that uses our code and calls the initialization method twice or before each script call again. And it breaks the test code that initially failed when going from 1.7.12 to 1.7.14.

Would it be a problem to not seal the init_scope and allow to have multiple calls of
init_scope.put(Variable.INIT_ENV, init_scope, env);
with different env maps even between subsequential exec calls?

I am not sure why a separate scope is used for every exec call. Is this to re-run with no variable values left from the previous run?

And the question is still if our original code with setParentScope is really wrong or only an "unusual" way to use this.

Regards,
Andreas

[rPraml]

I finally got it running in the real code, but also only without the sealObject() call. My problem is that I only inherited this code and tests and I have only a vague understanding of all this. A comment suggests that we set the variables on the init_scope so that we have it on multiple exec calls.

After thinking a while about it, the correct way seems to seal the object after the variables are set. This may break existing code that uses our code and calls the initialization method twice or before each script call again. And it breaks the test code that initially failed when going from 1.7.12 to 1.7.14.

It seems that you (or the original author) wanted to use a shared scope as described here: https://rhino.github.io/docs/scopes_and_contexts/#sharing-scopes

The idea of a shared scope is, that you init certain objects only once and reuse them in every exec call (initStandardObjects defines the standard javascript objects like Mathetc).
Normally, the shared scope should be sealed, so that it will not change accidently by such an invocation. BUT depending on the use case, it might be neccessary to chagen the shared scope.

Would it be a problem to not seal the init_scope and allow to have multiple calls of init_scope.put(Variable.INIT_ENV, init_scope, env); with different env maps even between subsequential exec calls?

No. if the scope is not sealed, it may allow the current script to change things in the scope. You might pay attention, if the sealed scope is used in several threads (probably not in your use case).

I am not sure why a separate scope is used for every exec call. Is this to re-run with no variable values left from the previous run?

Probably you could try to set Variable.INIT_ENV to your map in that scope. But I think it is required to have some basic understanding, what the intention of the original code was.

And the question is still if our original code with setParentScope is really wrong or only an "unusual" way to use this.

The implementation in your test case looks like an (incorrectly) implementation of the shared scope concept. (for example, it initalizes the standard objects on the init_scope and again on every call, which looks definitvely wrong or "unusual" in my eyes) But I do not know, what the original intention was.

When we merge #1676, I think, your problem will disappear, but the code may be inperformant, as class caching will no longer work. (The reflection class cache resides in the ImporterTopLevel, which is not the toplevel in your case)

Regards, Andreas

But I would still be happy if someone else could say something about the topic, because I'm not 100% sure about a few points either.

Roland

[Easy65]

I guess the original author of this did not know much about the internals in Rhino, maybe he inherited the code from a former author and tried to get it working with these scopes/contexts.
As far as I understand it now, it may be useful to use the ImporterTopLevel object as init_scope and seal it, use a first native object and set the prototype to init_scope, set the INIT_ENV at this object and use a second native object with the first native object as prototype for every exec?!
And if I understood this correctly, I could even use this init_scope object for other scripts as well. The current code seems to create one for every compiled script.