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

[Anthony Petrov]

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

[Christopher Deckers]

Hi Anthony,

> It's been a long time since we discussed the issue. Now is the time for
> revival.

Thanks for raising the undead! :)

> The fix has been tested quite thoroughly

I had a look at the code and I don't see anything wrong with the
logic. Good work!

If all is well, in which build should we expect to see that change?

Cheers,
-Christopher

[Anthony Petrov]

Hi Chris,

On 9/25/2009 8:39 PM Christopher Deckers wrote:
> If all is well, in which build should we expect to see that change?

Well, the fix should be reviewed here first. Then we need to get an
approval for the specification changes which might take a few weeks. So
I guess not earlier than in about 3 builds from now.

--
best regards,
Anthony

[Artem Ananiev-2]

Hi, Anthony,

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.

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

[Anthony Petrov]

Hi Artem,

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

[Alexander Potochkin]

Hello Anthony

I looked through the Swing code and didn't find any problems,
if all relevant tests passed I approve

Thanks
alexp

P.S.
Container.descendUnconditionallyWhenValidating looks fishy indeed,
it's better to get rig of it indeed

> Hi Artem,
>
> 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

[Anthony Petrov]

Hi Alex,

Thank you for the approval.

On 10/6/2009 8:23 PM Alexander Potochkin wrote:
> Container.descendUnconditionallyWhenValidating looks fishy indeed,
> it's better to get rig of it indeed

Could you elaborate please?

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

[dmitry.cherepanov]

Hi Anthony,

The fix looks good to me.

Thanks,
Dmitry

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
>

[Anthony Petrov]

Thanks, Dmitry.

I'm going to push the fix as soon as the specification changes get approved.

--
best regards,
Anthony


On 10/07/2009 02:08 PM, Dmitry Cherepanov wrote:

> Hi Anthony,
>
> The fix looks good to me.
>
> Thanks,
> Dmitry
>
> 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
>>
>

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

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

[Alexander Potochkin]

Hello Anthony

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

I'd recommend to get rid of the bare isValid() outside the synchronized
block as well

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

Yep, I see that you propose exactly the same in this CR

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

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?

If so I'll approve this one
;-)

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

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

[Alexander Potochkin]

Hello Anthony

>> 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! ;)

What?! already approved...

Okay, you got this time
:-)

Thanks
alexp

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

[Artem Ananiev-2]

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

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

[Anthony Petrov]

On 10/7/2009 7:05 PM Anthony Petrov wrote:
> 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.

Well, I revised the code, and it appears that the 'valid' boolean field
is declared volatile. Which basically means that we should only acquire
a lock when we need an atomic "read-then-update" sort of operation (like
validate() or invalidate() do.) When we need to read the value of this
field only w/o subsequent updating it, we don't actually need any
locking at all. So I tend to think that the fix for 6887249 should
modify the Container.validate() method only. What do you think?

--
best regards,
Anthony

[Artem Ananiev-2]

Anthony Petrov wrote:

> On 10/7/2009 7:05 PM Anthony Petrov wrote:
>> 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.
>
> Well, I revised the code, and it appears that the 'valid' boolean field
> is declared volatile. Which basically means that we should only acquire
> a lock when we need an atomic "read-then-update" sort of operation (like
> validate() or invalidate() do.) When we need to read the value of this
> field only w/o subsequent updating it, we don't actually need any
> locking at all. So I tend to think that the fix for 6887249 should
> modify the Container.validate() method only. What do you think?

If isValid() were a final method that just returns a value of 'isValid'
field, then yes, we wouldn't have to provide any external
synchronization. However, users might want to override isValid(), so I'd
better place all the calls to isValid() under the tree lock.

Thanks,

Artem

> --
> best regards,
> Anthony

[Anthony Petrov]

On 10/8/2009 12:34 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.
>>
>> Well, I revised the code, and it appears that the 'valid' boolean
>> field is declared volatile. Which basically means that we should only
>> acquire a lock when we need an atomic "read-then-update" sort of
>> operation (like validate() or invalidate() do.) When we need to read
>> the value of this field only w/o subsequent updating it, we don't
>> actually need any locking at all. So I tend to think that the fix for
>> 6887249 should modify the Container.validate() method only. What do
>> you think?
>
> If isValid() were a final method that just returns a value of 'isValid'
> field, then yes, we wouldn't have to provide any external
> synchronization. However, users might want to override isValid(), so I'd
> better place all the calls to isValid() under the tree lock.

What about the Component.paramString() method then? Couldn't it produce
some dead-locks while debugging is in progress?

Also, there's a number of isValid() calls in the Swing code (e.g.,
JViewport, BasicTabbedPaneUI, and possibly some more.) Should these be
modified as well? Alex, what's your opinion?

--
best regards,
Anthony

[Anthony Petrov]

While I'm still waiting for some answers to the questions outlined
below, I also just recalled another issue.

About a year ago or so we developed a fix to replace the components
array with a collection in the Container class. The initial version of
the fix included synchronized (getTreeLock()) sections in such methods
like countComponent(), getComponent(int), and some other. I recall we
indeed faced dead-locks back then, and decided that a developer must
hold the lock when calling these methods, so that we could avoid
acquiring the lock ourselves.

How does that decision correspond to the current proposal of forcibly
getting the lock in the preferredSize(), paramString(), some Swing
methods, and others? Wouldn't that be safer to shift the responsibility
of holding the lock to the user's code instead (as just assumed
currently, in fact)?

--
best regards,
Anthony


On 10/08/2009 01:04 PM, Anthony Petrov wrote:

> On 10/8/2009 12:34 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.
>>>
>>> Well, I revised the code, and it appears that the 'valid' boolean
>>> field is declared volatile. Which basically means that we should only
>>> acquire a lock when we need an atomic "read-then-update" sort of
>>> operation (like validate() or invalidate() do.) When we need to read
>>> the value of this field only w/o subsequent updating it, we don't
>>> actually need any locking at all. So I tend to think that the fix for
>>> 6887249 should modify the Container.validate() method only. What do
>>> you think?
>>
>> If isValid() were a final method that just returns a value of
>> 'isValid' field, then yes, we wouldn't have to provide any external
>> synchronization. However, users might want to override isValid(), so
>> I'd better place all the calls to isValid() under the tree lock.
>
> What about the Component.paramString() method then? Couldn't it produce
> some dead-locks while debugging is in progress?
>
> Also, there's a number of isValid() calls in the Swing code (e.g.,
> JViewport, BasicTabbedPaneUI, and possibly some more.) Should these be
> modified as well? Alex, what's your opinion?
>
> --
> best regards,
> Anthony
>