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

[Artem Ananiev]

Anthony Petrov wrote:
> 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?

Yes, that would be fine. Have we already introduced a warning about all 
the layout/validation methods (including the calls to isValid()) to be 
called by users under the tree lock into JDK7? I guess, we have, so it 
would be strange if we don't follow our own guidelines.

Thanks,

Artem

>> 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
>>>>>>
>>>>
>>