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
<[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.
--
Best regards, Sergey.
