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

[Alexey Ivanov]

Hi AWT and Swing teams,

Could you please review the updated fix:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/


What has changed since version .02?

During additional testing, I found another scenario where NPE was 
thrown. So the new version adds more checks to prevent access to XPStyle 
and ThemeReader where Windows visual styles become unavailable.

As in previous version, getters in XPStyle class check for null values 
and return dummy defaults if ThemeReader returned null. Skin painters 
also check whether theming is still available before asking ThemeReader 
to paint.

Access to XPStyle.xp instance is blocked as soon as user switched 
themes. The object will be cleaned when the corresponding 
PropertyChangeEvent is handled by Swing.

Thank you,
Alexey.

On 06.05.2014 12:14, Alexey Ivanov wrote:
> Hi AWT and Swing teams,
>
> Could you please review the updated fix:
> http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.02/
>
>
> What has changed since version .01?
>
> The issue was still reproducible with the .01 version of the fix, 
> however it was harder to reproduce.
>
> So in version .02 I added checks for null where possible. If theme 
> becomes unavailable, a dummy value will be used; this way applications 
> will be more stable. As soon as the theme change events are handled, 
> the entire UI will update properly.
>
> Thank you,
> Alexey.
>
> On 24.04.2014 16:23, Alexey Ivanov wrote:
>> Hi Anthony, AWT and Swing teams,
>>
>> Here's the updated fix:
>> http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.01/
>>
>> Description:
>> In the new version of the fix, I use a new xpstyleEnabled field of 
>> AtomicBoolean in WToolkit to track the current value of 
>> "win.xpstyle.themeActive" desktop property. Its value is updated in 
>> windowsSettingChange() and in updateProperties().
>> XPStyle.getXP() checks its cached value in themeActive and the 
>> current value in WToolkit; if the value changes, it schedules 
>> updateAllUIs and invalidates the current style right away, so that 
>> components would not access data from the theme that became unavailable.
>> Two functions in ThemeReader also check this special field from 
>> WToolkit which prevents throwing InternalError when paint is called 
>> before theme change is fully handled.
>>
>> Before updateAllUIs is invoked, it's possible that application will 
>> paint with some values cached from the previous theme. Usually it 
>> happens before "Theme change" dialog disappears from the screen, at 
>> least I noticed no UI glitches during normal theme change.
>>
>>
>> Regression test:
>> No regression test is provided due to its complexity. Additionally, 
>> 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.
>>
>>
>> Thank you,
>> Alexey.
>>
>> On 18.04.2014 18:52, Anthony Petrov wrote:
>>> Hi Alexey,
>>>
>>> No, unfortunately I don't have any suggestions right now.
>>>
>>> As for allowing executing user code on the toolkit thread, we can't 
>>> accept such a fix. Sorry about that.
>>>
>>> -- 
>>> best regards,
>>> Anthony
>>>
>>> On 4/18/2014 6:48 PM, Alexey Ivanov wrote:
>>>> Hi Anthony,
>>>>
>>>> Thank you for your review.
>>>>
>>>> Yes, user code can install a property change listener... It was my
>>>> concern too, that's why I explicitly noted about this.
>>>>
>>>> Do you have any suggestion how this situation can be handled?
>>>> Is it a general rule that all desktop property change listeners 
>>>> must be
>>>> called on EDT?
>>>>
>>>>
>>>> Thanks,
>>>> Alexey.
>>>>
>>>> On 18.04.2014 16:02, Anthony Petrov wrote:
>>>>> Hi Alexey,
>>>>>
>>>>>> With this change, property "win.xpstyle.themeActive" change is fired
>>>>>> on the toolkit thread
>>>>>
>>>>> Is it possible to install a change listener for this property from
>>>>> user code, and hence eventually allow executing some user code on the
>>>>> toolkit thread with your fix?
>>>>>
>>>>> -- 
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>>> On 4/18/2014 12:09 PM, Alexey Ivanov wrote:
>>>>>> Hello Swing and AWT teams,
>>>>>>
>>>>>> Could you please review the fix for jdk9:
>>>>>>      bug: https://bugs.openjdk.java.net/browse/JDK-8039383
>>>>>>      webrev: 
>>>>>> http://cr.openjdk.java.net/~dmarkov/8039383/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 "cached" in XPStyle object. When the theme is
>>>>>> switched to a classic one, HTHEME handle becomes unavailable and 
>>>>>> data
>>>>>> cannot be accessed from the theme any more. The change in theme in
>>>>>> posted to EDT via invokeLater. At the same time, the UI needs to 
>>>>>> repaint
>>>>>> itself as soon as Windows changed the theme, and paint code is often
>>>>>> called before the theme change is handled in Java. This leads to 
>>>>>> NPE and
>>>>>> InternalError as the code tries to access the data that has become
>>>>>> unavailable.
>>>>>>
>>>>>> The fix:
>>>>>> Update "win.xpstyle.themeActive" desktop property and invalidate the
>>>>>> cached XPStyle as soon as windowsSettingChange() is called from 
>>>>>> native
>>>>>> code. Thus when Swing code needs to access theme data, it will 
>>>>>> see no
>>>>>> theme is available and will fallback to classic painting.
>>>>>>
>>>>>> Note: Before the fix, PropertyChangeEvents for desktop properties in
>>>>>> Windows were fired on the Event Dispatch Thread. With this change,
>>>>>> property "win.xpstyle.themeActive" change is fired on the toolkit
>>>>>> thread; all other properties are changed on the EDT as before.
>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Alexey.
>>>>
>>
>