<AWT Dev> [8u/9] Review request: JDK-8028486 [TEST_BUG] [macosx] java/awt/Window/WindowsLeak/WindowsLeak.java fails

Fri May 27 19:03:06 UTC 2016

I approved this fix (I thought) but here it is again in case there was 
any question

+1

-phil.

On 05/12/2016 04:59 AM, Anton Tarasov wrote:
> Hi Sergey,
>
> I’m fine with the fix.
>
>> On 12 May 2016, at 14:43, Sergey Bylokhov <Sergey.Bylokhov at oracle.com 
>> <mailto:Sergey.Bylokhov at oracle.com>> wrote:
>>
>> Hi, Anton, Phil.
>> Do you have some comments about the current version or it can be 
>> considered as an approved?
>>
>> On 04.05.16 18:06, Philip Race wrote:
>>> I recall Anton's fix included a couple of unrelated JNI refs that 
>>> were not
>>> being deleted. A separate bug ID would be good for that as it really
>>> has nothing to do with this problem at all.
>
>
> I’d filed it (on review): https://bugs.openjdk.java.net/browse/JDK-8156116
>
> Regards,
> Anton.
>
>>>
>>> -phil.
>>>
>>> On 5/4/16, 7:08 AM, Sergey Bylokhov wrote:
>>>> On 04.05.16 16:07, Anton Tarasov wrote:
>>>>> Hi Sergey,
>>>>>
>>>>> As you fixed the surface leak separately (thanks!) should I create a
>>>>> dedicated CR for the unrelated JNI ones?
>>>>
>>>> Can you please clarify this case? Do you have a testcase or steps to
>>>> reproduce?(WindowsLeak.java will pass after the current fix).
>>>>
>>>>
>>>>>> On 29 Apr 2016, at 18:19, Sergey Bylokhov
>>>>>> <Sergey.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at oracle.com>> 
>>>>>> wrote:
>>>>>>
>>>>>> On 29.04.16 18:01, Anton Tarasov wrote:
>>>>>>> [CC’ing to 2d-dev to discuss the issue]
>>>>>>>
>>>>>>>> On 29 Apr 2016, at 16:14, Sergey Bylokhov
>>>>>>>> <Sergey.Bylokhov at oracle.com 
>>>>>>>> <mailto:Sergey.Bylokhov at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> On 29.04.16 15:53, Anton Tarasov wrote:
>>>>>>>>> It seems so. But that might be not that critical, because it
>>>>>>>>> doesn’t hold (it won’t) any UI controls and all the UI tree.
>>>>>>>>> Anyway it leaks, yes.
>>>>>>>>
>>>>>>>> Looks like this is crossplatform bug and it also affects d3d/ogl.
>>>>>>>> So probably we can fix it on the upper level?
>>>>>>>> validatedSrc/Dst/Data is stored the surfaces which were validated
>>>>>>>> and ready to paint. from the first point of view we can change
>>>>>>>> them to soft reference, and take care about null values.
>>>>>>>
>>>>>>> Are the validatedSrc/Dst/Data objects referenced from somewhere
>>>>>>> else? They are private, so from native? If not, soft refs won’t
>>>>>>> help I’m afraid...
>>>>>>
>>>>>> I guess it is used in BufferedContext only, to skip updates of the
>>>>>> native ogl/d3d context if the target/source surfaces were not
>>>>>> changed since the last update.
>>>>>>
>>>>>>>
>>>>>>> This is not the case for CGLLayer, which is referenced from JNI.
>>>>>>> And so, wrapping it with a weak ref will work. Also, if the
>>>>>>> SurfaceData is uniquely tight to the layer, then it seems natural
>>>>>>> to dispose (not flush) it with the layer disposal. And that’s
>>>>>>> actually the case: LWWindowPeer.disposeImpl() invalidates it.
>>>>>>> But the problem is that invalidation doesn’t release the layer.
>>>>>>
>>>>>> Yes, that's right the surface and layer are bound to each other(and
>>>>>> the layer can have more than one surface). So I do not see a reason
>>>>>> why we should break the link between them, which causes the surface
>>>>>> to live more time than its layer. I guess the right things to do is
>>>>>> to fix the "gc root", since we have no cycles here.
>>>>>>
>>>>>>>
>>>>>>> So, again the question is: should the layer be nullified on
>>>>>>> invalidation or it should be made a weak ref? For me this seems
>>>>>>> quite logical to release resources on disposal/invalidation, what
>>>>>>> do you think?
>>>>>>>
>>>>>>> As to the fixing the issue globally, I don’t have enough
>>>>>>> understanding of the pipe design so that to do that properly. For
>>>>>>> instance, as I wrote before, I don’t know under which conditions
>>>>>>> the context should/may be disposed…
>>>>>>>
>>>>>>> May be someone else can advice on it.
>>>>>>
>>>>>> I can take a look, but are you sure that the test "WindowsLeak.java"
>>>>>> reproduce exactly your problem?
>>>>>>
>>>>>>>> Note that such changes are 2d related code and should be reviewed
>>>>>>>> on 2d-dev.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Assigned the peer/surfaceData to null in CGLayer can causes an
>>>>>>>>>> NPE in all its usage, because there is no any synchronization
>>>>>>>>>> which will prevent the usage of CGLayer after disposing.
>>>>>>>>>
>>>>>>>>> That’s bad. Will wrapping the refs with AtomicReference help?
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Unrelated to the fix, but it seems we should call flush() on
>>>>>>>>>> surface when the layer is disposed, at least I do not understand
>>>>>>>>>> where we flush the native ogl data for the latest surface data.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This will trigger CGLLayerSurfaceData.invalidate(), but the
>>>>>>>>> “layer” will still not be nullified. What about nullifying it in
>>>>>>>>> invalidate()? Will we face the same synchronisation issue?
>>>>>>>>>
>>>>>>>>> Anton.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 29.04.16 15:00, Anton Tarasov wrote:
>>>>>>>>>>> Hi Sergey, Alexander,
>>>>>>>>>>>
>>>>>>>>>>> Please review the fix:
>>>>>>>>>>>
>>>>>>>>>>> bug: JDK-8028486 [TEST_BUG] [macosx]
>>>>>>>>>>> java/awt/Window/WindowsLeak/WindowsLeak.java fails
>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~ant/JDK-8028486/webrev.0 
>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eant/JDK-8028486/webrev.0>
>>>>>>>>>>>
>>>>>>>>>>> I’m copying my comment from CR:
>>>>>>>>>>>
>>>>>>>>>>> Please open the attached screenshot [*], made with YourKit,
>>>>>>>>>>> where a chain of links is shown from the GC roots.
>>>>>>>>>>> The frame is held by its peer which is held by CGLLayer which
>>>>>>>>>>> is held as validatedSrcData in the GL context.
>>>>>>>>>>> The point is that the GL context doesn't cleanup the last state
>>>>>>>>>>> until under some conditions, which are not applicable to this
>>>>>>>>>>> scenario.
>>>>>>>>>>> I'm not sure should the cleanup be triggered here or not, but
>>>>>>>>>>> the problem can be solved otherwise.
>>>>>>>>>>>
>>>>>>>>>>> The point is that in the chain the CGLLayer instance has been
>>>>>>>>>>> disposed, in response to the frame disposal.
>>>>>>>>>>> So, this is the only ref that holds it (the JNI ref is released
>>>>>>>>>>> by the native peer on disposal).
>>>>>>>>>>> Thus, as the layer is disposed it can at least zero all the
>>>>>>>>>>> java refs it holds (this change already fixes the problem).
>>>>>>>>>>> Then, the "layer" ref in CGLLayerSurfaceData should probably be
>>>>>>>>>>> made weak.
>>>>>>>>>>>
>>>>>>>>>>> [*]
>>>>>>>>>>> https://bugs.openjdk.java.net/secure/attachment/59121/8028486.png
>>>>>>>>>>>
>>>>>>>>>>> As to the “weak ref” mentioned in the comment. I didn’t do
>>>>>>>>>>> that, but if you find it reasonable I can add that change (or
>>>>>>>>>>> file a separate CR).
>>>>>>>>>>>
>>>>>>>>>>> Also, the fix contains some additional cleanup (not related to
>>>>>>>>>>> this CR): two more JNI local refs leak, fixed.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Anton.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Best regards, Sergey.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Best regards, Sergey.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Best regards, Sergey.
>>>>>
>>>>
>>>>
>>
>>
>> -- 
>> Best regards, Sergey.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20160527/b0024ca4/attachment.html>