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

Thu May 12 11:59:47 UTC 2016

Hi Sergey,

I’m fine with the fix.

> On 12 May 2016, at 14:43, Sergey Bylokhov <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 <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> 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> 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
>>>>>>>>>> 
>>>>>>>>>> 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/20160512/84b658b9/attachment.html>