Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Tue, 2 Feb 2021 22:33:09 GMT, Phil Race wrote: >> 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)? > >> 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. > 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 - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Tue, 2 Feb 2021 21:48:36 GMT, Sergey Bylokhov wrote: >> Look a few lines further up at my reply 3 days ago Gerard about this. > > 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)? > 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. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Tue, 2 Feb 2021 22:02:14 GMT, Sergey Bylokhov wrote: >> 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. > > 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. SWT does not use JNF at all. I've looked at their source code. I can clean up those comments. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Mon, 1 Feb 2021 23:47:06 GMT, Phil Race wrote:
>> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 611:
>>
>>> 609: const jchar *unichars = (*env)->GetStringChars(env, str, NULL);
>>> 610: if (unichars == NULL) {
>>> 611: return;
>>
>> Do not we need to throw an exception here? Otherwise, GetStringChars error
>> will be ignored?
>
> Look a few lines further up at my reply 3 days ago Gerard about this.
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)?
>> src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
>> line 967:
>>
>>> 965: static NSNumber* JavaNumberToNSNumber(JNIEnv *env, jobject jnumber) {
>>> 966: if (jnumber == NULL) {
>>> 967: return nil;
>>
>> Based on its usage it is probably should be zero on NULL number?
>
> 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.
@azuev-java Looks like a cleanup opportunity?
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Tue, 2 Feb 2021 21:18:42 GMT, Kevin Rushforth wrote:
>> 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.
>
> 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.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Tue, 2 Feb 2021 00:30:07 GMT, Phil Race wrote:
>> src/java.desktop/macosx/native/libosxapp/ThreadUtilities.m line 53:
>>
>>> 51: @implementation ThreadUtilities
>>> 52:
>>> 53: + (void)initialize {
>>
>> I think we need to check how this new modes will work when the AWT is
>> embedded inside SWT and FX.
>
> 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.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Mon, 1 Feb 2021 22:17:38 GMT, Sergey Bylokhov wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8260616: Removing remaining JNF dependencies in the java.desktop module
>
> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 611:
>
>> 609: const jchar *unichars = (*env)->GetStringChars(env, str, NULL);
>> 610: if (unichars == NULL) {
>> 611: return;
>
> Do not we need to throw an exception here? Otherwise, GetStringChars error
> will be ignored?
Look a few lines further up at my reply 3 days ago Gerard about this.
> src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
> line 967:
>
>> 965: static NSNumber* JavaNumberToNSNumber(JNIEnv *env, jobject jnumber) {
>> 966: if (jnumber == NULL) {
>> 967: return nil;
>
> Based on its usage it is probably should be zero on NULL number?
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.
> src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 30:
>
>> 28: NSString* JavaStringToNSString(JNIEnv *env, jstring jstr) {
>> 29: if (jstr == NULL) {
>> 30: return NULL;
>
> In other methods, the nil is used
Not understanding what you mean ? This is the same behavior as the function
being replaced.
> src/java.desktop/macosx/native/libosxapp/ThreadUtilities.m line 53:
>
>> 51: @implementation ThreadUtilities
>> 52:
>> 53: + (void)initialize {
>
> I think we need to check how this new modes will work when the AWT is
> embedded inside SWT and FX.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Mon, 1 Feb 2021 19:09:59 GMT, Phil Race wrote:
>> This completes the desktop module JNF removal
>>
>> * remove -framework JavaNativeFoundation from make files
>>
>> * remove #import 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)
>
> Phil Race has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8260616: Removing remaining JNF dependencies in the java.desktop module
src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 611:
> 609: const jchar *unichars = (*env)->GetStringChars(env, str, NULL);
> 610: if (unichars == NULL) {
> 611: return;
Do not we need to throw an exception here? Otherwise, GetStringChars error will
be ignored?
src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
line 967:
> 965: static NSNumber* JavaNumberToNSNumber(JNIEnv *env, jobject jnumber) {
> 966: if (jnumber == NULL) {
> 967: return nil;
Based on its usage it is probably should be zero on NULL number?
src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 30:
> 28: NSString* JavaStringToNSString(JNIEnv *env, jstring jstr) {
> 29: if (jstr == NULL) {
> 30: return NULL;
In other methods, the nil is used
src/java.desktop/macosx/native/libosxapp/ThreadUtilities.m line 53:
> 51: @implementation ThreadUtilities
> 52:
> 53: + (void)initialize {
I think we need to check how this new modes will work when the AWT is embedded
inside SWT and FX.
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
> This completes the desktop module JNF removal > > * remove -framework JavaNativeFoundation from make files > > * remove #import 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) Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8260616: Removing remaining JNF dependencies in the java.desktop module - Changes: - all: https://git.openjdk.java.net/jdk/pull/2305/files - new: https://git.openjdk.java.net/jdk/pull/2305/files/efab1de5..7ea57c85 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2305&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2305&range=01-02 Stats: 46 lines in 3 files changed: 34 ins; 1 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/2305.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2305/head:pull/2305 PR: https://git.openjdk.java.net/jdk/pull/2305
