<AWT Dev> RFR: 8170493: JNI exception pending in JavaComponentAccessibility.m:142

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> RFR: 8170493: JNI exception pending in JavaComponentAccessibility.m:142

Phil Race
silences a hypothetical problem of a NULL return in an out-of-memory
situation.
This was detected by static analysis, not an actualy observed problem.

bug : https://bugs.openjdk.java.net/browse/JDK-8170493
diff :
a/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
+++
b/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
@@ -45,6 +45,7 @@
  #import "JavaTextAccessibility.h"
  #import "ThreadUtilities.h"
  #import "AWTView.h"
+#import "jni_util.h"


  // these constants are duplicated in CAccessibility.java
@@ -136,7 +137,7 @@
          fJavaRole = [javaRole retain];

          fAccessible = (*env)->NewWeakGlobalRef(env, accessible);
-
+        if (accessible != NULL) CHECK_NULL_RETURN(fAccessible, self);
          jobject jcomponent = [(AWTView *)fView awtComponent:env];
          fComponent = (*env)->NewWeakGlobalRef(env, jcomponent);
          (*env)->DeleteLocalRef(env, jcomponent);

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8170493: JNI exception pending in JavaComponentAccessibility.m:142

prasanta sadhukhan
looks good to me.

Regards
Prasanta
On 1/14/2017 5:46 AM, Philip Race wrote:

> silences a hypothetical problem of a NULL return in an out-of-memory
> situation.
> This was detected by static analysis, not an actualy observed problem.
>
> bug : https://bugs.openjdk.java.net/browse/JDK-8170493
> diff :
> a/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
>
> +++
> b/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
> @@ -45,6 +45,7 @@
>  #import "JavaTextAccessibility.h"
>  #import "ThreadUtilities.h"
>  #import "AWTView.h"
> +#import "jni_util.h"
>
>
>  // these constants are duplicated in CAccessibility.java
> @@ -136,7 +137,7 @@
>          fJavaRole = [javaRole retain];
>
>          fAccessible = (*env)->NewWeakGlobalRef(env, accessible);
> -
> +        if (accessible != NULL) CHECK_NULL_RETURN(fAccessible, self);
>          jobject jcomponent = [(AWTView *)fView awtComponent:env];
>          fComponent = (*env)->NewWeakGlobalRef(env, jcomponent);
>          (*env)->DeleteLocalRef(env, jcomponent);
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8170493: JNI exception pending in JavaComponentAccessibility.m:142

Sergey Bylokhov
In reply to this post by Phil Race
Hi, Phi.
> // these constants are duplicated in CAccessibility.java
> @@ -136,7 +137,7 @@
>         fJavaRole = [javaRole retain];
>

I am not sure but it seems strange that we will return the "self"(and will use it later), because it is not nil and was not properly initialized (for example fActionsLOCK field which is used for synchronization will be nil).
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8170493: JNI exception pending in JavaComponentAccessibility.m:142

Phil Race
LIke I said in the bug report, if we get OOME here then we'll get more
from the following calls and if we want to make this accessibility code
robust
against this theoretical problem a massive destabilising re-write is needed.

How about this change instead
---
a/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
+++
b/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
@@ -136,7 +136,7 @@
          fJavaRole = [javaRole retain];

          fAccessible = (*env)->NewWeakGlobalRef(env, accessible);
-
+        (*env)->ExceptionClear(env); // in case of OOME
          jobject jcomponent = [(AWTView *)fView awtComponent:env];
          fComponent = (*env)->NewWeakGlobalRef(env, jcomponent);
          (*env)->DeleteLocalRef(env, jcomponent);

-----

ExceptionClear is harmless if there is no exception pending.
And we will be "using jni correctly" in making the next call to
NewWeakGlobalRef
.. .even if it fails.

fAccessible is mostly guarded against use of NULL so it might even make
things better
but definitely won't harm anything.

If we ever see a real issue here that de-stabilising re-write will be
needed.
But not in JDK 9.

Its either make the mostly useless small change or "will not fix" but
we are supposed to be fixing these JNI correctness bugs.

-phil.

