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

Fri Jun 6 14:03:36 UTC 2014

You're welcome.

--
best regards,
Anthony

On 6/6/2014 5:58 PM, Alexey Ivanov wrote:
> 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.
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>