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

Anthony Petrov anthony.petrov at oracle.com
Tue Oct 22 10:46:38 PDT 2013 

On 10/22/2013 07:16 PM, Sergey Bylokhov wrote:
> Hi, Anthony.
> On 22.10.2013 17:38, Anthony Petrov wrote:
>> Hi Sergey,
>>
>> 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.
>   - This method was added to the XCanvasPeer in this 4947530 to stop
> kind of recursion postRepaint->paint->postRepaint.
>   - Usage of target repaint is incorrect here, it should be used from
> the user's code only, because it trigger UPDATE event instead of PAINT
> event.
>> 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.
> What does it mean "not sure we trigger target's repainting in all cases
> depending on the current thread"? If background is changed on the EDT we

See above: if you call Canvas.setBackground(null), currently this 
triggers repainting of the canvas even if the background hasn't been set 
previously. After your fix removes XCanvasPeer.setBackground(), this is 
no longer true. So, -1 time we repaint the canvas. This is what I mean 
under "not sure...". Let's hope this won't break user code (though it 
would be worthwhile to investigate the previous logic and understand why 
this was done...)


> paint a native/target, if background changed on other thread we paint
> native part and post PAINT event, which repaint the whole component later.

Ah, so in this case we actually use the XComponentPeer.paint() which 
does paint the target synchronously. Now I see this. Could you please 
add a comment just above the line 503 in XWindow.java about that to 
clarify this? I'm OK with the fix with such a comment.


>> 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()?
> It is not possible, because a paint() method can be called from the
> Component.paintAll() on any thread and it should paint all parts of the
> component(native/user's) synchronously.

I see. This looks weird (at least, method naming-wise), but let it be 
so. Thanks for the clarification.

--
best regards,
Anthony

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

More information about the awt-dev mailing list