Hi Sergey,
The situation is not bad at all. We are sure that if, current opposite
HWND is mapped to "Component" instead of the "Window", then
"GetTopLevelParentForWindow()" will be definitely mapped to "Window",
just because the "java.awt.Component" is not hanging independently on
the screen and is always located in some instance of "java.awt.Window",
which can be that frame, dialog, embedded frame etc. The available
reproducer proves this. But I want to emphasize here again, that,
normally when JDK is used normally, the current opposite HWND is mapped
to "Window", it is mapped to "Component" only in case of this reproducer
which does unusual actions with JDK.
What I am not sure, and what I do not want to check by this fix, is a
normal case, when current opposite HWND is mapped to "Window" and we
would substitute it for value returned from
"GetTopLevelParentForWindow", I do not say that it will for sure return
something else than HWND equal to current opposite HWND, but I do not
want to check it through hypothetical regressions.
It is better to verify that "jobject jOpposite" is instance of "Window"
before passing it to the constructor of "TimedWindowEvent(Window, int,
Window opposite, int, int, long)", because it is called from native code
through JNI, which is not able to detect this type discrepancy and to
throw a corresponding Java exception. By JNI specification the native
code is responsible for validation of the input arguments before passing
them to Java code through JNI. And it would look very unusual if in Java
code in constructor of "TimedWindowEvent" we start checking "opposite"
which by source code is instance of "Window" for being instance of again
"Window" class. What do you think about this?
Thank you,
Anton
On 16/04/2020 14:50, Sergey Bylokhov wrote:
On 4/16/20 6:38 am, Anton Litvinov wrote:
I prefer to fix one problem at a time. There is no need to get
"jobject jOpposite" from HWND returned by
"GetTopLevelParentForWindow", when "jOpposite" got from the original
"opposite" HWND is instance of "java.awt.Window". I am not going to
explore all possible scenarios involving dialogs, modal dialogs,
owned windows, owner windows, components in those windows and check
if "GetTopLevelParentForWindow" will work in those scenarios, it is
not related to this bug. Also I will repeat I will not change the
existing behavior just for the reason of checking, if it cause any
regression or does not cause it.
So it means that after the fix, if the current opposite_HWND will be
map to the "Component" instead of the "Window", and we will start to
execute the new code and call GetTopLevelParentForWindow(), then we do
not sure how this method will work for the dialogs, modal dialogs,
owned windows, owner windows, components in those windows?
Then probably it is better to check that in java? Just pass Component
to the TimedWindowEvent, check the type and try to find the parent if
it is not Window.
Why do you think that the fix does not work?
I just tried to understand is it work or not.
Thank you,
Anton
On 15/04/2020 20:59, Sergey Bylokhov wrote:
On 4/15/20 12:26 pm, Anton Litvinov wrote:
When JDK is used in a normal way and not as it is used in the
reproducer, "opposite" is always HWND of a peer of
"java.awt.Window" instance, thus for such cases there is no need to
search for some parent HWND of the "opposite" HWND. I do not want
to break behavior of JDK for these normal scenarios, by getting
some parent HWND of the valid "opposite" HWND. I do not want to
rely on expectation that for valid "opposite" HWND the method
"AwtComponent::GetTopLevelParentForWindow" must return the same
"opposite" HWND, I do not want to rely on this expectation and run
the risk of using some incorrect HWND as opposite, when there was
already valid "opposite" HWND in hands, and by this to break
existing JDK behavior related to focus handling, because this
edited "AwtWindow::SendWindowEvent" method is really involved in
focus handling.
That was the question, when GetTopLevelParentForWindow may return
wrong/incorrect HWND and why the same situation may not happen after
fix when we call GetTopLevelParentForWindow later.
Thank you,
Anton
On 15/04/2020 07:56, Sergey Bylokhov wrote:
On 4/14/20 11:12 am, Anton Litvinov wrote:
I think that it is better not to change current well established
old behavior in the method "AwtWindow::SendWindowEvent(jint,
HWND, jint, jint)" - what is first to get "jobject" from the
original "opposite" HWND and try to use it for construction of
"TimedWindowEvent". I was trying to make a fix which will just
extend the code and will address just this very unreal and very
narrow scenario without running the risk of affecting or breaking
wide range of other normal scenarios under which this code was
working well from the moment of its creation. If we assign parent
HWND to the "opposite" from the very beginning, then we will
change the currently existing behavior in favor of addressing
this almost unreal bug and may for some cases, which we do not
know yet, start using parent HWND instead of a valid original
"opposite" HWND whose target will be instance of "java.awt.Window".
In my opinion it is better not to omit first attempt to use
original "opposite" with related to it "jobject" instance.
If it is not safe to replace the opposite by the
GetTopLevelParentForWindow(1) in the first place then
why it is safe to use it in the fix later(1)? What could go wrong
at step (1) and why it will work fine
at step(2)?
Thank you,
Anton
On 14/04/2020 16:41, Sergey Bylokhov wrote:
Hi, Anton.
Probably it is possible to simplify the code a little bit:
Can we just replace initial "opposite" by the top level HWND?
+ opposite = AwtComponent::GetTopLevelParentForWindow(opposite);
AwtComponent *awtOpposite = AwtComponent::GetComponent(opposite);
and add only one check
jOpposite = awtOpposite->GetTarget(env);
+ if ((jOpposite != NULL) &&
+ !env->IsInstanceOf(jOpposite, windowCls)) {
+ env->DeleteLocalRef(jOpposite);
+ jOpposite = NULL;
+ }
On 4/10/20 1:32 pm, Anton Litvinov wrote:
Hello,
Could you please review the following fix for the bug.
Bug: https://bugs.openjdk.java.net/browse/JDK-8242498
Webrev:
http://cr.openjdk.java.net/~alitvinov/8242498/jdk15/webrev.00
The bug is the JVM crash, which occurs because a not existing
method is called on a Java object which is not an instance of
the expected Java class that has such a method. Such
discrepancy of the expected type and the type in runtime is
possible, because the Java object, whose field value is set to
the instance of the not expected Java class, is instantiated by
AWT native code through JNI invocation. Since JNI does not
validate arguments passed to Java class constructor and since
AWT native code does not validate arguments prior to invoking
Java class constructor through JNI, such invalid object is
created.
REASON OF THE CRASH:
The fact that in the method
"java.awt.DefaultKeyboardFocusManager.dispatchEvent(AWTEvent)"
in the case "WindowEvent.WINDOW_LOST_FOCUS" of switch operator
the variable defined by the expression "Window oppositeWindow =
we.getOppositeWindow();" in runtime is instance of
"java.awt.Component" class instead of "java.awt.Window" class.
The crash occurs during attempt to call the method
"java.awt.Window.getTemporaryLostComponent()" on the object
"oppositeWindow" which in runtime is "Component" instead of the
expected "Window" object, and since the method
"getTemporaryLostComponent()" does not exist in
"java.awt.Component" class JVM cannot find this method and
initiates the crash.
ROOT CAUSE OF THE BUG:
Transfer of the object of the incompatible type
"java.awt.Component" instead of an object of "java.awt.Window"
type as "opposite" argument to the constructor
"TimedWindowEvent(Window source, int id, Window opposite, int
oldState, int newState, long time)" of the class
"sun.awt.TimedWindowEvent" through JNI invocation. This JNI
invocation occurs in the C++ class method
"AwtWindow::SendWindowEvent(jint, HWND, jint, jint)" in the
file
"src/java.desktop/windows/native/libawt/windows/awt_Window.cpp".
The exact expression creating the instance of Java class
"TimedWindowEvent" with the invalid value of the field
"opposite" is following:
jobject event = env->NewObject(wClassEvent, wEventInitMID,
target, id,
jOpposite, oldState, newState,
::JVM_CurrentTimeMillis(NULL, 0));
THE FIX:
The fix changes "AwtWindow::SendWindowEvent(jint, HWND, jint,
jint)" method in the file "awt_Window.cpp" to introduce the
code which verifies that the Java object "jOpposite" is really
instance of the class "java.awt.Window", and if it is not then
the fix tries to get Java object corresponding to parent window
of the original "opposite" HWND. And if this parent window
object also is not instance of "java.awt.Window" class, then
NULL value is passed to the constructor of "TimedWindowEvent"
class.
Thank you,
Anton
