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

[Petr Pchelko]

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.
>>> 
>>