<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hello Artem,<br>
    <br>
    Thank you very much for the review of this fix. My responses to your
    questions are provided below in the same order, which you defined.<br>
    <ol>
      <li>I think that "XErrorHandlerUtil.saved_error" field can surely
        be marked as private, but in this case the corresponding
        "XErrorHandlerUtil.getSavedError" method will be necessary,
        because this field is actively accessed from other classes which
        set a certain instance of XErrorHandler. For example
        "MotifDnDDropTargetProtocol.java", "XDragSourceProtocol.java"
        and a few other classes edited in this fix.</li>
      <li>Yes, I completely agree that "XErrorHandlerUtil.getDisplay()"
        is reduntant. This method will be eliminated.</li>
    </ol>
    Thank you,<br>
    Anton<br>
    <br>
    <div class="moz-cite-prefix">On 2/18/2013 3:16 PM, Artem Ananiev
      wrote:<br>
    </div>
    <blockquote cite="mid:51220DA4.7040608@oracle.com" type="cite">Hi,
      Anton,
      <br>
      <br>
      a few minor comments:
      <br>
      <br>
      1. XErrorHandlerUtil: can saved_error be private instead of
      package protected?
      <br>
      <br>
      2. XErrorHandlerUtil.getDisplay() seems to be redundant.
      <br>
      <br>
      In general, the fix looks perfectly fine to me. Please, wait for
      comments from Java2D team, though.
      <br>
      <br>
      Thanks,
      <br>
      <br>
      Artem
      <br>
      <br>
      On 2/13/2013 8:57 PM, Anton Litvinov wrote:
      <br>
      <blockquote type="cite">Hello Anthony,
        <br>
        <br>
        Could you please review the third version of the fix containing
        <br>
        modifications discussed with you in the previous letter.
        <br>
        <br>
        Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~alitvinov/8005607/webrev.02">http://cr.openjdk.java.net/~alitvinov/8005607/webrev.02</a>
        <br>
        <br>
        This version of the fix differs from the previous in the
        following places:
        <br>
        <br>
         1. A comment about the place of invocation of the method
        <br>
            "XErrorHandlerUtil.init" was added to a documentation block
        of the
        <br>
            method.
        <br>
         2. A code related to XShmAttach function common to the files
        <br>
            "src/solaris/native/sun/awt/awt_GraphicsEnv.c" and
        <br>
            "src/solaris/native/sun/java2d/x11/X11SurfaceData.c" was
        extracted
        <br>
            into a separate function "TryXShmAttach" declared in
        <br>
            "src/solaris/native/sun/awt/awt_GraphicsEnv.h" file.
        <br>
         3. All JNI code related to X error handling was implemented as
        <br>
            corresponding macros defined in
        <br>
            "src/solaris/native/sun/awt/awt_util.h" file.
        <br>
        <br>
        Thank you,
        <br>
        Anton
        <br>
        <br>
        On 1/31/2013 7:42 PM, Anton Litvinov wrote:
        <br>
        <blockquote type="cite">Hello Anthony,
          <br>
          <br>
          Thank you for the review and these remarks. Surely, the
          comment will
          <br>
          be added. I think that all JNI code related to XShmAttach can
          be
          <br>
          definitely transferred into a separate dedicated function,
          which will
          <br>
          be declared in "src/solaris/native/sun/awt/awt_GraphicsEnv.h"
          file. I
          <br>
          will try to wrap all JNU calls connected with XErrorHandler
          into the
          <br>
          particular "WITH_XERROR_HANDLER", "RESTORE_XERROR_HANDLER"
          functions
          <br>
          or macros.
          <br>
          <br>
          Thank you,
          <br>
          Anton
          <br>
          <br>
          On 1/31/2013 4:57 PM, Anthony Petrov wrote:
          <br>
          <blockquote type="cite">Hi Anton,
            <br>
            <br>
            A couple comments:
            <br>
            <br>
            1. src/solaris/classes/sun/awt/X11/XErrorHandlerUtil.java
            <br>
            <blockquote type="cite">  80     private static void
              init(long display) {
              <br>
            </blockquote>
            <br>
            This method is private and isn't called from anywhere in
            this class
            <br>
            itself. This looks confusing. Please add a comment stating
            that this
            <br>
            method is invoked from native code, and from where exactly.
            <br>
            <br>
            <br>
            2. Interesting that we use this machinery to call the
            XShmAttach()
            <br>
            from native code twice, and the code looks quite similar in
            each
            <br>
            case. Would it be possible to extract the common code in a
            separate
            <br>
            function (a-la BOOL TryXShmAttach(...)) to avoid code
            replication?
            <br>
            There are other usages as well, so we could also introduce a
            macro
            <br>
            (such as the old EXEC_WITH_XERROR_HANDLER but now with other
            <br>
            arguments) that would minimize all the JNU_ calls required
            to use
            <br>
            this machinery.
            <br>
            <br>
            <br>
            Otherwise the fix looks great.
            <br>
            <br>
            --
            <br>
            best regards,
            <br>
            Anthony
            <br>
            <br>
            On 1/30/2013 20:14, Anton Litvinov wrote:
            <br>
            <blockquote type="cite">  Hello Anthony,
              <br>
              <br>
              Could you, please, review a second version of the fix,
              which is
              <br>
              based on an idea of reusing the existing AWT native global
              error
              <br>
              handler from Java 2D native code.
              <br>
              <br>
              Webrev:
              <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~alitvinov/8005607/webrev.01">http://cr.openjdk.java.net/~alitvinov/8005607/webrev.01</a>
              <br>
              <br>
              The fix consists of the following parts:
              <br>
              <br>
                 1. Migration of all X error handling code from XToolkit
              to a new
              <br>
                    XErrorHandlerUtil class for resolution of
              interdependency between
              <br>
                    a static initialization block of XToolkit and a
              block
              <br>
              initializing
              <br>
                    java.awt.GraphicsEnvironment singleton. Such
              dependency is
              <br>
              created
              <br>
                    by new calls to XToolkit static methods from
              <br>
                    "src/solaris/native/sun/awt/awt_GraphicsEnv.c",
              <br>
                    "src/solaris/native/sun/java2d/x11/X11SurfaceData.c"
              files.
              <br>
                 2. Substitution of XToolkit.WITH_XERROR_HANDLER,
              <br>
                    XToolkit.RESTORE_XERROR_HANDLER ... for
              corresponding methods,
              <br>
                    fields of XErrorHandlerUtil class in all places of
              JDK source
              <br>
                    code, where they were used.
              <br>
                 3. Substitution of all found native X error handlers
              which are
              <br>
              set in
              <br>
                    native code (awt_GraphicsEnv.c, X11SurfaceData.c,
              <br>
                    GLXSurfaceData.c) for new synthetic Java error
              handlers.
              <br>
                 4. Removal of X error handling code used by the native
              error
              <br>
              handlers
              <br>
                    from "solaris/native/sun/awt/awt_util.c"
              <br>
                    "solaris/native/sun/awt/awt_util.h" files.
              <br>
              <br>
              Thank you,
              <br>
              Anton
              <br>
              <br>
              On 1/11/2013 3:45 PM, Anthony Petrov wrote:
              <br>
              <blockquote type="cite">I'm not Jim, but as I indicated
                earlier my opinion is that the
                <br>
                easiest way to fix this is to install the existing
                J2DXErrHandler()
                <br>
                only once. That is, it is the second option listed by
                you. Of
                <br>
                course, the J2DXErrHandler needs to be updated as well
                to detect
                <br>
                whether 2D code wants to use it at the moment or it must
                simply
                <br>
                delegate to the previous handler (i.e. where the code
                currently
                <br>
                installs/uninstalls the handler, it must instead set a
                global
                <br>
                boolean flag or something.)
                <br>
                <br>
                While the first option (reusing the existing AWT
                machinery) is an
                <br>
                interesting idea in general, I think it is complex and
                would
                <br>
                require too much additional testing and bring an
                unjustified risk
                <br>
                to the solution for such a basic problem.
                <br>
                <br>
                --
                <br>
                best regards,
                <br>
                Anthony
                <br>
                <br>
                On 1/11/2013 14:44, Anton Litvinov wrote:
                <br>
                <blockquote type="cite">Hello Jim,
                  <br>
                  <br>
                  Thank you very much for the review and provision of a
                  new idea of
                  <br>
                  a solution. Elimination of the logic, which
                  sets/unsets
                  <br>
                  J2DXErrHandler() for each call
                  "XShmAttach(awt_display,
                  <br>
                  &shminfo))" should effectively resolve the issue,
                  but only in case
                  <br>
                  if all other native error handlers, which were set by
                  the system
                  <br>
                  function "XSetErrorHandler()" in JDK or in any
                  external library,
                  <br>
                  observe the rule of relaying of all events, which are
                  not relative
                  <br>
                  to them, to the previously saved error handlers.
                  Otherwise an
                  <br>
                  error generated during "XShmAttach" function call will
                  not be
                  <br>
                  handled by J2DXErrHandler().
                  <br>
                  <br>
                  Could you answer the following question. By setting
                  <br>
                  J2DXErrHandler() only once and forever do you mean
                  usage of AWT
                  <br>
                  global event handler "static int
                  ToolkitErrorHandler(Display *
                  <br>
                  dpy, XErrorEvent * event)" from
                  <br>
                  "src/solaris/native/sun/xawt/XlibWrapper.c" with Java
                  synthetic
                  <br>
                  handlers or creation of another global native error
                  handler with
                  <br>
                  J2DXErrHandler as native synthetic handler?
                  <br>
                  <br>
                  Thank you,
                  <br>
                  Anton
                  <br>
                  <br>
                  On 1/10/2013 5:44 AM, Jim Graham wrote:
                  <br>
                  <blockquote type="cite">I think I'd rather see some
                    way to prevent double-adding the
                    <br>
                    handler in the first place as well.  Since it is
                    only ever used
                    <br>
                    on errors I also think it is OK to set it once and
                    leave it there
                    <br>
                    forever...
                    <br>
                    <br>
                                ...jim
                    <br>
                    <br>
                    On 1/9/13 8:08 AM, Anthony Petrov wrote:
                    <br>
                    <blockquote type="cite">Hi Anton et al.,
                      <br>
                      <br>
                      If I read the description of the bug correctly,
                      specifically
                      <br>
                      this part:
                      <br>
                      <br>
                      <blockquote type="cite">The problem occurs, if
                        another thread (for example, GTK thread) is
                        <br>
                        doing the same sort of thing concurrently. This
                        can lead to the
                        <br>
                        following situation.
                        <br>
                         JVM thread: Sets J2DXErrHandler(), saves
                        ANY_PREVIOUS_HANDLER as
                        <br>
                        previous      GTK thread: Sets some GTK_HANDLER,
                        saves
                        <br>
                        J2DXErrHandler() as previous  JVM thread:
                        Restores
                        <br>
                        ANY_PREVIOUS_HANDLER      GTK thread: Restores
                        <br>
                        J2DXErrHandler()  JVM
                        <br>
                        thread: Sets J2DXErrHandler(), saves
                        J2DXErrHandler() as previous
                        <br>
                      </blockquote>
                      <br>
                      It is obvious that at this final step 2D is in an
                      inconsistent
                      <br>
                      state. We
                      <br>
                      don't expect to replace our own error handler (and
                      it shouldn't
                      <br>
                      have
                      <br>
                      been there in the first place).
                      <br>
                      <br>
                      I realize that the fix you propose works around
                      this problem.
                      <br>
                      But this
                      <br>
                      doesn't look like an ideal solution to me.
                      <br>
                      <br>
                      BTW, IIRC, in JDK7 (and 6?) we decided to set the
                      actual X11 error
                      <br>
                      handler only once and never replace it. All the
                      rest of the
                      <br>
                      push_handler/pop_handler logic is now located in
                      Java code (see
                      <br>
                      XToolkit.SAVED_ERROR_HANDLER() and the surrounding
                      logic). I
                      <br>
                      believe
                      <br>
                      that we should somehow share this machinery with
                      the 2D code to
                      <br>
                      avoid
                      <br>
                      this sort of problems. Though I'm not sure if this
                      will
                      <br>
                      eliminate this
                      <br>
                      exact issue.
                      <br>
                      <br>
                      <br>
                      2D/AWT folks: any other thoughts?
                      <br>
                      <br>
                      --
                      <br>
                      best regards,
                      <br>
                      Anthony
                      <br>
                      <br>
                      On 12/29/2012 17:44, Anton Litvinov wrote:
                      <br>
                      <blockquote type="cite">Hello,
                        <br>
                        <br>
                        Please review the following fix for a bug.
                        <br>
                        <br>
                        Bug:
                        <a class="moz-txt-link-freetext" href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005607">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005607</a>
                        <br>
                        <a class="moz-txt-link-freetext" href="https://jbs.oracle.com/bugs/browse/JDK-8005607">https://jbs.oracle.com/bugs/browse/JDK-8005607</a>
                        <br>
                        Webrev:
                        <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~alitvinov/8005607/webrev.00">http://cr.openjdk.java.net/~alitvinov/8005607/webrev.00</a>
                        <br>
                        <br>
                        The bug consists in a crash which is caused by a
                        stack overflow
                        <br>
                        for
                        <br>
                        the reason of an infinite recursion in AWT
                        native function
                        <br>
                        J2DXErrHandler() under certain circumstances on
                        32-bit Linux
                        <br>
                        OS. The
                        <br>
                        fix is based on introduction of the logic, which
                        detects indirect
                        <br>
                        recursive calls to J2DXErrHandler() by means of
                        a simple
                        <br>
                        counter, to
                        <br>
                        J2DXErrHandler() native function. Such a
                        solution requires minimum
                        <br>
                        code changes, does not alter the handler's code
                        significantly and
                        <br>
                        eliminates this bug.
                        <br>
                        <br>
                        Adding <a class="moz-txt-link-abbreviated" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a> e-mail alias to
                        the list of
                        <br>
                        recipients
                        <br>
                        of this letter, because the edited function's
                        name is related
                        <br>
                        to Java
                        <br>
                        2D area of JDK, despite of the fact that the
                        edited file is
                        <br>
                        located in
                        <br>
                        AWT directory.
                        <br>
                        <br>
                        Thank you,
                        <br>
                        Anton
                        <br>
                      </blockquote>
                    </blockquote>
                  </blockquote>
                </blockquote>
              </blockquote>
            </blockquote>
          </blockquote>
        </blockquote>
        <br>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>