<AWT Dev> [8] Review Request: 7090424 TestGlyphVectorLayout failed automately with java.lang.StackOverflowError

[Artem Ananiev]

Hi, Sergey,

this version looks fine.

Thanks,

Artem

On 10/22/2013 3:19 PM, Sergey Bylokhov wrote:
> New version here:
> http://cr.openjdk.java.net/~serb/7090424/webrev.02/
>
> On 22.10.2013 13:52, Anthony Petrov wrote:
>> Hi Sergey,
>>
>> Sorry, but I can't review a fix w/o a webrev.
>>
>> What kind of problems do you experience with the tool? The cr.openjdk
>> should now have enough free space to host new webrevs. Please try to
>> re-upload the new webrev.
>>
>> --
>> best regards,
>> Anthony
>>
>> On 10/22/2013 12:28 PM, Sergey Bylokhov wrote:
>>> I have a problem with the review tool. I assume that version .00 with
>>> removed volatiles and renamed handleexpose event is ok for everybody?
>>>
>>> On 10/21/13 4:53 PM, Sergey Bylokhov wrote:
>>>> Hi, Anthony.
>>>> Since cr have some issues with the space. I upload 2 files, where
>>>> handleExposeEvent was renamed to postPaintEvent():
>>>>
>>>> http://cr.openjdk.java.net/~serb/7090424/webrev%2c01/src/solaris/classes/sun/awt/X11/XWindow.java.sdiff.html
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~serb/7090424/webrev%2c01/src/solaris/classes/sun/awt/X11/XContentWindow.java.sdiff.html
>>>>
>>>>
>>>>
>>>> On 18.10.2013 15:47, Sergey Bylokhov wrote:
>>>>> Hi, Anthony.
>>>>> On 18.10.2013 15:17, Anthony Petrov wrote:
>>>>>> Hi Sergey,
>>>>>>
>>>>>> In XAWT, we usually use the StateLock to synchronize access to peer
>>>>>> fields (such as background, label, etc.) I don't think that
>>>>>> switching to volatile is a good idea since it prevents us from
>>>>>> performing atomic read/writes to the fields. And this is exactly
>>>>>> what we need for this fix, actually. In other words, the following
>>>>>> pattern works perfectly:
>>>>>>
>>>>>> synchronized (lock) {
>>>>>>    if (a != b) {
>>>>>>       a = b;
>>>>>>       // do stuff, or set a flag to do it later w/o the lock
>>>>>>    }
>>>>>> }
>>>>>>
>>>>>> whereas the following doesn't:
>>>>>>
>>>>>> volatile a;
>>>>>> if (a != b) {
>>>>>>    a = b;
>>>>>>    // do stuff
>>>>>> }
>>>>>>
>>>>>> The latter doesn't work because the value of 'a' may change from
>>>>>> another thread after the if() statement in the first thread is
>>>>>> executed.
>>>>> If it will be changed that's ok. It is safe in this context since
>>>>> there is no races. a will be the latest setted value and repaint
>>>>> action will be done after a was set. Non trivial setters(like
>>>>> setLabel/setText) are called under the locks in shared code.
>>>>>>
>>>>>> Please note that this is critical for AWT because it is a
>>>>>> multi-threaded GUI toolkit.
>>>>>>
>>>>>> src/solaris/classes/sun/awt/X11/XListPeer.java
>>>>>>> -                target.paint(g);
>>>>>>> +                handleExposeEvent(target, 0, 0, getWidth(),
>>>>>>> getHeight());
>>>>>>
>>>>>> (the same applies to XWindow.repaint): can you please rename
>>>>>> XWindow.handleExposeEvent(Component...) to postPaintEvent() and make
>>>>>> it final? A good javadoc for this method would also be appreciated,
>>>>>> because currently seeing the name handle*() I'd think it needs to be
>>>>>> done under the awtLock().
>>>>> Will do.
>>>>>>
>>>>>> Also, for the corresponding changes in XWindow.repaint(), could you
>>>>>> please elaborate a bit more? Looking at the code I see that
>>>>>> XWindow.paint() calls paintPeer(). And in repaint(), you either call
>>>>>> paint() or paintPeer() depending on the current thread. Why is it
>>>>>> needed? Can we just call paintPeer() (or paint() for that matter)
>>>>>> unconditionally since they both seem to result in the same call?
>>>>> paintPeer is used to draw the native part of the peer(text of the
>>>>> label, border of the button etc).
>>>>> paint is a part of Component.paintAll(). And as a result of it call
>>>>> it should paint the native part of the peer and it should call
>>>>> appropriate paint method from the target.
>>>>>> Also, why don't we post an event if reapint() is invoked on the EDT?
>>>>> We do this in osx, but in xawt there are "smart" caches, which are
>>>>> initialized during the paint of the peer. See
>>>>> XLabelPeer&cachedFontMetrics for example . Note that this cache is
>>>>> completely broken.
>>>>>>
>>>>>> --
>>>>>> best regards,
>>>>>> Anthony
>>>>>>
>>>>>> On 10/16/2013 06:00 PM, Sergey Bylokhov wrote:
>>>>>>> Hello.
>>>>>>> Please review the fix for jdk 8.
>>>>>>> The fix has two parts
>>>>>>>   - Repaint method in the peer now paint the component in place
>>>>>>> if it
>>>>>>> was called on EDT only.
>>>>>>>   - Most of setters were changed to stop recursion if they were
>>>>>>> called
>>>>>>> on EDT.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-7090424
>>>>>>> Webrev can be found at:
>>>>>>> http://cr.openjdk.java.net/~serb/7090424/webrev.00
>>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>
>