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

[Anthony Petrov]

Hi Sergey,

Thanks for the updated webrev, and thanks for renaming the 
postPaintEvent() method. It now looks much better.

I'm comparing the changes to XCanvasPeer and XComponentPeer, and I see 
that previously we would trigger target.repaint() if the current 
background of the peer is null (regardless of the argument value passed 
to XCanvasPeer.setBackground().

The XComponentPeer.setBackground() won'd trigger a repaint if both the 
current background color and the argument are nulls. Also, it doesn't 
call target.repaint() directly as the former method in XCanvasPeer did. 
I see that in XWindow.repaint() there's some logic that might send a 
repaint event asynchronously depending on the current thread, but still 
may not do so.

Also, XListPeer now dispatches target's paint() asynchronously through 
an event whereas previously this method has always been called directly. 
I believe I understand the reason for this: we want to post an event 
instead of calling the method directly to avoid a StackOverflowError. 
And this looks fine then. However, in case of the XCanvasPeer I'm not 
sure we trigger target's repainting in all cases depending on the 
current thread. I feel there's some change in logic which is not 
properly documented or may even be a bug that could cause some 
regressions in user code dealing with canvases.

Could you please elaborate on the changes in canvas?

Another point is regarding the XWindow.reapint() changes alone. 
Previously it has called paint(), and the latter simply called paintPeer 
w/o repainting the target. Only the XComponentPeer repainted the target 
in its overridden repaint() method. And this does seem logical since 
it's the XComponentPeer that should maintain the link between a peer and 
its target. So my question is, would it be more logical to implement the 
if (dispatchThread) logic in XComponentPeer.paint() instead of 
XWindow.repaint()?

--
best regards,
Anthony

On 10/22/2013 03: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
>>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>
>