<AWT Dev> [15] Review request for 8242498: Invalid "sun.awt.TimedWindowEvent" object leads to JVM crash

[Anton Litvinov]

Hi Sergey,

I explained, why "GetTopLevelParentForWindow" works in the original 
version of my previous e-mail.

Topic of how correctly or incorrectly 
"AwtComponent::GetTopLevelParentForWindow" method works is not related 
to this bug. This bug is about "jobject jOpposite" not being an instance 
of "java.awt.Window" class in the C++ method 
"AwtWindow::SendWindowEvent". And the fix detects this situation either 
"jOpposite" is taken from original "opposite" or from HWND returned then 
by "GetTopLevelParentForWindow". If it is defined that for both cases 
"jOpposite" is not instance of "java.awt.Window" the fix uses "NULL" for 
creation of "TimedWindowEvent" object, and this also works as you 
already saw it offline in my pilot version of the fix.

I prefer to fix one problem at a time. There is no need to get "jobject 
jOpposite" from HWND returned by "GetTopLevelParentForWindow", when 
"jOpposite" got from the original "opposite" HWND is instance of 
"java.awt.Window". I am not going to explore all possible scenarios 
involving dialogs, modal dialogs, owned windows, owner windows, 
components in those windows and check if "GetTopLevelParentForWindow" 
will work in those scenarios, it is not related to this bug. Also I will 
repeat I will not change the existing behavior just for the reason of 
checking, if it cause any regression or does not cause it.

Why do you think that the fix does not work?

Thank you,
Anton

