Re: [10] RFR JDK-8190456: sanity/client/SwingSet/src/ComboBoxDemoTest.java failed with NPE from java.awt.EventQueue.getCurrentEventImpl()

[Prasanta Sadhukhan]

+1

Regards
Prasanta
On 11/8/2017 11:58 AM, Muneer Kolarkunnu wrote:


Hi Prasanta,

I corrected it and please find the updated webrev: 
http://cr.openjdk.java.net/~akolarkunnu/8190456/webrev.01/ 



I tested fix with client sanity tests for 1000 iterations on SBR and 
passed all tests without any new issues.


Regards,

Muneer

*From:*Prasanta Sadhukhan
*Sent:* Friday, November 03, 2017 11:49 AM
*To:* Muneer Kolarkunnu; awt-dev@openjdk.java.net
*Subject:* Re:  [10] RFR JDK-8190456: 
sanity/client/SwingSet/src/ComboBoxDemoTest.java failed with NPE from 
java.awt.EventQueue.getCurrentEventImpl()


Hi Muneer,

Earlier, before your fix, if Thread.currentThread() is not equal to 
dispatchThread, then it was returning " null", but now it will not 
return anything even though return value should be AWTEvent.

Is it not giving any compilation problem?

Regards
Prasanta

On 11/1/2017 2:03 PM, Muneer Kolarkunnu wrote:

Hi All,

Please review fix for the below bug:

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

Webrev: http://cr.openjdk.java.net/~akolarkunnu/8190456/webrev.00/


This issue observed during SBR(Same Binary Run) execution of
client sanity tests. Same exception observed from test case
sanity/client/SwingSet/src/DialogDemoTest.java also.
It is a random failure, observed 2 times in 1000 iterations.

In ComboBoxDemoTest, it is happening while changing the selection
on a combo box.

In DialogDemoTest, it is happening during button press.

Fix: As currentEvent is a WeakReference, added null check for
dispatchThread use case also, similar to
fxAppThreadIsDispatchThread use case.

Regards,

Muneer

Re: [10] RFR JDK-8190456: sanity/client/SwingSet/src/ComboBoxDemoTest.java failed with NPE from java.awt.EventQueue.getCurrentEventImpl()

[Muneer Kolarkunnu]

Hi Prasanta,

 

I corrected it and please find the updated webrev: 
http://cr.openjdk.java.net/~akolarkunnu/8190456/webrev.01/

I tested fix with client sanity tests for 1000 iterations on SBR and passed all 
tests without any new issues.

 

Regards,

Muneer

 

From: Prasanta Sadhukhan 
Sent: Friday, November 03, 2017 11:49 AM
To: Muneer Kolarkunnu; awt-dev@openjdk.java.net
Subject: Re:  [10] RFR JDK-8190456: 
sanity/client/SwingSet/src/ComboBoxDemoTest.java failed with NPE from 
java.awt.EventQueue.getCurrentEventImpl()

 

Hi Muneer,

Earlier, before your fix, if Thread.currentThread() is not equal to  
dispatchThread, then it was returning " null", but now it will not return 
anything even though return value should be AWTEvent. 
Is it not giving any compilation problem?

Regards
Prasanta

On 11/1/2017 2:03 PM, Muneer Kolarkunnu wrote:

Hi All,

 

Please review fix for the below bug:

 

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

 

Webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Eakolarkunnu/8190456/webrev.00/"http://cr.openjdk.java.net/~akolarkunnu/8190456/webrev.00/

 

This issue observed during SBR(Same Binary Run) execution of client sanity 
tests. Same exception observed from test case 
sanity/client/SwingSet/src/DialogDemoTest.java also. 
It is a random failure, observed 2 times in 1000 iterations.

In ComboBoxDemoTest, it is happening while changing the selection on a combo 
box.

In DialogDemoTest, it is happening during button press.

 

Fix: As currentEvent is a WeakReference, added null check for dispatchThread 
use case also, similar to fxAppThreadIsDispatchThread use case.

 

Regards,

Muneer

 

Re: RFR: 8185739: Memory leak in BitmapUtil::BitmapToRgn

[Prasanta Sadhukhan]

+1

Regards
Prasanta
On 11/8/2017 3:44 AM, Phil Race wrote:

Might as well ..

http://cr.openjdk.java.net/~prr/8185739.1

-phil.

On 11/06/2017 11:58 PM, Prasanta Sadhukhan wrote:

Hi Phil,

Since you are checking for buf value in line248 and doing cleanup, 
shouldn't we do the same in line322 also in BlendCopy()?


Regards
Prasanta
On 11/7/2017 1:31 AM, Phil Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8185739
Webrev: http://cr.openjdk.java.net/~prr/8185739/

