Re: [14] Review Request: 8233827 Enable screenshots in the enhanced failure handler on Linux/macOS

[Igor Ignatyev]

Hi Sergey,

overall looks good to me. a question about linux, is there an alternative to 
gnome-screenshot for DEs other than GNOME?

-- Igor

> On Dec 11, 2019, at 1:00 AM, Sergey Bylokhov  
> wrote:
> 
> Hello.
> Please review the fix for JDK 14.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8233827
> Fix: http://cr.openjdk.java.net/~serb/8233827/webrev.01
> 
> This change adds the "screen capture on the test failure" feature on macOS 
> and Linux.
> - On Linux, it is implemented by the "gnome-screenshot" command(in case of
>   multiscreen+xineramathe whole big screen will be saved to the 
> "screen.png" file).
> - On macOS it is implemented by the "screencapture" command, note that I have
>   used 1 file per screen, if the number of screens less than 5, other files 
> will be ignored.
> 
> -- 
> Best regards, Sergey.

Re: [14] Review Request: 8235739 Rare NPE at WComponentPeer.getGraphics()

[Sergey Bylokhov]

webrev is re-uploaded.

On 12/10/19 9:23 pm, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8235739
Fix: http://cr.openjdk.java.net/~serb/8235739/webrev.00

In the WComponentPeer.getGraphics() we may get NPE because
AWTAccessorgetPeer("window") can return null if the "window"
was disposed already and its peer was set to null.




--
Best regards, Sergey.

Re: [14] Review Request: 8235638 NPE in LWWindowPeer.getOnscreenGraphics()

[Sergey Bylokhov]

I am sorry, I have uploaded it from one system, and then accidentally removed
it by synchronization from another system. webrev is accessible now.

On 12/11/19 9:58 am, Phil Race wrote:

Maybe Sergey forgot to post it.

-phil.

On 12/11/19 1:53 AM, Prasanta Sadhukhan wrote:


Hi Dmitry,

It seems that webrev is not accessible anymore, but I guess it will be good if 
both the fix can be clubbed together.

Regards

Prasanta

On 11-Dec-19 3:15 PM, Dmitry Markov wrote:

Hi Prasanta,

I guess the NPE, you observed, is already addressed by 
https://bugs.openjdk.java.net/browse/JDK-8235739 (which is on review now, see 
http://mail.openjdk.java.net/pipermail/awt-dev/2019-December/015621.html).
Can you try with fix for 8235739, please?

Thanks,
Dmitry


On 11 Dec 2019, at 09:16, Prasanta Sadhukhan mailto:prasanta.sadhuk...@oracle.com>> wrote:

Hi Sergey,

The regression test fails for me on windows with NPE

java.lang.NullPointerException
at 
java.desktop/sun.awt.windows.WComponentPeer.getGraphics(WComponentPeer.java:582)
at java.desktop/java.awt.Component.getGraphics(Component.java:3124)
at GetGraphicsStressTest.lambda$test$2(GetGraphicsStressTest.java:63)
at java.base/java.lang.Thread.run(Thread.java:833)
Probably we need to do a null check there in windows too.

Regards
Prasanta
On 11-Dec-19 1:02 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8235638
Fix: http://cr.openjdk.java.net/~serb/8235638/webrev.00

I have found a root cause of intermittent failures of some stress tests in the 
JDK on macOS.
Such tests usually show/hide a lot of frames, and fails because of NPE in the
LWWindowPeer.getOnscreenGraphics()
The reason is incorrect null check. We should read the surfaceData to the local 
var apply a
null check and then use it, otherwise, the data may be changed to null after 
the check.









--
Best regards, Sergey.

Re: [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

[Stuart Marks]

For what it's worth, I've looked at the code and the CSR and I think where this 
has ended up is fine.


Phil had previously raised some concerns about the change in behavior. I think 
those are reasonable concerns; obviously I don't know about every place 
Robot.delay() is used that could possibly be affected. However, I believe the 
change makes its interrupt handling more reasonable.


Interrupts don't come out of nowhere; if this thread is interrupted, then 
whoever sent the interrupt wants this thread to stop what it's doing. In the old 
code, the sleep() would return, but the interrupt status would be cleared, and 
the thread would continue doing whatever it was doing. Effectively, the 
interrupt would be ignored!


In the new code, sleep() will return immediately, and delay() will leave the 
interrupt status set. This is the right thing; we want it to affect subsequent 
code, including subsequent calls to Robot.delay(). We want those delays to be 
skipped, instead of further delaying the handling of the interrupt. Eventually 
something will cause InterruptedException to be thrown, stopping this thread's 
activities, which is what the interrupter wanted.


s'marks



On 12/5/19 4:53 PM, Sergey Bylokhov wrote:

Any volunteers to review? =)

