Remove env() getter from JavaRef<>

    Due to threadsafety ambiguity, it's not generally safe to call env()
    on a random Ref you've been passed, so this removes the temptation.
    The useful LocalRef optimizations are retained though, just in that
    sub-class.

(upstreaming CL, cherrypicked cleanly)

BUG=
TEST=


Review URL: http://codereview.chromium.org/9584014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128204 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 82 insertions, 61 deletions

diff --git a/base/android/jni_string.cc b/base/android/jni_string.cc
index 94ec0df..d0ffeec 100644
--- a/base/android/jni_string.cc
+++ b/base/android/jni_string.cc
@@ -29,7 +29,7 @@ std::string ConvertJavaStringToUTF8(JNIEnv* env, jstring str) {
 }
 
 std::string ConvertJavaStringToUTF8(const JavaRef<jstring>& str) {
-  return ConvertJavaStringToUTF8(str.env(), str.obj());
+  return ConvertJavaStringToUTF8(AttachCurrentThread(), str.obj());
 }
 
 ScopedJavaLocalRef<jstring> ConvertUTF8ToJavaString(
@@ -57,7 +57,7 @@ string16 ConvertJavaStringToUTF16(JNIEnv* env, jstring str) {
 }
 
 string16 ConvertJavaStringToUTF16(const JavaRef<jstring>& str) {
-  return ConvertJavaStringToUTF16(str.env(), str.obj());
+  return ConvertJavaStringToUTF16(AttachCurrentThread(), str.obj());
 }
 
 // TODO(joth): change this to accept const StringPiece16&.
diff --git a/base/android/scoped_java_ref.cc b/base/android/scoped_java_ref.cc
index 2cbcc75..21b466e 100644
--- a/base/android/scoped_java_ref.cc
+++ b/base/android/scoped_java_ref.cc
@@ -4,60 +4,67 @@
 
 #include "base/android/scoped_java_ref.h"
 
+#include "base/android/jni_android.h"
 #include "base/logging.h"
 
 namespace base {
 namespace android {
 
-JavaRef<jobject>::JavaRef() : env_(NULL), obj_(NULL) {}
+JavaRef<jobject>::JavaRef() : obj_(NULL) {}
 
-JavaRef<jobject>::JavaRef(JNIEnv* env, jobject obj)
-    : env_(env),
-      obj_(obj) {
+JavaRef<jobject>::JavaRef(JNIEnv* env, jobject obj) : obj_(obj) {
   if (obj) {
-    DCHECK(env);
-    DCHECK_EQ(JNILocalRefType, env->GetObjectRefType(obj));
+    DCHECK(env && env->GetObjectRefType(obj) == JNILocalRefType);
   }
 }
 
 JavaRef<jobject>::~JavaRef() {
 }
 
-void JavaRef<jobject>::SetNewLocalRef(JNIEnv* env, jobject obj) {
+JNIEnv* JavaRef<jobject>::SetNewLocalRef(JNIEnv* env, jobject obj) {
+  if (!env) {
+    env = AttachCurrentThread();
+  } else {
+    DCHECK_EQ(env, AttachCurrentThread());  // Is |env| on correct thread.
+  }
   if (obj)
     obj = env->NewLocalRef(obj);
   if (obj_)
-    env_->DeleteLocalRef(obj_);
-  env_ = env;
+    env->DeleteLocalRef(obj_);
   obj_ = obj;
+  return env;
 }
 
 void JavaRef<jobject>::SetNewGlobalRef(JNIEnv* env, jobject obj) {
+  if (!env) {
+    env = AttachCurrentThread();
+  } else {
+    DCHECK_EQ(env, AttachCurrentThread());  // Is |env| on correct thread.
+  }
   if (obj)
     obj = env->NewGlobalRef(obj);
   if (obj_)
-    env_->DeleteGlobalRef(obj_);
-  env_ = env;
+    env->DeleteGlobalRef(obj_);
   obj_ = obj;
 }
 
-void JavaRef<jobject>::ResetLocalRef() {
-  if (obj_)
-    env_->DeleteLocalRef(obj_);
-  env_ = NULL;
-  obj_ = NULL;
+void JavaRef<jobject>::ResetLocalRef(JNIEnv* env) {
+  if (obj_) {
+    DCHECK_EQ(env, AttachCurrentThread());  // Is |env| on correct thread.
+    env->DeleteLocalRef(obj_);
+    obj_ = NULL;
+  }
 }
 
 void JavaRef<jobject>::ResetGlobalRef() {
-  if (obj_)
-    env_->DeleteGlobalRef(obj_);
-  env_ = NULL;
-  obj_ = NULL;
+  if (obj_) {
+    AttachCurrentThread()->DeleteGlobalRef(obj_);
+    obj_ = NULL;
+  }
 }
 
 jobject JavaRef<jobject>::ReleaseInternal() {
   jobject obj = obj_;
-  env_ = NULL;
   obj_ = NULL;
   return obj;
 }
diff --git a/base/android/scoped_java_ref.h b/base/android/scoped_java_ref.h
index dd93b25..0d1e590 100644
--- a/base/android/scoped_java_ref.h
+++ b/base/android/scoped_java_ref.h
@@ -22,7 +22,6 @@ template<typename T> class JavaRef;
 template<>
 class JavaRef<jobject> {
  public:
-  JNIEnv* env() const { return env_; }
   jobject obj() const { return obj_; }
 
   bool is_null() const { return obj_ == NULL; }
@@ -39,14 +38,13 @@ class JavaRef<jobject> {
 
   // The following are implementation detail convenience methods, for
   // use by the sub-classes.
-  void SetNewLocalRef(JNIEnv* env, jobject obj);
+  JNIEnv* SetNewLocalRef(JNIEnv* env, jobject obj);
   void SetNewGlobalRef(JNIEnv* env, jobject obj);
-  void ResetLocalRef();
+  void ResetLocalRef(JNIEnv* env);
   void ResetGlobalRef();
   jobject ReleaseInternal();
 
  private:
-  JNIEnv* env_;
   jobject obj_;
 
   DISALLOW_COPY_AND_ASSIGN(JavaRef);
@@ -71,31 +69,36 @@ class JavaRef : public JavaRef<jobject> {
 };
 
 // Holds a local reference to a Java object. The local reference is scoped
-// to the lifetime of this object. Note that since a JNI Env is only suitable
-// for use on a single thread, objects of this class must be created, used and
-// destroyed on the same thread.
-// In general, this class should only be used as a stack-based object. If you
-// wish to have the reference outlive the current callstack (e.g. as a class
-// member) use ScopedJavaGlobalRef instead.
+// to the lifetime of this object.
+// Instances of this class may hold onto any JNIEnv passed into it until
+// destroyed. Therefore, since a JNIEnv is only suitable for use on a single
+// thread, objects of this class must be created, used, and destroyed, on a
+// single thread.
+// Therefore, this class should only be used as a stack-based object and from a
+// single thread. If you wish to have the reference outlive the current
+// callstack (e.g. as a class member) or you wish to pass it across threads,
+// use a ScopedJavaGlobalRef instead.
 template<typename T>
 class ScopedJavaLocalRef : public JavaRef<T> {
  public:
-  ScopedJavaLocalRef() {}
+  ScopedJavaLocalRef() : env_(NULL) {}
 
   // Non-explicit copy constructor, to allow ScopedJavaLocalRef to be returned
   // by value as this is the normal usage pattern.
-  ScopedJavaLocalRef(const ScopedJavaLocalRef<T>& other) {
-    this->Reset(other);
+  ScopedJavaLocalRef(const ScopedJavaLocalRef<T>& other)
+      : env_(other.env_) {
+    this->SetNewLocalRef(env_, other.obj());
   }
 
   template<typename U>
-  explicit ScopedJavaLocalRef(const U& other) {
+  explicit ScopedJavaLocalRef(const U& other)
+      : env_(NULL) {
     this->Reset(other);
   }
 
   // Assumes that |obj| is a local reference to a Java object and takes
   // ownership  of this local reference.
-  ScopedJavaLocalRef(JNIEnv* env, T obj) : JavaRef<T>(env, obj) {}
+  ScopedJavaLocalRef(JNIEnv* env, T obj) : JavaRef<T>(env, obj), env_(env) {}
 
   ~ScopedJavaLocalRef() {
     this->Reset();
@@ -108,18 +111,28 @@ class ScopedJavaLocalRef : public JavaRef<T> {
   }
 
   void Reset() {
-    this->ResetLocalRef();
+    this->ResetLocalRef(env_);
+  }
+
+  template<typename U>
+  void Reset(const ScopedJavaLocalRef<U>& other) {
+    // We can copy over env_ here as |other| instance must be from the same
+    // thread as |this| local ref. (See class comment for multi-threading
+    // limitations, and alternatives).
+    this->Reset(other.env_, other.obj());
   }
 
   template<typename U>
   void Reset(const U& other) {
-    this->Reset(other.env(), other.obj());
+    // If |env_| was not yet set (is still NULL) it will be attached to the
+    // current thread in SetNewLocalRef().
+    this->Reset(env_, other.obj());
   }
 
   template<typename U>
   void Reset(JNIEnv* env, U obj) {
     implicit_cast<T>(obj);  // Ensure U is assignable to T
-    this->SetNewLocalRef(env, obj);
+    env_ = this->SetNewLocalRef(env, obj);
   }
 
   // Releases the local reference to the caller. The caller *must* delete the
@@ -127,12 +140,17 @@ class ScopedJavaLocalRef : public JavaRef<T> {
   T Release() {
     return static_cast<T>(this->ReleaseInternal());
   }
+
+ private:
+  // This class is only good for use on the thread it was created on so
+  // it's safe to cache the non-threadsafe JNIEnv* inside this object.
+  JNIEnv* env_;
 };
 
 // Holds a global reference to a Java object. The global reference is scoped
-// to the lifetime of this object. Note that since a JNI Env is only suitable
-// for use on a single thread, objects of this class must be created, used and
-// destroyed on the same thread.
+// to the lifetime of this object. This class does not hold onto any JNIEnv*
+// passed to it, hence it is safe to use across threads (within the constraints
+// imposed by the underlying Java object that it references).
 template<typename T>
 class ScopedJavaGlobalRef : public JavaRef<T> {
  public:
@@ -157,7 +175,7 @@ class ScopedJavaGlobalRef : public JavaRef<T> {
 
   template<typename U>
   void Reset(const U& other) {
-    this->Reset(other.env(), other.obj());
+    this->Reset(NULL, other.obj());
   }
 
   template<typename U>
diff --git a/content/browser/renderer_host/java/java_bound_object.cc b/content/browser/renderer_host/java/java_bound_object.cc
index ce13300..a9f7920 100644
--- a/content/browser/renderer_host/java/java_bound_object.cc
+++ b/content/browser/renderer_host/java/java_bound_object.cc
@@ -691,21 +691,18 @@ NPObject* JavaBoundObject::Create(const JavaRef<jobject>& object) {
 }
 
 JavaBoundObject::JavaBoundObject(const JavaRef<jobject>& object)
-    : java_object_(object.env()->NewGlobalRef(object.obj())) {
+    : java_object_(object) {
   // We don't do anything with our Java object when first created. We do it all
   // lazily when a method is first invoked.
 }
 
 JavaBoundObject::~JavaBoundObject() {
-  // Use the current thread's JNI env to release our global ref to the Java
-  // object.
-  AttachCurrentThread()->DeleteGlobalRef(java_object_);
 }
 
 jobject JavaBoundObject::GetJavaObject(NPObject* object) {
   DCHECK_EQ(&JavaNPObject::kNPClass, object->_class);
   JavaBoundObject* jbo = reinterpret_cast<JavaNPObject*>(object)->bound_object;
-  return jbo->java_object_;
+  return jbo->java_object_.obj();
 }
 
 bool JavaBoundObject::HasMethod(const std::string& name) const {
@@ -746,8 +743,8 @@ bool JavaBoundObject::Invoke(const std::string& name, const NPVariant* args,
   }
 
   // Call
-  *result = CallJNIMethod(java_object_, method->return_type(), method->id(),
-                          &parameters[0]);
+  *result = CallJNIMethod(java_object_.obj(), method->return_type(),
+                          method->id(), &parameters[0]);
   return true;
 }
 
@@ -758,7 +755,7 @@ void JavaBoundObject::EnsureMethodsAreSetUp() const {
 
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jclass> clazz(env, static_cast<jclass>(
-      env->CallObjectMethod(java_object_,  GetMethodIDFromClassName(
+      env->CallObjectMethod(java_object_.obj(),  GetMethodIDFromClassName(
           env,
           kJavaLangObject,
           kGetClass,
diff --git a/content/browser/renderer_host/java/java_bound_object.h b/content/browser/renderer_host/java/java_bound_object.h
index 61a1dfb..cf0c403 100644
--- a/content/browser/renderer_host/java/java_bound_object.h
+++ b/content/browser/renderer_host/java/java_bound_object.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -44,10 +44,9 @@ class JavaBoundObject {
 
   void EnsureMethodsAreSetUp() const;
 
-  // Global ref to the underlying Java object. We use a naked jobject, rather
-  // than a ScopedJavaGlobalRef, as the global ref will be added and dropped on
-  // different threads.
-  jobject java_object_;
+  // The global ref to the underlying Java object that this JavaBoundObject
+  // instance represents.
+  base::android::ScopedJavaGlobalRef<jobject> java_object_;
 
   // Map of public methods, from method name to Method instance. Multiple
   // entries will be present for overloaded methods. Note that we can't use
diff --git a/content/browser/renderer_host/java/java_method.cc b/content/browser/renderer_host/java/java_method.cc
index 01235e8..6f6878c 100644
--- a/content/browser/renderer_host/java/java_method.cc
+++ b/content/browser/renderer_host/java/java_method.cc
@@ -95,7 +95,7 @@ JavaMethod::JavaMethod(const base::android::JavaRef<jobject>& method)
     : java_method_(method),
       have_calculated_num_parameters_(false),
       id_(NULL) {
-  JNIEnv* env = java_method_.env();
+  JNIEnv* env = AttachCurrentThread();
   // On construction, we do nothing except get the name. Everything else is
   // done lazily.
   ScopedJavaLocalRef<jstring> name(env, static_cast<jstring>(
@@ -139,7 +139,7 @@ void JavaMethod::EnsureNumParametersIsSetUp() const {
   // The number of parameters will be used frequently when determining
   // whether to call this method. We don't get the ID etc until actually
   // required.
-  JNIEnv* env = java_method_.env();
+  JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jarray> parameters(env, static_cast<jarray>(
       env->CallObjectMethod(java_method_.obj(), GetMethodIDFromClassName(
           env,
@@ -155,7 +155,7 @@ void JavaMethod::EnsureTypesAndIDAreSetUp() const {
   }
 
   // Get the parameters
-  JNIEnv* env = java_method_.env();
+  JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobjectArray> parameters(env, static_cast<jobjectArray>(
       env->CallObjectMethod(java_method_.obj(), GetMethodIDFromClassName(
           env,