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

[Anthony Petrov]

On 10/07/2009 06:57 PM, Artem Ananiev wrote:
>> 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 

OK, I'll modify the fix for 6887249 accordingly.


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

Hmm... AFAIR, there isn't such a requirement expressed explicitly in the 
specification now. I recall we planned to do that in 7, but I'm not sure 
if we submitted any CR for that purpose. Should I file one?

--
best regards,
Anthony

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