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

[Sergey Bylokhov]

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.


-- 
Best regards, Sergey.