<AWT Dev> [16] RFR 8249183: JVM crash in "AwtFrame::WmSize" method

[Alexey Ivanov]

Looks fine to me too.

Regards,
Alexey

On 28/08/2020 14:16, Anton Litvinov wrote:
> Thank you, Sergey, for review and approval of this fix.
>
> Hello reviewers,
>
> Can anybody else become the second reviewer of this fix?
>
> Thank you,
> Anton
>
> On 27/08/2020 03:03, Sergey Bylokhov wrote:
>> Looks fine.
>>
>> On 24.08.2020 16:26, Anton Litvinov wrote:
>>> Hi Sergey,
>>>
>>> Thank you for confirmation about which code in the function 
>>> "AwtFrame::WmSize" you suggested to move to Java code level. I agree 
>>> with your suggestion and also came to a conclusion in my previous 
>>> e-mail that only this code can be moved out. The new version of the 
>>> fix, which implements your suggestion, was created and tested. Could 
>>> you please look at the second fix version.
>>>
>>> Webrev (the 2nd version): 
>>> http://cr.openjdk.java.net/~alitvinov/8249183/jdk16/webrev.01
>>>
>>> I studied the involved C++ code flow and confirm that the next 3 C++ 
>>> function calls from the defined by you part of the function 
>>> "AwtFrame::WmSize" do not alter any fields of the involved C++ 
>>> object "AwtFrame", "AwtDialog" and are just nothing else then the 
>>> next 3 corresponding Java expressions:
>>>
>>> 1.  C++ function call 
>>> "SendWindowEvent(java_awt_event_WindowEvent_WINDOW_ICONIFIED);" --> 
>>> Java expression "sun.awt.windows.WComponentPeer.postEvent(new 
>>> TimedWindowEvent((Window) target, WindowEvent.WINDOW_ICONIFIED, 
>>> null, 0, 0, System.currentTimeMillis()));"
>>>
>>> 2.  C++ function call 
>>> "SendWindowEvent(java_awt_event_WindowEvent_WINDOW_DEICONIFIED);" 
>>> --> Java expression "sun.awt.windows.WComponentPeer.postEvent(new 
>>> TimedWindowEvent((Window) target, WindowEvent.WINDOW_DEICONIFIED, 
>>> null, 0, 0, System.currentTimeMillis()));"
>>>
>>> 3.  C++ function call "SendWindowStateEvent(oldState, newState);" 
>>> --> Java expression "sun.awt.windows.WComponentPeer.postEvent(new 
>>> TimedWindowEvent((Window) target, WindowEvent.WINDOW_STATE_CHANGED, 
>>> null, oldState, newState, System.currentTimeMillis()));"
>>>
>>> Regarding your remark about the need to take into account 
>>> "getExtendedState" Java method, I agree with you and confirm that it 
>>> is impossible that this method will be called on not instance of 
>>> "WFramePeer" Java class and agree that there is no need to change 
>>> code related to "getExtendedState" method.
>>>
>>> CHANGES IN THE SECOND FIX VERSION:
>>>
>>> Change 1 - The pointed by you part of "AwtFrame::WmSize" function 
>>> was reimplemented in Java language as the new method 
>>> "sun.awt.windows.WWindowPeer.notifyWindowStateChanged(int oldState, 
>>> int newState)" in the file "WWindowPeer.java". JNI Invocation of 
>>> this new Java method was wrapped in the new C++ function "void 
>>> AwtWindow::NotifyWindowStateChanged(jint oldState, jint newState)" 
>>> in the file "awt_Window.cpp".
>>>
>>> Change 2 - The function "void SendWindowStateEvent(int oldState, int 
>>> newState);" was removed from the C++ class "AwtFrame" in the files 
>>> "awt_Frame.h", "awt_Frame.cpp", because it is not called from 
>>> anywhere in JDK source code after this fix.
>>>
>>> Change 3 - Java method ID variable "static jmethodID 
>>> setExtendedStateMID;" with code using it was removed from the C++ 
>>> class "AwtFrame" in the files "awt_Frame.h", "awt_Frame.cpp", 
>>> because it is not called from anywhere in JDK source code after this 
>>> fix.
>>>
>>> Thank you,
>>> Anton
>>>
>>> On 20/08/2020 05:21, Sergey Bylokhov wrote:
>>>> On 19.08.2020 09:16, Anton Litvinov wrote:
>>>>> Thank you very much for review of this fix. I have been evaluating 
>>>>> your suggestion and my opinion is following. On macOS that code 
>>>>> was implemented already from the moment JDK 7 code for macOS 
>>>>> appeared. Although macOS-specific platform-dependent code on Java 
>>>>> level has some similarities with Windows platform-dependent code 
>>>>> on Java level, native platform-dependent code is very different 
>>>>> between these two OSes.
>>>>>
>>>>> Of course it is technically possible to move some part of 
>>>>> "AwtFrame::WmSize" function 
>>>>> (http://hg.openjdk.java.net/jdk/client/file/2c7b5ae39c19/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#l915) 
>>>>> to some new method or methods in "sun.awt.windows.WWindowPeer", 
>>>>> but that fix will be much bigger, will require different changes 
>>>>> both in C++ and Java code, and will be necessary to prove that 
>>>>> such refactoring did not break anything previously working well. 
>>>>> "AwtFrame::WmSize" function depends on:
>>>>> - "AwtFrame" class fields: "m_iconic", "m_zoomed" through the 
>>>>> functions: "isIconic()", "isZoomed()", "setIconic(BOOL)", 
>>>>> "setZoomed(BOOL)".
>>>>> - Win32 API constants: "SIZE_MINIMIZED", "SIZE_MAXIMIZED".
>>>>> - Call to the function 
>>>>> "AwtWindow::UpdateSecurityWarningVisibility" is not movable to 
>>>>> Java level.
>>>>
>>>>
>>>> No I did not meant to move the whole "AwtFrame::WmSize" to the 
>>>> java, but only the part related for notification, same as on mac.
>>>> So instead of calling " AwtFrame::setExtendedStateMID," from the 
>>>> native, just call something like "WWindowPeer.notifyState(int old, 
>>>> int new, zoom, iconify,... etc)", and on
>>>> the java side call all necessary things like on mac:
>>>> http://hg.openjdk.java.net/jdk/jdk/file/6b984aa424e3/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#l661 
>>>>
>>>>  - postEvent iconify/zoom;
>>>>  - postWindowStateChangedEvent(newWindowState);
>>>> etc
>>>>
>>>> Probably this part of the code in awt_Frame.cpp can be moved to 
>>>> "WWindowPeer.notifyState(...)?
>>>>
>>>>         DTRACE_PRINTLN2("AwtFrame::WmSize: reporting state change 
>>>> %x -> %x",
>>>>                 oldState, newState);
>>>>
>>>>         // sync target with peer
>>>>         JNIEnv *env = (JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2);
>>>>         env->CallVoidMethod(GetPeer(env), 
>>>> AwtFrame::setExtendedStateMID, newState);
>>>>
>>>>         // report (de)iconification to old clients
>>>>         if (changed & java_awt_Frame_ICONIFIED) {
>>>>             if (newState & java_awt_Frame_ICONIFIED) {
>>>> SendWindowEvent(java_awt_event_WindowEvent_WINDOW_ICONIFIED);
>>>>             } else {
>>>> SendWindowEvent(java_awt_event_WindowEvent_WINDOW_DEICONIFIED);
>>>>             }
>>>>         }
>>>>
>>>>         // New (since 1.4) state change event
>>>>         SendWindowStateEvent(oldState, newState);
>>>>
>>>>
>>>>
>>>>>
>>>>> It looks that only contents of "if (changed != 0) {" block 
>>>>> (http://hg.openjdk.java.net/jdk/client/file/2c7b5ae39c19/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#l975) 
>>>>> can be moved to some new Java method. But what to do with JNI 
>>>>> invocation of "getExtendedState()" method 
>>>>> (http://hg.openjdk.java.net/jdk/client/file/2c7b5ae39c19/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#l281).
>>>>>
>>>>> To my mind, such refactoring will not give much benefit, new Java 
>>>>> method or methods will not be used from anywhere else, but it will 
>>>>> be required to change code which currently works stably and maybe 
>>>>> the changes will bring some new problems. I would prefer to make 
>>>>> the fix as narrow as possible for this not very usual scenario 
>>>>> leading to the crash.
>>>>>
>>>>> ALTERNATIVE SUGGESTION FROM ME:
>>>>>
>>>>> By doing this minimal version of the fix, I tried to avoid even 
>>>>> JNI usage without the need. If you do not like this custom flag, 
>>>>> then would you agree for its change to a normal check of peer 
>>>>> object class using "instanceof" operator?
>>>>>
>>>>> For example, to use instead of this flag the next if condition in 
>>>>> "awt_Frame.cpp":
>>>>>
>>>>> "jobject peer = GetPeer();
>>>>> if ((peer != NULL) && (JNU_IsInstanceOfByName(env, peer, 
>>>>> "sun/awt/windows/WFramePeer") > 0)) {"
>>>>>
>>>>> Or if you do not like variant with "instanceof" operator, then 
>>>>> instead of introduction any checks anywhere, I can suggest you as 
>>>>> another alternative option to implement 2 new empty private 
>>>>> methods: "int getExtendedState()", "void setExtendedState(int)" in 
>>>>> the class "sun.awt.windows.WWindowPeer".
>>>>
>>>> Do we really need to change the code related to the 
>>>> "getExtendedState" during creation of AWTFrame?
>>>> That one should never be used by the AwtDialog, so the crash is 
>>>> kind of assertion that something go wrong already.
>>>>
>>>>
>>>
>>
>>
>