Can you think of ways we could catch this with the instrumented tests?

For the case where force_object is true, we could create a Callable on the Godot side which returns null, pass that to an Android plugin, and then call it from there. Before my PR, that should crash 99% of time (or, in like 1% of cases lead to weird behavior or even work fine - that's the fun of memory corruption bugs ;-))

For the force_object is false case, we could create a Callable on the Godot side which returns Dictionary including values that are bool, int or float, pass that to an Android plugin which will call it and compare against the expected values. In this case, I think it won't crash before my PR, but the Dictionary values would be random and not necessarily match what was returned from the Callable

UPDATE: Actually, no, it looks like dictionaries will take the force_object is true path as well. I'm actually not seeing anything that end up creating plain bools, ints or floats, rather than the object versions? Should we just remove the force_object parameter and always create the object versions?

I haven't had a chance to try the instrumented tests yet! I'll take a look

I've just push a new version of this which makes much more drastic changes:

• It replaces _variant_to_jvalue() with _variant_to_jobject() since I couldn't find anywhere the force_object is false path was ever even used. So, now it always makes jobjects, and we don't need the fix for the path where it doesn't
• It adds a test to the instrumented tests that can theoretically trigger the memory corruption issue with callables that return null, but in my testing I couldn't get it to happen without really messing with the C++ side. For whatever reason, the top 32-bits of the jvalue (from before the PR when it was a jvalue) would always be all zeros me, even though they aren't guaranteed to be - that's just what happened to be on the stack when running the instrumented tests. But I was able to confirm that it would catch it if it happened by temporarily engineering the right conditions :-)

However, I still have the more minimal version of this PR saved in a branch, if we want to go back to that

I've just push a new version of this which makes much more drastic changes:

• It replaces _variant_to_jvalue() with _variant_to_jobject() since I couldn't find anywhere the force_object is false path was ever even used. So, now it always makes jobjects, and we don't need the fix for the path where it doesn't
• It adds a test to the instrumented tests that can theoretically trigger the memory corruption issue with callables that return null, but in my testing I couldn't get it to happen without really messing with the C++ side. For whatever reason, the top 32-bits of the jvalue (from before the PR when it was a jvalue) would always be all zeros me, even though they aren't gaurnteed to be - that's just what happened to be on the stack when running the instrumented tests. But I was able to confirm that it would catch it if it happened by temporarily engineering the right conditions :-)

The changes look good and make sense. I tried to think of a scenario where the current version with force_jobject==false would be useful but can't think of any:

• either we know we're returning a primitive value and can just retrieve that value directly from a Variant and return the jni version of it
• or we don't know the type of the Variant we're handling, and thus can only return a jobject

However, I still have the more minimal version of this PR saved in a branch, if we want to go back to that

We will still need the minimal version to cherry-pick against 4.5

Edit: nevermind, my changes were merged in 4.6 so there's nothing to fix in 4.5.

Thanks!