Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12313042:
[android_webview] Add a generic callback JNI bridge. (Closed)
|
Created:
7 years, 10 months ago by mkosiba (inactive) Modified:
7 years, 6 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[android_webview] Add a generic callback JNI bridge.
This is an attempt to centralize code related to dealing with callbacks
across the JNI boundary.
BUG=
Patch Set 1 #
Total comments: 16
Patch Set 2 : #
Total comments: 8
Messages
Total messages: 14 (0 generated)
So this is what I thought would be a nice mechanism for callbacks. It's a proof-of-concept impl than the final thing, but you get the picture. After implementing this I'm getting the feeling that memory management is more complicated than I initially thought it would be, but given that our 99% use case are short-lived one-shot callbacks I think the solution I'm proposing isn't that bad. Trying to use a Java generic interface doesn't look like a good idea as we lose all the type safety on the JNI layer anyway, the next iteration of this will most likely use separate interfaces for separate types. PTAL and let me know what you think. https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java (right): https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:49: @SuppressWarnings("unchecked") I'll probably want to go the direction of having separate interfaces for different types as type erasure is making use of webkit.ValueCallback a bit unsafe. Also, the JNI generator doesn't understand templates, which makes using ValueCallback directly even more problematic. https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:65: clear(); the idea here is that these are one-shot callbacks, which simplifies memory management a lot. Depending on the finalizer to always free up native resources seems like asking for trouble...
https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java (right): https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:33: * android::ScopedLocalJavaRef jcallback = JavaIntCallback(object, &Object::onIntValue); uh.. didn't update this - it should be ... = CallbackJNIBridge::ToJavaValueCallbackInt( base::Bind(&Object::onIntValue, object));
Only looked at the half I would use, ie calling java ValueCallback from native. I guess usefulness depends on how far we can push the template magic...(and I've never written any templated code!) https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... File android_webview/native/callback_jni_bridge.cc (right): https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:26: DCHECK(!jvalue_callback_.is_null()); Would be nice if it can automatically check threading error too. I assume most callbacks need to happen on the thread it was originally passed in...but maybe I'm asking for too much :) https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:40: Callback<void(int)> CallbackJNIBridge::FromJavaValueCallbackInt( There is no way to use templates to interpret the types needed since it's just the basic native types + jobject and that's it. But how about this (if it's at all possible?): pass in a templated thing (class/function?) that converts the required native arguments to the single jobject (native types are boxed into objects in java anyway), so usage would be something like: Java: nativeDoSomething(new ValueCallback<String[]>{...}); Native: void object::doSomethingInCpp(jobject callback) { Callback<std::vector<std::string> > callback = CallbackJNIBridge::FromJavaValueCallback( callback, &base::android::ToJavaArrayOfStrings) } Basically template the return type and the second conversion argument. Slightly mitigate the type unsafety too?...not really... https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:52: Callback<void(int)>* native_callback = new Callback<void(int)>(callback); new vs copy by value, the native object only holds a GlobalRef, so copy by value should be cheap too. But it's a scoped ref...ok, maybe not.
https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java (right): https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:65: clear(); On 2013/02/21 19:26:42, Martin Kosiba wrote: > the idea here is that these are one-shot callbacks, which simplifies memory > management a lot. Depending on the finalizer to always free up native resources > seems like asking for trouble... THe other pattern oft used is to couple the native object lifetime to something else suitably scoped (e.g. a map<int, Foo> inside AwContents) and just hold the int on the java side (i.e. a weak ref) this doesn't really work here for the case of a generic library. https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:70: protected void finalize() throws Throwable { use CleanupReference rather than finalizer
I think it can be useful for general use cases. In my case where I need to deal with passing a callback to the java side, I could make use of it. It seems at present I am using ~20 lines of code to store callbacks at the native side and run (not that many I know).
just replies - unfortunately no update (expect one on Monday) https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java (right): https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:65: clear(); On 2013/02/21 23:30:47, joth wrote: > On 2013/02/21 19:26:42, Martin Kosiba wrote: > > the idea here is that these are one-shot callbacks, which simplifies memory > > management a lot. Depending on the finalizer to always free up native > resources > > seems like asking for trouble... > > THe other pattern oft used is to couple the native object lifetime to something > else suitably scoped (e.g. a map<int, Foo> inside AwContents) and just hold the > int on the java side (i.e. a weak ref) > this doesn't really work here for the case of a generic library. I was considering whether I should make the CallbackJNIBridge be a member of whatever class is using it. That way it would be easier to bind the lifetime of the callback to the object that's holding on to the Bridge but would make using the Bridge a lot less convenient... https://codereview.chromium.org/12313042/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:70: protected void finalize() throws Throwable { On 2013/02/21 23:30:47, joth wrote: > use CleanupReference rather than finalizer ok https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... File android_webview/native/callback_jni_bridge.cc (right): https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:26: DCHECK(!jvalue_callback_.is_null()); On 2013/02/21 23:11:12, boliu wrote: > Would be nice if it can automatically check threading error too. I assume most > callbacks need to happen on the thread it was originally passed in...but maybe > I'm asking for too much :) nope, the more auto-checking we have the better. I can add the checks and if we need thread-hopping callbacks there could be a more explicitly-named method to create one. https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:40: Callback<void(int)> CallbackJNIBridge::FromJavaValueCallbackInt( On 2013/02/21 23:11:12, boliu wrote: > There is no way to use templates to interpret the types needed since it's just > the basic native types + jobject and that's it. > > But how about this (if it's at all possible?): pass in a templated thing > (class/function?) that converts the required native arguments to the single > jobject (native types are boxed into objects in java anyway), so usage would be > something like: > > Java: > nativeDoSomething(new ValueCallback<String[]>{...}); > > Native: > void object::doSomethingInCpp(jobject callback) { > Callback<std::vector<std::string> > callback = > CallbackJNIBridge::FromJavaValueCallback( > callback, &base::android::ToJavaArrayOfStrings) > } > > > Basically template the return type and the second conversion argument. > > Slightly mitigate the type unsafety too?...not really... I talked to Marcus and he'd prefer us to make the types explicit (so ValueCallbackStringArray in your example) exactly because the JNI boundary is unsafe. https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:52: Callback<void(int)>* native_callback = new Callback<void(int)>(callback); On 2013/02/21 23:11:12, boliu wrote: > new vs copy by value, the native object only holds a GlobalRef, so copy by value > should be cheap too. But it's a scoped ref...ok, maybe not. The Callback has an internal scoped_refptr<BindState> that has all of the actual state. The reason I use new here is so that I can pass ownership of the callback (and the BindState) to the Java side.
https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... File android_webview/native/callback_jni_bridge.cc (right): https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:31: env, jvalue_callback_.obj(), value); Also just realized, generic types must be an object, so does this actually work? ie does autobox also work from jni code? https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:40: Callback<void(int)> CallbackJNIBridge::FromJavaValueCallbackInt( On 2013/02/22 18:54:31, Martin Kosiba wrote: > I talked to Marcus and he'd prefer us to make the types explicit (so > ValueCallbackStringArray in your example) exactly because the JNI boundary is > unsafe. I'm gonna push back on this a bit at least for non-general complex types, example being AwQuotaManagerBridge.Origin in my other patch. If we go down the explicit route, we'll have something like Callback<void(std::vector, std::vector, std::vector) FromJavaValueCallbackOrigin(JNIEnv, jobject jcallback) And the corresponding JavaValueCallbackOrigin class that needs to convert the 3 native vectors into the correct java Origin object. This code needs a lot of knowledge from AwQuotaManagerBridge and will probably have 0 reuse value elsewhere, so it should live in AwQuotaMangerBridge. But at that point, it seems better to just do it like the way in my patch instead of doing all this acrobatics just to consistent with everywhere else. Another point is that as long as we are calling callback.onValueReceived from native side, we are going to lose type safety somewhere. We are counting on the caller to call the right function here, which is not that much better than having the caller write the native->java conversion. Of course the conversion for the basic types can still be provided here.
On 22 February 2013 15:02, <boliu@chromium.org> wrote: > > https://codereview.chromium.**org/12313042/diff/1/android_** > webview/native/callback_jni_**bridge.cc<https://codereview.chromium.org/12313042/diff/1/android_webview/native/callback_jni_bridge.cc> > File android_webview/native/**callback_jni_bridge.cc (right): > > https://codereview.chromium.**org/12313042/diff/1/android_** > webview/native/callback_jni_**bridge.cc#newcode31<https://codereview.chromium.org/12313042/diff/1/android_webview/native/callback_jni_bridge.cc#newcode31> > android_webview/native/**callback_jni_bridge.cc:31: env, > jvalue_callback_.obj(), value); > Also just realized, generic types must be an object, so does this > actually work? ie does autobox also work from jni code? > > > https://codereview.chromium.**org/12313042/diff/1/android_** > webview/native/callback_jni_**bridge.cc#newcode40<https://codereview.chromium.org/12313042/diff/1/android_webview/native/callback_jni_bridge.cc#newcode40> > android_webview/native/**callback_jni_bridge.cc:40: Callback<void(int)> > CallbackJNIBridge::**FromJavaValueCallbackInt( > On 2013/02/22 18:54:31, Martin Kosiba wrote: > >> I talked to Marcus and he'd prefer us to make the types explicit (so >> ValueCallbackStringArray in your example) exactly because the JNI >> > boundary is > >> unsafe. >> > > I'm gonna push back on this a bit at least for non-general complex > types, example being AwQuotaManagerBridge.Origin in my other patch. > > If we go down the explicit route, we'll have something like > Callback<void(std::vector, std::vector, std::vector) > FromJavaValueCallbackOrigin(**JNIEnv, jobject jcallback) > > Following the jni standard practices CallbackJNIBridge should just have a single Callback<void(jobject)> FromJavaValueCallbackObject(JNIEnv*, jcallback); and off you'd go from there. As you say, a generic method like this can't know how to usefully convert multiple base::Callback bound params into a single compound java object to pass to ValueCallback<Object>. right? > And the corresponding JavaValueCallbackOrigin class that needs to > convert the 3 native vectors into the correct java Origin object. This > code needs a lot of knowledge from AwQuotaManagerBridge and will > probably have 0 reuse value elsewhere, so it should live in > AwQuotaMangerBridge. But at that point, it seems better to just do it > like the way in my patch instead of doing all this acrobatics just to > consistent with everywhere else. > > Another point is that as long as we are calling callback.onValueReceived > from native side, we are going to lose type safety somewhere. We are > counting on the caller to call the right function here, which is not > that much better than having the caller write the native->java > conversion. Of course the conversion for the basic types can still be > provided here. > > https://codereview.chromium.**org/12313042/<https://codereview.chromium.org/1... >
On 2013/02/22 23:43:20, joth wrote: > Following the jni standard practices CallbackJNIBridge should just have a > single > Callback<void(jobject)> FromJavaValueCallbackObject(JNIEnv*, jcallback); > and off you'd go from there. > > As you say, a generic method like this can't know how to usefully convert > multiple base::Callback bound params into a single compound java object to > pass to ValueCallback<Object>. > > right? Ha, this seems even more unsafe than what I was suggesting, passing around callbacks that all look exactly the same in native. Also constructing the callback argument jobject in native code will involve some (potentially ugly) jni calls. Whatever this ends up being should be convenient enough to justify those additional calls I guess.
On 22 February 2013 16:10, <boliu@chromium.org> wrote: > On 2013/02/22 23:43:20, joth wrote: > >> Following the jni standard practices CallbackJNIBridge should just have a >> single >> Callback<void(jobject)> FromJavaValueCallbackObject(**JNIEnv*, >> jcallback); >> and off you'd go from there. >> > > As you say, a generic method like this can't know how to usefully convert >> multiple base::Callback bound params into a single compound java object to >> pass to ValueCallback<Object>. >> > > right? >> > > Ha, this seems even more unsafe than what I was suggesting, passing around > callbacks that all look exactly the same in native. > > I lost track, which suggestion? lack of type safety on jobject is not a huge deal, as it's reliably caught at runtime anyway (so long as a single test path covers it....). Also, we could probably write extra debug-only reflection code to verify the first level too. (i.e. that jcallback is of type ValueCallback<Object> -- the generics make that more awkward mind). Incorrectly munging jint or jbool or even native object pointers, on the other hand..... ouch. > Also constructing the callback argument jobject in native code will > involve some > (potentially ugly) jni calls. Whatever this ends up being should be > convenient > enough to justify those additional calls I guess. > > this would typically be done via a @CalledByNative method which ever route was taken. > https://codereview.chromium.**org/12313042/<https://codereview.chromium.org/1... >
On 2013/02/23 00:20:40, joth wrote: > I lost track, which suggestion? > > lack of type safety on jobject is not a huge deal, as it's reliably caught > at runtime anyway (so long as a single test path covers it....). Also, we > could probably write extra debug-only reflection code to verify the first > level too. (i.e. that jcallback is of type ValueCallback<Object> -- the > generics make that more awkward mind). Type erasure makes this an unpleasant problem, you completely lose the generic type information at runtime. The only thing you can do is inspect the type of the parameter passed into onReceivedValue, but that's about it. No, T.class doesn't work either. > > Incorrectly munging jint or jbool or even native object pointers, on the > other hand..... ouch. > yeath, for some of these combinations JNI will complain, for others it won't
Ok, so I'm going to work on this a bit more tomorrow, but feel free to pile on comments as I sleep ;) I put in some comments in places where I'd like a bit more input so feel free to start there. PS. This CL was endorsed by the 'Tea, Template and Knitting Lovers Association' and is part of the 'Templates for Job Security' campaign. https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... File android_webview/native/callback_jni_bridge.cc (right): https://codereview.chromium.org/12313042/diff/1/android_webview/native/callba... android_webview/native/callback_jni_bridge.cc:31: env, jvalue_callback_.obj(), value); On 2013/02/22 23:02:52, boliu wrote: > Also just realized, generic types must be an object, so does this actually work? > ie does autobox also work from jni code? no, you'd have to new up an Integer object if you wanted to call a ValueCallback<Integer> https://codereview.chromium.org/12313042/diff/10001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java (right): https://codereview.chromium.org/12313042/diff/10001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:84: @SuppressWarnings("unchecked") I think this guy will definitely stay - ie. I don't want to pass ValueCallbacks directly to native. This wrap method gives us a nice point to do potential future customizations (like memory ownership changes or the exception handling that's here) https://codereview.chromium.org/12313042/diff/10001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:104: public static JavaCallbackInt wrapCallbackInt(final ValueCallback<Integer> vc) { It would be useful if the same method call could pick out the 'specialized' method for some set of types, but fall back on the default object path (like the C++ counterpart does). Unfortunately that's not possible. Why is this a problem? I wrote wrapCallback() instead of wrapCallbackInt in my unittest code and had a bad time figuring out what the JNI error was trying to tell me. That seems like a strong argument against providing possibly maybe faster[1] specialized versions of the wrapCallback method. The initial reason I added these was for more safety[2], but now that I've got all the code out there I don't think we gain that much - there are at least two places where you need to call the correctly named method variant and even though there is a way to mitigate some of this pain (see the comment about adding a DCHECK in the .cc file) I'm not sure if keeping the specializations around makes any sense. [1] using a ValueCallback<Integer> through the JavaCallback interface (instead of JavaCallbackInt) would require two JNI calls - one for the callback and one for creating the Integer. [2] it *looks* safer since the JNI methods take in a jint, jlong and jboolean but since you still pass the callback itself as a jobject there isn't anything stopping you from calling the wrong C++ method (call a FromJavaCallback<int> on a ValueCallback<String>) https://codereview.chromium.org/12313042/diff/10001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/CallbackJNIBridge.java:159: private abstract static class NativeCallbackBase<T> implements ValueCallback<T> { these guys don't seem to have the problem the JavaCallbackXXX interface had. Would it make sense to go down to an Object-only version for the sake of symmetry? (and some test simplification) https://codereview.chromium.org/12313042/diff/10001/android_webview/native/ca... File android_webview/native/callback_jni_bridge.cc (right): https://codereview.chromium.org/12313042/diff/10001/android_webview/native/ca... android_webview/native/callback_jni_bridge.cc:81: jvalue_callback_.Reset(env, jvalue_callback.obj()); I think I'll add a DCHECK here to see if jvalue_callback implements the JavaCallback interface, that should change the very puzzling JNI error when you try and invoke a Native JavaCallback<int> with a Java JavaCallback<String> to a friendly error :) https://codereview.chromium.org/12313042/diff/10001/android_webview/native/ca... File android_webview/native/callback_jni_bridge.h (right): https://codereview.chromium.org/12313042/diff/10001/android_webview/native/ca... android_webview/native/callback_jni_bridge.h:19: private: uh.. torn between this and an 'internal' namespace. https://codereview.chromium.org/12313042/diff/10001/android_webview/native/ca... android_webview/native/callback_jni_bridge.h:43: template <typename N, typename J> as you'll see in the unittest code converting to and from jobject doesn't actually work that well since some of the conversion functions actually operate on the more specific JNI type (jstring, jarray, etc..). Most of the blame goes to ScopedJavaLocalRef for not supporting implict conversions: ScopedJavaLocalRef<jobject> = ScopedJavaLocalRef<jstring> doesn't work. In this case I think I'll just go and fix ScopedJavaLocalRef to support that. https://codereview.chromium.org/12313042/diff/10001/android_webview/native/ca... android_webview/native/callback_jni_bridge.h:86: base::Callback<N(ScopedJobject)> converter) { now this is going to be fun - since we always get a jobject in I guess there is no other option than to trust the caller and convert the jobject to whatever the converter expects. The snag is that the reinterpret_cast will happily do that for types that are obviously incompatible with jobject (like const char* for example) but hey, nobody said C++ is a safe language.
Thanks for persevering. Can I ask, before you spend more time on this, you 1/ find one or two existing places and use to illustrate how much cleaner this feature would make that code in practice 2/ gauge roughly how many places will benefit given we've written a heck of a lot of code very successfully without it, I really want to see a case for how much better life will be with it. (as this will be a non-trivial patch to review and land, and on past experience will need subsequent finessing to get to point of full genericness..) https://codereview.chromium.org/12313042/diff/10001/android_webview/native/ca... File android_webview/native/callback_jni_bridge.h (right): https://codereview.chromium.org/12313042/diff/10001/android_webview/native/ca... android_webview/native/callback_jni_bridge.h:43: template <typename N, typename J> Name N and J something more descriptive like above, or at last documenting them, will help read this.
I assume this is not going to be landed, can we abort it? |