On 1/16/17, 7:17 AM, Sergey Bylokhov wrote:
> Hi, Phi.
>> // these constants are duplicated in CAccessibility.java
>> @@ -136,7 +137,7 @@
>>          fJavaRole = [javaRole retain];
>>
> I am not sure but it seems strange that we will return the "self"(and will use it later), because it is not nil and was not properly initialized (for example fActionsLOCK field which is used for synchronization will be nil).
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8170493: JNI exception pending in JavaComponentAccessibility.m:142

Phil Race
Sergey,

Any [additonial] comment on this ?

-phil.

On 01/26/2017 01:55 PM, Philip Race wrote:

> LIke I said in the bug report, if we get OOME here then we'll get more
> from the following calls and if we want to make this accessibility
> code robust
> against this theoretical problem a massive destabilising re-write is
> needed.
>
> How about this change instead
> ---
> a/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
> +++
> b/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
> @@ -136,7 +136,7 @@
>          fJavaRole = [javaRole retain];
>
>          fAccessible = (*env)->NewWeakGlobalRef(env, accessible);
> -
> +        (*env)->ExceptionClear(env); // in case of OOME
>          jobject jcomponent = [(AWTView *)fView awtComponent:env];
>          fComponent = (*env)->NewWeakGlobalRef(env, jcomponent);
>          (*env)->DeleteLocalRef(env, jcomponent);
>
> -----
>
> ExceptionClear is harmless if there is no exception pending.
> And we will be "using jni correctly" in making the next call to
> NewWeakGlobalRef
> .. .even if it fails.
>
> fAccessible is mostly guarded against use of NULL so it might even
> make things better
> but definitely won't harm anything.
>
> If we ever see a real issue here that de-stabilising re-write will be
> needed.
> But not in JDK 9.
>
> Its either make the mostly useless small change or "will not fix" but
> we are supposed to be fixing these JNI correctness bugs.
>
> -phil.
>
> On 1/16/17, 7:17 AM, Sergey Bylokhov wrote:
>> Hi, Phi.
>>> // these constants are duplicated in CAccessibility.java
>>> @@ -136,7 +137,7 @@
>>>          fJavaRole = [javaRole retain];
>>>
>> I am not sure but it seems strange that we will return the "self"(and
>> will use it later), because it is not nil and was not properly
>> initialized (for example fActionsLOCK field which is used for
>> synchronization will be nil).

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8170493: JNI exception pending in JavaComponentAccessibility.m:142

Sergey Bylokhov
Hi, Phil.
Nothing from my side.

>
> Any [additonial] comment on this ?
>
> -phil.
>
> On 01/26/2017 01:55 PM, Philip Race wrote:
>> LIke I said in the bug report, if we get OOME here then we'll get more
>> from the following calls and if we want to make this accessibility code robust
>> against this theoretical problem a massive destabilising re-write is needed.
>>
>> How about this change instead
>> --- a/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
>> +++ b/src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
>> @@ -136,7 +136,7 @@
>>         fJavaRole = [javaRole retain];
>>
>>         fAccessible = (*env)->NewWeakGlobalRef(env, accessible);
>> -
>> +        (*env)->ExceptionClear(env); // in case of OOME
>>         jobject jcomponent = [(AWTView *)fView awtComponent:env];
>>         fComponent = (*env)->NewWeakGlobalRef(env, jcomponent);
>>         (*env)->DeleteLocalRef(env, jcomponent);
>>
>> -----
>>
>> ExceptionClear is harmless if there is no exception pending.
>> And we will be "using jni correctly" in making the next call to NewWeakGlobalRef
>> .. .even if it fails.
>>
>> fAccessible is mostly guarded against use of NULL so it might even make things better
>> but definitely won't harm anything.
>>
>> If we ever see a real issue here that de-stabilising re-write will be needed.
>> But not in JDK 9.
>>
>> Its either make the mostly useless small change or "will not fix" but
>> we are supposed to be fixing these JNI correctness bugs.
>>
>> -phil.
>>
>> On 1/16/17, 7:17 AM, Sergey Bylokhov wrote:
>>> Hi, Phi.
>>>> // these constants are duplicated in CAccessibility.java
>>>> @@ -136,7 +137,7 @@
>>>>         fJavaRole = [javaRole retain];
>>>>
>>> I am not sure but it seems strange that we will return the "self"(and will use it later), because it is not nil and was not properly initialized (for example fActionsLOCK field which is used for synchronization will be nil).
>