Sounds good. Thanks for the clarification. If our existing regression
tests don't fail with the fix, then I'm OK with it.
--
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 <[email protected]> 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 <[email protected]> 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.
