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

[Phil Race]

I filed P1 bug https://bugs.openjdk.java.net/browse/JDK-8046239

You really should at least build on other platforms and you can use JPRT 
for that ..
Testing would be good too ..

-phil.

On 6/6/2014 10:48 AM, Phil Race wrote:
> 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.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>