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

Alexey Ivanov alexey.ivanov at oracle.com
Fri Jun 6 13:58:03 UTC 2014 

Hi Anthony, Petr,

Thank you for reviewing my fix.

Regards,
Alexey.


On 06.06.2014 15:19, Anthony Petrov wrote:
> +1
>
> -- 
> best regards,
> Anthony
>
> On 6/6/2014 3:20 PM, Petr Pchelko wrote:
>> Hello, Alexey.
>>
>> The final version still looks good.
>>
>> With best regards. Petr.
>>
>> On 06 июня 2014 г., at 15:02, Alexey Ivanov 
>> <alexey.ivanov at oracle.com> wrote:
>>
>>> 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.
>>>>>>>
>>>>>>
>>>>
>>>
>>

More information about the swing-dev mailing list