[OpenJDK 2D-Dev] RFR 8148886: SEGV in sun.java2d.marlin.Renderer._endRendering

[Laurent Bourgès]

Jim & Phil,

Here is an updated webrev according to jim's comments:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.1/

Changes:
- AAShapePipe rewritten to use a new ReentrantThreadLocal class
(generalization) that use TL + CLQ (like Marlin) to use other (cached)
TileState instances in case of reentrancy; such class may be useful for
other use cases (replacing synchronized / static refs); it do not use
WeakReference as TileState instances are very small (to be improved later ?)
- Improved CrashPaintTest to check few pixel values to ensure rendering is
correct

PS: I noticed that AlphaPaintPipe has a constant int TILE_SIZE = 32 that
seems useless as the method has a tilesize argument; maybe it should be
cleaned up ?

Regards,
Laurent

2016-02-05 2:20 GMT+01:00 Jim Graham <james.graham at oracle.com>:

> Ah, at least one error spotted here after my initial review...
>
> The abox values could be changed by the code in lines 157-160 so you can't
> cache their values until after that.
>
> Also, as you pointed out in another thread in the grdev group, we should
> probably do the same treatment for the TileState in case we have reentrance
> for the AAShapePipe code...
>
>                 ...jim
>
>
> On 2/4/2016 5:10 PM, Jim Graham wrote:
>
>> Hi Laurent,
>>
>> In AAShapePipe you load the values from abox[] into variables named
>> xywh, but these are not xywh values, they are min/max values.  We
>> typically use any of the following naming conventions for these types of
>> values:
>>
>> - x0, y0, x1, y1
>> - x1, y1, x2, y2
>> - minX, minY, maxX, maxY
>>
>> Other than that naming inconsistency the changes look great...
>>
>>              ...jim
>>
>> On 2/4/2016 2:21 PM, Laurent Bourgès wrote:
>>
>>> Please review the webrev fixing SEGFAULT (P2) in the Marlin Renderer
>>> when using thread-local storage with custom Paint (reentrance):
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8148886
>>> webrev: http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.0/
>>>
>>> Changes:
>>> - detect reentrance in MarlinRenderingEngine using flag
>>> RendererContext.usedTL (true/false) and use another context from CLQ
>>> - added few more details in array checks (XXXArrayCache)
>>> - fixed AAShapePipe to support reentrancy: added defensive copy of int[]
>>> abox in local variables + TODO to discuss
>>> - updated Version to 0.7.3.2
>>> - fixed copyright headers
>>>
>>> Best regards,
>>> Laurent
>>>
>>


-- 
-- 
Laurent Bourgès
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20160206/535f90db/attachment.html>