<AWT Dev> Review request for 8047288: [macosx] Endless loop in EDT on Mac

[artem malinko]

Thank you, Anton.

On 7/24/2014 5:33 PM, Anton V. Tarasov wrote:
> Artem,
>
> I've just noticed the Window.isFocusableWindow method is _final_ . So, 
> that was a false alert, sorry. And thanks for the check.
> No more questions from me.
>
> Regards,
> Anton.
>
> On 24.07.2014 15:37, artem malinko wrote:
>> Call stack and sysout show that in case of applet Window.setVisible() 
>> is called from the main thread or from EDT thread. So I think toolkit 
>> thread won't call this method.
>>
>> On 7/24/2014 2:02 PM, Anton V. Tarasov wrote:
>>> On 24.07.2014 13:48, artem malinko wrote:
>>>> Hi Anton.
>>>>
>>>> Sorry, didn't understand you well. Do you mean we should check(and 
>>>> test) that this method(isFocusableWindow()) is also called on EDT 
>>>> if we run applet?
>>>
>>> IMHO, it's enough to just make sure (via inspecting the sources) 
>>> that Window.setVisible() is called on EDT in case of an applet. That 
>>> logic is triggered on the plugin side.
>>>
>>> Anton.
>>>
>>>>
>>>> On 7/24/2014 11:42 AM, Anton V. Tarasov wrote:
>>>>> Hi Artem,
>>>>>
>>>>> I'm Ok with the fix, provided that LWWindowPeer.setVisibleImpl() 
>>>>> is called on EDT for applets (if I'm not mistaken). Otherwise, we 
>>>>> still have a client method (getTarget().isFocusableWindow()) 
>>>>> called on the toolkit thread, which is generally no good.
>>>>>
>>>>> Regards,
>>>>> Anton.
>>>>>
>>>>> On 24.07.2014 10:25, artem malinko wrote:
>>>>>> Thank you, Petr.
>>>>>>
>>>>>> Could anyone else review the fix, please?
>>>>>>
>>>>>> On 7/23/2014 10:30 PM, Petr Pchelko wrote:
>>>>>>> Hello, Artem.
>>>>>>>
>>>>>>> The new version (it’s .00 for some reason) looks good.
>>>>>>>
>>>>>>> With best regards. Petr.
>>>>>>>
>>>>>>>> On Jul 23, 2014, at 6:55 PM, artem malinko 
>>>>>>>> <artem.malinko at oracle.com <mailto:artem.malinko at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Hi, Petr. I ran focus regression tests and jck tests on awt. 
>>>>>>>> For fixed jdk results is the same. Except my new test, of 
>>>>>>>> course which is not passed on not fixed jdk:) And also I fixed 
>>>>>>>> issues in test. New webrev:
>>>>>>>> http://cr.openjdk.java.net/~bae/8047288/9/webrev.00/
>>>>>>>>
>>>>>>>> On 7/22/2014 8:23 PM, Petr Pchelko wrote:
>>>>>>>>> Hello, Artem.
>>>>>>>>>
>>>>>>>>> A couple of comments:
>>>>>>>>> 1. LWWindowPeer: 268 - please remove an empty line.
>>>>>>>>> 2. LWWIndowPeer. isTargetFocusable - the method is not needed 
>>>>>>>>> at all.
>>>>>>>>> 3. I’m concerned about the test. Do you really need the close 
>>>>>>>>> button?
>>>>>>>>> 4. frame and window variables are set from main thread and 
>>>>>>>>> read from EDT. They should be declared volatile.
>>>>>>>>>
>>>>>>>>> Also please run all focus regression and JCK tests, because 
>>>>>>>>> this area is very sensitive.
>>>>>>>>>
>>>>>>>>> With best regards. Petr.
>>>>>>>>>
>>>>>>>>>> On Jul 22, 2014, at 8:04 PM, artem malinko 
>>>>>>>>>> <artem.malinko at oracle.com <mailto:artem.malinko at oracle.com>> 
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Hello, AWT Team.
>>>>>>>>>>
>>>>>>>>>> Please review a fix for the issue:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8047288
>>>>>>>>>> The fix is available at:
>>>>>>>>>> http://cr.openjdk.java.net/~mcherkas/artem/8047288/webrev.01/ 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Emcherkas/artem/8047288/webrev.01/>
>>>>>>>>>>
>>>>>>>>>> Window.isFocusableWindow() could lead to deadlock if it is 
>>>>>>>>>> invoked on AppKit thread. Fix caches result of 
>>>>>>>>>> Window.isFocusableWindow() on a peer level and method is not 
>>>>>>>>>> invoked on AppkitThread.
>>>>>>>>>>
>>>>>>>>>> Thank you.
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20140724/aa27b970/attachment.html>