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

Alexey Ivanov alexey.ivanov at oracle.com
Mon Jun 9 13:17:03 UTC 2014 

Hi Phil,

My bad, I didn't pay due attention to other platforms because I modified 
files specific to Windows and Windows LaF. Since Windows LaF is not 
available at other platforms, I was sure these classes are compiled only 
for Windows. My assumption proved to be wrong. In future, I will always 
build all the platforms even if the changes are platform-specific.


Anthony, Petr,

Could you please review my fix for build failure in another thread?

I am really sorry about build failure. I won't make it happen again.


Thanks,
Alexey.

On 06.06.2014 21:54, Phil Race wrote:
> 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.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
>

More information about the swing-dev mailing list