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

[Anton Litvinov]

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.
>>>
>>>
>>
>
>