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

[Philip Race]

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