<Swing Dev> <AWT Dev> Review request #4: 6852592 (invalidate() must be smarter) - approved

[Anthony Petrov]

On 10/07/2009 06:24 PM, Alexander Potochkin wrote:
> I am just not comfortable with having a kind of a half-done fix in JDK,
> however I see that there are several more instance of that antipattern 
> in the Container class, see preferredSize(), minimumSize() getMaximumSize()
> 
> are you going to fix them as well in #6887249?

That's being currently discussed.

Artem, would you agree on placing all calls to the isValid() under the 
TreeLock?


> If so I'll approve this one
> ;-)

I already got your approval in your message on 10/06/2009 08:23 PM. The 
only "if" was about whether the tests pass, and they do! ;)

--
best regards,
Anthony

> 
> Thanks
> alexp
> 
>>
>> -- 
>> best regards,
>> Anthony
>>
>>>
>>> Moreover your variant is even more risky:
>>> any field that is going to be accessed via different threads
>>> must always be used under the lock,
>>> otherwise it is possible that one thread
>>> will see the field in an invalid state
>>>
>>> However in your fix the descendUnconditionallyWhenValidating  field
>>> is mutated under the treeLock and then accessed in the validate() method
>>> outside the synchronization block which is unsafe
>>>
>>> The following comment: /* Avoid grabbing lock unless really necessary.*/
>>>
>>> is obsolete, the modern JVM do a great job to optimize synchronization
>>> so there is no need to invent tricky patterns to avoid synchronization
>>>
>>> I see two options here:
>>>
>>> 1) Refactor Container.validate to alway get the treeLock and
>>> do everything inside it. This is the most preferable choice.
>>>
>>> 2) Define descendUnconditionallyWhenValidating as volatile,
>>> this is better than nothing but less safe than #1
>>>
>>> Thanks
>>> alexp
>>>
>>>>
>>>> -- 
>>>> best regards,
>>>> Anthony
>>>>
>>>>>> On 10/01/2009 12:24 PM, Artem Ananiev wrote:
>>>>>>> I've quickly looked through the changes, the fix looks fine with 
>>>>>>> the exception of using double-check idiom in Container.validate() 
>>>>>>> - I told you about that the other day.
>>>>>>
>>>>>> To make sure we don't get messed up when discovering potential 
>>>>>> regressions, I've just filed a separate CR 6887249 (Get rid of 
>>>>>> double-check for isValid() idiom in validate() methods) to fix the 
>>>>>> issue later. Thanks for the suggestion.
>>>>>>
>>>>>> -- 
>>>>>> best regards,
>>>>>> Anthony
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Artem
>>>>>>>
>>>>>>> Anthony Petrov wrote:
>>>>>>>> It's been a long time since we discussed the issue. Now is the 
>>>>>>>> time for revival.
>>>>>>>>
>>>>>>>> Last time we came across a failing test [1] that had a JApplet 
>>>>>>>> embedded in a JFrame. The frame was expected to be validated 
>>>>>>>> upon showing. However, the components in the JApplet were not 
>>>>>>>> validated, since the JApplet itself was marked valid, but the 
>>>>>>>> invalidate() requests from the children of the applet stopped on 
>>>>>>>> the RootPane of the JApplet because it was a validate root.
>>>>>>>>
>>>>>>>> Later we found out a possible solution for that problem [2]: the 
>>>>>>>> show() (as well as the pack()) should validate the whole 
>>>>>>>> component hierarchy unconditionally.
>>>>>>>>
>>>>>>>> So, here's the fix with this solution implemented. Please review:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~anthony/7-23-invalidate-6852592.3/
>>>>>>>>
>>>>>>>> The fix has been tested quite thoroughly: all sort of related 
>>>>>>>> automatic tests for both Swing and AWT areas have been run 
>>>>>>>> (including layout-related tests, bare (J)Component and 
>>>>>>>> Container-related tests, and some other.) All manual 
>>>>>>>> layout-related tests from AWT and Swing have also been run and 
>>>>>>>> passed. Mixing-related regression tests pass as well.
>>>>>>>>
>>>>>>>> Please note that I've also changed the synopsis of the change 
>>>>>>>> request by replacing revalidate() with invalidate() because the 
>>>>>>>> fix actually affects the invalidate() method only.
>>>>>>>>
>>>>>>>> [1] 
>>>>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2009-August/000831.html 
>>>>>>>>
>>>>>>>>
>>>>>>>> [2] 
>>>>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2009-August/000835.html 
>>>>>>>>
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> best regards,
>>>>>>>> Anthony
>>>>>
>>>
>