The complaint in the bug is about this code  ..
254 if (!IS_SAFE_SIZE_MUL(width / 2 + 1, height)) {
255 throw std::bad_alloc();

.. potentially leaking memory allocated and stored in "buf" if the 
exception is thrown.


This is indeed a potential leak but
- the same location is also failing to release "hdc"
- it is unclear why this needs to throw bad_alloc instead of 
returning NULL as
is happening elsewhere in this function and the only caller checks 
for NULL

and can handle it.
- There are two other return sites in the same function that may 
leak ..


I have updated all of these.


-phil.

Re: [10] Review request for 8185634, 8185634: Java Fx-Swing dialogs appearing behind main stage

[Alexander Zvegintsev]

HI Semyon,

Please see my answer to Dmitry


Hi Dmitry,

 From my understanding JavaFX stage can't be easily integrated in JDK to 
support orderWindow() approach,

addChildWindow() is a native(and the simplest) way to maintain one window above 
other one, should be called only once.

IIUC the main concert of JDK-8080729 that child windows jumping to parent's 
display upon focus receiving, this is not an issue with current fix,

because addChildWindow() will be called only upon dialog creation in case of 
JavaFX-Swing interop.

Jump may happen if user want to create a child swing dialog on display other 
than JavaFX stage's one,

but such rare scenario can be easily workarounded on a user side by calling 
setLocation() right after setVisible() call.

So I would prefer to use addChildWindow() to make this fix as simple as 
possible.


Thanks,
Alexander.

On 08/11/2017 02:45, Semyon Sadetsky wrote:


Hi Alexander,

In CPlatformWindow.java change you used addChildWindow(), but we get 
rig of addChildWindow() in 8080729 and start to manage windows order 
on java side. Can you test that this change doesn't cause any ordering 
issues with modal and non-modal child and sibling windows on mac?


--Semyon

On 11/07/2017 10:11 AM, Alexander Zvegintsev wrote:


Hi Sergey,

I am not able to crash it on several platforms, except one case:

if we are terminating JavaFX application while EDT processing some 
long task. But it is unrelated to the fix and reproducible on current 
builds.


I've filed a separate bug JDK-8190329 
.


Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows? 
It will not, Windows already fixed by JDK-8088395 




Test added:
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/05/
Thanks,
Alexander.
On 13/10/2017 01:14, Sergey Bylokhov wrote:

Looks fine.
I am not sure but it looks like the fix has an assumption that the 
CPlatformWindow.setVisible() code will be executed on EDT/Appkit but 
it is not the case. This code can be executed on any 
thread(intentionally for crash), and it will be good to check that 
it works as expected by some stress test which will try to force the 
possible crash: 4 threads:

 - show/dispose Swing Node
 - show/dispose Dialog1/2/3 using different timeouts

Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows?


On 10/10/2017 13:54, Kevin Rushforth wrote:

Note that there is now a 04 version.

It looks good to me, although someone more familiar with AWT should 
also check the AWT changes.


We will need a test program for this (as a follow-on issue if not 
now).


-- Kevin


Alexander Zvegintsev wrote:

Please review the updated version

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/02/
Now we are postponing actual window closing, it happens only after 
we ensure that native window pointer is valid on Swing side.


Thanks,
Alexander.

On 23/09/2017 08:01, Sergey Bylokhov wrote:

Hi, Alexander.
How can we be sure that the parent frame will not be disposed 
while we use a pointer?


long ownerWindowPtr = peer.getOverridenWindowHandle();
< Dispose the peer
if (ownerWindowPtr != 0) {
    //Place window above JavaFX stage
    CWrapper.NSWindow.addChildWindow(
    ownerWindowPtr, ptr, CWrapper.NSWindow.NSWindowAbove);
< Boom
}


On 9/21/17 22:56, Alexander Zvegintsev wrote:

Hi Phil,

Please review the updated fix with reflection incorporated
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/01/

New issue created JDK-8187803 
 as JDK 
counterpart of this issue.


Thanks,
Alexander.

On 21/09/2017 22:25, Phil Race wrote:

Some procedural comments :
Since 90% of this is in AWT code, I'd have thought awt-dev 
should be included here.

I've added that list.

And apart from needing separate bug ids, I don't see why the 
bug below is confidential.



I agree with what Kevin pointed out off-line that as in the 
dialog case, the FX side
of the code can use reflection and simply be a harmless 
non-functional no-op

if the SwingAccessor does not provide the new method.

BTW
264 inline HWND GetOverridenHWnd() { return m_overridenHwnd; }
should be "dd" not "d".

-phil.

On 09/21/2017 03:38 AM, Alexander Zvegintsev wrote:

Hello,

please review the fix

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/00/

for the issue

https://bugs.openjdk.java.net/browse/JDK-8185634

Re: RFR: 8185739: Memory leak in BitmapUtil::BitmapToRgn

[Phil Race]

Might as well ..

http://cr.openjdk.java.net/~prr/8185739.1

-phil.

On 11/06/2017 11:58 PM, Prasanta Sadhukhan wrote:

Hi Phil,

Since you are checking for buf value in line248 and doing cleanup, 
shouldn't we do the same in line322 also in BlendCopy()?


Regards
Prasanta
On 11/7/2017 1:31 AM, Phil Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8185739
Webrev: http://cr.openjdk.java.net/~prr/8185739/

The complaint in the bug is about this code  ..
254 if (!IS_SAFE_SIZE_MUL(width / 2 + 1, height)) {
255 throw std::bad_alloc();

.. potentially leaking memory allocated and stored in "buf" if the 
exception is thrown.


This is indeed a potential leak but
- the same location is also failing to release "hdc"
- it is unclear why this needs to throw bad_alloc instead of 
returning NULL as
is happening elsewhere in this function and the only caller checks 
for NULL

and can handle it.
- There are two other return sites in the same function that may leak ..

I have updated all of these.


-phil.

Re: [10] Review request for 8185634, 8185634: Java Fx-Swing dialogs appearing behind main stage

[Kevin Rushforth]

_FX patch_

The new tests run fine for me on all three platforms. I noticed a couple 
issues while reviewing it:


1. All robot-based tests should be in the test.robot.** package 
hierarchy, such that they are only enabled with "-PUSE_ROBOT=true" -- I 
recommend to put them in a new test.robot.javafx.embed.swing package.


2. Can you replace the wild-card imports in the tests with explicit imports?


_JDK patch_

You need to rebase the patch against the latest tip of jdk/client. Once 
you do that, you will notice that CPlatformWindow.java no longer 
compiles on Mac -- you will need to add an explicit import of 
sun.lwawt.LWLightweightFramePeer.


-- Kevin


Alexander Zvegintsev wrote:


Hi Sergey,

I am not able to crash it on several platforms, except one case:

if we are terminating JavaFX application while EDT processing some 
long task. But it is unrelated to the fix and reproducible on current 
builds.


I've filed a separate bug JDK-8190329 
.


Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows? 
It will not, Windows already fixed by JDK-8088395 




Test added:
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/05/
Thanks,
Alexander.
On 13/10/2017 01:14, Sergey Bylokhov wrote:

Looks fine.
I am not sure but it looks like the fix has an assumption that the 
CPlatformWindow.setVisible() code will be executed on EDT/Appkit but 
it is not the case. This code can be executed on any 
thread(intentionally for crash), and it will be good to check that it 
works as expected by some stress test which will try to force the 
possible crash: 4 threads:

 - show/dispose Swing Node
 - show/dispose Dialog1/2/3 using different timeouts

Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows?


On 10/10/2017 13:54, Kevin Rushforth wrote:

Note that there is now a 04 version.

It looks good to me, although someone more familiar with AWT should 
also check the AWT changes.


We will need a test program for this (as a follow-on issue if not now).

-- Kevin


Alexander Zvegintsev wrote:

Please review the updated version

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/02/
Now we are postponing actual window closing, it happens only after 
we ensure that native window pointer is valid on Swing side.


Thanks,
Alexander.

On 23/09/2017 08:01, Sergey Bylokhov wrote:

Hi, Alexander.
How can we be sure that the parent frame will not be disposed 
while we use a pointer?


long ownerWindowPtr = peer.getOverridenWindowHandle();
< Dispose the peer
if (ownerWindowPtr != 0) {
//Place window above JavaFX stage
CWrapper.NSWindow.addChildWindow(
ownerWindowPtr, ptr, CWrapper.NSWindow.NSWindowAbove);
< Boom
}


On 9/21/17 22:56, Alexander Zvegintsev wrote:

Hi Phil,

Please review the updated fix with reflection incorporated
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/01/

New issue created JDK-8187803 
 as JDK 
counterpart of this issue.


Thanks,
Alexander.

On 21/09/2017 22:25, Phil Race wrote:

Some procedural comments :
Since 90% of this is in AWT code, I'd have thought awt-dev 
should be included here.

I've added that list.

And apart from needing separate bug ids, I don't see why the bug 
below is confidential.



I agree with what Kevin pointed out off-line that as in the 
dialog case, the FX side
of the code can use reflection and simply be a harmless 
non-functional no-op

if the SwingAccessor does not provide the new method.

BTW
264 inline HWND GetOverridenHWnd() { return m_overridenHwnd; }
should be "dd" not "d".

-phil.

On 09/21/2017 03:38 AM, Alexander Zvegintsev wrote:

Hello,

please review the fix

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/00/

for the issue

https://bugs.openjdk.java.net/browse/JDK-8185634

Re: [10] Review request for 8185634, 8185634: Java Fx-Swing dialogs appearing behind main stage

[Semyon Sadetsky]

Hi Alexander,

In CPlatformWindow.java change you used addChildWindow(), but we get rig 
of addChildWindow() in 8080729 and start to manage windows order on java 
side. Can you test that this change doesn't cause any ordering issues 
with modal and non-modal child and sibling windows on mac?


--Semyon

On 11/07/2017 10:11 AM, Alexander Zvegintsev wrote:


Hi Sergey,

I am not able to crash it on several platforms, except one case:

if we are terminating JavaFX application while EDT processing some 
long task. But it is unrelated to the fix and reproducible on current 
builds.


I've filed a separate bug JDK-8190329 
.


Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows? 
It will not, Windows already fixed by JDK-8088395 




Test added:
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/05/
Thanks,
Alexander.
On 13/10/2017 01:14, Sergey Bylokhov wrote:

Looks fine.
I am not sure but it looks like the fix has an assumption that the 
CPlatformWindow.setVisible() code will be executed on EDT/Appkit but 
it is not the case. This code can be executed on any 
thread(intentionally for crash), and it will be good to check that it 
works as expected by some stress test which will try to force the 
possible crash: 4 threads:

 - show/dispose Swing Node
 - show/dispose Dialog1/2/3 using different timeouts

Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows?


On 10/10/2017 13:54, Kevin Rushforth wrote:

Note that there is now a 04 version.

It looks good to me, although someone more familiar with AWT should 
also check the AWT changes.


We will need a test program for this (as a follow-on issue if not now).

-- Kevin


Alexander Zvegintsev wrote:

Please review the updated version

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/02/
Now we are postponing actual window closing, it happens only after 
we ensure that native window pointer is valid on Swing side.


Thanks,
Alexander.

On 23/09/2017 08:01, Sergey Bylokhov wrote:

Hi, Alexander.
How can we be sure that the parent frame will not be disposed 
while we use a pointer?


long ownerWindowPtr = peer.getOverridenWindowHandle();
< Dispose the peer
if (ownerWindowPtr != 0) {
    //Place window above JavaFX stage
    CWrapper.NSWindow.addChildWindow(
    ownerWindowPtr, ptr, CWrapper.NSWindow.NSWindowAbove);
< Boom
}


On 9/21/17 22:56, Alexander Zvegintsev wrote:

Hi Phil,

Please review the updated fix with reflection incorporated
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/01/

New issue created JDK-8187803 
 as JDK 
counterpart of this issue.


Thanks,
Alexander.

On 21/09/2017 22:25, Phil Race wrote:

Some procedural comments :
Since 90% of this is in AWT code, I'd have thought awt-dev 
should be included here.

I've added that list.

And apart from needing separate bug ids, I don't see why the bug 
below is confidential.



I agree with what Kevin pointed out off-line that as in the 
dialog case, the FX side
of the code can use reflection and simply be a harmless 
non-functional no-op

if the SwingAccessor does not provide the new method.

BTW
264 inline HWND GetOverridenHWnd() { return m_overridenHwnd; }
should be "dd" not "d".

-phil.

On 09/21/2017 03:38 AM, Alexander Zvegintsev wrote:

Hello,

please review the fix

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/00/

for the issue

https://bugs.openjdk.java.net/browse/JDK-8185634

Re: [10] Review request for 8189204: Possible NPE in Component::getLocationOnScreen()

[Phil Race]

+1

-phil

On 10/25/2017 05:52 PM, Sergey Bylokhov wrote:

Looks fine.

On 25/10/2017 17:37, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK10:

bug: https://bugs.openjdk.java.net/browse/JDK-8189204

webrev: http://cr.openjdk.java.net/~ssadetsky/8189204/webrev.00/

The fix eliminates possibility of NPEs in Component 
::getLocationOnScreen().  Method Component 
::getLocationOnScreen_NoTreeLock() does not consistently scan the 
component hierarchy because Component::getParent() may be overridden. 
Also there are may be issues caused by peer field concurrent updates.


--Semyon

Re: [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

[Phil Race]

> The fields cannot be changed to transient because this would break 
the logic after deserialization of the component.


Can you explain what logic would be broken since changing these to 
transient looks tempting ..


It can't be a problem for serialisation of actual non-null values since 
demonstrably that

can't ever have worked.

If there is broken logic for when they are null, what is it ?

-phil.

On 11/07/2017 10:42 AM, Semyon Sadetsky wrote:



On 11/07/2017 10:21 AM, Sergey Bylokhov wrote:

On 07/11/2017 08:48, Semyon Sadetsky wrote:
But from the other point of view all our listeners like 
"componentListener", "keyListener" etc are transient and have some 
special steps in Component.writeObject().
Yes, it may be necessary to to do something with 
propertyListenersCount in writeObject() as well. But this may change 
the format compatibility. I have created a separate bug to 
investigate this: https://bugs.openjdk.java.net/browse/JDK-8190867


But it does not explain why other containers for listeners marked as 
transient. I guess the reason is in this part of spec in 
Component.writeObject():

 * Writes default serializable fields to stream.  Writes
 * a variety of serializable listeners as optional data.
 * The non-serializable listeners are detected and
 * no attempt is made to serialize them.

This is why all of them marked as transient and during serialization 
we iterates over them and save only listeners which implements 
Serializable interface.
I did get this. This spec is unrelated to the current issue. The 
current issue is very simple:


AccessibleAWTComponent class is Serializable but its fields 
accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.


This violates the Java serialization policy and causes exception in 
JDK code. The proposed fix eliminates this issue. The fields cannot be 
changed to transient because this would break the logic after 
deserialization of the component.




--Semyon


On 31/10/2017 09:05, Semyon Sadetsky wrote:
The updated webrev: 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/



On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
Actually ScreenMenu is fully removed in uninstallUI() (line 54 of 
AquaMenuBarUI), so there is no need to make its property listener 
Serializable, please, ignore the change in 
ScreenMenuPropertyListener.


--Semyon


On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:

On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:


On 23/10/2017 12:41, Semyon Sadetsky wrote:
The AquaMenuBarBorder class is L specific, and it should 
not be serialized/deserialized. Such information(plus l 
client properties/listeners/etc) should be removed from the 
component before serialization. And during deserialization 
the L of the target system should be applied.
That is valid concern. The webrev is updated 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/


The same is applicable to ScreenMenuPropertyListener as well 
because it is also L specific.
I assume that the changes in AccessibleAWTComponentHandler/etc 
are necessary because somecode installs the listeners on the 
components, since the bug is not reproduced in Metal I assume 
that this listeners are installed by Aqua, in this case these 
listeners also should be removed from the component before 
serialization.

ScreenMenu is not a Swing component.

--Semyon

Re: [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

[Sergey Bylokhov]

On 07/11/2017 10:42, Semyon Sadetsky wrote:
I did get this. This spec is unrelated to the current issue. The current 
issue is very simple:


AccessibleAWTComponent class is Serializable but its fields 
accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.


This violates the Java serialization policy and causes exception in JDK 
code. The proposed fix eliminates this issue. The fields cannot be 
changed to transient because this would break the logic after 
deserialization of the component.


Ok, its up to you.





--Semyon


On 31/10/2017 09:05, Semyon Sadetsky wrote:
The updated webrev: 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/



On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
Actually ScreenMenu is fully removed in uninstallUI() (line 54 of 
AquaMenuBarUI), so there is no need to make its property listener 
Serializable, please, ignore the change in 
ScreenMenuPropertyListener.


--Semyon


On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:

On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:


On 23/10/2017 12:41, Semyon Sadetsky wrote:
The AquaMenuBarBorder class is L specific, and it should not 
be serialized/deserialized. Such information(plus l client 
properties/listeners/etc) should be removed from the component 
before serialization. And during deserialization the L of 
the target system should be applied.
That is valid concern. The webrev is updated 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/


The same is applicable to ScreenMenuPropertyListener as well 
because it is also L specific.
I assume that the changes in AccessibleAWTComponentHandler/etc 
are necessary because somecode installs the listeners on the 
components, since the bug is not reproduced in Metal I assume 
that this listeners are installed by Aqua, in this case these 
listeners also should be removed from the component before 
serialization.

ScreenMenu is not a Swing component.

--Semyon


















--
Best regards, Sergey.

Re: [10] Review request for 8185634, 8185634: Java Fx-Swing dialogs appearing behind main stage

[Sergey Bylokhov]

Thank yo for clarification, looks fine.

On 07/11/2017 10:11, Alexander Zvegintsev wrote:

Hi Sergey,

I am not able to crash it on several platforms, except one case:

if we are terminating JavaFX application while EDT processing some long 
task. But it is unrelated to the fix and reproducible on current builds.


I've filed a separate bug JDK-8190329 
.


Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows? 
It will not, Windows already fixed by JDK-8088395 




Test added:
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/05/

Thanks,
Alexander.

On 13/10/2017 01:14, Sergey Bylokhov wrote:

Looks fine.
I am not sure but it looks like the fix has an assumption that the 
CPlatformWindow.setVisible() code will be executed on EDT/Appkit but 
it is not the case. This code can be executed on any 
thread(intentionally for crash), and it will be good to check that it 
works as expected by some stress test which will try to force the 
possible crash: 4 threads:

 - show/dispose Swing Node
 - show/dispose Dialog1/2/3 using different timeouts

Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows?


On 10/10/2017 13:54, Kevin Rushforth wrote:

Note that there is now a 04 version.

It looks good to me, although someone more familiar with AWT should 
also check the AWT changes.


We will need a test program for this (as a follow-on issue if not now).

-- Kevin


Alexander Zvegintsev wrote:

Please review the updated version

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/02/
Now we are postponing actual window closing, it happens only after 
we ensure that native window pointer is valid on Swing side.


Thanks,
Alexander.

On 23/09/2017 08:01, Sergey Bylokhov wrote:

Hi, Alexander.
How can we be sure that the parent frame will not be disposed while 
we use a pointer?


long ownerWindowPtr = peer.getOverridenWindowHandle();
< Dispose the peer
if (ownerWindowPtr != 0) {
    //Place window above JavaFX stage
    CWrapper.NSWindow.addChildWindow(
    ownerWindowPtr, ptr, CWrapper.NSWindow.NSWindowAbove);
< Boom
}


On 9/21/17 22:56, Alexander Zvegintsev wrote:

Hi Phil,

Please review the updated fix with reflection incorporated
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/01/

New issue created JDK-8187803 
 as JDK 
counterpart of this issue.


Thanks,
Alexander.

On 21/09/2017 22:25, Phil Race wrote:

Some procedural comments :
Since 90% of this is in AWT code, I'd have thought awt-dev should 
be included here.

I've added that list.

And apart from needing separate bug ids, I don't see why the bug 
below is confidential.



I agree with what Kevin pointed out off-line that as in the 
dialog case, the FX side
of the code can use reflection and simply be a harmless 
non-functional no-op

if the SwingAccessor does not provide the new method.

BTW
264 inline HWND GetOverridenHWnd() { return m_overridenHwnd; }
should be "dd" not "d".

-phil.

On 09/21/2017 03:38 AM, Alexander Zvegintsev wrote:

Hello,

please review the fix

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/00/

for the issue

https://bugs.openjdk.java.net/browse/JDK-8185634


















--
Best regards, Sergey.

Re: [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

[Semyon Sadetsky]

On 11/07/2017 10:21 AM, Sergey Bylokhov wrote:

On 07/11/2017 08:48, Semyon Sadetsky wrote:
But from the other point of view all our listeners like 
"componentListener", "keyListener" etc are transient and have some 
special steps in Component.writeObject().
Yes, it may be necessary to to do something with 
propertyListenersCount in writeObject() as well. But this may change 
the format compatibility. I have created a separate bug to 
investigate this: https://bugs.openjdk.java.net/browse/JDK-8190867


But it does not explain why other containers for listeners marked as 
transient. I guess the reason is in this part of spec in 
Component.writeObject():

 * Writes default serializable fields to stream.  Writes
 * a variety of serializable listeners as optional data.
 * The non-serializable listeners are detected and
 * no attempt is made to serialize them.

This is why all of them marked as transient and during serialization 
we iterates over them and save only listeners which implements 
Serializable interface.
I did get this. This spec is unrelated to the current issue. The current 
issue is very simple:


AccessibleAWTComponent class is Serializable but its fields 
accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.


This violates the Java serialization policy and causes exception in JDK 
code. The proposed fix eliminates this issue. The fields cannot be 
changed to transient because this would break the logic after 
deserialization of the component.




--Semyon


On 31/10/2017 09:05, Semyon Sadetsky wrote:
The updated webrev: 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/



On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
Actually ScreenMenu is fully removed in uninstallUI() (line 54 of 
AquaMenuBarUI), so there is no need to make its property listener 
Serializable, please, ignore the change in 
ScreenMenuPropertyListener.


--Semyon


On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:

On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:


On 23/10/2017 12:41, Semyon Sadetsky wrote:
The AquaMenuBarBorder class is L specific, and it should not 
be serialized/deserialized. Such information(plus l client 
properties/listeners/etc) should be removed from the component 
before serialization. And during deserialization the L of 
the target system should be applied.
That is valid concern. The webrev is updated 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/


The same is applicable to ScreenMenuPropertyListener as well 
because it is also L specific.
I assume that the changes in AccessibleAWTComponentHandler/etc 
are necessary because somecode installs the listeners on the 
components, since the bug is not reproduced in Metal I assume 
that this listeners are installed by Aqua, in this case these 
listeners also should be removed from the component before 
serialization.

ScreenMenu is not a Swing component.

--Semyon

Re: [10] JDK-8190326: Robot.mouseMove uses scaling factor of main display on unscaled second display

[Sergey Bylokhov]

On 07/11/2017 10:18, shashidhara veerabhadraiah wrote:

Hi Sergey, The problem is with using the wrong scaling for the cursor position. 
To explain:
Primary Monitor res: 1920 * 1080 with scaling factor: 1.25 and a 
secondary monitor with res: 1600 * 1200 with scaling factor: 1.00.

When we pass a mouseMove(1921, 500) we would apply a scaling factor of 1.25 and 
calculate the device coordinates and send the mouse event. Currently this is 
the behaviour.
Actually we should be applying the secondary monitor’s scaling factor of 
1.00(as this position is outside the primary monitor’s range) and then 
calculate the device coordinates and send the mouse event to move there. As you 
can see the diff in the awn_Robot.cpp(Only line nos. 83-87), previously we 
always used to use the primaryIndex(primary monitor) scaling factor. Now the 
scaling factor is updated to use the intended cursor move’s position of the 
respective monitor. So I do not think we can handle this by changing in the 
Java code than in the native code as the issue is with native code of the use 
of the wrong scaling factor.


If I am not missing something, this is how WWindowPeer.setBounds() is 
implemented? It finds the target GConfig and then does some math. Its a 
generic algorithm for moving windows/cursors or taking a screenshot.


The Robot.createCompatibleImage()/WWindowPeer.setBounds() already use 
this code:

gc = SwingUtilities2.getGraphicsConfigurationAtPoint()


--
Best regards, Sergey.

Re: [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

[Sergey Bylokhov]

On 07/11/2017 08:48, Semyon Sadetsky wrote:
But from the other point of view all our listeners like 
"componentListener", "keyListener" etc are transient and have some 
special steps in Component.writeObject().
Yes, it may be necessary to to do something with propertyListenersCount 
in writeObject() as well. But this may change the format compatibility. 
I have created a separate bug to investigate this: 
https://bugs.openjdk.java.net/browse/JDK-8190867


But it does not explain why other containers for listeners marked as 
transient. I guess the reason is in this part of spec in 
Component.writeObject():

 * Writes default serializable fields to stream.  Writes
 * a variety of serializable listeners as optional data.
 * The non-serializable listeners are detected and
 * no attempt is made to serialize them.

This is why all of them marked as transient and during serialization we 
iterates over them and save only listeners which implements Serializable 
interface.




--Semyon


On 31/10/2017 09:05, Semyon Sadetsky wrote:
The updated webrev: 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/



On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
Actually ScreenMenu is fully removed in uninstallUI() (line 54 of 
AquaMenuBarUI), so there is no need to make its property listener 
Serializable, please, ignore the change in ScreenMenuPropertyListener.


--Semyon


On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:

On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:


On 23/10/2017 12:41, Semyon Sadetsky wrote:
The AquaMenuBarBorder class is L specific, and it should not 
be serialized/deserialized. Such information(plus l client 
properties/listeners/etc) should be removed from the component 
before serialization. And during deserialization the L of the 
target system should be applied.
That is valid concern. The webrev is updated 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/


The same is applicable to ScreenMenuPropertyListener as well 
because it is also L specific.
I assume that the changes in AccessibleAWTComponentHandler/etc are 
necessary because somecode installs the listeners on the 
components, since the bug is not reproduced in Metal I assume that 
this listeners are installed by Aqua, in this case these listeners 
also should be removed from the component before serialization.

ScreenMenu is not a Swing component.

--Semyon













--
Best regards, Sergey.

Re: [10] JDK-8190326: Robot.mouseMove uses scaling factor of main display on unscaled second display

[shashidhara veerabhadraiah]

Hi Sergey, The problem is with using the wrong scaling for the cursor position. 
To explain:
Primary Monitor res: 1920 * 1080 with scaling factor: 1.25 and a 
secondary monitor with res: 1600 * 1200 with scaling factor: 1.00.

When we pass a mouseMove(1921, 500) we would apply a scaling factor of 1.25 and 
calculate the device coordinates and send the mouse event. Currently this is 
the behaviour. 
Actually we should be applying the secondary monitor’s scaling factor of 
1.00(as this position is outside the primary monitor’s range) and then 
calculate the device coordinates and send the mouse event to move there. As you 
can see the diff in the awn_Robot.cpp(Only line nos. 83-87), previously we 
always used to use the primaryIndex(primary monitor) scaling factor. Now the 
scaling factor is updated to use the intended cursor move’s position of the 
respective monitor. So I do not think we can handle this by changing in the 
Java code than in the native code as the issue is with native code of the use 
of the wrong scaling factor.

Thanks and regards,
Shashi

> On 07-Nov-2017, at 2:08 AM, Sergey Bylokhov  
> wrote:
> 
> Hi, Shashi
> I think that the logic of how we use {x,y} coordinates in the users space and 
> how we convert them to device space should be similar for robots API like 
> mouseMove/getPixelColor/createScreenCapture and Window.setBounds().
> So it will be good to use the similar java code instaed of native code for 
> these cases, see WWindowPeer.setBounds()
> 
> ps: It seems that webrev contains a part of the fix from 8148344.
> 
> 
> On 05/11/2017 21:34, Shashidhara Veerabhadraiah wrote:
>> Hi, Please review a fix for the below bug.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8190326
>> Webrev: http://cr.openjdk.java.net/~sveerabhadra/8190326/webrev.00/
>> (Please see from line no.83-87 for awt_Robot.cpp review and ignore other 
>> lines as they are under review for another bug)
>> Summary: Robot cursor move used to apply the primary monitor’s scaling 
>> factor to derive device coordinates even though there was move requested to 
>> another monitor having a different scaling factor. Now this is changed to 
>> use the logical point’s monitor scaling factor to which it may be moved to.
>> Thanks and regards,
>> Shashi
> 
> 
> -- 
> Best regards, Sergey.

Re: [10] Review request for 8185634, 8185634: Java Fx-Swing dialogs appearing behind main stage

[Alexander Zvegintsev]

Hi Sergey,

I am not able to crash it on several platforms, except one case:

if we are terminating JavaFX application while EDT processing some long 
task. But it is unrelated to the fix and reproducible on current builds.


I've filed a separate bug JDK-8190329 
.


Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows? 
It will not, Windows already fixed by JDK-8088395 




Test added:
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/05/

Thanks,
Alexander.

On 13/10/2017 01:14, Sergey Bylokhov wrote:

Looks fine.
I am not sure but it looks like the fix has an assumption that the 
CPlatformWindow.setVisible() code will be executed on EDT/Appkit but 
it is not the case. This code can be executed on any 
thread(intentionally for crash), and it will be good to check that it 
works as expected by some stress test which will try to force the 
possible crash: 4 threads:

 - show/dispose Swing Node
 - show/dispose Dialog1/2/3 using different timeouts

Will the current fix cover the native dialogs like 
FileDialog/PrintDialog on linux and windows?


On 10/10/2017 13:54, Kevin Rushforth wrote:

Note that there is now a 04 version.

It looks good to me, although someone more familiar with AWT should 
also check the AWT changes.


We will need a test program for this (as a follow-on issue if not now).

-- Kevin


Alexander Zvegintsev wrote:

Please review the updated version

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/02/
Now we are postponing actual window closing, it happens only after 
we ensure that native window pointer is valid on Swing side.


Thanks,
Alexander.

On 23/09/2017 08:01, Sergey Bylokhov wrote:

Hi, Alexander.
How can we be sure that the parent frame will not be disposed while 
we use a pointer?


long ownerWindowPtr = peer.getOverridenWindowHandle();
< Dispose the peer
if (ownerWindowPtr != 0) {
    //Place window above JavaFX stage
    CWrapper.NSWindow.addChildWindow(
    ownerWindowPtr, ptr, CWrapper.NSWindow.NSWindowAbove);
< Boom
}


On 9/21/17 22:56, Alexander Zvegintsev wrote:

Hi Phil,

Please review the updated fix with reflection incorporated
http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/01/

New issue created JDK-8187803 
 as JDK 
counterpart of this issue.


Thanks,
Alexander.

On 21/09/2017 22:25, Phil Race wrote:

Some procedural comments :
Since 90% of this is in AWT code, I'd have thought awt-dev should 
be included here.

I've added that list.

And apart from needing separate bug ids, I don't see why the bug 
below is confidential.



I agree with what Kevin pointed out off-line that as in the 
dialog case, the FX side
of the code can use reflection and simply be a harmless 
non-functional no-op

if the SwingAccessor does not provide the new method.

BTW
264 inline HWND GetOverridenHWnd() { return m_overridenHwnd; }
should be "dd" not "d".

-phil.

On 09/21/2017 03:38 AM, Alexander Zvegintsev wrote:

Hello,

please review the fix

http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/00/

for the issue

https://bugs.openjdk.java.net/browse/JDK-8185634

Re: [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

[Semyon Sadetsky]

Hi Sergey,

On 11/02/2017 04:57 PM, Sergey Bylokhov wrote:


Some small question while I do a review of other parts.
After the fix the classes which store the listeners like 
AccessibleAWTFocusHandler supports serialisation, but most of them are 
used in pair with propertyListenersCount which is transient. Should we 
serialize the count of listeners?
But from the other point of view all our listeners like 
"componentListener", "keyListener" etc are transient and have some 
special steps in Component.writeObject().
Yes, it may be necessary to to do something with propertyListenersCount 
in writeObject() as well. But this may change the format compatibility. 
I have created a separate bug to investigate this: 
https://bugs.openjdk.java.net/browse/JDK-8190867


--Semyon


On 31/10/2017 09:05, Semyon Sadetsky wrote:
The updated webrev: 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/



On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
Actually ScreenMenu is fully removed in uninstallUI() (line 54 of 
AquaMenuBarUI), so there is no need to make its property listener 
Serializable, please, ignore the change in ScreenMenuPropertyListener.


--Semyon


On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:

On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:


On 23/10/2017 12:41, Semyon Sadetsky wrote:
The AquaMenuBarBorder class is L specific, and it should not 
be serialized/deserialized. Such information(plus l client 
properties/listeners/etc) should be removed from the component 
before serialization. And during deserialization the L of the 
target system should be applied.
That is valid concern. The webrev is updated 
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/


The same is applicable to ScreenMenuPropertyListener as well 
because it is also L specific.
I assume that the changes in AccessibleAWTComponentHandler/etc are 
necessary because somecode installs the listeners on the 
components, since the bug is not reproduced in Metal I assume that 
this listeners are installed by Aqua, in this case these listeners 
also should be removed from the component before serialization.

ScreenMenu is not a Swing component.

--Semyon

[10] Review request for 8190228: Remove redundant modifiers in java.desktop module.

[Semyon Sadetsky]

Hello,

Please review fix for JDK10:

bug: https://bugs.openjdk.java.net/browse/JDK-8190228

webrev: http://cr.openjdk.java.net/~ssadetsky/8190228/webrev.00/

The fix cleanups Swing and AWT code to remove the redundant code like:

SomeClass *extends Object*

private *final* ...someMethod()

--Semyon

Run AWT on MacOSX with -XstartOnFirstThread

[Langer, Christoph]

Hi,

I've got a question to the experts: I have a tool that launches the Eclipse 
Framework (with SWT). When I start the JVM on MacOSX, I (have to) set the 
option -XstartOnFirstThread. However, I want to do some work with AWT before 
launching Eclipse/SWT. But as the AWT NSLoop is not activated in that case, AWT 
stuff is not working. Is there any chance I can run the AWT NSLoop manually 
before Swing is active? Is that possible from within Java code? I don't find 
this topic well documented...

Thanks in advance for any hints.

Best regards
Christoph

Re: [10] JDK-8190192: Double click on the title bar no longer repositions the window

[Shashidhara Veerabhadraiah]

Hi, Please find the updated Webrev at:

 

http://cr.openjdk.java.net/~sveerabhadra/8190192/webrev.01/

 

The issue was that upon clicking on the title bar twice would invoke 
windowShouldZoom() which was returning false always to the appkit, thereby 
blocking the window from getting zoomed all the time. Now the logic is 
simplified as there is an existing zoom state of the window being maintained 
internally by the appkit and my change would use it instead of trying to make 
up our own logic.

 

Thanks and regards,

Shashi

 

From: Philip Race 
Sent: Sunday, October 29, 2017 10:16 PM
To: shashidhara veerabhadraiah 
Cc: Sergey Bylokhov ; awt-dev@openjdk.java.net
Subject: Re:  [10] JDK-8190192: Double click on the title bar no 
longer repositions the window

 

Hello,

> This seems a regression in JDK 8 as it worked fine in 7u80. 

That is too vague for my taste. Precisely what fix caused this regression ?
Once you find that you may find some additional insight into the issue .. and
not regress something else.

>From a quick scan of the history of AWTWindow.m my top candidate is
8026143: [macosx] Maximized state could be inconsistent between peer and frame

So if this regression starts from 8b115 that is quite likely the cause.

Also why is it not possible to include an automated Robot regression test ?

-phil.

On 10/29/17, 7:51 AM, shashidhara veerabhadraiah wrote: 

Hi All, Please review a fix for the below bug:

 

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

 

Webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Esveerabhadra/8190192/webrev.00/"http://cr.openjdk.java.net/~sveerabhadra/8190192/webrev.00/

 

Summary: Double clicking on the title bar of a java frame supposed to maximise 
the window which is not happening. Above fix makes corrects that behaviour by 
listening to double clicks on the title bar and accordingly raising a toggle 
full screen functionality on the window.

 

Thanks and regards,

Shashi