RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v2]

[Weijun Wang]

On Fri, 29 Jan 2021 20:51:04 GMT, Phil Race <prr at openjdk.org> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   same behavior as before -- empty realm map
>
> make/modules/java.security.jgss/Lib.gmk line 84:
> 
>> 82:             $(call SET_SHARED_LIBRARY_ORIGIN), \
>> 83:         LIBS := -framework Cocoa -framework SystemConfiguration \
>> 84:             -framework Kerberos -ljava, \
> 
> The need to add -ljava is interesting. It implies we were getting something from the platform that usually we'd expect to come from the JDK itself ??

I tried removing it and the build runs fine. But I remember I have seen some error before.

> src/java.base/macosx/classes/apple/security/KeychainStore.java line 820:
> 
>> 818:     private void createKeyEntry(String alias, long creationDate, long secKeyRef,
>> 819:                                 long[] secCertificateRefs, byte[][] rawCertData) {
>> 820:         KeyEntry ke = new KeyEntry();
> 
> removing these exceptions is presumably just clean up - not directly related ??

Yes.

> src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 28:
> 
>> 26: #import "apple_security_KeychainStore.h"
>> 27: #include "jni.h"
>> 28: #include "jni_util.h"
> 
> jni_util.h includes jni.h so I don't understand the need for this change.
> Also why did you change import to include ? import is the Obj-C norm ...

Will fix.

> src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 619:
> 
>> 617:             (*env)->ReleaseCharArrayElements(env, passwordObj, passwordChars,
>> 618:                 JNI_ABORT);
>> 619:         }
> 
> Although you have it in the later code, here you are missing
>  @catch (NSException *e) { 
>      NSLog(@"%@", [e callStackSymbols]); 
>  }

Will add.

BTW, will these be shown on stdout or stderr? If so, is this a good idea?

> src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 57:
> 
>> 55:         }
>> 56:     }
>> 57:     (*localVM)->DetachCurrentThread(localVM);
> 
> I think you only want to detach if you actually attached ! you don't want to be detaching VM threads.

So I should remember how env was retrieved and only detach when it's from `AttachCurrentThreadAsDaemon`? In fact, in my test the plain `GetEnv` has never succeeded and it was always attached.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1845