-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempt to remove non-JNI local reference with GlobalObject #207
Comments
This feels like a deficiency of JNI Bind. I've had it in my head to have a -- I think something like this would be good: LocalObjectND<kClass> obj { obj };
LocalStringND str { "Foo" }; I think there probably should be ways to convert this into a local (in the same way globals have mechanisms for locals), however, I think as a first pass, I might be able to make something useful quickly. I'll take a quick detour from solving your Windows problem as this might be easy (and I'd like it pre 1.0). Apologies that has taken so long, but as a non Windows dev it probably takes me 5x that amount of time to do things as you. -- As to why you''re not seeing failure, I agree, you're probably violating the contract. I've seen endless idiosyncrasies like this, e.g. you can call The only certain way feels like properly obeying the contract. |
I managed to address the "Attempt to remove" with
I am about 90% (but not 100%) sure the call to the |
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574202123
I built some wrappers to make the code more expressive.
Function then becomes:
Lowering my probability estimate to fifty-fifty. I think the crashes might have been #190. Now I'm thinking that the cache was being warmed up on Desktop from the main (read: UI) thread, and I misattributed the fail to the warning I saw in the logs on Android. |
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574202123
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574202123
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574202123
I took a stab at trying to implement a I think, for now, since As for you helper functions, I just realised I haven't put |
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574202123
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574202123
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574202123
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574202123
…mplementations executed before proxied object. This proxy could also forward operator(), but frustratingly, it isn't sufficient to forward for types like `LocalObject`, because it would imply passing `const char*` which the InvocableMap can't see through in a constexpr way. I had hoped this coudl be used for a "non-deleting" LocalObject. See #207 for context. PiperOrigin-RevId: 574264923
Sorry, I'm unsure if I can mark this complete or not, it sounds like |
The problem is not (just) the
That's the minimal repro. One line. If I don't handle the Release or no release of the The work around I am using is:
Please confirm: Is that the correct work-around for now? At minimum, it violates this from the README:
That |
I understand what's happening here I think. While not on the public repo, I do have The issue is that JNI objects passed into the JNI call have local mechanics, but are not "deletable". Android appears to treat them differently than plain Java (or Java is deciding to surface the failure in a silent way). If you delete a passed in To test this, I simply capture an input argument with With your JNIEXPORT void JNICALL
Java_com_jnibind_test(JNIEnv* env, jclass, jobject object) {
// jni::LocalObject<kObjectClass> obj1{object}; // fails, releases unowned `object`
// jni::LocalObject<kObjectClass> obj2{}; // passes, releases owned implicit jobject
// jni::LocalObject<kObjectClass> obj3{jni::CreateCopy{}, object}; // passes, creates local deletable copy
// jni::GlobalObject<kObjectClass> obj4{jni::PromoteToGlobal{}, object}; // fails, deletes unowned `object`
// jni::GlobalObject<kObjectClass> obj5{jni::CreateCopy{}, object}; // passes, creates local deletable copy
} I clearly need to add documentation for |
I'm too slow to know what that comment means. I don't want a local anything, nor a copy of anything. It could be that I am reading too much into Is
? Either way, I did try:
If that usage is (scary quote) "correct" on your say-so, that is good enough for me. I can confirm the above does not result in the "Attempt to remove" warning and does work. Can't say I fully grasp how to use
If/when you are doing the doc updates, some guidance would be helpful. |
I will update the documentation as soon as possible. re: I could have phrased this better. If you give a re: your code block, I don't think you need your Your I think it might make |
I don't have a
I realize, acutely, that it is a stupid quesiton, but what is the idiomatic method to get a
Throwing it out there, fwiw, I am not clear on the use case for Making CreateCopy "the default" would be good, as it would steer consumers in the right direction. That said, the name is counterintuitive. It isn't a copy we want. It's a new global reference. If |
Yah, I agree it's hard to understand, but I unfortunately need it for the case of "no really, I have a local I agree As for the "idiomatic" way, I have used blocks like this: Is that not working as expected for you? |
It's not my jstr. From the top:
I am just going to go ahead and assume I need the helper; there is only so many ways I can ask the question. Do appreciate you help; looking forward to the 1.0 release. |
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. This also could break if you are passing nulls to objects (which is not allowed but would passively fail). Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. All existing usages can add `AdoptLocal{}` to duplicate existing behaviour. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. This also could break if you are passing nulls to objects (which is not allowed but would passively fail). Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. All existing usages can add `AdoptLocal{}` to duplicate existing behaviour. PiperOrigin-RevId: 577476928
Some additional discussion on #207. PiperOrigin-RevId: 581787792
See #207. PiperOrigin-RevId: 581788597
See #207. Also, fix minor typos to be able to finally cut a new release. PiperOrigin-RevId: 581788597
Some additional discussion on #207. PiperOrigin-RevId: 581787792
See #207. Also, fix minor typos to be able to finally cut a new release. PiperOrigin-RevId: 581788597
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. This also could break if you are passing nulls to objects (which is not allowed but would passively fail). Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. All existing usages can add `AdoptLocal{}` to duplicate existing behaviour. PiperOrigin-RevId: 577476928
Some additional discussion on #207. PiperOrigin-RevId: 581787792
See #207. Also, fix minor typos to be able to finally cut a new release. PiperOrigin-RevId: 581788597
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. This also could break if you are passing nulls to objects (which is not allowed but would passively fail). Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. All existing usages can add `AdoptLocal{}` to duplicate existing behaviour. PiperOrigin-RevId: 577476928
This follows from the discussion on #207. Values passed through JNI are seemingly owned by the calling context and should not be deleted. This changes behaviour in existing applications, but at most should harmlessly leak a single local reference to a jobject. This *could* be a break a breaking change if locally created JNI objects were being wrapped by a `LocalObject` in an inner loop, but can be obviated by using `AdoptLocal` anywhere this is done. This also could break if you are passing nulls to objects (which is not allowed but would passively fail). Because `LocalObject<kClass>{input_obj}` is a "default path", this is going to become the default. All existing usages can add `AdoptLocal{}` to duplicate existing behaviour. PiperOrigin-RevId: 582053094
Some additional discussion on #207. PiperOrigin-RevId: 581787792
Some additional discussion on #207. PiperOrigin-RevId: 582062820
See #207. Also, fix minor typos to be able to finally cut a new release. PiperOrigin-RevId: 581788597
See #207. Also, fix minor typos to be able to finally cut a new release. PiperOrigin-RevId: 582074658
Alright, I've finally had a chance to make some progress on this. I cut a new 0.9.8 prerelease which makes Sorry to take so long. |
When using jni-bind with Android Studio, I am getting a warning in my logs. Looks like:
Minimal repro:
My real code is basically replicating the pattern in this test case. Meaning, whether one was to stick a
new()
on thatGlobalObject
construction makes no difference.Checking the Interwebs I found this comment. "If you're a developer getting this message in your own app, make sure you're not accidentally deleting local references given as parameters to your JNI methods."
The jni-bind README states:
While the object always be a local, like the
LocalString
in the minimal example above, it isn't necessarily our local on which we can do anenv->DeleteLocalRef()
. I suspect but can't prove that is what is happening withjobject jcb
above. Or, at the very least, if I comment out the one line promotingjcb
to global, the warning disappears.The code appears to function well enough with the warning, and indeed I didn't notice until I started porting my Java language binding from Desktop to Android. It is tempting to just ignore it, but this is a protocol thing so there are going to be hundreds or thousands a second.
The text was updated successfully, but these errors were encountered: