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

[Sergey Bylokhov]

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