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

content/browser/renderer_host/java/java_bound_object.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/java/java_bound_object.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/memory/singleton.h"
 #include "base/string_number_conversions.h"
 #include "base/stringprintf.h"
 #include "content/browser/renderer_host/java/java_type.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebBindings.h"
 
 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;
 
 // The conversion between JavaScript and Java types is based on the Live
 // Connect 2 spec. See
 // http://jdk6.java.net/plugin2/liveconnect/#JS_JAVA_CONVERSIONS.
 
 // Note that in some cases, we differ from from the spec in order to maintain
 // existing behavior. These areas are marked LIVECONNECT_COMPLIANCE. We may
@@ skipping 444 matching lines @@
     case NPVariantType_Object:
       return CoerceJavaScriptObjectToJavaValue(variant, target_type);
     case NPVariantType_Null:
     case NPVariantType_Void:
       return CoerceJavaScriptNullOrUndefinedToJavaValue(variant, target_type);
   }
   NOTREACHED();
   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
 
 
 NPObject* JavaBoundObject::Create(const JavaRef<jobject>& object) {
   // The first argument (a plugin's instance handle) is passed through to the
   // allocate function directly, and we don't use it, so it's ok to be 0.
   // The object is created with a ref count of one.
   NPObject* np_object = WebBindings::createObject(0, const_cast<NPClass*>(
       &JavaNPObject::kNPClass));
@@ skipping 63 matching lines @@
   return true;
 }
 
 void JavaBoundObject::EnsureMethodsAreSetUp() const {
   if (!methods_.empty()) {
     return;
   }
 
   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) {
     ScopedJavaLocalRef<jobject> java_method(
         env,
         env->GetObjectArrayElement(methods.obj(), i));
     JavaMethod* method = new JavaMethod(java_method);
     methods_.insert(std::make_pair(method->name(), method));
   }
 }