On 10/31/19 5:04 pm, Sergey Bylokhov wrote:

Hello.
Please review an updated version of the fix for jdk14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
CSR: https://bugs.openjdk.java.net/browse/JDK-8230783
Fix: http://cr.openjdk.java.net/~serb/8210231/webrev.03

The logic of the method is skipped if the thread was interrupted already.

Re: [14] Review Request: 8235638 NPE in LWWindowPeer.getOnscreenGraphics()

[Phil Race]

Maybe Sergey forgot to post it.

-phil.

On 12/11/19 1:53 AM, Prasanta Sadhukhan wrote:


Hi Dmitry,

It seems that webrev is not accessible anymore, but I guess it will be 
good if both the fix can be clubbed together.


Regards

Prasanta

On 11-Dec-19 3:15 PM, Dmitry Markov wrote:

Hi Prasanta,

I guess the NPE, you observed, is already addressed by 
https://bugs.openjdk.java.net/browse/JDK-8235739 (which is on review 
now, see 
http://mail.openjdk.java.net/pipermail/awt-dev/2019-December/015621.html).

Can you try with fix for 8235739, please?

Thanks,
Dmitry

On 11 Dec 2019, at 09:16, Prasanta Sadhukhan 
> wrote:


Hi Sergey,

The regression test fails for me on windows with NPE

java.lang.NullPointerException
at 
java.desktop/sun.awt.windows.WComponentPeer.getGraphics(WComponentPeer.java:582)
at java.desktop/java.awt.Component.getGraphics(Component.java:3124)
at GetGraphicsStressTest.lambda$test$2(GetGraphicsStressTest.java:63)
at java.base/java.lang.Thread.run(Thread.java:833)
Probably we need to do a null check there in windows too.

Regards
Prasanta
On 11-Dec-19 1:02 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8235638
Fix: http://cr.openjdk.java.net/~serb/8235638/webrev.00

I have found a root cause of intermittent failures of some stress 
tests in the JDK on macOS.
Such tests usually show/hide a lot of frames, and fails because of 
NPE in the

LWWindowPeer.getOnscreenGraphics()
The reason is incorrect null check. We should read the surfaceData 
to the local var apply a
null check and then use it, otherwise, the data may be changed to 
null after the check.

Re: [14] Review Request: 8235620 Broken merge between JDK-8006406 and JDK-8003559

[Phil Race]

Looks good.

-phil

On 12/10/19 10:50 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8235620
Fix: http://cr.openjdk.java.net/~serb/8235620/webrev.00

I have found this bug during the hunt for the reason of why our 
disposing machinery is so slow.


In the sun.lwawt.macosx.CPlatformWindow both fixes changed 
initialize() method, pseudo diff:


JDK-8006406:
    public void initialize() {
    + initializeBase(_target, _peer, _owner, new CPlatformView());
    - contentView = new CPlatformView();
  contentView.initialize(peer, responder);
    }

