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

[Anton Litvinov]

Hi Sergey,

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.

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

Thank you,
Anton

On 17/08/2020 21:15, Sergey Bylokhov wrote:
> Hi, Anton.
>
> On 14.08.2020 10:00, Anton Litvinov wrote:
>> Could you please review the following fix for the bug.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8249183
>> Webrev: http://cr.openjdk.java.net/~alitvinov/8249183/jdk16/webrev.00
>
> Maybe we can get rid most part of this native method and do all stuff 
> in java in WWindowPeer?
>
> Something like already done on macOS:
> notifyIconify():
> http://hg.openjdk.java.net/jdk/jdk/file/6b984aa424e3/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#l661
> notifyZoom():
> http://hg.openjdk.java.net/jdk/jdk/file/6b984aa424e3/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#l686
>
> So we will be able to use plain "instanceof Frame" instead of custom 
> flags:
> http://hg.openjdk.java.net/jdk/jdk/file/6b984aa424e3/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#l1075
>