<html>
  <head>
    <meta content="text/html; charset=windows-1252"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Looks ok.<br>
    <br>
    Regards<br>
    Prasanta<br>
    <div class="moz-cite-prefix">On 3/29/2016 2:57 PM, Jayathirth D V
      wrote:<br>
    </div>
    <blockquote cite="mid:c0d2fabe-b59d-413e-b8b2-d7468f9015b1@default"
      type="cite">
      <meta http-equiv="Content-Type" content="text/html;
        charset=windows-1252">
      <meta name="Generator" content="Microsoft Word 15 (filtered
        medium)">
      <style><!--
/* Font Definitions */
@font-face
        {font-family:"Cambria Math";
        panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
        {font-family:Calibri;
        panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
        {font-family:Consolas;
        panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:12.0pt;
        font-family:"Times New Roman",serif;
        color:black;}
a:link, span.MsoHyperlink
        {mso-style-priority:99;
        color:blue;
        text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
        {mso-style-priority:99;
        color:purple;
        text-decoration:underline;}
pre
        {mso-style-priority:99;
        mso-style-link:"HTML Preformatted Char";
        margin:0in;
        margin-bottom:.0001pt;
        font-size:10.0pt;
        font-family:"Courier New";
        color:black;}
span.HTMLPreformattedChar
        {mso-style-name:"HTML Preformatted Char";
        mso-style-priority:99;
        mso-style-link:"HTML Preformatted";
        font-family:Consolas;
        color:black;}
span.changed
        {mso-style-name:changed;}
span.EmailStyle20
        {mso-style-type:personal;
        font-family:"Calibri",sans-serif;
        color:#1F497D;}
span.EmailStyle21
        {mso-style-type:personal;
        font-family:"Calibri",sans-serif;
        color:#1F497D;}
span.EmailStyle22
        {mso-style-type:personal-reply;
        font-family:"Calibri",sans-serif;
        color:#1F497D;}
.MsoChpDefault
        {mso-style-type:export-only;
        font-size:10.0pt;}
@page WordSection1
        {size:8.5in 11.0in;
        margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
        {page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
      <div class="WordSection1">
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hi
            Prasanta,<o:p></o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Actually
            returns null in test description is for
            createImageXXXStream() and not for read() and write()
            functions. But for more clarity I have added why we catch
            IOException also in test description. <o:p></o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Please
            review the updated webrev:<o:p></o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><a
              moz-do-not-send="true"
              href="http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.05/"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jdv/8044289/webrev.05/">http://cr.openjdk.java.net/~jdv/8044289/webrev.05/</a></a>
            <o:p></o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks,<o:p></o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Jay<o:p></o:p></span></p>
        <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
        <div>
          <div style="border:none;border-top:solid #E1E1E1
            1.0pt;padding:3.0pt 0in 0in 0in">
            <p class="MsoNormal"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">
                prasanta sadhukhan <br>
                <b>Sent:</b> Tuesday, March 29, 2016 2:21 PM<br>
                <b>To:</b> Jayathirth D V<br>
                <b>Cc:</b> Philip Race; <a class="moz-txt-link-abbreviated" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><br>
                <b>Subject:</b> Re: [OpenJDK 2D-Dev] Review Request for
                JDK-8044289 : In ImageIO.read() and write() NPE is not
                handled properly for stream<o:p></o:p></span></p>
          </div>
        </div>
        <p class="MsoNormal"><o:p> </o:p></p>
        <p class="MsoNormal">+1.<br>
          Looks ok to me. Only thing is that whether the test summary is
          appropriate "<o:p></o:p></p>
        <pre>Test verifies that when createImageInputStream() or<o:p></o:p></pre>
        <pre>  28  *          createImageOutputStream() returns null while read or write,"<o:p></o:p></pre>
        <pre>Should it not be "verifies whether ...throws appropriate exception"?<o:p></o:p></pre>
        <pre><o:p> </o:p></pre>
        <pre>Regards<o:p></o:p></pre>
        <pre>Prasanta<o:p></o:p></pre>
        <div>
          <p class="MsoNormal">On 3/29/2016 3:42 AM, Philip Race wrote:<o:p></o:p></p>
        </div>
        <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
          <p class="MsoNormal">This all looks fine to me. Although no
            new Exception types are thrown<br>
            I think we need a CCC anyway, so please prepare and get that
            approved<br>
            before pushing.<br>
            <br>
            -phil.<br>
            <br>
            On 3/28/16, 3:30 AM, Jayathirth D V wrote: <o:p></o:p></p>
          <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hi
                Phil,</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">After
                discussion with Sergey we have come to conclusion that
                Sergey is okay with throwing exception or returning
                null/false.</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">So
                I have modified the webrev to throw exception.</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Also
                in write() I have modified the catch block to just throw
                the caught exception without adding message since it
                will create confusion between the case where we are not
                able to create cache and when createImageOutputStream()
                returns null.</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Regarding
                catch block of createImageOutputStream() only when we
                usecache is true we use FileCacheImageOutputStream() and
                it throws IOException so I think there is no need to add
                extra check before throwing IOException.</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Please
                find updated webrev for review :</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><a
                  moz-do-not-send="true"
                  href="http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.04/"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jdv/8044289/webrev.04/">http://cr.openjdk.java.net/~jdv/8044289/webrev.04/</a></a></span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks,</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Jay</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> </span><o:p></o:p></p>
            <div>
              <div style="border:none;border-top:solid #E1E1E1
                1.0pt;padding:3.0pt 0in 0in 0in">
                <p class="MsoNormal"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">
                    Jayathirth D V <br>
                    <b>Sent:</b> Thursday, March 24, 2016 3:52 PM<br>
                    <b>To:</b> Philip Race<br>
                    <b>Cc:</b> Sergey Bylokhov; <a
                      moz-do-not-send="true"
                      href="mailto:2d-dev@openjdk.java.net"><a class="moz-txt-link-abbreviated" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a></a><br>
                    <b>Subject:</b> RE: [OpenJDK 2D-Dev] Review Request
                    for JDK-8044289 : In ImageIO.read() and write() NPE
                    is not handled properly for stream</span><o:p></o:p></p>
              </div>
            </div>
            <p class="MsoNormal"> <o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Hi
                Phil,</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">We
                can change the code to something like :</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span><o:p></o:p></p>
            <pre><span class="changed"><span style="color:windowtext">1522      * @return <code>false</code> if no appropriate writer is found or</span></span><o:p></o:p></pre>
            <pre><span class="changed"><span style="color:windowtext">1523      * when needed stream is null.</span></span><o:p></o:p></pre>
            <p class="MsoNormal"><span
                style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
            <pre><span style="color:windowtext">1545         try {</span><o:p></o:p></pre>
            <pre><span style="color:windowtext">1546             output.delete();</span><o:p></o:p></pre>
            <pre><span style="color:windowtext">1547             stream = createImageOutputStream(output);</span><o:p></o:p></pre>
            <pre><span style="color:windowtext">1548         } catch (IOException e) {</span><o:p></o:p></pre>
            <pre><span style="color:windowtext">1549             <b>throw e;</b></span><o:p></o:p></pre>
            <pre><span style="color:windowtext">1550         }</span><o:p></o:p></pre>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">In
                this case we will be throwing IOException when we are
                not able to create cache file and stream is null.</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">And
                return null/false when createImageXXXStream returns
                null. Also we can add check before throwing IIOException
                from createImageXXXStream for usecache.</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span><o:p></o:p></p>
            <pre><span style="color:windowtext">350 </span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 351         boolean usecache = getUseCache() && hasCachePermission();</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 352 </span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 353         while (iter.hasNext()) {</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 354             ImageInputStreamSpi spi = iter.next();</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 355             if (spi.getInputClass().isInstance(input)) {</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 356                 try {</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 357                     return spi.createInputStreamInstance(input,</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 358                                                          usecache,</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 359                                                          getCacheDirectory());</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 360                 } catch (IOException e) {</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 361                     <b>if (usecache)</b></span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 362                         throw new IIOException("Can't create cache file!", e);</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 363                 }</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 364             }</span><o:p></o:p></pre>
            <pre><span style="color:windowtext"> 365         }</span><o:p></o:p></pre>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">This
                will be one of the approach.</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Or
                we can throw IOException in all the cases with
                additional check in createImageXXXStream for usecache.
                This will be like throwing IOException when
                createImageXXXStream returns null/false or it throws
                IIOException when it cant create cache file. This will
                be another approach. Please let us know your inputs.</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Thanks,</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Jay</span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> </span><o:p></o:p></p>
            <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> </span><o:p></o:p></p>
            <div>
              <div style="border:none;border-top:solid #E1E1E1
                1.0pt;padding:3.0pt 0in 0in 0in">
                <p class="MsoNormal"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">
                    Philip Race <br>
                    <b>Sent:</b> Thursday, March 24, 2016 5:10 AM<br>
                    <b>To:</b> Jayathirth D V<br>
                    <b>Cc:</b> Sergey Bylokhov; <a
                      moz-do-not-send="true"
                      href="mailto:2d-dev@openjdk.java.net"><a class="moz-txt-link-abbreviated" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a></a><br>
                    <b>Subject:</b> Re: [OpenJDK 2D-Dev] Review Request
                    for JDK-8044289 : In ImageIO.read() and write() NPE
                    is not handled properly for stream</span><o:p></o:p></p>
              </div>
            </div>
            <p class="MsoNormal"> <o:p></o:p></p>
            <p class="MsoNormal">I don't think this is ready and we need
              to discuss whether to rework it.<o:p></o:p></p>
            <pre>In general I think I prefer IIO rather than null when we have a stream problem.<o:p></o:p></pre>
            <p class="MsoNormal" style="margin-bottom:12.0pt"><br>
              For one thing you have this change :-<o:p></o:p></p>
            <pre><span class="changed">1522      * @return <code>false</code> if no appropriate writer is found or</span><o:p></o:p></pre>
            <pre><span class="changed">1523      * not able to create required ImageOutputStream.</span><o:p></o:p></pre>
            <pre><span class="changed"> </span><o:p></o:p></pre>
            <pre><span class="changed">yet the implementation says :-</span><o:p></o:p></pre>
            <pre>1545         try {<o:p></o:p></pre>
            <pre>1546             output.delete();<o:p></o:p></pre>
            <pre>1547             stream = createImageOutputStream(output);<o:p></o:p></pre>
            <pre>1548         } catch (IOException e) {<o:p></o:p></pre>
            <pre>1549             throw new IIOException("Can't create output stream!", e);<o:p></o:p></pre>
            <pre>1550         }<o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <pre>So whilst "null" would mean we cannot find an IOS SPI it isn't<o:p></o:p></pre>
            <pre>the only reason we fail to create the IOS<o:p></o:p></pre>
            <pre>A null return seems to get passed through to doWrite()  which probably throws IIOE<o:p></o:p></pre>
            <pre>And I think that the implementation is actually right here where<o:p></o:p></pre>
            <pre>it throws an exception.<o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <pre>The ability to create an image input or output stream could be because<o:p></o:p></pre>
            <pre>of some kind of problem opening the file, or with the IIS or IOS implementations -<o:p></o:p></pre>
            <pre>like they were de-registered. I think in many if not all of these cases IOE - or IIOE is<o:p></o:p></pre>
            <pre>what we want.<o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <pre>Also since we call createImageInputStream() - which is public - we<o:p></o:p></pre>
            <pre>need to look at that, and what it does, which is also something to consider.<o:p></o:p></pre>
            <pre>If it returns null, that seems to mean that no suitable SPI is registered<o:p></o:p></pre>
            <pre>which can only happen if they are de-registered. For the methods where<o:p></o:p></pre>
            <pre>we use  createImageInputStream() I think it is fair to turn that into IIOE<o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <pre>BTW I note that the code there says :-<o:p></o:p></pre>
            <pre> 350 <o:p></o:p></pre>
            <pre> 351         boolean usecache = getUseCache() && hasCachePermission();<o:p></o:p></pre>
            <pre> 352 <o:p></o:p></pre>
            <pre> 353         while (iter.hasNext()) {<o:p></o:p></pre>
            <pre> 354             ImageInputStreamSpi spi = iter.next();<o:p></o:p></pre>
            <pre> 355             if (spi.getInputClass().isInstance(input)) {<o:p></o:p></pre>
            <pre> 356                 try {<o:p></o:p></pre>
            <pre> 357                     return spi.createInputStreamInstance(input,<o:p></o:p></pre>
            <pre> 358                                                          usecache,<o:p></o:p></pre>
            <pre> 359                                                          getCacheDirectory());<o:p></o:p></pre>
            <pre> 360                 } catch (IOException e) {<o:p></o:p></pre>
            <pre> 361                     throw new IIOException("Can't create cache file!", e);<o:p></o:p></pre>
            <pre> 362                 }<o:p></o:p></pre>
            <pre> 363             }<o:p></o:p></pre>
            <pre> 364         }<o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <pre>So far I can see that means it will claim any Exception that is generated is because it<o:p></o:p></pre>
            <pre>could not create the cache file without any proof of that and even if "useCache" is false.<o:p></o:p></pre>
            <pre>It seems to imply to me that the author was not considering things like someone<o:p></o:p></pre>
            <pre>passing a closed InputStream .. probably we ought to be more vague in this case<o:p></o:p></pre>
            <pre>or look more deeply at what we can tell from whether it is IIOE or IOE.<o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <pre>-phil.<o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <pre> <o:p></o:p></pre>
            <p class="MsoNormal">On 3/22/16, 10:49 AM, Jayathirth D V
              wrote: <o:p></o:p></p>
            <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
              <pre>Thanks for the review Sergey.<o:p></o:p></pre>
              <pre>Can I get +1 for this please?<o:p></o:p></pre>
              <pre> <o:p></o:p></pre>
              <pre>-Jay<o:p></o:p></pre>
              <pre> <o:p></o:p></pre>
              <pre>-----Original Message-----<o:p></o:p></pre>
              <pre>From: Sergey Bylokhov <o:p></o:p></pre>
              <pre>Sent: Tuesday, March 22, 2016 9:52 PM<o:p></o:p></pre>
              <pre>To: Jayathirth D V; Philip Race<o:p></o:p></pre>
              <pre>Cc: <a moz-do-not-send="true" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><o:p></o:p></pre>
              <pre>Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In ImageIO.read() and write() NPE is not handled properly for stream<o:p></o:p></pre>
              <pre> <o:p></o:p></pre>
              <pre>This fix looks fine to me.<o:p></o:p></pre>
              <pre>At least it made all code work in a similar way. But probably someone will have other opinion?<o:p></o:p></pre>
              <pre> <o:p></o:p></pre>
              <pre>On 22.03.16 12:34, Jayathirth D V wrote:<o:p></o:p></pre>
              <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
                <pre>Hi Sergey,<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>I have unified changes related to ImageIO.read() and ImageIO.write().<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>In case of  read() we will be returning null when createImageInputStream() returns null.<o:p></o:p></pre>
                <pre>In case of write() we will be returning false when createImageOutputStream() returns null.<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>Please find updated webrev for review:<o:p></o:p></pre>
                <pre><a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.03/">http://cr.openjdk.java.net/~jdv/8044289/webrev.03/</a><o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>Thanks,<o:p></o:p></pre>
                <pre>Jay<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>-----Original Message-----<o:p></o:p></pre>
                <pre>From: Sergey Bylokhov<o:p></o:p></pre>
                <pre>Sent: Tuesday, March 22, 2016 7:39 AM<o:p></o:p></pre>
                <pre>To: Jayathirth D V; Philip Race<o:p></o:p></pre>
                <pre>Cc: <a moz-do-not-send="true" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><o:p></o:p></pre>
                <pre>Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In <o:p></o:p></pre>
                <pre>ImageIO.read() and write() NPE is not handled properly for stream<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>Hi, Jay.<o:p></o:p></pre>
                <pre>Since we decided to update the specification we get an additional flexibility in the fix, because we can update the "@exception IOException" part of javadoc and describe that it can be thrown if ImageXXXStream cannot be created.<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>I don't see a big differences of both approaches in case of write() methods, but in case of read(which has the similar issue but was skipped in the fix) there is a difference.<o:p></o:p></pre>
                <pre>Note that the read(File) unlike other methods throws the unspecified <o:p></o:p></pre>
                <pre>IIOException exception if ImageXXXStream cannot be created. We can <o:p></o:p></pre>
                <pre>think about unification of read/write methods in ImageIO. Hmm but even <o:p></o:p></pre>
                <pre>then<o:p></o:p></pre>
                <pre>read() in some cases will return null and in some cases throw an exception....<o:p></o:p></pre>
                <pre>and createImageInputStream/createImageOutputStream() will look alien as they always return null not an exception.<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>Another possible unification is to update javadoc for all methods, <o:p></o:p></pre>
                <pre>that they will return null if related providers are not found(in this <o:p></o:p></pre>
                <pre>case "throw new IIOException("Can't create an ImageInputStream!"))" <o:p></o:p></pre>
                <pre>should be removed from read(File);<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>On 21.03.16 10:26, Jayathirth D V wrote:<o:p></o:p></pre>
                <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
                  <pre>Hi Sergey,<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>For the second approach I have created webrev for review.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>Please review updated webrev :<o:p></o:p></pre>
                  <pre><a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.02/">http://cr.openjdk.java.net/~jdv/8044289/webrev.02/</a><o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>Thanks,<o:p></o:p></pre>
                  <pre>Jay<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>-----Original Message-----<o:p></o:p></pre>
                  <pre>From: Jayathirth D V<o:p></o:p></pre>
                  <pre>Sent: Friday, March 18, 2016 2:23 PM<o:p></o:p></pre>
                  <pre>To: Sergey Bylokhov<o:p></o:p></pre>
                  <pre>Cc: <a moz-do-not-send="true" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a>; Philip Race<o:p></o:p></pre>
                  <pre>Subject: RE: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In<o:p></o:p></pre>
                  <pre>ImageIO.read() and write() NPE is not handled properly for stream<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>Hi Sergey,<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>Thanks for your inputs.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>Indeed all the null values returned by createImageInputStream() and createImageInputStream() are valid only.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>I went through the code more and found that there is no need for fix in ImageIO.read() methods as null stream is handled in read(ImageInputStream stream) which is called by all the remaining read() API's through IAE. So i can remove the changes made  in ImageIO.read() functions. Also istream is closed in finally block of read(url).<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>For ImageIO.write(File) &  ImageIO.write(OutputStream) NPE we have two approaches for fix:<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>1) Keep the present changes of throwing IOException when stream is null and change the throw clause of write() spec mentioning:<o:p></o:p></pre>
                  <pre>     "IOException - if an error occurs during writing or not able to create required ImageOutputStream"<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>2) Remove the present changes and handle null in doWrite() method by returning false when stream is null & add code in finally block to close the stream      only when stream is not null. Along with this we can also change Returns clause of spec to mention that write() might return false when we are not      able to create ImageOutputStream.<o:p></o:p></pre>
                  <pre>     <o:p></o:p></pre>
                  <pre>         private static boolean doWrite(RenderedImage im, ImageWriter writer,<o:p></o:p></pre>
                  <pre>                                                                                     ImageOutputStream output) throws IOException {<o:p></o:p></pre>
                  <pre>                    if (writer == null) {<o:p></o:p></pre>
                  <pre>                                   return false;<o:p></o:p></pre>
                  <pre>                    }<o:p></o:p></pre>
                  <pre>                    + if (output == null) {<o:p></o:p></pre>
                  <pre>                            +       return false;<o:p></o:p></pre>
                  <pre>                    + }<o:p></o:p></pre>
                  <pre>     <o:p></o:p></pre>
                  <pre>             try {<o:p></o:p></pre>
                  <pre>                                return doWrite(im, writer, stream);<o:p></o:p></pre>
                  <pre>                     } finally {<o:p></o:p></pre>
                  <pre>                             + if (stream != null)<o:p></o:p></pre>
                  <pre>                         stream.close();<o:p></o:p></pre>
                  <pre>                     }<o:p></o:p></pre>
                  <pre>     <o:p></o:p></pre>
                  <pre>     Returns :<o:p></o:p></pre>
                  <pre>             false if no appropriate writer is found or not able to create required ImageOutputStream.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>Please let me know your inputs.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>Thanks,<o:p></o:p></pre>
                  <pre>Jay<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>-----Original Message-----<o:p></o:p></pre>
                  <pre>From: Sergey Bylokhov<o:p></o:p></pre>
                  <pre>Sent: Friday, March 18, 2016 12:24 AM<o:p></o:p></pre>
                  <pre>To: Jayathirth D V<o:p></o:p></pre>
                  <pre>Cc: <a moz-do-not-send="true" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a>; Philip Race<o:p></o:p></pre>
                  <pre>Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In<o:p></o:p></pre>
                  <pre>ImageIO.read() and write() NPE is not handled properly for stream<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>On 17.03.16 21:14, Jayathirth D V wrote:<o:p></o:p></pre>
                  <blockquote
                    style="margin-top:5.0pt;margin-bottom:5.0pt">
                    <pre>There are many places in createImageInputStream() and createImageOutputStream() where it will return null for stream.<o:p></o:p></pre>
                  </blockquote>
                  <pre> <o:p></o:p></pre>
                  <pre>But when why you change the write/read methods? They should take null into account and work according to specification. If some bogus null returns from createImageInputStream/createImageOutputStream then it should be fixed in these methods.<o:p></o:p></pre>
                  <blockquote
                    style="margin-top:5.0pt;margin-bottom:5.0pt">
                    <pre> <o:p></o:p></pre>
                    <pre>So for NPE described in bug to occur there can be multiple paths:<o:p></o:p></pre>
                    <pre>1) One of the path described in bug is when theRegistry.getServiceProviders() throws IAE and we return null.<o:p></o:p></pre>
                  </blockquote>
                  <pre> <o:p></o:p></pre>
                  <pre>This is the case when we have no installed spi and should return null/false.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <blockquote
                    style="margin-top:5.0pt;margin-bottom:5.0pt">
                    <pre>2) When we pass non-existent file path to ImageIO (as described in <a moz-do-not-send="true" href="http://stackoverflow.com/questions/11153200/with-imageio-write-api-call-i-get-nullpointerexception">http://stackoverflow.com/questions/11153200/with-imageio-write-api-call-i-get-nullpointerexception</a> ). Here we catch FileNotFoundException but still we return stream as null at the end which will cause NPE further.<o:p></o:p></pre>
                  </blockquote>
                  <pre> <o:p></o:p></pre>
                  <pre>This is same thing described in the bug(about stream.close()). The write method call createImageOutputStream() and got FileNotFoundException.<o:p></o:p></pre>
                  <pre>This exception should be propagated to the user, but it is replaced in the finally block, because we try to close the stream=null. So the fix for this particular case is to check the stream to null in finally, No?<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <blockquote
                    style="margin-top:5.0pt;margin-bottom:5.0pt">
                    <pre>3) When we don't support the provider and theRegistry.getServiceProviders() return iterator with size zero.<o:p></o:p></pre>
                  </blockquote>
                  <pre> <o:p></o:p></pre>
                  <pre>This means that our providers do not support this case, and we should return null/false.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <blockquote
                    style="margin-top:5.0pt;margin-bottom:5.0pt">
                    <pre> <o:p></o:p></pre>
                    <pre>For us to follow first scenario we need access to categoryMap of IIORegistry and delete the ImageInputStreamSpi/ ImageOutputStreamSpi but I was not able to find a way to tamper it from test case. If we follow second case FileNotFoundException will be thrown by default eventhough we handle NPE.<o:p></o:p></pre>
                  </blockquote>
                  <pre> <o:p></o:p></pre>
                  <pre>But you change the code and we now contradicts the specification, in your case the test should expect false/null from write()/read().<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <blockquote
                    style="margin-top:5.0pt;margin-bottom:5.0pt">
                    <pre> <o:p></o:p></pre>
                    <pre>In the test case we are trying to following third scenario to get stream as null to verify the code changes.<o:p></o:p></pre>
                  </blockquote>
                  <pre> <o:p></o:p></pre>
                  <pre>To catch the bogus null we should try to use another approach, or change the spec. But it seems all null values above are correct, we should be ready for them.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <blockquote
                    style="margin-top:5.0pt;margin-bottom:5.0pt">
                    <pre> <o:p></o:p></pre>
                    <pre>As part of collective fix we are throwing IOException which will indicate that we are not able to create required stream.<o:p></o:p></pre>
                    <pre> <o:p></o:p></pre>
                    <pre>Thanks,<o:p></o:p></pre>
                    <pre>Jay<o:p></o:p></pre>
                    <pre> <o:p></o:p></pre>
                    <pre>-----Original Message-----<o:p></o:p></pre>
                    <pre>From: Sergey Bylokhov<o:p></o:p></pre>
                    <pre>Sent: Thursday, March 17, 2016 7:27 PM<o:p></o:p></pre>
                    <pre>To: Phil Race<o:p></o:p></pre>
                    <pre>Cc: Jayathirth D V; <a moz-do-not-send="true" href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><o:p></o:p></pre>
                    <pre>Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In<o:p></o:p></pre>
                    <pre>ImageIO.read() and write() NPE is not handled properly for stream<o:p></o:p></pre>
                    <pre> <o:p></o:p></pre>
                    <pre>On 17.03.16 0:36, Phil Race wrote:<o:p></o:p></pre>
                    <blockquote
                      style="margin-top:5.0pt;margin-bottom:5.0pt">
                      <pre>I don't think that is the actual reason here<o:p></o:p></pre>
                    </blockquote>
                    <pre> <o:p></o:p></pre>
                    <pre>But the test doing exactly this thing -> deregister all spi => in <o:p></o:p></pre>
                    <pre>this case according to specification all related<o:p></o:p></pre>
                    <pre>methods(read/write/creatImageInputStream/creatImageOutputStream) should return null or false.<o:p></o:p></pre>
                    <pre> <o:p></o:p></pre>
                    <pre>- it seems likely that there<o:p></o:p></pre>
                    <blockquote
                      style="margin-top:5.0pt;margin-bottom:5.0pt">
                      <pre>was some kind of internal error or bad SPI causing this.<o:p></o:p></pre>
                      <pre>So throwing an exception seems more appropriate.<o:p></o:p></pre>
                    </blockquote>
                    <pre> <o:p></o:p></pre>
                    <pre>the problem was in some of our methods which do not expect null in some places(like stream.close() from the bug description).<o:p></o:p></pre>
                    <pre> <o:p></o:p></pre>
                    <blockquote
                      style="margin-top:5.0pt;margin-bottom:5.0pt">
                      <pre>And also the bug was originally solely about write(..).<o:p></o:p></pre>
                      <pre> <o:p></o:p></pre>
                      <pre>-phil.<o:p></o:p></pre>
                      <pre> <o:p></o:p></pre>
                      <pre> <o:p></o:p></pre>
                      <pre>On 03/16/2016 01:35 PM, Sergey Bylokhov wrote:<o:p></o:p></pre>
                      <blockquote
                        style="margin-top:5.0pt;margin-bottom:5.0pt">
                        <pre>As far as I understand the createImageInputStream() returns null <o:p></o:p></pre>
                        <pre>is this stream is unsupported by current plugins via spi. Probably <o:p></o:p></pre>
                        <pre>the<o:p></o:p></pre>
                        <pre>read() method should return null too?<o:p></o:p></pre>
                        <pre> <o:p></o:p></pre>
                        <pre>       /**<o:p></o:p></pre>
                        <pre>        * Returns a {@code BufferedImage} as the result of decoding<o:p></o:p></pre>
                        <pre>        * a supplied {@code InputStream} with an {@code ImageReader}<o:p></o:p></pre>
                        <pre>        * chosen automatically from among those currently registered.<o:p></o:p></pre>
                        <pre>        * The {@code InputStream} is wrapped in an<o:p></o:p></pre>
                        <pre>        * {@code ImageInputStream}.  If no registered<o:p></o:p></pre>
                        <pre>        * {@code ImageReader} claims to be able to read the<o:p></o:p></pre>
                        <pre>        * resulting stream, {@code null} is returned.<o:p></o:p></pre>
                        <pre>        *<o:p></o:p></pre>
                        <pre>.......<o:p></o:p></pre>
                        <pre>        * @return a {@code BufferedImage} containing the decoded<o:p></o:p></pre>
                        <pre>        * contents of the input, or {@code null}.<o:p></o:p></pre>
                        <pre>        */<o:p></o:p></pre>
                        <pre>       public static BufferedImage read(InputStream input) throws <o:p></o:p></pre>
                        <pre>IOException<o:p></o:p></pre>
                        <pre> <o:p></o:p></pre>
                        <pre> <o:p></o:p></pre>
                        <pre>On 16.03.16 20:29, Philip Race wrote:<o:p></o:p></pre>
                        <blockquote
                          style="margin-top:5.0pt;margin-bottom:5.0pt">
                          <pre>The test writes out into "test.src".<o:p></o:p></pre>
                          <pre>I believe that you should treat that as a "read-only" location.<o:p></o:p></pre>
                          <pre>Write to a tempfile (File.createTempFile()) or TESTCLASSES.<o:p></o:p></pre>
                          <pre> <o:p></o:p></pre>
                          <pre>-phil.<o:p></o:p></pre>
                          <pre> <o:p></o:p></pre>
                          <pre>On 3/15/16, 10:50 PM, Jayathirth D V wrote:<o:p></o:p></pre>
                          <blockquote
                            style="margin-top:5.0pt;margin-bottom:5.0pt">
                            <pre> <o:p></o:p></pre>
                            <pre>Hi Phil,All<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>_Please review the following fix in JDK9:_<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>__<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>Bug : <a moz-do-not-send="true" href="https://bugs.openjdk.java.net/browse/JDK-8044289">https://bugs.openjdk.java.net/browse/JDK-8044289</a><o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>Webrev : <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/">http://cr.openjdk.java.net/~jdv/8044289/webrev.00/</a><o:p></o:p></pre>
                            <pre><a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/"><http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/></a><o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>Issue : When createImageInputStream() or createImageOutputStream <o:p></o:p></pre>
                            <pre>returns null in ImageIO.read() or ImageIO.write() we are seeing <o:p></o:p></pre>
                            <pre>NPE as it stream is used further without null check.<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>Root cause : "stream" is used without checking whether it is <o:p></o:p></pre>
                            <pre>null or not.<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>Solution : Handle null pointer of stream and throw IIOException.<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>Test case contains all possible scenarios where<o:p></o:p></pre>
                            <pre>createImageInputStream() or createImageOutputStream can return null.<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>Thanks,<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                            <pre>Jay<o:p></o:p></pre>
                            <pre> <o:p></o:p></pre>
                          </blockquote>
                        </blockquote>
                        <pre> <o:p></o:p></pre>
                        <pre> <o:p></o:p></pre>
                      </blockquote>
                      <pre> <o:p></o:p></pre>
                    </blockquote>
                    <pre> <o:p></o:p></pre>
                    <pre> <o:p></o:p></pre>
                    <pre>--<o:p></o:p></pre>
                    <pre>Best regards, Sergey.<o:p></o:p></pre>
                    <pre> <o:p></o:p></pre>
                  </blockquote>
                  <pre> <o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                  <pre>--<o:p></o:p></pre>
                  <pre>Best regards, Sergey.<o:p></o:p></pre>
                  <pre> <o:p></o:p></pre>
                </blockquote>
                <pre> <o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
                <pre>--<o:p></o:p></pre>
                <pre>Best regards, Sergey.<o:p></o:p></pre>
                <pre> <o:p></o:p></pre>
              </blockquote>
              <pre> <o:p></o:p></pre>
              <pre> <o:p></o:p></pre>
              <pre>--<o:p></o:p></pre>
              <pre>Best regards, Sergey.<o:p></o:p></pre>
            </blockquote>
          </blockquote>
        </blockquote>
        <p class="MsoNormal"><o:p> </o:p></p>
      </div>
    </blockquote>
    <br>
  </body>
</html>