JNI crashes in FontManager code

[Joseph D. Darcy]

Hello.

2D-team, I'm not an expert in this part of the platform, but the 
reasoning Martin has sent seems sound; please review the fix.

Thanks,

-Joe

Martin Buchholz wrote:
> Hi font maintainers,
>
> This is a bug report, with fix,
> provided by Hiroshi Yamauchi.
> This is a crash detected by a google test
> (which is unfortunately impractical to share)
> which makes this at least a P2 bug.
>
> Description:
>
> 	Fixing the bad JNI code in the font manager code. Two issues:
> 	
> 	  o The JNIEnv is unique to the thread. It cannot be saved by one thread and
> 	    reused by another. Use GetEnv instead.
> 	
> 	  o The 'font2D' jobject needs to be converted into a global reference because
> 	    its lifetime exceeds the lifetime of a native method call.
> 	
> Evaluation:
>
> Appropriately register/free everything with the garbage collector.
>
> Fix:
>
> # HG changeset patch
> # User martin
> # Date 1224202830 25200
> # Node ID 3c9d6001d8a90698a3540a2a483717f26a98db78
> # Parent  68730f05449cd4f39ce1cb82adc6c4e57f87554f
> Crash in freetypeScaler.c due to insufficient GC protection
> Summary: NewGlobalRef/DeleteGlobalRef as needed.
> Reviewed-by:
> Contributed-by: yamauchi at google.com
>
> diff --git a/make/sun/font/mapfile-vers.openjdk
> b/make/sun/font/mapfile-vers.openjdk
> --- a/make/sun/font/mapfile-vers.openjdk
> +++ b/make/sun/font/mapfile-vers.openjdk
> @@ -29,6 +29,7 @@
>
>  SUNWprivate_1.1 {
>  	global:
> +                JNI_OnLoad;
>                  getSunFontIDs;
>                  newLayoutTableCache;
>                  freeLayoutTableCache;
> diff --git a/src/share/native/sun/font/freetypeScaler.c
> b/src/share/native/sun/font/freetypeScaler.c
> --- a/src/share/native/sun/font/freetypeScaler.c
> +++ b/src/share/native/sun/font/freetypeScaler.c
> @@ -48,16 +48,6 @@
>  #define  ROUND(x) ((int) (x+0.5))
>
>  typedef struct {
> -    /* Important note:
> -         JNI forbids sharing same env between different threads.
> -         We are safe, because pointer is overwritten every time we get into
> -         JNI call (see setupFTContext).
> -
> -         Pointer is used by font data reading callbacks
> -         such as ReadTTFontFileFunc.
> -
> -         NB: We may consider switching to JNI_GetEnv. */
> -    JNIEnv* env;
>      FT_Library library;
>      FT_Face face;
>      jobject font2D;
> @@ -90,6 +80,13 @@
>  void z_error(char *s) {}
>  #endif
>
> +static JavaVM* jvm = NULL;
> +
> +JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *reserved) {
> +    jvm = vm;
> +    return JNI_VERSION_1_2;
> +}
> +
>  /**************** Error handling utilities *****************/
>
>  static jmethodID invalidateScalerMID;
> @@ -107,6 +104,10 @@
>
>      FT_Done_Face(scalerInfo->face);
>      FT_Done_FreeType(scalerInfo->library);
> +
> +    if (scalerInfo->font2D != NULL) {
> +        (*env)->DeleteGlobalRef(env, scalerInfo->font2D);
> +    }
>
>      if (scalerInfo->directBuffer != NULL) {
>          (*env)->DeleteGlobalRef(env, scalerInfo->directBuffer);
> @@ -131,10 +132,9 @@
>
>  #define FILEDATACACHESIZE 1024
>
> -/* NB: is it ever called? */
>  static void CloseTTFontFileFunc(FT_Stream stream) {
> +    JNIEnv* env = (JNIEnv*) JNU_GetEnv(jvm, JNI_VERSION_1_2);
>      FTScalerInfo *scalerInfo = (FTScalerInfo *) stream->pathname.pointer;
> -    JNIEnv* env = scalerInfo->env;
>      jclass tmpClass = (*env)->FindClass(env, "sun/font/TrueTypeFont");
>      jfieldID platNameField =
>           (*env)->GetFieldID(env, tmpClass, "platName", "Ljava/lang/String;");
> @@ -150,8 +150,8 @@
>                                          unsigned char* destBuffer,
>                                          unsigned long numBytes)
>  {
> +    JNIEnv* env = (JNIEnv*) JNU_GetEnv(jvm, JNI_VERSION_1_2);
>      FTScalerInfo *scalerInfo = (FTScalerInfo *) stream->pathname.pointer;
> -    JNIEnv* env = scalerInfo->env;
>      jobject bBuffer;
>      int bread = 0;
>
> @@ -245,8 +245,7 @@
>      if (scalerInfo == NULL)
>          return 0;
>
> -    scalerInfo->env = env;
> -    scalerInfo->font2D = font2D;
> +    scalerInfo->font2D = (*env)->NewGlobalRef(env, font2D);
>      scalerInfo->fontDataOffset = 0;
>      scalerInfo->fontDataLength = 0;
>      scalerInfo->fileSize = filesize;
> @@ -263,6 +262,7 @@
>      */
>      error = FT_Init_FreeType(&scalerInfo->library);
>      if (error) {
> +        (*env)->DeleteGlobalRef(env, scalerInfo->font2D);
>          free(scalerInfo);
>          return 0;
>      }
> @@ -331,6 +331,7 @@
>          }
>          if (scalerInfo->fontData != NULL)
>              free(scalerInfo->fontData);
> +        (*env)->DeleteGlobalRef(env, scalerInfo->font2D);
>          free(scalerInfo);
>          return 0;
>      }
> @@ -391,8 +392,10 @@
>                            FTScalerContext *context) {
>      int errCode = 0;
>
> -    scalerInfo->env = env;
> -    scalerInfo->font2D = font2D;
> +    if (scalerInfo->font2D != NULL) {
> +        (*env)->DeleteGlobalRef(env, scalerInfo->font2D);
> +    }
> +    scalerInfo->font2D = (*env)->NewGlobalRef(env, font2D);
>
>      FT_Set_Transform(scalerInfo->face, &context->transform, NULL);
>