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.

Reply via email to