Precedent for "#define I_KNOW_WHAT_IM_DOING"?

[Torne (Richard Coles)]

I'm trying to come up with a way to prevent misuse of ScopedJavaLocalRef (for http://crbug.com/506850) and a fairly tempting option is to restrict the class's two-argument constructor to only be usable in parts of the codebase responsible for implementing JNI wrapping/utility functions (such as base/android/jni_*), where implementors hopefully are familiar with the exact details of how JNI is supposed to work.

It seems like the simplest way to do this would be to wrap the constructor declaration in something like "#ifdef INSIDE_JNI_WRAPPER_IMPLEMENTATION" and define that in the relevant implementation files, but I'm not aware of any precedent for this in the chromium code and suspect that this might be bad style. ;)

Is there a recommended way to deal with this kind of thing? It needs to be usable in various source files, not all contained within base, and listing them in friend declarations will get *extremely* verbose as there are dozens of references, many from non-class functions. :/

[Alexei Svitkine]

One approach I've seen is putting code either in an "internal" namespace or in an "internal" subdirectory - indicating that it shouldn't generally be used except by the component it's intended for.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Torne (Richard Coles)]

Hm. If I split the ability to construct the type that way into a subclass, I could potentially put the definition into a different header in some internal directory, and control it with DEPS, yeah..

[Nico Weber]

I agree that making parts of a class available only in some TUs is bad style. Consider what happens if the conditional part happens to contain virtual methods: The class will end up with vtables of different size in different TUs.

--

[Torne (Richard Coles)]

This is a single inlined constructor of a type with no virtuals, so that can't happen in this case, but yeah.

I'll experiment with splitting it into a subclass defined in a different place.

[Anton Vayvod]

Can't you make the ctor private, make one static creator function a friend and define it in the internal namespace/subdir, then call the function from everywhere needed instead of the ctor? I assume there's no issue of the class implementation calling the ctor.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Newton Allen]

Am I understanding the problem correctly?

Example problem code:

  static void FetchStorageInfo(JNIEnv* env, jclass clazz, jobject java_callback) {

    ...

    scoped_refptr<StorageInfoFetcher> storage_info_fetcher(new StorageInfoFetcher(

        ScopedJavaLocalRef<jobject>(env, java_callback)));

    ...

  }

ScopedJavaLocalRef's 2-arg constructor takes ownership of java_callback and will call DeleteLocalRef() on java_callback when it goes out of scope. However, java_callback is not a local ref and the 2-arg constructor doesn't call NewLocalRef() to create a local ref.

By comparison, ScopedJavaLocalRef<jobject>(java_callback) or scoped_java_local_ref.Reset(java_callback) would call NewLocalRef() and later DeleteLocalRef(), as desired.

On Wed, Jul 29, 2015 at 10:19 AM, Torne (Richard Coles) <to...@chromium.org> wrote:

--

[Torne (Richard Coles)]

No, we shouldn't be creating new local refs for these either. We just need to not delete them.

[Newton Allen]

Why shouldn't we create a new local ref here? StorageInfoFetcher takes a ScopedJavaLocalRef, so how would you rewrite that problematic line?

[Torne (Richard Coles)]

The JNI spec does not promise that an unlimited number of local references can be created (in fact if I recall correctly it only has to guarantee 8), and creating them does have some cost.

Nothing should take a ScopedJavaLocalRef as a parameter; it is too specific a type. Functions should take JavaRef instead. To deal with these cases one option is to make a new JavaRef type for parameters.

[Selim Gurun]

for the curious, the limit seems to be 16: http://developer.android.com/training/articles/perf-jni.html

 The implementation is only required to reserve slots for 16 local references, so if you need more than that you should either delete as you go or use EnsureLocalCapacity/PushLocalFrame to reserve more.

[Dmitry Skiba]

Wild idea: introduce 'param' jobject types, change generator to use those in generated files, add a constructor to ScopedJavaLocalRef that will remember not to destroy them:

