<Swing Dev> <AWT Dev> [9] Review request for JDK-8046559: NPE when changing Windows theme

[Alexey Ivanov]

Hello Petr,

Thank you for your review.

Regards,
Alexey.

On 04.07.2014 15:14, Petr Pchelko wrote:
> Hello, Alexey.
>
>> Your test does not test the JFileChooser hang, because the bug is reproducible only in plugin mode where TK thread doesn't
>> have an AppContext and we update the ThemeReader on the Toolkit thread. Your test runs in standalone mode
>> where completely different codepath is used. So I suggest either remove the test or update it to emulate multi-appcontext
>> environment. Please see test/javax/swing/JComponent/8043610/bug8043610.java for an example on how to emulate this.
> Sorry, disregard this comment. You've updated the code so that now it could deadlock in standalone too.
>
> The fix looks good.
>
> With best regards. Petr.
>
> On 04 июля 2014 г., at 14:28, Petr Pchelko <petr.pchelko at oracle.com> wrote:
>
>> Hello, Alexey.
>>
>> Your test does not test the JFileChooser hang, because the bug is reproducible only in plugin mode where TK thread doesn't
>> have an AppContext and we update the ThemeReader on the Toolkit thread. Your test runs in standalone mode
>> where completely different codepath is used. So I suggest either remove the test or update it to emulate multi-appcontext
>> environment. Please see test/javax/swing/JComponent/8043610/bug8043610.java for an example on how to emulate this.
>>
>> Otherwise the fix looks good assuming the JPRT job runs fine.
>>
>> With best regards. Petr.
>>
>> On 04 июля 2014 г., at 14:12, Anthony Petrov <anthony.petrov at oracle.com> wrote:
>>
>>> Hi Alexey,
>>>
>>> src/windows/classes/sun/awt/windows/WToolkit.java
>>>> 940         final Map<String, Object> props = getWProps();
>>>> 941         updateXPStyleEnabled(props.get(XPSTYLE_THEME_ACTIVE));
>>>> 971     private synchronized Map<String, Object> getWProps() {
>>>> 972         return (wprops != null) ? wprops.getProperties() : null;
>>> There is a potential NPE at line 941.
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 7/2/2014 5:28 PM, Alexey Ivanov wrote:
>>>> Hi AWT, Swing teams,
>>>>
>>>> Could you please review the fix to JDK-8046559 for jdk9:
>>>>     bug: https://bugs.openjdk.java.net/browse/JDK-8046559
>>>>     webrev: http://cr.openjdk.java.net/~aivanov/8046559/jdk9/webrev.00/
>>>>
>>>> Problem description:
>>>> When changing Windows themes from a theme with visual styles (Windows
>>>> Aero or Windows Basic) to a classic one, NullPointerException could be
>>>> thrown from Swing code during component tree validation, or
>>>> InternalError could be thrown during component painting.
>>>>
>>>> Root cause:
>>>> Windows theme data are accessed via XPStyle and ThemeReader. When the
>>>> theme is switched to a classic one, theme handles become unavailable,
>>>> and theme data cannot be accessed any more. The change in theme is
>>>> posted to EDT; at the same time, the UI often needs to repaint before
>>>> the theme change is completely handled in Swing. This leads to NPE and
>>>> InternalError as the code tries to access the data that has become
>>>> unavailable.
>>>>
>>>> The fix:
>>>> Windows desktop properties are updated right on Windows Toolkit thread,
>>>> and the value of "win.xpstyle.themeActive" is stored in
>>>> ThemeReader.xpStyleEnabled field. PropertyChangeEvents for desktop
>>>> properties are delivered either synchronously on EDT or asynchronously
>>>> via DesktopPropertyChangeSupport, as it used to be.
>>>>
>>>> Getters in XPStyle class check for null values and return dummy defaults
>>>> if ThemeReader returned null. SkinPainters also check whether theming is
>>>> still available before asking ThemeReader to paint.
>>>>
>>>> Access to XPStyle.xp instance is blocked as soon as user switches
>>>> themes. The object will be cleaned when the corresponding
>>>> PropertyChangeEvent is handled by Swing.
>>>>
>>>>
>>>> This new version of the fix addresses issues found with the initial
>>>> submission of JDK-8039383 fix:
>>>>   - JDK-8046239: Build failure in 9-client on all non-Windows platforms
>>>>   - JDK-8046391: Hang displaying JFileChooser with Windows L&F
>>>>
>>>> See also
>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2014-June/007975.html
>>>> and http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/
>>>>
>>>>
>>>> Regression test:
>>>> No regression test is provided due to its complexity. Whether
>>>> NullPointerException or InternalError are thrown depends on the order of
>>>> event handling, sometimes exceptions do not occur when changing theme of
>>>> visual styles enabled theme to a classic theme.
>>>>
>>>> I included regression test for hang in JFileChooser, JDK-8046391.
>>>>
>>>>
>>>> Thank you,
>>>> Alexey.