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