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

[Anthony Petrov]

Hi Alex,

On 10/07/2009 05:36 PM, Alexander Potochkin wrote:
>>> 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

That's really surprising to hear from a developer of a single-threaded 
GUI-toolkit! We, AWT people, should have this book instead! ;)

Now back to serious matters, at first I thought you were talking about 
the presence of the descendUnconditionallyWhenValidating variable 
itself, that's why I asked for elaboration. Now I see that basically 
you're speaking about the double-checked locking issue only. Thanks for 
clarifying that.

Indeed, Artem noticed this anti-pattern long-long time ago. Even before 
the new variable is introduced, the idiom involving the bare isValid() 
check has existed in our code for ages. And I agree that we need to get 
rid of that idiom.

However, the current fix for 6852592 does not seem to make things worse: 
it just adds one more check that is basically equal (in possible 
side-effects) to the existing check for isValid(). On the other hand, 
making that change may theoretically bring some regressions if some code 
relies on that behavior. That's why I decided to separate the issues. 
Now we have the following CR:

http://bugs.sun.com/view_bug.do?bug_id=6887249

The fix is actually in progress, and apparently going to hit the 
repository prior to the fix for 6852592 (due to a bit lagging 
specification changes approval process). So I don't have any concern 
regarding the issue in terms of the fix that we're currently reviewing: 
at least the regression testing indicates nothing extremely terrible. I 
hope this explanation calms down your worries. If not, please speak up.

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