Remove ScopedGlobalRef (and other cleanups)

ScopedGlobalRef caused more trouble that it was worth. Rather than
trying to fix it to require updating of the JNIEnv, remove it to
remove the temptation for others to use it.

Also update SSL_set_ciphers_lists to use ScopedLocalRef and add HTML
anchors to Standard names javadoc JSEE references.

Change-Id: Ic3ed1bae3f29ee971d4461de31395b78c4949090

3 files changed, 67 insertions, 96 deletions

diff --git a/include/ScopedGlobalRef.h b/include/ScopedGlobalRef.h
deleted file mode 100644
index 1fbeabb..0000000
--- a/include/ScopedGlobalRef.h
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * Copyright (C) 2010 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef SCOPED_GLOBAL_REF_H_included
-#define SCOPED_GLOBAL_REF_H_included
-
-#include "JNIHelp.h"
-
-// A smart pointer that creates a JNI global reference from a weak reference.
-// The global reference is deleted when the smart pointer goes out of scope.
-class ScopedGlobalRef {
-public:
-    ScopedGlobalRef(JNIEnv* env, jobject localRef)
-    : mEnv(env), mGlobalRef(NULL)
-    {
-        mGlobalRef = env->NewGlobalRef(localRef);
-    }
-
-    ~ScopedGlobalRef() {
-        reset();
-    }
-
-    void reset() {
-        if (mGlobalRef != NULL) {
-            mEnv->DeleteGlobalRef(mGlobalRef);
-            mGlobalRef = NULL;
-        }
-    }
-
-    jobject get() const {
-        return mGlobalRef;
-    }
-
-private:
-    JNIEnv* mEnv;
-    jobject mGlobalRef;
-
-    // Disallow copy and assignment.
-    ScopedGlobalRef(const ScopedGlobalRef&);
-    void operator=(const ScopedGlobalRef&);
-};
-
-#endif  // SCOPED_GLOBAL_REF_H_included
diff --git a/luni/src/main/native/org_apache_harmony_xnet_provider_jsse_NativeCrypto.cpp b/luni/src/main/native/org_apache_harmony_xnet_provider_jsse_NativeCrypto.cpp
index 0e9d641..87cedca 100644
--- a/luni/src/main/native/org_apache_harmony_xnet_provider_jsse_NativeCrypto.cpp
+++ b/luni/src/main/native/org_apache_harmony_xnet_provider_jsse_NativeCrypto.cpp
@@ -37,7 +37,7 @@
 #include <openssl/ssl.h>
 
 #include "ScopedByteArray.h"
-#include "ScopedGlobalRef.h"
+#include "ScopedLocalRef.h"
 #include "ScopedUtfChars.h"
 #include "UniquePtr.h"
 