template <class T>

class jparam: public std::remove_pointer<T>::type {};

typedef jparam<jobject>* jobject_param;

typedef jparam<jarray>* jarray_param;

...

ScopedJavaLocalRef(JNIEnv* env, jparam<T>* param) {

    delete_ref_ = false;

    ...

}

And example above would use _param types:

static void FetchStorageInfo(JNIEnv* env, jclass clazz, jobject_param java_callback)...

Since _param types is-a normal JNI types, there shouldn't be any errors.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Torne (Richard Coles)]

I was looking at implementing a different variant of this (just wrapping them in a different subclass of JavaRef that doesn't delete the jobject) but the JNI generator is already *extremely* complicated and this got very fiddly very quickly, and will in any case require every single JNI method in the entire chromium codebase to have its signature updated :/

[Dmitry Skiba]

Can we make generator to create local references for object arguments before calling handlers?

I.e.

__attribute__((visibility("default")))

jboolean

    Java_org_chromium_chrome_browser_preferences_PrefServiceBridge_nativeIsContentSettingEnabled(JNIEnv*

    env, jobject jcaller,

    jint contentSettingType) {

  return IsContentSettingEnabled(env, jcaller, contentSettingType);

}

->

__attribute__((visibility("default")))

jboolean

    Java_org_chromium_chrome_browser_preferences_PrefServiceBridge_nativeIsContentSettingEnabled(JNIEnv*

    env, jobject jcaller,

    jint contentSettingType) {

  return IsContentSettingEnabled(env, env->NewLocalRef(jcaller), contentSettingType);

}

[Torne (Richard Coles)]

I don't want to make new local references for every parameter; this really shouldn't be necessary and might impact performance..

[Torne (Richard Coles)]

OK, I've experimented with some of the ideas I had and some of the ideas suggested here, and it seems to boil down to three possible solutions for the JNI parameter lifetime issue:

1) Make the 2-arg constructor for ScopedJavaLocalRef inaccessible to most code by some mechanism (exactly which isn't important), thus banning "general" code from promoting jobject to ScopedJavaLocalRef. This is what I was talking about originally in this thread.

Then go find all the places that no longer compile (looks like there's about 60 files that do this) and either exempt them (if they're JNI helper/utility functions) or introduce new helpers to remove their need to do it (for things like array element accesses, which aren't currently wrapped but could be).

The problem with this is that the cases which are actually causing the problem (i.e. the ones where people really are wrapping JNI method parameters in ScopedJavaLocalRef) don't have any alternative in many cases because they want to pass the object to a function that takes a JavaRef, so we'd need to introduce a new type of JavaRef for them to use, a JavaParamRef that doesn't delete the reference it refers to. If JavaParamRef was misused for objects that aren't parameters it would introduce the opposite bug: objects that are held referenced longer than they should be (though local refs are all freed in any case when we return to java, so it's not a permanent leak).

2) Introduce a JavaParamRef type that works as in 1), and make the JNI generator wrap all object parameters of JNI methods in one before passing them to the actual implementations. I've prototyped this in the JNI generator and the code to do it adds yet more complexity to the already overcomplicated JNI generator, but given enough time this could probably be refactored to be somewhat less terrible.

This works pretty reasonably in theory and removes most of the cases where we ever pass bare jobjects around in chromium, but the practical problem is that there are something like 800 files containing several thousand JNI methods which would all need to be updated to have a new signature and to actually use the variables differently (calling .obj() on them to get the jobject back, or interacting with other JNI helper functions differently). This is going to be a very long and boring job that I'm not sure could be entirely automated (though some cases probably could be).

3) Just have the JNI generator make a new local ref for every parameter so we can delete them without worrying. No real changes to the actual chromium code. Adds the performance cost of calling back into the VM at least one additional time per JNI method call (every method has at least one jobject parameter: the calling object/class). Uses up more of the local reference slots allowed by the spec  (though in practise the android runtime seems to dynamically allocate them anyway and there may not be a real meaningful limit).

