mozilla / rhino

Rhino is an open-source implementation of JavaScript written entirely in Java
https://rhino.github.io
Other
4.19k stars 851 forks source link

Skip importing same class twice (fixes #1463) #1676

Open rPraml opened 1 month ago

rPraml commented 1 month ago

See #1463 - I would like to have a testcase for this situation ;)

gbrail commented 1 month ago

So, do you have an idea how to test? Or do we feel that existing tests cover this well enough?

rPraml commented 1 month ago

I found one case, where I can trigger that error:

importPackage(java.util);
UUID.randomUUID(); // calls getPkgProperty("UUID", global, false)
importClass(java.util.UUID);
UUID.randomUUID();

While the first access calls getPkgProperty("UUID", global, false), the second one calls calls getPkgProperty("UUID",nativeJavaPackage, true) We could skip this, if we would follow the parent chain here:

synchronized Object getPkgProperty(String name, Scriptable start, boolean createPkg) {
        Object cached = super.get(name, start);
        if (cached != NOT_FOUND) return cached;
        // TODO: check start.getParentScope()

I doubt, that the above use case is the same case as @Easy65 had, as this code fails also in 1.7.11. I do not have much objections, to use equals instead of ==, but it would be nice, to understand what was the breaking change. (yes #826 is very suspicious)

rPraml commented 1 month ago

Sorry, I've run out of ideas on how to write a test case that shows the regression between 1.7.11 and the current version. I tried different things, shared scopes etc., but either the test fails under both 1.7.11 and current version or passes under both versions.

If no further input comes in, then I would merge this, as it is an improvement.

Easy65 commented 1 month ago

Hi, I could finally provide a test, see my latest comment in the issue. Regards, Andreas

rPraml commented 1 month ago

Thanks for the test. I'll try to convert this to a unittest the next days