> On 04 May 2016, at 17:08, Sergey Bylokhov <sergey.bylok...@oracle.com> 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).
No, I don’t have a standalone test. The linking chain for the CGE ref is here: https://i.imgur.com/hhrdjln.png <https://i.imgur.com/hhrdjln.png> (the one I posted before didn’t contain "JNI Local” ref because that JDK build had the proposed fix in) I’m not sure if CGE can be recreated, but if it can, this is a potential leak. For the second one (CPlatformWindow), unfortunately I couldn’t find the memory dump that contained it, from which I tracked it down. It wasn’t me who reproduced the leak, I had only the dump in my disposal (and I didn’t save it because the leak seemed obvious to me). But if you search for "jobject platformWindow = [self.javaPlatformWindow jObjectWithEnv:env]” in the code, you’ll find that with its every usage platformWindow gets deleted (because it’s a temp JNI local) except for the one occurance which I fixed. Anyway, both the JNI locals don’t seem to be left intentionally, but look like a simple misses. Unfortunately (as I wrote during the “CAccessible fix" discussion), PushLocalFrame/PopLocalFrame doesn’t take care of the locals in all the cases. So, a missed JNI object can be a potential leak. Regards Anton. > > >>> On 29 Apr 2016, at 18:19, Sergey Bylokhov <sergey.bylok...@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.bylok...@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.
