Node::ValueAt may be leaking memory

[ncordon]

After a glimpse to the methods in Node (C++ side), I have the (untested of course) hypothesis that ValueAt may be leaking memory.

• ValueAt creates new global ref for and object obj and calls lookupOrCreate(obj)
• lookupOrCreate tries to find a Node for obj (if it previously existed, we created a new reference to it, and we are only deallocating one). If no Node existed, calls the constructor of Node, which creates a new reference to obj, but we do not release the original one at any point D:

Proposed solution is end ValueAt method with:

return lookupOrCreate(val);

instead of

return lookupOrCreate(env->NewGlobalRef(val));

[bzz]

This is a good observation, but merely implementing the suggestion will result in test failures.

I have submitted a patch for what seems to be one of the causes of this at #116 (cann't use refs as keys for the map really) but it is still a partial solution - if global ref is removed there, it will fail the PreOrder managed iterator test :/

I suggest also adding to the scope of this issue identifying the reason of why does Interface::toJ has to do getJNIEnv()->NewGlobalRef()?

It does not seem to need one for the same reasons, doesn't it? But removing it causes BblfshClientLoadTest to fail with java.lang.AbstractMethodError: org.bblfsh.client.v2.JNode.size()I on Context.encode.

I hope that new materials from #115 on JNI debugging at CONTRIBUTING.m will help to find an answer.

IMO it makes sense to start with this one and when fixed 🔪 a release, so added it to v2.0.1 milestone.

[ncordon]

Closed by #116