On 15/04/2020 20:59, Sergey Bylokhov wrote:
> On 4/15/20 12:26 pm, Anton Litvinov wrote:
>> When JDK is used in a normal way and not as it is used in the 
>> reproducer, "opposite" is always HWND of a peer of "java.awt.Window" 
>> instance, thus for such cases there is no need to search for some 
>> parent HWND of the "opposite" HWND. I do not want to break behavior 
>> of JDK for these normal scenarios, by getting some parent HWND of the 
>> valid "opposite" HWND. I do not want to rely on expectation that for 
>> valid "opposite" HWND the method 
>> "AwtComponent::GetTopLevelParentForWindow" must return the same 
>> "opposite" HWND, I do not want to rely on this expectation and run 
>> the risk of using some incorrect HWND as opposite, when there was 
>> already valid "opposite" HWND in hands, and by this to break existing 
>> JDK behavior related to focus handling, because this edited 
>> "AwtWindow::SendWindowEvent" method is really involved in focus 
>> handling.
>
> That was the question, when GetTopLevelParentForWindow may return 
> wrong/incorrect HWND and why the same situation may not happen after 
> fix when we call GetTopLevelParentForWindow later.
>
>>
>> Thank you,
>> Anton
>>
>> On 15/04/2020 07:56, Sergey Bylokhov wrote:
>>> On 4/14/20 11:12 am, Anton Litvinov wrote:
>>>> I think that it is better not to change current well established 
>>>> old behavior in the method "AwtWindow::SendWindowEvent(jint, HWND, 
>>>> jint, jint)" - what is first to get "jobject" from the original 
>>>> "opposite" HWND and try to use it for construction of 
>>>> "TimedWindowEvent". I was trying to make a fix which will just 
>>>> extend the code and will address just this very unreal and very 
>>>> narrow scenario without running the risk of affecting or breaking 
>>>> wide range of other normal scenarios under which this code was 
>>>> working well from the moment of its creation. If we assign parent 
>>>> HWND to the "opposite" from the very beginning, then we will change 
>>>> the currently existing behavior in favor of addressing this almost 
>>>> unreal bug and may for some cases, which we do not know yet, start 
>>>> using parent HWND instead of a valid original "opposite" HWND whose 
>>>> target will be instance of "java.awt.Window".
>>>>
>>>> In my opinion it is better not to omit first attempt to use 
>>>> original "opposite" with related to it "jobject" instance.
>>>
>>> If it is not safe to replace the opposite by the 
>>> GetTopLevelParentForWindow(1) in the first place then
>>> why it is safe to use it in the fix later(1)? What could go wrong at 
>>> step (1) and why it will work fine
>>> at step(2)?
>>>
>>>>
>>>> Thank you,
>>>> Anton
>>>>
>>>> On 14/04/2020 16:41, Sergey Bylokhov wrote:
>>>>> Hi, Anton.
>>>>>
>>>>> Probably it is possible to simplify the code a little bit:
>>>>>
>>>>> Can we just replace initial "opposite" by the top level HWND?
>>>>>
>>>>> + opposite = AwtComponent::GetTopLevelParentForWindow(opposite);
>>>>>   AwtComponent *awtOpposite = AwtComponent::GetComponent(opposite);
>>>>>
>>>>> and add only one check
>>>>>
>>>>>   jOpposite = awtOpposite->GetTarget(env);
>>>>> + if ((jOpposite != NULL) &&
>>>>> +   !env->IsInstanceOf(jOpposite, windowCls)) {
>>>>> +       env->DeleteLocalRef(jOpposite);
>>>>> +       jOpposite = NULL;
>>>>> +  }
>>>>>
>>>>> On 4/10/20 1:32 pm, Anton Litvinov wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Could you please review the following fix for the bug.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8242498
>>>>>> Webrev: 
>>>>>> http://cr.openjdk.java.net/~alitvinov/8242498/jdk15/webrev.00
>>>>>>
>>>>>> The bug is the JVM crash, which occurs because a not existing 
>>>>>> method is called on a Java object which is not an instance of the 
>>>>>> expected Java class that has such a method. Such discrepancy of 
>>>>>> the expected type and the type in runtime is possible, because 
>>>>>> the Java object, whose field value is set to the instance of the 
>>>>>> not expected Java class, is instantiated by AWT native code 
>>>>>> through JNI invocation. Since JNI does not validate arguments 
>>>>>> passed to Java class constructor and since AWT native code does 
>>>>>> not validate arguments prior to invoking Java class constructor 
>>>>>> through JNI, such invalid object is created.
>>>>>>
>>>>>> REASON OF THE CRASH:
>>>>>> The fact that in the method 
>>>>>> "java.awt.DefaultKeyboardFocusManager.dispatchEvent(AWTEvent)" in 
>>>>>> the case "WindowEvent.WINDOW_LOST_FOCUS" of switch operator the 
>>>>>> variable defined by the expression "Window oppositeWindow = 
>>>>>> we.getOppositeWindow();" in runtime is instance of 
>>>>>> "java.awt.Component" class instead of "java.awt.Window" class. 
>>>>>> The crash occurs during attempt to call the method 
>>>>>> "java.awt.Window.getTemporaryLostComponent()" on the object 
>>>>>> "oppositeWindow" which in runtime is "Component" instead of the 
>>>>>> expected "Window" object, and since the method 
>>>>>> "getTemporaryLostComponent()" does not exist in 
>>>>>> "java.awt.Component" class JVM cannot find this method and 
>>>>>> initiates the crash.
>>>>>>
>>>>>> ROOT CAUSE OF THE BUG:
>>>>>> Transfer of the object of the incompatible type 
>>>>>> "java.awt.Component" instead of an object of "java.awt.Window" 
>>>>>> type as "opposite" argument to the constructor 
>>>>>> "TimedWindowEvent(Window source, int id, Window opposite, int 
>>>>>> oldState, int newState, long time)" of the class 
>>>>>> "sun.awt.TimedWindowEvent" through JNI invocation. This JNI 
>>>>>> invocation occurs in the C++ class method 
>>>>>> "AwtWindow::SendWindowEvent(jint, HWND, jint, jint)" in the file 
>>>>>> "src/java.desktop/windows/native/libawt/windows/awt_Window.cpp". 
>>>>>> The exact expression creating the instance of Java class 
>>>>>> "TimedWindowEvent" with the invalid value of the field "opposite" 
>>>>>> is following:
>>>>>>
>>>>>> jobject event = env->NewObject(wClassEvent, wEventInitMID, 
>>>>>> target, id,
>>>>>>          jOpposite, oldState, newState, 
>>>>>> ::JVM_CurrentTimeMillis(NULL, 0));
>>>>>>
>>>>>> THE FIX:
>>>>>> The fix changes "AwtWindow::SendWindowEvent(jint, HWND, jint, 
>>>>>> jint)" method in the file "awt_Window.cpp" to introduce the code 
>>>>>> which verifies that the Java object "jOpposite" is really 
>>>>>> instance of the class "java.awt.Window", and if it is not then 
>>>>>> the fix tries to get Java object corresponding to parent window 
>>>>>> of the original "opposite" HWND. And if this parent window object 
>>>>>> also is not instance of "java.awt.Window" class, then NULL value 
>>>>>> is passed to the constructor of "TimedWindowEvent" class.
>>>>>>
>>>>>> Thank you,
>>>>>> Anton
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>