Re: RFR: 8211810 X11 Time stamp data should be unsigned

[Ichiroh Takiguchi]

Hello Sergey.

Thank you for reviewing.

Since xclient.get_data(3) is long,
So it is necessary to cast the value ​​from "long" to "int", like:
  Integer.toUnsignedLong((int)xclient.get_data(3))
I'm not sure, which is good...

Also same kind of codes are there:
src/java.desktop/unix/classes/sun/awt/X11/MotifDnDDropTargetProtocol.java: 
   long time_stamp = MotifDnDConstants.Swapper.getInt(data + 4, 
eventByteOrder) & 0xL;
src/java.desktop/unix/classes/sun/awt/X11/MotifDnDDropTargetProtocol.java: 
   long time_stamp = MotifDnDConstants.Swapper.getInt(data + 4, 
eventByteOrder) & 0xL;
src/java.desktop/unix/classes/sun/awt/X11/XDnDDropTargetProtocol.java:   
 time_stamp = xclient.get_data(3) & 0xL;
src/java.desktop/unix/classes/sun/awt/X11/XDnDDropTargetProtocol.java:   
 time_stamp = xclient.get_data(2) & 0xL;


It is necessary to cast the values ​​from "long" to "int".

What do you think ?

Ichiroh Takiguchi

On 2018-10-25 08:16, Sergey Bylokhov wrote:

Hi, Ichiroh.

I think you can simplify the fix a little bit by using
Integer.toUnsignedLong(int).

On 07/10/2018 19:33, Ichiroh Takiguchi wrote:

Hello.
(Sorry, I put wrong bug id, please ignore previous mail)
Could you review the fix ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8211810
Change: https://cr.openjdk.java.net/~itakiguchi/8211810/webrev.00/

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-10-08 11:20, Ichiroh Takiguchi wrote:

Hello.
Could you review the fix ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8211826
Change: https://cr.openjdk.java.net/~itakiguchi/8211826/webrev.00/

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-06-19 22:22, Ichiroh Takiguchi wrote:

Hello,
IBM would like to contribute a patch to OpenJDK project.

Time stamp data on X11 ClientMessage should be 32 bit unsigned int.
But it's converted to signed long on Java.
It should be masked as unsigned 32bit data

I'd like to obtain a sponsor.

--
---
old/src/java.desktop/unix/classes/sun/awt/X11/XDnDDropTargetProtocol.java
   2018-06-19 22:10:49.313678852 +0900
+++
new/src/java.desktop/unix/classes/sun/awt/X11/XDnDDropTargetProtocol.java
   2018-06-19 22:10:48.670692432 +0900
@@ -620,7 +620,7 @@

 /* Time stamp - new in XDnD version 1. */
 if (sourceProtocolVersion > 0) {
-    time_stamp = xclient.get_data(3);
+    time_stamp = xclient.get_data(3) & 
0xL;

 }

 /* User action - new in XDnD version 2. */
