8260616: Removing remaining JNF dependencies in the java.desktop module #2305
Conversation
|
|
openjdk
bot
added
2d
Webrevs
|
|
I mostly have questions about what is missing from this PR. :-) (If this is supposed to remove the final remnants of JNF)
|
|
Mailing list message from erik.joelsson at oracle.com on awt-dev: On 2021-01-29 02:56, Magnus Ihse Bursie wrote:
libosxsecurity is being worked on separately, see That said, we may need a separate bug for build to remove the any /Erik |
|
Right, this is the desktop module as per the first line of the comment. |
|
|
||
| // Draw the text context | ||
| DrawTextContext(env, qsdo, awtStrike, unichars, len, x, y); | ||
| } | ||
| else | ||
| { | ||
| // Get string to draw and the length | ||
| const jchar *unichars = JNFGetStringUTF16UniChars(env, str); |
There was a problem hiding this comment.
According to JNFString.h, the JNFGetStringUTF16UniChars() API:
/*
- Gets UTF16 unichars from a Java string, and checks for errors and raises a JNFException if
- the unichars cannot be obtained from Java.
*/
raises a (JNFException) if it can't get the chars, but now we simply return if GetStringChars() can not return the chars.
Seems like we handle this case differently in the new code now - is this change desired and correct?
There was a problem hiding this comment.
You are correct, but I decided that here it is fine.
If GetStringChars fails it will return NULL (it does not raise any excptions itself).
I added a check for NULL if NULL we then return from the JNI method to Java.
In the old case it also would return but would additionally propagate that raised exception to Java which JNF
decided to throw into the mix when there's already a standard way of testing failure.
And since we already know str != NULL we are in the realms of something went badly wrong inside the VM for
this to fail. So I decided this was all fine.
| @@ -597,20 +598,23 @@ render using the right font (see bug 7183516) when attribString is not | |||
| { | |||
| jchar unichars[len]; | |||
| (*env)->GetStringRegion(env, str, 0, len, unichars); | |||
| JNF_CHECK_AND_RETHROW_EXCEPTION(env); | |||
| CHECK_EXCEPTION(); | |||
There was a problem hiding this comment.
Are JNF_CHECK_AND_RETHROW_EXCEPTION and CHECK_EXCEPTION equivalent?
JNF_CHECK_AND_RETHROW_EXCEPTION seems to throw exception, but CHECK_EXCEPTION does not?
There was a problem hiding this comment.
JNF_CHECK_AND_RETHROW_EXCEPTION is this
/Library/Developer/CommandLineTools/SDKs/MacOSX11.1.sdk/System/Library/Frameworks/JavaNativeFoundation.framework/Versions/A/Headers/JNFJNI.h
// JNF_CHECK_AND_RETHROW_EXCEPTION - rethrows exceptions from Java
//
// Takes an exception thrown from Java, and transforms it into an
// NSException. The NSException should bubble up to the upper-most
// JNF_COCOA_ENTER/JNF_COCOA_EXIT pair, and then be re-thrown as
// a Java exception when returning from JNI. This check should be
// done after raw JNI operations which could cause a Java exception
// to be be thrown. The JNF{Get/Set/Call} macros below do this
// check automatically.
#define JNF_CHECK_AND_RETHROW_EXCEPTION(env)
{
jthrowable _exception = (*env)->ExceptionOccurred(env);
if (_exception) [JNFException raise:env throwable:_exception];
}
So what it actually does is clear the exception, raise an NSException and when
the code reaches JNF_COCOA_EXIT - which then has to be there - it clears the NSException
and re-throws the Java exception.
So the name of the macro is misleading since this does NOT itself rethrow the exception,
it relies on other code to do that.
CHECK_EXCEPTION will amount to the same - it throws an NSException if there is a Java exception pending.
And it will clear the NSException before exiting back to Java.
The difference is that it doesn't clear the Java Exception and later rethrow it since there is no need
There is one exception to this in CHECK_EXCEPTION - if this is the main (ie appkit) thread the java exception is cleared since there's no one to return it to ...
| * There is a Cocoa constant representing that difference : NSTimeIntervalSince1970 | ||
| */ | ||
| static jlong NSTimeIntervalToJavaMilliseconds(NSTimeInterval interval) { | ||
| NSTimeInterval interval1970 = interval + NSTimeIntervalSince1970; |
There was a problem hiding this comment.
Is it required for the APIs using the value from this function to expect the time calculated since Jan 1st 1970?
| NSString* NormalizedPathNSStringFromJavaString(JNIEnv *env, jstring pathStr) { | ||
| if (pathStr == NULL) { | ||
| return nil; | ||
| } | ||
| NSString *nsStr = JavaStringToNSString(env, pathStr); | ||
| if (nsStr == NULL) { | ||
| return nil; | ||
| } | ||
| const char* chs = [nsStr fileSystemRepresentation]; | ||
| int len = strlen(chs); | ||
| NSString* result = [[NSFileManager defaultManager] | ||
| stringWithFileSystemRepresentation:chs length:len]; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Why are we doing:
java_string -> chars -> NSString -> chars -> [NSFileManager stringWithFileSystemRepresentation]
?
Why not just get the raw characters form JNI and feed them directly into [NSFileManager stringWithFileSystemRepresentation], ie:
java_string -> chars -> [NSFileManager stringWithFileSystemRepresentation]
and skip the JavaStringToNSString step altogether?
There was a problem hiding this comment.
I tried to explain the odd-ness of this in the comments preceding the function definition.
Near as I could figure out that intermediate call is needed to do the decomposition since the NSFileManager won't do that.
But this is not exactly my area of expertise and there may be a better way. Who is an expert on the intricacies of the macOS file system naming ?
There was a problem hiding this comment.
How about a comment like:
/*
Returns an NSString in decomposed UTF16 format that is compatible with HFS's
expectation of the UTF16 format for file system paths.
Example string: "/Users/Amélie/"
Java's UTF16 string is "/ U s e r s / A m \351 l i e /"
macOS UTF16 string suitable for HFS is "/ U s e r s / A m e \314 \201 l i e /"
There is no direct API that takes in NSString UTF16 encoded by Java
and produces NSString UTF16 for HFS, so we first need to decompose it
into chars (suitable for low level C file APIs), and only then
create NSString representation of this decomposition back into UTF16 string.
*/
?
I borrowed the API description from Apple's original JNFNormalizedNSStringForPath API, but added an example and different description.
|
Lots of small changes you had to handle here. Thank you for doing it! |
|
I pushed a commit with the remaining -lframework ... removals in the desktop makefiles that I had somehow missed ... |
|
@prrace This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the
|
update on this. It Is used by precisely one jtreg AWT test. So I guess it is fair game to add resolving this to the desktop module update. I am currently testing it before committing - will take a couple of hours as the headful tests take a while to run. |
|
The changes look good to me, but please see my comment from my review about documenting |
| const jchar *unichars = JNFGetStringUTF16UniChars(env, str); | ||
| const jchar *unichars = (*env)->GetStringChars(env, str, NULL); | ||
| if (unichars == NULL) { | ||
| return; |
There was a problem hiding this comment.
Do not we need to throw an exception here? Otherwise, GetStringChars error will be ignored?
There was a problem hiding this comment.
Look a few lines further up at my reply 3 days ago Gerard about this.
There was a problem hiding this comment.
I read it and not sure that it is fine to ignore this error, why not throw an exception and signal the CTextPipe_doDrawString that an error occurred like InvalidPipeException or something(Sometimes we wrap other exception like OOM into the InvalidPipeException and this seems similar case)?
There was a problem hiding this comment.
The changes look good to me, but please see my comment from my review about documenting
NormalizedPathNSStringFromJavaString()API.
I see it. I will copy what you wrote in there.
There was a problem hiding this comment.
I read it and not sure that it is fine to ignore this error, why not throw an exception and signal the CTextPipe_doDrawString that an error occurred like InvalidPipeException or something(Sometimes we wrap other exception like OOM into the InvalidPipeException and this seems similar case)?
OK. I will do something like throw OOME or InternalError. Since we already checked for NULL as an arg any failure here implies a deep VM problem rather than anytjhing like a 2D invalid pipe
| */ | ||
| static NSNumber* JavaNumberToNSNumber(JNIEnv *env, jobject jnumber) { | ||
| if (jnumber == NULL) { | ||
| return nil; |
There was a problem hiding this comment.
Based on its usage it is probably should be zero on NULL number?
There was a problem hiding this comment.
Not an unreasonable idea and I considered it but :
- It is never called with NULL. There is always a null check
- The JNF equivalent returns nil on NULL
BTW two of the functions in which the code appears : accessibilityMinValueAttribute and accessibilityMaxValueAttribute (SFAIC) aren't used anywhere.
There was a problem hiding this comment.
@azuev-java Looks like a cleanup opportunity?
|
|
||
| NSString* JavaStringToNSString(JNIEnv *env, jstring jstr) { | ||
| if (jstr == NULL) { | ||
| return NULL; |
There was a problem hiding this comment.
In other methods, the nil is used
There was a problem hiding this comment.
Not understanding what you mean ? This is the same behavior as the function being replaced.
| @@ -49,6 +50,15 @@ static inline void attachCurrentThread(void** env) { | |||
|
|
|||
| @implementation ThreadUtilities | |||
|
|
|||
| + (void)initialize { | |||
There was a problem hiding this comment.
I think we need to check how this new modes will work when the AWT is embedded inside SWT and FX.
There was a problem hiding this comment.
We are just specifying an additional run mode for JDK internal use.
It means that when we are saying to process only events for that mode, then only those will be processed.
And it is used only for nested event loops.
Nothing eternal should be assuming AWT uses JNF at all, never mind in a particular way.
There is a special case for FX but I don't see a problem.
If you look at Java_sun_lwawt_macosx_LWCToolkit_doAWTRunLoopImpl
there's a variable "inAWT".
isRunning = [[NSRunLoop currentRunLoop] runMode:(inAWT ? [JNFRunLoop javaRunLoopMode] : NSDefaultRunLoopMode) ... ]];
This is specified as
inAWT = AccessController.doPrivileged(new PrivilegedAction() {
@OverRide
public Boolean run() {
return !Boolean.parseBoolean(System.getProperty("javafx.embed.singleThread", "false"));
}
});
}
So generally FX doesn't care, and is ignorant of this new mode.
Unless you set that property to true, in which case AWT use the NSDefaultRunLoopMode and so again FX is unaffected.
Nothing in the FX sources goes anywhere near JNF .. or has a reference to the special mode.
Bottom line I don't see it being affected.
I'll check with Kevin but also Gerard had a lot to do with the creation of the current FX toolkit.
There was a problem hiding this comment.
I ran some tests embedding JavaFX into Swing and vice versa both with and without -Djavafx.embed.singleThread=true and I don't see any regression in behavior.
There was a problem hiding this comment.
I am mostly worried about the usage of JNF by someone else's native code, as far as I understand it could be "broken" now. But it is good that FX does not use it.
BTW looks like all comments like "// AWT_THREADING Safe (AWTRunLoop)" could be removed now.
There was a problem hiding this comment.
SWT does not use JNF at all. I've looked at their source code.
I can clean up those comments.
|
Mailing list message from Magnus Ihse Bursie on build-dev: On 2021-01-29 17:41, Phil Race wrote:
Fair enough. Do you have a JBS issue tracking the remaining build system /Magnus |
|
Mailing list message from Philip Race on build-dev: Per my comment in the PR I am currently working on updating it to handle Once the other in-flight PRs are integrated, my grepping says that the needed after that is to remove one disabled warning in I am going to try to find out if we can remove that now or have to wait FWIW it looks like you added it here : If we can roll this into an existing PR that might be easier. If it -phil On 2/1/21 3:33 AM, Magnus Ihse Bursie wrote:
|
|
Mailing list message from Philip Race on build-dev: I can confirm that the autoconf warning disabling is currently still It'll have to be removed at the very end. In file included from ./open/src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m:27: /System/Library/Frameworks/JavaVM.framework/Frameworks/JavaNativeFoundation.framework/Headers/JNFJNI.h:30: -phil. On 2/1/21 8:02 AM, Philip Race wrote:
|
|
The following commit was integrated to jdk master since you created this branch: acbcde8 : JDK-8256111 : Create implementation for NSAccessibilityStaticText protocol with a new reference to JNF, so it fails to compile with this patch. You will need to merge openjdk/jdk master into your PR branch and then fix the new JNF reference. |
|
Updated as follows
|
openjdk
bot
added
ready
|
/integrate |
openjdk
bot
added
integrated
This completes the desktop module JNF removal
remove -framework JavaNativeFoundation from make files
remove #import <JavaNativeFoundation/JavaNativeFoundation.h> from all source files. If needed add import of JNIUtilities.h to get jni.h definitions - better anyway since then it gets the current JDK ones not the ones from the O/S
replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString with JavaStringToNSString
replace JNFNormalizedNSStringForPath with NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with NormalizedPathJavaStringFromNSString
replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI
Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast majority already did this)
Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject perform* methods.
define new javaRunLoopMode in ThreadUtilities to replace the JNF one and use where needed.
Remove the single usage of JNFPerformEnvBlock
replace JNFJavaToNSNumber in single A11Y file with local replacement
replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with local replacement
remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m
misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION)
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2305/head:pull/2305$ git checkout pull/2305