Don't use Singleton to cache JNI method IDs in Java Bridge

content/browser/renderer_host/java/java_bound_object.cc

Index: content/browser/renderer_host/java/java_bound_object.cc
diff --git a/content/browser/renderer_host/java/java_bound_object.cc b/content/browser/renderer_host/java/java_bound_object.cc
index e139d4696a775748068d49d9434b00894d9a8b92..7251ae8ec41ec380f036a3ccbdaf748ac003525c 100644
--- a/content/browser/renderer_host/java/java_bound_object.cc
+++ b/content/browser/renderer_host/java/java_bound_object.cc
@@ -15,8 +15,8 @@
 using base::StringPrintf;
 using base::android::AttachCurrentThread;
 using base::android::ConvertUTF8ToJavaString;
+using base::android::GetMethodIDFromClassName;
 using base::android::JavaRef;
-using base::android::MethodID;
 using base::android::ScopedJavaGlobalRef;
 using base::android::ScopedJavaLocalRef;
 using WebKit::WebBindings;
@@ -481,33 +481,22 @@ jvalue CoerceJavaScriptValueToJavaValue(const NPVariant& variant,
   return jvalue();
 }
 
-class ObjectGetClassID : public MethodID {
- public:
-  static ObjectGetClassID* GetInstance() {
-    return Singleton<ObjectGetClassID>::get();

[M-A Ruel, 2011/12/01 14:36:03]

If you had used LazyInstance in the first place, you wouldn't have had atexit()
registration. Ironically by using a static local variable, you add an implicit
atexit call.

[joth, 2011/12/02 11:39:02]

I don't quite follow this bit-
- both Singleton<> and LazyInstance<> register with AtExit to call the
destructor, unless you use the leaky traits variants of the same. The choice
Singleton vs LazyInstnace doesn't seem to matter?
- A jmethodID is typedef for a struct pointer. A pointer is POD so why would
declaring a static pointer cause an implicit atexit call?

related observations:
=> thakis is making great headway in removing static initializers and atexit
calls, and a good chunk of his fixes are just replacing local static class
instances with a static reference (e.g.
http://codereview.chromium.org/8480033/diff/27/chrome/browser/chromeos/cros/n...)

=> Should CR_DEFINE_STATIC_LOCAL even exist if this is not the correct solution?
http://codesearch.google.com/#OAMlx_jo-ck/src/base/basictypes.h&exact_package...
(oh heck, I hope this isn't a static pointer vs reference difference?)

-  }
- private:
-  friend struct DefaultSingletonTraits<ObjectGetClassID>;
-  ObjectGetClassID()
-      : MethodID(AttachCurrentThread(), "java/lang/Object", "getClass",
-                 "()Ljava/lang/Class;") {
-  }
-  DISALLOW_COPY_AND_ASSIGN(ObjectGetClassID);
-};
+jmethodID GetObjectGetClassID() {
+  static jmethodID id = GetMethodIDFromClassName(AttachCurrentThread(),

[M-A Ruel, 2011/12/01 14:36:03]

I'm really not a fan of static locals. Especially that your copy pasted code
could have been simplified even more than what you just wrote:

template<const char X, const char Y, const char Z>
class ObjectGetXXID : MethodID {
 public:
  ObjectGetXXID()
      : MethodID(AttachCurrentThread(), X, Y, Z) {
  }
 private:
  DISALLOW_COPY_AND_ASSIGN(ObjectGetXXID);
}

const char kJavaLangObject[] = "java/lang/Object";
const char kGetClass[] = "getClass";
const char kQJavaLangClass[] = "()Ljava/lang/Class;";
const char kGetMethods[] = "getMethods";
const char kQBJavaLangReflectMethod[] = "()[Ljava/lang/reflect/Method;";

const char kJavaLangReflectMethod[] = "java/lang/reflect/Method";
const char kGetParameterTypes[] = "getParameterTypes";
const char kQBJavaLangClass[] = "()[Ljava/lang/Class;";

typedef ObjectGetXXId<kJavaLangObject, kGetClass, kQJavaLangClass>
ObjectGetClassID;
typedef ObjectGetXXId<kJavaLangObject, kGetMethods, kQBJavaLangClass>
ObjectGetClassID;
typedef ObjectGetXXId<kJavaLangReflectMethod, kGetParameterTypes,
kQBJavaLangClass> ObjectGetClassID;

Note the implementation above is incomplete, it also needs the LazyInstance
implementation but in any case, I think it's the wrong layer to do caching so it
was just to make a point that the copy pasted classes could have been simplified
already.

+                                                 "java/lang/Object",
+                                                 "getClass",
+                                                 "()Ljava/lang/Class;");
+  return id;
+}
 
-class ClassGetMethodsID : public MethodID {
- public:
-  static ClassGetMethodsID* GetInstance() {
-    return Singleton<ClassGetMethodsID>::get();
-  }
- private:
-  friend struct DefaultSingletonTraits<ClassGetMethodsID>;
-  ClassGetMethodsID()
-      : MethodID(AttachCurrentThread(), "java/lang/Class", "getMethods",
-                 "()[Ljava/lang/reflect/Method;") {
-  }
-  DISALLOW_COPY_AND_ASSIGN(ClassGetMethodsID);
-};
+jmethodID GetClassGetMethodsID() {
+  static jmethodID id = GetMethodIDFromClassName(
+      AttachCurrentThread(),
+      "java/lang/Class",
+      "getMethods",
+      "()[Ljava/lang/reflect/Method;");
+  return id;
+}
 
 }  // namespace
 
@@ -591,11 +580,9 @@ void JavaBoundObject::EnsureMethodsAreSetUp() const {
 
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jclass> clazz(env, static_cast<jclass>(
-      env->CallObjectMethod(java_object_,
-                            ObjectGetClassID::GetInstance()->id())));
+      env->CallObjectMethod(java_object_, GetObjectGetClassID())));
   ScopedJavaLocalRef<jobjectArray> methods(env, static_cast<jobjectArray>(
-      env->CallObjectMethod(clazz.obj(),
-                            ClassGetMethodsID::GetInstance()->id())));
+      env->CallObjectMethod(clazz.obj(), GetClassGetMethodsID())));
   size_t num_methods = env->GetArrayLength(methods.obj());
   DCHECK(num_methods) << "Java objects always have public methods";
   for (size_t i = 0; i < num_methods; ++i) {