<AWT Dev> [9] Review Request: JDK-8042087 [macosx] LWCToolkit.inokeAndWait is calling EventQueue.invokeLater

[Sergey Bylokhov]

On 4/29/14 9:57 PM, Anthony Petrov wrote:
> Sounds good. Thanks for the clarification. If our existing regression 
> tests don't fail with the fix, then I'm OK with it.
Looks fine to me too. Do not forget to file CR against 
EventQueue.invokeLater in systemColorsChanged.
>
> -- 
> best regards,
> Anthony
>
> On 4/29/2014 9:54 PM, Petr Pchelko wrote:
>> Hello, Anthony.
>>
>> Than you for the review.
>>
>>> While the bug description and the solution sound reasonable, I'm 
>>> still a bit concerned about the presence of the if(component==null) 
>>> branch in the old code. I see that the code has been updated 
>>> recently (with lambdas and friends), and is aware of the app 
>>> contexts thing. So someone who wrote it, added that branch 
>>> consciously. Or left it there from the former version of the code 
>>> that might not be aware of the app contexts problems. In either 
>>> case, w/o knowing why the branch is there in the first place, it 
>>> seems a bit scary to just chop it off.
>>> Could you please investigate a bit more and provide some details so 
>>> that we could be sure this change doesn't cause any regressions?
>> I’ve updated this code many times and the null branch was there from 
>> the very beginning. At first Appkit thread was in the main thread 
>> group, so this branch could work and it was scary to remove it. It 
>> was obviously wrong for security reasons, because it was posting an 
>> event to plugin event queue.
>> After we’ve moved Appkit to the root thread group which does not have 
>> any AppContext in plugin mode, this branch will always fail with NPE.
>>
>> This could lead to regressions, but these would be not “regressions”, 
>> but existing bugs that we didn’t find yet. If applets were tested 
>> more, all the regressions would have already been filed in JBS. So 
>> the idea of this fix is to remove the branch which will always fail 
>> in plugin mode so that we could find plugin bugs while running in 
>> desktop mode. I’m not going to back port this change, because it’s 
>> too risky for update releases, but I think it’s OK for JDK 9. It’s 
>> always better to catch bugs early.
>>
>> With best regards. Petr.
>>
>> On Apr 29, 2014, at 9:37 PM, Anthony Petrov 
>> <anthony.petrov at oracle.com> wrote:
>>
>>> Hi Petr,
>>>
>>> While the bug description and the solution sound reasonable, I'm 
>>> still a bit concerned about the presence of the if(component==null) 
>>> branch in the old code. I see that the code has been updated 
>>> recently (with lambdas and friends), and is aware of the app 
>>> contexts thing. So someone who wrote it, added that branch 
>>> consciously. Or left it there from the former version of the code 
>>> that might not be aware of the app contexts problems. In either 
>>> case, w/o knowing why the branch is there in the first place, it 
>>> seems a bit scary to just chop it off.
>>>
>>> Could you please investigate a bit more and provide some details so 
>>> that we could be sure this change doesn't cause any regressions?
>>>
>>> -- 
>>> best regards,
>>> Anthony
>>>
>>> On 4/29/2014 3:04 PM, Petr Pchelko wrote:
>>>> Hello, Sergey.
>>>>
>>>>>> Hello, AWT Team.
>>>>>>
>>>>>> Please review the fix for the issue:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8042087
>>>>>> The fix is available at:
>>>>>> http://cr.openjdk.java.net/~pchelko/9/8042087/webrev/
>>>>>>
>>>>>> The problem is that we are using EventQueue.invokeLater on the 
>>>>>> Toolkit thread.
>>>>> I guess the fix changes 
>>>>> getSystemEventQueueForInvokeAndWait().postEvent(), and 
>>>>> EventQueue.invokeLater is used in another place of LWCToolkit in 
>>>>> systemColorsChanged().
>>>>>> In applet mode this would fail with NPE. So I've removed the 
>>>>>> non-working code branch, made general cleanup and added a null 
>>>>>> check for the component provided to invokeAndWait and invokeLater 
>>>>>> methods.
>>>> Yes, I've called the bug incorrectly) It should be called "[macosx] 
>>>> LWCToolkit.inokeAndWait is relying on main AppContext".. Sorry 
>>>> for     inaccuracy. I've renamed the issue.
>>>>
>>>> With best regards. Petr.
>>>>
>>>> On 29.04.2014, at 14:38, Sergey Bylokhov 
>>>> <Sergey.Bylokhov at oracle.com> wrote:
>>>>
>>>>> On 4/29/14 12:32 PM, Petr Pchelko wrote:
>>>>>> Hello, AWT Team.
>>>>>>
>>>>>> Please review the fix for the issue:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8042087
>>>>>> The fix is available at:
>>>>>> http://cr.openjdk.java.net/~pchelko/9/8042087/webrev/
>>>>>>
>>>>>> The problem is that we are using EventQueue.invokeLater on the 
>>>>>> Toolkit thread.
>>>>> I guess the fix changes 
>>>>> getSystemEventQueueForInvokeAndWait().postEvent(), and 
>>>>> EventQueue.invokeLater is used in another place of LWCToolkit in 
>>>>> systemColorsChanged().
>>>>>> In applet mode this would fail with NPE. So I've removed the 
>>>>>> non-working code branch, made general cleanup and added a null 
>>>>>> check for the component provided to invokeAndWait and invokeLater 
>>>>>> methods.
>>>>>> We don't have open bugs on Mac about NPE in applet mode, so most 
>>>>>> likely the removed branch was never executed. But with this fix 
>>>>>> we would catch possible errors early.
>>>>>>
>>>>>> With best regards. Petr.
>>>>>
>>>>>
>>>>> -- 
>>>>> Best regards, Sergey.
>>>>>
>>>>
>>


-- 
Best regards, Sergey.