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

[Alexey Ivanov]

Hi Anthony, Petr, AWT and Swing teams,

I've addressed Anthony's and Petr's comments in the updated webrev:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.04/

Thank you,
Alexey.

On 06.06.2014 12:16, Alexey Ivanov wrote:
> Hi Anthony,
>
> Thank you for your review.
>
> I've removed synchronized modifier from updateProperties() method 
> which protected access to wprops field. Now this field is accessed 
> from synchronized methods lazilyInitWProps() and getWProps(); 
> setDesktopProperty also has proper synchronization - that was my 
> reasoning for removing synchronized.
>
> But you're right, it was not the right decision as the update loop now 
> could execute simultaneously which is undesirable. I'll put 
> synchronized modifier to updateProperties() method.
>
> Regards,
> Alexey.
>
> On 06.06.2014 1:07, Anthony Petrov wrote:
>> Hi Alexey,
>>
>> In WToolkit.java you're removing the synchronized modifier from the 
>> private updateProperties() method. And it looks like the method 
>> itself does allow for calling from multiple threads. So I'm concerned 
>> about the lack of synchronization in this method. Was the removal 
>> intentional? How is the method supposed to work now when called from 
>> multiple threads?
>>
>> -- 
>> best regards,
>> Anthony
>>
>> On 6/5/2014 12:16 PM, Petr Pchelko wrote:
>>> Thank you for clarifications, I've been most interested in #2 
>>> obviously.
>>>
>>> The fix looks good to me.
>>>
>>> With best regards. Petr.
>>>
>>> On 05 июня 2014 г., at 11:57, Alexey Ivanov 
>>> <alexey.ivanov at oracle.com> wrote:
>>>
>>>> Hello Petr,
>>>>
>>>> Thank you for your comments.
>>>>
>>>> 1. Sure, I'll remove the comment before the change set is pushed.
>>>> 2. No, I didn't try stub XPStyle object. First of all, UI delegates 
>>>> decide what painting method to use depending on whether 
>>>> XPStyle.getXP() returns null or not. If XPStyle.getXP() always 
>>>> returns non-null value, we'll have re-implement all UI delegates 
>>>> for Windows plaf, and I believe it would result in larger 
>>>> changeset. Additionally, some classes fallback to inherited 
>>>> behavior where XPStyle.getXP() is not available.
>>>> Another reason is that UI delegates cache Skins: those objects 
>>>> shouldn't paint where theming is unavailable.
>>>> 3. No, I haven't filed any bugs yet. I'll file all the issues I've 
>>>> found in the nearest future.
>>>>
>>>> Regards,
>>>> Alexey.
>>>>
>>>> On 05.06.2014 11:08, Petr Pchelko wrote:
>>>>> Hello, Alexey.
>>>>>
>>>>> A couple of comments:
>>>>> 1. ThemeReader:64 - I suggest to remove that comment as it does 
>>>>> not add any value. The variable name is self explanatory.
>>>>> 2. XPStyle - did you try providing a stub XPStyle object instead 
>>>>> of changing many of it's methods? Do I understand correctly that this
>>>>> is not possible because XPstyle is cached in many place is our code?
>>>>> 3. In offline discussion you've mentioned that you've found 
>>>>> another related issue. Did you have a chance to file a bug about it?
>>>>>
>>>>> Thank you.
>>>>> With best regards. Petr.
>>>>>
>>>>> On 05 июня 2014 г., at 10:35, Alexey Ivanov 
>>>>> <alexey.ivanov at oracle.com> wrote:
>>>>>
>>>>>> 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.
>>>>
>>>
>