These all seem to be pretty sucky options. Any thoughts? :)

[Anton Vayvod]

Do you think it doesn't make sense measuring the performance cost of #3 given that sounds not hard to implement (no change to Chromium code) and run through perf tests? Maybe most of the JNI methods are not in the critical path so an extra JVM call doesn't make much difference?

[Dmitry Skiba]

I personally like #2 because it's safe and obvious - functions take JavaRefs, which are objects, can be safely copied / passed / stored, etc. Same thing should be done to return values - it should also be JavaRef. We could also create JNIEnv wrapper that returns / takes JavaRefs everywhere, to make this approach complete.

There is an issue with #3 - there is a possibility for a leak if an argument is not wrapped by ScopedJavaLocalRef on the calling side. To counter that we could use PushFrame/PopFrame, but that would make the approach even more expensive. We could also do something like per-thread 'param' ref registry and have ScopedJavaLocalRef to consult that, but that feels hacky.

[Torne (Richard Coles)]

On Thu, 30 Jul 2015 at 15:49 Dmitry Skiba <dsk...@google.com> wrote:

I personally like #2 because it's safe and obvious - functions take JavaRefs, which are objects, can be safely copied / passed / stored, etc. Same thing should be done to return values - it should also be JavaRef. We could also create JNIEnv wrapper that returns / takes JavaRefs everywhere, to make this approach complete.

Return values of C++ object methods are already JavaRefs, but not for regular C++ functions. The JNI generator is pretty inconsistent in how it chooses to wrap/not wrap things. I think the reasoning was that C++ method calls have to be wrapped by the generator to convert one of the JNI method parameters into a "this" pointer anyway, and so if you're already going to generate a stub you already have somewhere to put the return type conversion (though it still didn't convert the parameters), whereas non-methods are normally just mapped directly to the JNI interface without a stub inbetween.. :/

I do kinda like this idea as well, especially if we also wrap the rest of JNIEnv, but that's going to be a huuuuge task..

There is an issue with #3 - there is a possibility for a leak if an argument is not wrapped by ScopedJavaLocalRef on the calling side.

It's not really a leak, because local refs go away on return anyway and the actual object it refers to already has a local ref for the original parameter (the one you aren't allowed to delete). I don't think it's actually possible for the lifetime of any object to change if someone makes this mistake, so it doesn't matter?

[Dmitry Skiba]

Oh right, in #3 we're not really leaking. Well, not more than a normal JNI call from Java would "leak" anyway - so in degenerate case of deeply nested Java->native->Java->native calls #3 will run out of local references (the limit is 512 I believe) twice as fast.

[Torne (Richard Coles)]

You're only allowed to assume you can have 16, per spec. ART's actual limit in practise appears to be 65536, but has currently-disabled code to check you don't use more than 16 and print a warning if you do.

[Dmitry Skiba]

There is kLocalsMax = 512 in jni_env_ext.h, which causes infamous JNI ERROR (app bug): local reference table overflow (max=512) in indirect_reference_table.cc

But anyway, #3 seems the way to go.

[Torne (Richard Coles)]

Having now looked into several other JNI binding bugs it seems like we probably should actually do the work to restructure everything to be in terms of JavaRefs at some point (incrementally, maybe?). There's a pretty big range of possible reference handling bugs at the moment: the one I just found was someone passing "some_scoped_local_ref.Release()" to a Java function as a parameter, which leaks the local reference (they probably meant .obj() instead). Unfortunately this use of Release() is really common because every JNI method that returns an object ends up doing "return foo.Release()" since the JNI bindings expect a jobject to be returned. If the binding generator instead expected methods to return JavaRef<jobject> then these Release() calls would be unnecessary and much less likely to be copied into inappropriate places :/

I may still do #3 here as a short term fix. The JNI generator really needs some substantial refactoring and simplification in any case..