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

[Anton Litvinov]

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