@@ -986,50 +986,61 @@ class AppData {
     int fdsEmergency[2];
     MUTEX_TYPE mutex;
     JNIEnv* env;
-    ScopedGlobalRef certificateChainVerifier;
-    ScopedGlobalRef handshakeCompletedCallback;
+    jobject certificateChainVerifier;
+    jobject handshakeCompletedCallback;
 
     /**
      * Creates our application data and attaches it to a given SSL connection.
      *
-     * @param ssl The SSL connection to attach the data to.
      * @param env The JNIEnv
      * @param ccv The CertificateChainVerifier
      * @param hcc The HandshakeCompletedCallback
      */
   public:
-    static AppData* create(JNIEnv* e,
-                           jobject certificateChainVerifier,
-                           jobject handshakeCompletedCallback) {
-        if (certificateChainVerifier == NULL) {
+    static AppData* create(JNIEnv* env,
+                           jobject ccv,
+                           jobject hcc) {
+        if (ccv == NULL || hcc == NULL) {
             return NULL;
         }
-        if (handshakeCompletedCallback == NULL) {
+        AppData* appData = new AppData(env);
+        if (pipe(appData->fdsEmergency) == -1) {
+            destroy(env, appData);
             return NULL;
         }
-        UniquePtr<AppData> appData(new AppData(e, certificateChainVerifier, handshakeCompletedCallback));
-        if (appData->certificateChainVerifier.get() == NULL) {
+        if (MUTEX_SETUP(appData->mutex) == -1) {
+            destroy(env, appData);
             return NULL;
         }
-        if (appData->handshakeCompletedCallback.get() == NULL) {
+        appData->certificateChainVerifier = env->NewGlobalRef(ccv);
+        if (appData->certificateChainVerifier == NULL) {
+            destroy(env, appData);
             return NULL;
         }
-        if (pipe(appData->fdsEmergency) == -1) {
+        appData->handshakeCompletedCallback = env->NewGlobalRef(hcc);
+        if (appData->handshakeCompletedCallback == NULL) {
+            destroy(env, appData);
             return NULL;
         }
-        if (MUTEX_SETUP(appData->mutex) == -1) {
-            return NULL;
+        return appData;
+    }
+
+    static void destroy(JNIEnv* env, AppData* appData) {
+        if (appData == NULL) {
+            return;
         }
-        return appData.release();
+        appData->cleanupGlobalRefs(env);
+        delete appData;
     }
 
   private:
-    AppData(JNIEnv* e, jobject ccv, jobject hcc) :
+    AppData(JNIEnv* env) :
             aliveAndKicking(1),
             waitingThreads(0),
-            env(e),
-            certificateChainVerifier(e, ccv),
-            handshakeCompletedCallback(e, hcc) {
+            env(NULL),
+            certificateChainVerifier(NULL),
+            handshakeCompletedCallback(NULL) {
+        setEnv(env);
         fdsEmergency[0] = -1;
         fdsEmergency[1] = -1;
     }
@@ -1037,7 +1048,6 @@ class AppData {
     /**
      * Destroys our application data, cleaning up everything in the process.
      */
-  public:
     ~AppData() {
         aliveAndKicking = 0;
         if (fdsEmergency[0] != -1) {
@@ -1049,20 +1059,29 @@ class AppData {
         MUTEX_CLEANUP(mutex);
     }
 
-    void setEnv(JNIEnv* e) {
-        if (handshakeCompletedCallback.get() == NULL) {
-            return;
+    void cleanupGlobalRefs(JNIEnv* env) {
+        if (certificateChainVerifier != NULL) {
+            env->DeleteGlobalRef(certificateChainVerifier);
+            certificateChainVerifier = NULL;
         }
+        if (handshakeCompletedCallback != NULL) {
+            env->DeleteGlobalRef(handshakeCompletedCallback);
+            handshakeCompletedCallback = NULL;
+        }
+        clearEnv();
+    }
+
+  public:
+    void setEnv(JNIEnv* e) {
         env = e;
     }
+
     void clearEnv() {
         env = NULL;
     }
 
-    void handshakeCompleted() {
-        certificateChainVerifier.reset();
-        handshakeCompletedCallback.reset();
-        clearEnv();
+    void handshakeCompleted(JNIEnv* e) {
+        cleanupGlobalRefs(e);
     }
 };
 
@@ -1229,7 +1248,7 @@ static int cert_verify_callback(X509_STORE_CTX* x509_store_ctx, void* arg __attr
         JNI_TRACE("ssl=%p cert_verify_callback => 0", ssl);
         return 0;
     }
-    jobject certificateChainVerifier = appData->certificateChainVerifier.get();
+    jobject certificateChainVerifier = appData->certificateChainVerifier;
 
     jclass cls = env->GetObjectClass(certificateChainVerifier);
     jmethodID methodID = env->GetMethodID(cls, "verifyCertificateChain", "([[BLjava/lang/String;)V");
@@ -1281,7 +1300,7 @@ static void info_callback(const SSL *ssl, int where, int ret __attribute__ ((unu
         JNI_TRACE("ssl=%p info_callback env error", ssl);
         return;
     }
-    jobject handshakeCompletedCallback = appData->handshakeCompletedCallback.get();
+    jobject handshakeCompletedCallback = appData->handshakeCompletedCallback;
 
     jclass cls = env->GetObjectClass(handshakeCompletedCallback);
     jmethodID methodID = env->GetMethodID(cls, "handshakeCompleted", "()V");
@@ -1293,7 +1312,7 @@ static void info_callback(const SSL *ssl, int where, int ret __attribute__ ((unu
       JNI_TRACE("ssl=%p info_callback exception", ssl);
     }
 
-    appData->handshakeCompleted();
+    appData->handshakeCompleted(env);
     JNI_TRACE("ssl=%p info_callback completed", ssl);
 }
 
@@ -1603,8 +1622,8 @@ static void NativeCrypto_SSL_set_cipher_lists(JNIEnv* env, jclass,
     int length = env->GetArrayLength(cipherSuites);
     JNI_TRACE("ssl=%p NativeCrypto_SSL_set_cipher_lists length=%d", ssl, length);
     for (int i = 0; i < length; i++) {
-        jstring cipherSuite = (jstring) env->GetObjectArrayElement(cipherSuites, i);
-        ScopedUtfChars c(env, cipherSuite);
+        ScopedLocalRef cipherSuite(env, env->GetObjectArrayElement(cipherSuites, i));
+        ScopedUtfChars c(env, reinterpret_cast<jstring>(cipherSuite.get()));
         JNI_TRACE("ssl=%p NativeCrypto_SSL_set_cipher_lists cipherSuite=%s", ssl, c.c_str());
         bool found = false;
         for (int j = 0; j < num_ciphers; j++) {
@@ -1776,6 +1795,7 @@ static jint NativeCrypto_SSL_do_handshake(JNIEnv* env, jclass,
         return 0;
     }
     SSL_set_app_data(ssl, (char*) appData);
+    JNI_TRACE("ssl=%p AppData::create => %p", ssl, appData);
 
     if (client_mode) {
         SSL_set_connect_state(ssl);
@@ -2368,11 +2388,12 @@ static void NativeCrypto_SSL_free(JNIEnv* env, jclass, jint ssl_address)
     SSL* ssl = to_SSL(env, ssl_address, true);
     JNI_TRACE("ssl=%p NativeCrypto_SSL_free", ssl);
     if (ssl == NULL) {
-      return;
+        return;
     }
     AppData* appData = (AppData*) SSL_get_app_data(ssl);
     SSL_set_app_data(ssl, NULL);
-    delete appData;
+    JNI_TRACE("ssl=%p AppData::destroy(%p)", ssl, appData);
+    AppData::destroy(env, appData);
     SSL_free(ssl);
 }
 
diff --git a/support/src/test/java/javax/net/ssl/StandardNames.java b/support/src/test/java/javax/net/ssl/StandardNames.java
index 86dbd78..bc0e4e3 100644
--- a/support/src/test/java/javax/net/ssl/StandardNames.java
+++ b/support/src/test/java/javax/net/ssl/StandardNames.java
@@ -23,10 +23,16 @@ import java.util.Set;
 
 /**
  * This class defines expected string names for protocols, key types, client and server auth types, cipher suites.
- * 
- * Based on documentation from http://java.sun.com/j2se/1.5.0/docs/guide/security/jsse/JSSERefGuide.html#AppA
  *
- * Java 6 version http://java.sun.com/javase/6/docs/technotes/guides/security/SunProviders.html
+ * Initially based on "Appendix A: Standard Names" of
+ * <a href="http://java.sun.com/j2se/1.5.0/docs/guide/security/jsse/JSSERefGuide.html#AppA">
+ * Java Secure Socket Extension (JSSE) Reference Guide for the JavaTM 2 Platform Standard Edition 5
+ * </a>
+ *
+ * Updated based on the "The SunJSSE Provider" section of
+ * <a href="java.sun.com/javase/6/docs/technotes/guides/security/SunProviders.html#SunJSSEProvider">
+ * Java Cryptography Architecture Sun Providers Documentation for JavaTM Platform Standard Edition 6
+ * </a>
  */
 public final class StandardNames {
 
@@ -161,12 +167,12 @@ public final class StandardNames {
         // Old non standard exportable encryption
         CIPHER_SUITES_NEITHER.add("SSL_RSA_EXPORT1024_WITH_DES_CBC_SHA");
         CIPHER_SUITES_NEITHER.add("SSL_RSA_EXPORT1024_WITH_RC4_56_SHA");
-        
+
         // No RC2
         CIPHER_SUITES_NEITHER.add("SSL_RSA_EXPORT_WITH_RC2_CBC_40_MD5");
         CIPHER_SUITES_NEITHER.add("TLS_KRB5_EXPORT_WITH_RC2_CBC_40_SHA");
         CIPHER_SUITES_NEITHER.add("TLS_KRB5_EXPORT_WITH_RC2_CBC_40_MD5");
-        
+
         CIPHER_SUITES = (TestSSLContext.IS_RI) ? CIPHER_SUITES_RI : CIPHER_SUITES_OPENSSL;
     }
 }