@@ -867,7 +867,7 @@
  */
 if (dropAction == DnDConstants.ACTION_MOVE && 
success) {


-    long time_stamp = xclient.get_data(2);
+    long time_stamp = xclient.get_data(2) & 
0xL;

 long xdndSelectionAtom =
XDnDConstants.XDnDSelection.getSelectionAtom().getAtom();

--- old/src/java.desktop/unix/classes/sun/awt/X11/XMSelection.java
   2018-06-19 22:10:50.223659632 +0900
+++ new/src/java.desktop/unix/classes/sun/awt/X11/XMSelection.java
   2018-06-19 22:10:49.576673297 +0900
@@ -200,7 +200,7 @@
 if 
(log.isLoggable(PlatformLogger.Level.FINE)) {
 log.fine("client messags = " + 
xce);

 }
-    long timestamp = xce.get_data(0);
+    long timestamp = xce.get_data(0) & 
0xL;

 long atom = xce.get_data(1);
 long owner = xce.get_data(2);
 long data = xce.get_data(3);
--

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

Re: OpenJdk11-28-EA JDialog hanging

[Martin Balao]

Yes, I think we now have a good understanding of why this hangs.

The remaining discussion, in my opinion, is if we should block or not while
handling SequencedEvents. I'd like to see strong arguments on both sides to
get this right. Even though my original position was for not-blocking -for
the reasons already shared-, I acknowledge that a deeper investigation is
needed and will do it as soon as possible.

On Thu, Oct 25, 2018 at 11:40 AM, Mario Torre  wrote:

> I'm not a reviewer, but FWIW I concur with the analysis and with the
> proposed solution.
>
> Cheers,
> Mario
> On Thu, Oct 25, 2018 at 10:05 AM Laurent Bourgès
>  wrote:
> >
> > Hi Sergey & Martin,
> >
> >>
> >> > AWT experts, what do you advice about asynchronous events: to Block
> or to dispatch selected awt events...
> >>
> >> I think that before answer this question we need to clarify why the
> current code hangs.
> >
> >
> > According to me, Martin already exposed his detailled analysis of 2
> cases making AWT to hang with several AppContexts: in summary, like a
> deadlock, the EDT threads are waiting for each other to dispatch
> SequencedEvents !
> >
> > Please Martin correct me, or maybe give us an updated diagnostic of the
> problem ?
> >
> > This thread is already quite long and both Martin & me invested a lot of
> time on debugging, fixing & testing, please give us your understanding,
> Sergey.
> >
> > Finally I am in favor of Martin's patch 2 sent by Oct 16th:
> > http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.02
> >
> > Cheers,
> > Laurent
>
>
>
> --
> Mario Torre
> Associate Manager, Software Engineering
> Red Hat GmbH 
> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
>

Re: OpenJdk11-28-EA JDialog hanging

[Mario Torre]

I'm not a reviewer, but FWIW I concur with the analysis and with the
proposed solution.

Cheers,
Mario
On Thu, Oct 25, 2018 at 10:05 AM Laurent Bourgès
 wrote:
>
> Hi Sergey & Martin,
>
>>
>> > AWT experts, what do you advice about asynchronous events: to Block or to 
>> > dispatch selected awt events...
>>
>> I think that before answer this question we need to clarify why the current 
>> code hangs.
>
>
> According to me, Martin already exposed his detailled analysis of 2 cases 
> making AWT to hang with several AppContexts: in summary, like a deadlock, the 
> EDT threads are waiting for each other to dispatch SequencedEvents !
>
> Please Martin correct me, or maybe give us an updated diagnostic of the 
> problem ?
>
> This thread is already quite long and both Martin & me invested a lot of time 
> on debugging, fixing & testing, please give us your understanding, Sergey.
>
> Finally I am in favor of Martin's patch 2 sent by Oct 16th:
> http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.02
>
> Cheers,
> Laurent



-- 
Mario Torre
Associate Manager, Software Engineering
Red Hat GmbH 
9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898

Re: OpenJdk11-28-EA JDialog hanging

[Krishna Addepalli]

Hi Sergey,

I also agree with Laurent about root cause of hang provided by Martin.
However, we just need to make sure that non Sequenced Events are not dispatched 
when SequencedEvents are being dealt with.

Thanks,
Krishna

> On 25-Oct-2018, at 1:35 PM, Laurent Bourgès  wrote:
> 
> Hi Sergey & Martin,
> 
> 
> > AWT experts, what do you advice about asynchronous events: to Block or to 
> > dispatch selected awt events...
> 
> I think that before answer this question we need to clarify why the current 
> code hangs.
> 
> According to me, Martin already exposed his detailled analysis of 2 cases 
> making AWT to hang with several AppContexts: in summary, like a deadlock, the 
> EDT threads are waiting for each other to dispatch SequencedEvents !
> 
> Please Martin correct me, or maybe give us an updated diagnostic of the 
> problem ?
> 
> This thread is already quite long and both Martin & me invested a lot of time 
> on debugging, fixing & testing, please give us your understanding, Sergey.
> 
> Finally I am in favor of Martin's patch 2 sent by Oct 16th:
> http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.02 
> 
> 
> Cheers,
> Laurent

Re: [12] Review Request: 8211435 Exception in thread "AWT-EventQueue-1" java.lang.IllegalArgumentException: null source

[Krishna Addepalli]

Looks good to me.

> On 25-Oct-2018, at 5:41 PM, Laurent Bourgès  wrote:
> 
> Hi Sergey,
> 
> (I am not a Reviewer).
> 
> Thanks for both the bug report and the fix, it looks good for me.
> 
> Laurent
> 
> Le mer. 24 oct. 2018 à 23:32, Sergey Bylokhov  > a écrit :
> Hello.
> Please review the fix for jdk 12.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8211435 
> 
> Webrev: http://cr.openjdk.java.net/~serb/8211435/webrev.00 
> 
> 
> Bug description:
> 
>In the DefaultKeyboardFocusManager class we have a special field 
> "activeWindow", which stores the currently active window. It is used in two 
> similar cases:
>   1. If the java window gets "WINDOW_ACTIVATED" event it will try to send 
> "WINDOW_DEACTIVATED" to the old active window, which is stored in the 
> "activeWindow" field.
>   2. If the java component lost the focus, and the opposite component is not 
> a java component, then it will try to send "WINDOW_DEACTIVATED" to the old 
> active window, which is stored in the "activeWindow" field.
> 
> The difference in these two cases is that in "case 1" we check the old active 
> window to null[1], and the second case has no such check. The bug is 
> reproduced in non-standalone mode, when we have a few Appcontexts and this 
> field might be updated by different EDT in parallel.
> 
> Note that the test is for OSX only, because of another bug: JDK-8204142[2]
> 
> 
> [1] 
> http://hg.openjdk.java.net/jdk/jdk/file/ad9077f044be/src/java.desktop/share/classes/java/awt/DefaultKeyboardFocusManager.java#l527
>  
> 
> [2] https://bugs.openjdk.java.net/browse/JDK-8204142 
> 
> 
> Laurent 

Re: [12] Review request for JDK-8208543: [macos] Support for apple.awt.documentModalSheet incomplete

[Dmitry Markov]

Hi Manajit,

Currently the test is marked as ‘passed’ if timeout takes place. I think we 
should indicate an error or mark it as ‘failed’ in such case.

Thanks,
Dmitry

> On 25 Oct 2018, at 11:35, Manajit Halder  wrote:
> 
> Hi Dmitry,
> 
> Thanks for your comments. I have addressed all your review comments in the 
> new webrev.
> Additional changes:
> NSDocModalWindowMask is deprecated and hence changed it to 
> NSWindowStyleMaskDocModalWindow.
> Window is created a Panel, required for style mask 
> NSWindowStyleMaskDocModalWindow.
> Test case was modified to add a case for the failed scenario "Dialog 
> without owner".
> 
> Please review the modified webrev:
> http://cr.openjdk.java.net/~mhalder/8208543/webrev.01/ 
> 
> 
> Regards,
> Manajit
> 
> On 13/10/18 12:14 AM, Dmitry Markov wrote:
>> Hi Manajit,
>> 
>> There is an inconsistency between the proposed implementation and Apple JDK: 
>> if the property applied to the dialog which does not have an owner on the 
>> build with your changes it appears as sheet, but on Apple JDK it appears as 
>> a window.
>> 
>> I think every frame/dialog inside dispose() method in the regression test 
>> should be checked for null-value before usage.
>> 
>> I noticed that the regression test uses Timer API (see 
>> createAndShowModalSheet() method). Shall we stop/cancel the timer when 
>> “Pass”/“Fail” button is press?
>> 
>> I suppose it is better to declare createAndShowModalSheet() and 
>> createAndShowInstructionFrame() as static. In such case the creation of 
>> class instance may be omitted.
>> 
>> Thanks,
>> Dmitry
>> 
>>> On 12 Oct 2018, at 05:36, Manajit Halder  wrote:
>>> 
>>> Hi Dmitry,
>>> 
>>> Could you please review this fix related to Modal sheet on Mac OS?
>>> 
>>> Regards,
>>> Manajit
>>> 
>>> 
>>> On 10/10/18 3:33 PM, Manajit Halder wrote:
 Hi All,
 
 Kindly review the fix for JDK12.
 
 Bug:
 https://bugs.openjdk.java.net/browse/JDK-8208543
 
 
 Webrev:
 http://cr.openjdk.java.net/~mhalder/8208543/webrev.00/ 
 
 
 Problem:
 "apple.awt.documentModalSheet" was getting set on the Dialog while its 
 creations, but appearance of Dialog was not changing.
 
 Fix:
 Setting "apple.awt.documentModalSheet" on Window after its creation.
 
 Regards,
>>> Manajit
> 

Re: [12] Review Request: 8211435 Exception in thread "AWT-EventQueue-1" java.lang.IllegalArgumentException: null source

[Laurent Bourgès]

Hi Sergey,

(I am not a Reviewer).

Thanks for both the bug report and the fix, it looks good for me.

Laurent

Le mer. 24 oct. 2018 à 23:32, Sergey Bylokhov 
a écrit :

> Hello.
> Please review the fix for jdk 12.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8211435
> Webrev: http://cr.openjdk.java.net/~serb/8211435/webrev.00
>
> Bug description:
>
>In the DefaultKeyboardFocusManager class we have a special field
> "activeWindow", which stores the currently active window. It is used in two
> similar cases:
>   1. If the java window gets "WINDOW_ACTIVATED" event it will try to send
> "WINDOW_DEACTIVATED" to the old active window, which is stored in the
> "activeWindow" field.
>   2. If the java component lost the focus, and the opposite component is
> not a java component, then it will try to send "WINDOW_DEACTIVATED" to the
> old active window, which is stored in the "activeWindow" field.
>
> The difference in these two cases is that in "case 1" we check the old
> active window to null[1], and the second case has no such check. The bug is
> reproduced in non-standalone mode, when we have a few Appcontexts and this
> field might be updated by different EDT in parallel.
>
> Note that the test is for OSX only, because of another bug: JDK-8204142[2]
>
>
> [1]
> http://hg.openjdk.java.net/jdk/jdk/file/ad9077f044be/src/java.desktop/share/classes/java/awt/DefaultKeyboardFocusManager.java#l527
> [2] https://bugs.openjdk.java.net/browse/JDK-8204142


Laurent

Re: [12]RFR: JDK-8197811: Test java/awt/Choice/PopupPosTest/PopupPosTest.html fails on Windows

[Krishna Addepalli]

Hi Sergey,

 

I checked the test and it passed in 6, whereas in 8u191, the behavior is as 
observed in the current jdk. I think the behavior change happened somewhere in 
that timeframe, to have the selected item appear in the popup. In jdk6, it was 
showing some list of items which was not containing the currently selected 
item, which is why the test was passing.

 

Thanks,

Krishna





On 25-Oct-2018, at 9:57 AM, Sergey Bylokhov  wrote:

 

Hi, Krishna.

This test was integrated 14 years ago, and this bug was filed 1 year ago. Can 
you please confirm that this is not regression and the test did not work all 
this time?

On 24/10/2018 21:04, Krishna Addepalli wrote:



Hi All,
Please review a fix for JDK-8197811: 
https://bugs.openjdk.java.net/browse/JDK-8197811
Webrev: http://cr.openjdk.java.net/~kaddepalli/8197811/webrev00/
The problem is that, the mouse click happens in the item display area (and not 
on the arrow button in the right side of the choice). Because of this, the 
choice comes up and closes immediately, and hence the next mouse move doesnot 
result in another item being selected, because of which the test fails.
Fix is to click on the arrow button, so that the choice popup stays on, and the 
subsequent mouse move selects a different item.
Along with the fix, I have removed the applet based code, and also the test now 
passes on Mac (which was earlier excluded).
Thanks,
Krishna



-- 
Best regards, Sergey.

 

Re: [12] Review Request: 8211435 Exception in thread "AWT-EventQueue-1" java.lang.IllegalArgumentException: null source

[Dmitry Markov]

Hi Sergey,

The fix looks good to me.
I think you can make the test generic once JDK-8204142 is fixed.

Thanks,
Dmitry

> On 24 Oct 2018, at 22:31, Sergey Bylokhov  wrote:
> 
> Hello.
> Please review the fix for jdk 12.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8211435
> Webrev: http://cr.openjdk.java.net/~serb/8211435/webrev.00
> 
> Bug description:
> 
>  In the DefaultKeyboardFocusManager class we have a special field 
> "activeWindow", which stores the currently active window. It is used in two 
> similar cases:
> 1. If the java window gets "WINDOW_ACTIVATED" event it will try to send 
> "WINDOW_DEACTIVATED" to the old active window, which is stored in the 
> "activeWindow" field.
> 2. If the java component lost the focus, and the opposite component is not a 
> java component, then it will try to send "WINDOW_DEACTIVATED" to the old 
> active window, which is stored in the "activeWindow" field.
> 
> The difference in these two cases is that in "case 1" we check the old active 
> window to null[1], and the second case has no such check. The bug is 
> reproduced in non-standalone mode, when we have a few Appcontexts and this 
> field might be updated by different EDT in parallel.
> 
> Note that the test is for OSX only, because of another bug: JDK-8204142[2]
> 
> 
> [1] 
> http://hg.openjdk.java.net/jdk/jdk/file/ad9077f044be/src/java.desktop/share/classes/java/awt/DefaultKeyboardFocusManager.java#l527
> [2] https://bugs.openjdk.java.net/browse/JDK-8204142
> 
> 
> -- 
> Best regards, Sergey.

Re: [12] Review request for JDK-8208543: [macos] Support for apple.awt.documentModalSheet incomplete

[Manajit Halder]

Hi Dmitry,

Thanks for your comments. I have addressed all your review comments in 
the new webrev.

Additional changes:
    NSDocModalWindowMask is deprecated and hence changed it to 
NSWindowStyleMaskDocModalWindow.
    Window is created a Panel, required for style mask 
NSWindowStyleMaskDocModalWindow.
    Test case was modified to add a case for the failed scenario 
"Dialog without owner".


Please review the modified webrev:
http://cr.openjdk.java.net/~mhalder/8208543/webrev.01/ 



Regards,
Manajit

On 13/10/18 12:14 AM, Dmitry Markov wrote:

Hi Manajit,

There is an inconsistency between the proposed implementation and Apple JDK: if 
the property applied to the dialog which does not have an owner on the build 
with your changes it appears as sheet, but on Apple JDK it appears as a window.

I think every frame/dialog inside dispose() method in the regression test 
should be checked for null-value before usage.

I noticed that the regression test uses Timer API (see 
createAndShowModalSheet() method). Shall we stop/cancel the timer when 
“Pass”/“Fail” button is press?

I suppose it is better to declare createAndShowModalSheet() and 
createAndShowInstructionFrame() as static. In such case the creation of class 
instance may be omitted.

Thanks,
Dmitry


On 12 Oct 2018, at 05:36, Manajit Halder  wrote:

Hi Dmitry,

Could you please review this fix related to Modal sheet on Mac OS?

Regards,
Manajit


On 10/10/18 3:33 PM, Manajit Halder wrote:

Hi All,

Kindly review the fix for JDK12.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8208543


Webrev:
http://cr.openjdk.java.net/~mhalder/8208543/webrev.00/ 


Problem:
"apple.awt.documentModalSheet" was getting set on the Dialog while its 
creations, but appearance of Dialog was not changing.

Fix:
Setting "apple.awt.documentModalSheet" on Window after its creation.

Regards,

Manajit

Re: OpenJdk11-28-EA JDialog hanging

[Laurent Bourgès]

Hi Sergey & Martin,


> > AWT experts, what do you advice about asynchronous events: to Block or
> to dispatch selected awt events...
>
> I think that before answer this question we need to clarify why the
> current code hangs.
>

According to me, Martin already exposed his detailled analysis of 2 cases
making AWT to hang with several AppContexts: in summary, like a deadlock,
the EDT threads are waiting for each other to dispatch SequencedEvents !

Please Martin correct me, or maybe give us an updated diagnostic of the
problem ?

This thread is already quite long and both Martin & me invested a lot of
time on debugging, fixing & testing, please give us your understanding,
Sergey.

Finally I am in favor of Martin's patch 2 sent by Oct 16th:
http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.02

Cheers,
Laurent