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

[Alexander Potochkin]

Hello Anthony

>> Container.descendUnconditionallyWhenValidating looks fishy indeed,
>> it's better to get rig of it indeed
> 
> Could you elaborate please?

Sure

By the way, I meant "the extra if statements containing 
Container.descendUnconditionallyWhenValidating look fishy"

now it should be a bit more clear :-)

I guess the following thoughts were already expressed by Artem, anyway:

first of all it is a sort of infamous Double-checked locking antipattern
http://en.wikipedia.org/wiki/Double-checked_locking

There are enough serious reasons why not to use it,
I have read them in a "Java Concurrency" book
which is alway on my table

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