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

[Phil Race]

ahem, this is a "bad fix". It has broken all non-windows builds because 
shared code
is now referencing WToolkit and ThemeReader. Please back out this fix 
ASAP ..

-phil.

On 6/6/2014 7:03 AM, Anthony Petrov wrote:
> 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.
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>