<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hello Anthony,<br>
    <br>
    Could you please review the third version of the fix containing
    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>
    <ol>
      <li>A comment about the place of invocation of the method
        "XErrorHandlerUtil.init" was added to a documentation block of
        the method.</li>
      <li>A code related to XShmAttach function common to the files
        "src/solaris/native/sun/awt/awt_GraphicsEnv.c" and
        "src/solaris/native/sun/java2d/x11/X11SurfaceData.c" was
        extracted into a separate function "TryXShmAttach" declared in
        "src/solaris/native/sun/awt/awt_GraphicsEnv.h" file.</li>
      <li>All JNI code related to X error handling was implemented as
        corresponding macros defined in
        "src/solaris/native/sun/awt/awt_util.h" file.</li>
    </ol>
    Thank you,<br>
    Anton<br>
    <br>
    <div class="moz-cite-prefix">On 1/31/2013 7:42 PM, Anton Litvinov
      wrote:<br>
    </div>
    <blockquote cite="mid:510A90DF.2060501@oracle.com" type="cite">Hello
      Anthony,
      <br>
      <br>
      Thank you for the review and these remarks. Surely, the comment
      will be added. I think that all JNI code related to XShmAttach can
      be definitely transferred into a separate dedicated function,
      which will be declared in
      "src/solaris/native/sun/awt/awt_GraphicsEnv.h" file. I will try to
      wrap all JNU calls connected with XErrorHandler into the
      particular "WITH_XERROR_HANDLER", "RESTORE_XERROR_HANDLER"
      functions 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 itself. This looks confusing. Please add a comment stating
        that this method is invoked from native code, and from where
        exactly.
        <br>
        <br>
        <br>
        2. Interesting that we use this machinery to call the
        XShmAttach() from native code twice, and the code looks quite
        similar in each case. Would it be possible to extract the common
        code in a separate function (a-la BOOL TryXShmAttach(...)) to
        avoid code replication? There are other usages as well, so we
        could also introduce a macro (such as the old
        EXEC_WITH_XERROR_HANDLER but now with other arguments) that
        would minimize all the JNU_ calls required to use 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 based on an idea of reusing the existing AWT native global
          error 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
          initializing
          <br>
                java.awt.GraphicsEnvironment singleton. Such dependency
          is 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 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 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 easiest way to fix this is to
            install the existing J2DXErrHandler() only once. That is, it
            is the second option listed by you. Of course, the
            J2DXErrHandler needs to be updated as well to detect whether
            2D code wants to use it at the moment or it must simply
            delegate to the previous handler (i.e. where the code
            currently installs/uninstalls the handler, it must instead
            set a global boolean flag or something.)
            <br>
            <br>
            While the first option (reusing the existing AWT machinery)
            is an interesting idea in general, I think it is complex and
            would require too much additional testing and bring an
            unjustified risk 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 a solution. Elimination of the logic, which
              sets/unsets J2DXErrHandler() for each call
              "XShmAttach(awt_display, &shminfo))" should
              effectively resolve the issue, but only in case if all
              other native error handlers, which were set by the system
              function "XSetErrorHandler()" in JDK or in any external
              library, observe the rule of relaying of all events, which
              are not relative to them, to the previously saved error
              handlers. Otherwise an error generated during "XShmAttach"
              function call will not be handled by J2DXErrHandler().
              <br>
              <br>
              Could you answer the following question. By setting
              J2DXErrHandler() only once and forever do you mean usage
              of AWT global event handler "static int
              ToolkitErrorHandler(Display * dpy, XErrorEvent * event)"
              from "src/solaris/native/sun/xawt/XlibWrapper.c" with Java
              synthetic handlers or creation of another global native
              error handler with 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 handler in the first place as
                well.  Since it is only ever used on errors I also think
                it is OK to set it once and leave it there 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 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
                    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 state. We
                  <br>
                  don't expect to replace our own error handler (and it
                  shouldn't have
                  <br>
                  been there in the first place).
                  <br>
                  <br>
                  I realize that the fix you propose works around this
                  problem. 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 believe
                  <br>
                  that we should somehow share this machinery with the
                  2D code to avoid
                  <br>
                  this sort of problems. Though I'm not sure if this
                  will 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 for
                    <br>
                    the reason of an infinite recursion in AWT native
                    function
                    <br>
                    J2DXErrHandler() under certain circumstances on
                    32-bit Linux OS. The
                    <br>
                    fix is based on introduction of the logic, which
                    detects indirect
                    <br>
                    recursive calls to J2DXErrHandler() by means of a
                    simple 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 recipients
                    <br>
                    of this letter, because the edited function's name
                    is related to Java
                    <br>
                    2D area of JDK, despite of the fact that the edited
                    file is located in
                    <br>
                    AWT directory.
                    <br>
                    <br>
                    Thank you,
                    <br>
                    Anton
                    <br>
                  </blockquote>
                </blockquote>
              </blockquote>
            </blockquote>
          </blockquote>
        </blockquote>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>