fix logic for obtaining TypeInfoFactory

[ZZZank]

The TypeInfoFactory used in FunctionObject is currently obtained via:

TypeInfoFactory.get(this)

where the this is FunctionObject itself, which doesn't make any sense because it's trying to obtained a TypeInfoFactory from a not-yet-initialized object. scope should be used instead.

I also added a test case for it

EDIT: lazy-init for TypeInfoFactory is removed now, using a custom factory now requires:

var scope = new NativeObject();
YourCustomFactory.create().associate(scope);
cx.initStandardObject(scope);

[aardvark179]

While you're in there, I think you may have a race in TypeInfoFactory.

    static TypeInfoFactory get(Scriptable scope) {
        TypeInfoFactory got =
                (TypeInfoFactory) ScriptableObject.getTopScopeValue(scope, "TypeInfoFactory");
        if (got == null) {
            // we expect this to not happen frequently, so computing top scope twice is acceptable
            var topScope = ScriptableObject.getTopLevelScope(scope);
            if (!(topScope instanceof ScriptableObject)) {
                // Note: it's originally a RuntimeException, the super class of
                // IllegalArgumentException, so this will not break error catching
                throw new IllegalArgumentException(
                        "top scope have no associated TypeInfoFactory and cannot have TypeInfoFactory associated due to not being a ScriptableObject");
            }
            got = new ConcurrentFactory();
            got.associate(((ScriptableObject) topScope));
        }
        return got;
    }

You aren't checking the result of got.associate, and got may have had another thread race to do the association.

associate should probably return the factory that was associated with the scope, not a boolean.

[rPraml]

@ZZZank I think, we should remove the lazy init and change the code

        if (got == null) {
            // we expect this to not happen frequently, so computing top scope twice is acceptable
            var topScope = ScriptableObject.getTopLevelScope(scope);
            if (!(topScope instanceof ScriptableObject)) {
                // Note: it's originally a RuntimeException, the super class of
                // IllegalArgumentException, so this will not break error catching
                throw new IllegalArgumentException(
                        "top scope have no associated TypeInfoFactory and cannot have TypeInfoFactory associated due to not being a ScriptableObject");
            }
            got = new ConcurrentFactory();
            got.associate(((ScriptableObject) topScope));
        }

to

        if (got == null) { // or just return null?
                throw new IllegalArgumentException(
                        "top scope have no associated TypeInfoFactory and cannot have TypeInfoFactory associated due to not being a ScriptableObject");
        }

Unfortunately, I don't remember why we need this lazy-init: the (de)serialization roundtrip should be fixed and I think if you get no TypeInfoFactory, this means there is a programming error. Or am I missing something?

[ZZZank]

Tests failed again due to the removal of lazy init. Will do some more research tomorrow

And a note for myself: global factory for scope independent action

[gbrail]

Thanks - there's a lot in this TypeInfoFactory stuff, but I see what it's doing

I'm trying to figure out the impact of this -- since the "ConcurrentFactory" is global, and doesn't do any GC via weak references, I'm trying to ensure that an environment that might use a JVM to start and stop lots and lots of tests doesn't end up with a memory leak. I don't think so because it seems like the default behavior is that each context gets its own factory associated with the top level scope, right?

[ZZZank]

The default behaviour now is:

• Each top level scope will get a new ConcurrentFactory associated at cx.initStandardObject(scope), which can then be GCed after the scope itself is GCed
• new FunctionObject(...) and ScriptableObject#defineProperty(...) will first search for factory associated with provided scope, and fallback to the global factory (with weak reference mechanism) if not found.

So either the factory itself or the cached objects in factory can be garbage collected, so no memory leak I believe

[gbrail]

That seems OK to me. Looks like a few people have had a chance to look at it, so I'll get to it soon. Thanks!

[gbrail]

Oooh -- first time for this -- upon rebasing, we have a failure in the "decycle" test, which we added recently.

Execution failed for task ':rhino:decycleMain'.
> A failure occurred while executing de.obqo.decycle.gradle.DecycleWorker
   > Violation(slicing=Package, name=cycle, dependencies=[org.mozilla.javascript → org.mozilla.javascript.lc.type, org.mozilla.javascript → org.mozilla.javascript.lc.type.impl.factory, org.mozilla.javascript.lc.type → org.mozilla.javascript, org.mozilla.javascript.lc.type.impl → org.mozilla.javascript, org.mozilla.javascript.lc.type.impl → org.mozilla.javascript.lc.type, org.mozilla.javascript.lc.type.impl.factory → org.mozilla.javascript.lc.type, org.mozilla.javascript.lc.type.impl.factory → org.mozilla.javascript.lc.type.impl])

@rPraml is our expert in this, I believe, as this either suggests that the implementation should change or we should add some exceptions in the configuration of the cycle detector.

[rPraml]

@rPraml is our expert in this, I believe, as this either suggests that the implementation should change or we should add some exceptions in the configuration of the cycle detector.

Yes, this was my intention. I'm fine with these changes, as they do not conflict with the overall plan in #1983 👍

[rPraml]

I'm fine with that. We can move that later to LcBridge (or whatever concept we make)
https://github.com/mozilla/rhino/pull/1983/files#diff-24249e911005a5f57fee7790be93c8a477f7584f853110544bd704cc971277e9R169

[rPraml]

Do we need both? (probably... TypeInfoFactory implements TypeInfo)

[ZZZank]

Yes we need. My guess is that the .associate(...) method is from TypeInfoFactory, and Decycle consider it as a reference to TypeInfoFactory

[gbrail]

OK -- this looks good now. Thanks!