JDK-8003559:
    public void initialize() {
    - contentView = new CPlatformView();
    + contentView = createContentView();
  contentView.initialize(peer, responder);

Resulted merge:
    + initializeBase(_target, _peer, _owner, new CPlatformView());
    - contentView = new CPlatformView()
    + contentView = createContentView();
  contentView.initialize(peer, responder);

Note that now we create CPlatformView twice. It could be not a big 
issue (one more object to allocated), but it has one unexpected 
problem. CPlatformView is a CFRetainedResource, which calls 
CPlatformView.dispose() during finalization, which tried to call 
windowLayer.dispose(); But since we did not call 
contentView.initialize() on the first CPlatformView its layer is 
null-> this cause an NPE on the finalization thread, which eventually 
slow down our disposal machinery.

Re: [14] Review Request: 8234522 [macos] Crash with use of native file dialog

[Phil Race]

Seems OK.
Although I can't find any Apple documentation on practices to avoid clashes
with someone else's application defined event .. which I expect could 
happen in an application

assembled from independently developer libraries.

-phil.

On 12/11/19 1:35 AM, Dmitry Markov wrote:

Hi Sergey,

I agree, it would be better to replace NSApplicationDefined with 
NSEventTypeApplicationDefined under separate task.

The fix still looks good to me.

Thanks,
Dmitry

PS
Do not forget to add a summary to the regression test


On 10 Dec 2019, at 18:20, Sergey Bylokhov  wrote:

Hi, Dmitry.

We probably can replace NSApplicationDefined by NSEventTypeApplicationDefined, 
but then we will need
to replace NSApplicationDefinedMask by NSEventMaskApplicationDefined, and 
probably something else.
So I leave it for some future deprecation-cleanup.

On 12/10/19 4:03 am, Dmitry Markov wrote:

Hi Sergey,
The changes look OK, but I have a few remarks. I would recommend using 
NSEventTypeApplicationDefined due to deprecation of NSApplicationDefined. Also 
it would be good to add some kind of summary to the regression test. I do not 
need a new webrev with suggested changes.
Thanks,
Dmitry

On 6 Dec 2019, at 23:05, Sergey Bylokhov  wrote:

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8234522
Fix: http://cr.openjdk.java.net/~serb/8234522/webrev.00

This bug is tricky, we have a special mechanism to postpone deallocation
of the objects on the Appkit thread.
It solves the next problem: the native window pointer is disposed in the
inner AppKit loop, while it is still referenced on the stack in the native
Cocoa method which caused the mentioned inner loop. When the inner loop is
exited Cocoa crashes while dereferencing this window pointer.

This mechanism is implemented via posting NSEvent using NSApplicationDefined
type and filter out this event in the nested event loop.

But for some reason, macOS send us exactly the same event(type+subtype), which
we try to use for deallocation and of course, this attempt will crashes.
(probably because all constants of NSEvent we use were deprecated?)

In the fix I have added additional marks(via data fields) to the event to make
it easy to check it was created by us, also I have bump the initial value of
subtype to minimize possible collisions.


--
Best regards, Sergey.


--
Best regards, Sergey.

Re: [14] Review Request: 8235638 NPE in LWWindowPeer.getOnscreenGraphics()

[Prasanta Sadhukhan]

Hi Dmitry,

It seems that webrev is not accessible anymore, but I guess it will be 
good if both the fix can be clubbed together.


Regards

Prasanta

On 11-Dec-19 3:15 PM, Dmitry Markov wrote:

Hi Prasanta,

I guess the NPE, you observed, is already addressed by 
https://bugs.openjdk.java.net/browse/JDK-8235739 (which is on review 
now, see 
http://mail.openjdk.java.net/pipermail/awt-dev/2019-December/015621.html).

Can you try with fix for 8235739, please?

Thanks,
Dmitry

On 11 Dec 2019, at 09:16, Prasanta Sadhukhan 
> wrote:


Hi Sergey,

The regression test fails for me on windows with NPE

java.lang.NullPointerException
at 
java.desktop/sun.awt.windows.WComponentPeer.getGraphics(WComponentPeer.java:582)
at java.desktop/java.awt.Component.getGraphics(Component.java:3124)
at GetGraphicsStressTest.lambda$test$2(GetGraphicsStressTest.java:63)
at java.base/java.lang.Thread.run(Thread.java:833)
Probably we need to do a null check there in windows too.

Regards
Prasanta
On 11-Dec-19 1:02 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8235638
Fix: http://cr.openjdk.java.net/~serb/8235638/webrev.00

I have found a root cause of intermittent failures of some stress 
tests in the JDK on macOS.
Such tests usually show/hide a lot of frames, and fails because of 
NPE in the

LWWindowPeer.getOnscreenGraphics()
The reason is incorrect null check. We should read the surfaceData 
to the local var apply a
null check and then use it, otherwise, the data may be changed to 
null after the check.

Re: [14] Review Request: 8235638 NPE in LWWindowPeer.getOnscreenGraphics()

[Dmitry Markov]

Hi Prasanta,

I guess the NPE, you observed, is already addressed by 
https://bugs.openjdk.java.net/browse/JDK-8235739 
 (which is on review now, see 
http://mail.openjdk.java.net/pipermail/awt-dev/2019-December/015621.html 
).
Can you try with fix for 8235739, please?

Thanks,
Dmitry

> On 11 Dec 2019, at 09:16, Prasanta Sadhukhan  
> wrote:
> 
> Hi Sergey,
> 
> The regression test fails for me on windows with NPE
> 
> java.lang.NullPointerException
>   at 
> java.desktop/sun.awt.windows.WComponentPeer.getGraphics(WComponentPeer.java:582)
>   at java.desktop/java.awt.Component.getGraphics(Component.java:3124)
>   at GetGraphicsStressTest.lambda$test$2(GetGraphicsStressTest.java:63)
>   at java.base/java.lang.Thread.run(Thread.java:833)
> Probably we need to do a null check there in windows too.
> 
> Regards
> Prasanta
> On 11-Dec-19 1:02 AM, Sergey Bylokhov wrote:
>> Hello. 
>> Please review the fix for JDK 14. 
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8235638 
>>  
>> Fix: http://cr.openjdk.java.net/~serb/8235638/webrev.00 
>>  
>> 
>> I have found a root cause of intermittent failures of some stress tests in 
>> the JDK on macOS. 
>> Such tests usually show/hide a lot of frames, and fails because of NPE in 
>> the 
>> LWWindowPeer.getOnscreenGraphics() 
>> The reason is incorrect null check. We should read the surfaceData to the 
>> local var apply a 
>> null check and then use it, otherwise, the data may be changed to null after 
>> the check. 
>> 
>> 

Re: [14] Review Request: 8234522 [macos] Crash with use of native file dialog

[Dmitry Markov]

Hi Sergey,

I agree, it would be better to replace NSApplicationDefined with 
NSEventTypeApplicationDefined under separate task.

The fix still looks good to me.

Thanks,
Dmitry

PS
Do not forget to add a summary to the regression test

> On 10 Dec 2019, at 18:20, Sergey Bylokhov  wrote:
> 
> Hi, Dmitry.
> 
> We probably can replace NSApplicationDefined by 
> NSEventTypeApplicationDefined, but then we will need
> to replace NSApplicationDefinedMask by NSEventMaskApplicationDefined, and 
> probably something else.
> So I leave it for some future deprecation-cleanup.
> 
> On 12/10/19 4:03 am, Dmitry Markov wrote:
>> Hi Sergey,
>> The changes look OK, but I have a few remarks. I would recommend using 
>> NSEventTypeApplicationDefined due to deprecation of NSApplicationDefined. 
>> Also it would be good to add some kind of summary to the regression test. I 
>> do not need a new webrev with suggested changes.
>> Thanks,
>> Dmitry
>>> On 6 Dec 2019, at 23:05, Sergey Bylokhov  wrote:
>>> 
>>> Hello.
>>> Please review the fix for JDK 14.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8234522
>>> Fix: http://cr.openjdk.java.net/~serb/8234522/webrev.00
>>> 
>>> This bug is tricky, we have a special mechanism to postpone deallocation
>>> of the objects on the Appkit thread.
>>> It solves the next problem: the native window pointer is disposed in the
>>> inner AppKit loop, while it is still referenced on the stack in the native
>>> Cocoa method which caused the mentioned inner loop. When the inner loop is
>>> exited Cocoa crashes while dereferencing this window pointer.
>>> 
>>> This mechanism is implemented via posting NSEvent using NSApplicationDefined
>>> type and filter out this event in the nested event loop.
>>> 
>>> But for some reason, macOS send us exactly the same event(type+subtype), 
>>> which
>>> we try to use for deallocation and of course, this attempt will crashes.
>>> (probably because all constants of NSEvent we use were deprecated?)
>>> 
>>> In the fix I have added additional marks(via data fields) to the event to 
>>> make
>>> it easy to check it was created by us, also I have bump the initial value of
>>> subtype to minimize possible collisions.
>>> 
>>> 
>>> -- 
>>> Best regards, Sergey.
> 
> 
> -- 
> Best regards, Sergey.

Re: [14] Review Request: 8235638 NPE in LWWindowPeer.getOnscreenGraphics()

[Prasanta Sadhukhan]

Hi Sergey,

The regression test fails for me on windows with NPE

java.lang.NullPointerException
at 
java.desktop/sun.awt.windows.WComponentPeer.getGraphics(WComponentPeer.java:582)
at java.desktop/java.awt.Component.getGraphics(Component.java:3124)
at GetGraphicsStressTest.lambda$test$2(GetGraphicsStressTest.java:63)
at java.base/java.lang.Thread.run(Thread.java:833)
Probably we need to do a null check there in windows too.

Regards
Prasanta

On 11-Dec-19 1:02 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8235638
Fix: http://cr.openjdk.java.net/~serb/8235638/webrev.00

I have found a root cause of intermittent failures of some stress 
tests in the JDK on macOS.
Such tests usually show/hide a lot of frames, and fails because of NPE 
in the

LWWindowPeer.getOnscreenGraphics()
The reason is incorrect null check. We should read the surfaceData to 
the local var apply a
null check and then use it, otherwise, the data may be changed to null 
after the check.

[14] Review Request: 8233827 Enable screenshots in the enhanced failure handler on Linux/macOS

[Sergey Bylokhov]

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8233827
Fix: http://cr.openjdk.java.net/~serb/8233827/webrev.01

This change adds the "screen capture on the test failure" feature on macOS and 
Linux.
 - On Linux, it is implemented by the "gnome-screenshot" command(in case of
   multiscreen+xineramathe whole big screen will be saved to the 
"screen.png" file).
 - On macOS it is implemented by the "screencapture" command, note that I have
   used 1 file per screen, if the number of screens less than 5, other files 
will be ignored.

--
Best regards, Sergey.