<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: Times New Roman; font-size: 12pt; color: #000000'>+1<div>Thanks.</div><div><br><div>----- philip.race@oracle.com wrote:
<br>> 
  
    
  
  <div>
    Ok then .. +1<br>> 
    <br>> 
    -phil.<br>> 
    <br>> 
    <div class="moz-cite-prefix">> On 05/09/2017 01:46 AM, Prem
      Balakrishnan wrote:<br>> 
    </div>
    <blockquote cite="mid:2d60bf0c-877c-4117-9b34-59f41edd8f00@default">
      
      
      <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:Tahoma;
        panose-1:2 11 6 4 3 5 4 4 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:11.0pt;
        font-family:"Calibri","sans-serif";
        color:black;}
h1
        {mso-style-priority:9;
        mso-style-link:"Heading 1 Char";
        mso-margin-top-alt:auto;
        margin-right:0in;
        mso-margin-bottom-alt:auto;
        margin-left:0in;
        font-size:24.0pt;
        font-family:"Times New Roman","serif";
        font-weight:bold;}
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;}
p
        {mso-style-priority:99;
        margin:0in;
        margin-bottom:.0001pt;
        font-size:12.0pt;
        font-family:"Times New Roman","serif";}
pre
        {mso-style-priority:99;
        mso-style-link:"HTML Preformatted Char";
        margin:0in;
        margin-bottom:.0001pt;
        font-size:10.0pt;
        font-family:"Courier New";}
p.MsoAcetate, li.MsoAcetate, div.MsoAcetate
        {mso-style-priority:99;
        mso-style-link:"Balloon Text Char";
        margin:0in;
        margin-bottom:.0001pt;
        font-size:8.0pt;
        font-family:"Tahoma","sans-serif";
        color:black;}
span.Heading1Char
        {mso-style-name:"Heading 1 Char";
        mso-style-priority:9;
        mso-style-link:"Heading 1";
        font-family:"Times New Roman","serif";
        font-weight:bold;}
span.HTMLPreformattedChar
        {mso-style-name:"HTML Preformatted Char";
        mso-style-priority:99;
        mso-style-link:"HTML Preformatted";
        font-family:Consolas;}
span.BalloonTextChar
        {mso-style-name:"Balloon Text Char";
        mso-style-priority:99;
        mso-style-link:"Balloon Text";
        font-family:"Tahoma","sans-serif";
        color:black;}
span.new
        {mso-style-name:new;}
span.EmailStyle24
        {mso-style-type:personal;
        font-family:"Calibri","sans-serif";
        color:windowtext;}
span.EmailStyle25
        {mso-style-type:personal;
        font-family:"Calibri","sans-serif";
        color:#1F497D;}
span.EmailStyle26
        {mso-style-type:personal;
        font-family:"Calibri","sans-serif";
        color:#1F497D;}
span.EmailStyle27
        {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="color:#1F497D">Yes
            GodMode/ControlPanel(All Tasks) works on Windows 7,8 and 10.</span></p>
        <p class="MsoNormal"><span style="color:#1F497D">I have removed
            the OS version check.</span></p>
        <p class="MsoNormal"><span style="color:#1F497D"><a href="http://cr.openjdk.java.net/%7Epkbalakr/8179014/webrev.02/" target="_blank">http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.02/</a>
            </span></p>
        <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
        <p class="MsoNormal"><span style="color:#1F497D">Regards,</span></p>
        <p class="MsoNormal"><span style="color:#1F497D">Prem</span></p>
        <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
        <div>
          <div style="border:none;border-top:solid #B5C4DF
            1.0pt;padding:3.0pt 0in 0in 0in">> 
            <p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext">
                Sergey Bylokhov <br>> 
                <b>Sent:</b> Tuesday, May 09, 2017 1:21 AM<br>> 
                <b>To:</b> Philip Race<br>> 
                <b>Cc:</b> <a class="moz-txt-link-abbreviated" href="mailto:awt-dev@openjdk.java.net" target="_blank">awt-dev@openjdk.java.net</a>; Prem Balakrishnan;
                Alexander Scherbatiy; Kevin Rushforth<br>> 
                <b>Subject:</b> Re: <AWT Dev> Review Request:
                JDK-8179014 JFileChooser with Windows look and feel
                crashes on win 10 + jre-8 131</span></p>
          </div>
        </div>
        <p class="MsoNormal"> </p>
        <div>
          <p class="MsoNormal">I had an impression that this mode works
            on windows 7/8/10, no?</p>
          <div>
            <p class="MsoNormal"><br>> 
              ----- <a href="mailto:philip.race@oracle.com" target="_blank">philip.race@oracle.com</a>
              wrote: </p>
            <div>
              <div id="AppleMailSignature">> 
                <p class="MsoNormal">> Not reproducible only because
                  of the fix.</p>
              </div>
              <div id="AppleMailSignature">> 
                <p class="MsoNormal">> So it is a valid question.</p>
              </div>
              <div id="AppleMailSignature">> 
                <p class="MsoNormal">> I think the better answer is
                  that this exact god mode setting is windows 10 only
                  and there is no indication ms will review the version
                  any time soon.</p>
              </div>
              <div id="AppleMailSignature">> 
                <p class="MsoNormal">> Do it is fine as is I think.</p>
              </div>
              <div id="AppleMailSignature">> 
                <p class="MsoNormal"><br>> 
                  > <br>> 
                  > -Phil.</p>
              </div>
              <div>
                <p class="MsoNormal"><br>> 
                  > On May 6, 2017, at 1:34 AM, Prem Balakrishnan
                  <<a href="mailto:prem.balakrishnan@oracle.com" target="_blank">prem.balakrishnan@oracle.com</a>>
                  wrote:<br>> 
                  > <br>> 
                  > </p>
              </div>
              <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
                <div>
                  <p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times
                      New Roman","serif"">> </span></p>
                  <p class="MsoNormal">Thank you for the review.</p>
                  <p class="MsoNormal"> </p>
                  <p class="MsoNormal">>But I have a question about
                    test, should it be run on win=10.0 only? I guess in
                    this case it will be skipped on some new/other
                    windows which will be released someday?</p>
                  <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                  <p class="MsoNormal"><span style="color:#1F497D">This
                      crash is due to a Windows 10 method returning null
                      in the latest update(v1703). </span></p>
                  <p class="MsoNormal"><span style="color:#1F497D">since
                      both null and not null cases are handled, with
                      upcoming windows updates/releases this crash will
                      not be reproducible.</span></p>
                  <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                  <p class="MsoNormal"><span style="color:#1F497D">Regards,</span></p>
                  <p class="MsoNormal"><span style="color:#1F497D">Prem</span></p>
                  <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                  <div>
                    <div style="border:none;border-top:solid #B5C4DF
                      1.0pt;padding:3.0pt 0in 0in 0in">> 
                      <p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times
                          New Roman","serif"">> </span></p>
                      <p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext">
                          Sergey Bylokhov <br>> 
                          > <b>Sent:</b> Saturday, May 06, 2017 7:06
                          AM<br>> 
                          > <b>To:</b> Philip Race<br>> 
                          > <b>Cc:</b> <a href="mailto:awt-dev@openjdk.java.net" target="_blank">awt-dev@openjdk.java.net</a>;
                          Prem Balakrishnan; Alexander Scherbatiy; Kevin
                          Rushforth<br>> 
                          > <b>Subject:</b> Re: <AWT Dev>
                          Review Request: JDK-8179014 JFileChooser with
                          Windows look and feel crashes on win 10 +
                          jre-8 131</span></p>
                    </div>
                  </div>
                  <p class="MsoNormal"> </p>
                  <div>
                    <p class="MsoNormal">The fix looks fine.</p>
                    <div>
                      <p class="MsoNormal">But I have a question about
                        test, should it be run on win=10.0 only? I guess
                        in this case it will be skipped on some
                        new/other windows which will be released
                        someday?</p>
                      <div>
                        <p class="MsoNormal"> </p>
                        <div>
                          <p class="MsoNormal">----- <a href="mailto:philip.race@oracle.com" target="_blank">philip.race@oracle.com</a>
                            wrote: <br>> 
                            > > </p>
                          <div>
                            <p class="MsoNormal">OK +1 to this fix.
                              Please follow RDP2 procedures to request
                              this fix in 9 ..<br>> 
                              > > <br>> 
                              > > -phil.<br>> 
                              > > <br>> 
                              > > <br>> 
                              > > </p>
                            <div>
                              <p class="MsoNormal">> On 05/04/2017
                                02:55 AM, Prem Balakrishnan wrote:<br>> 
                                > > </p>
                            </div>
                            <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
                              <p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times
                                  New Roman","serif"">>
                                </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">Thanks Kevin and
                                  Phil for the review.</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">I shouldn't have
                                  missed the 'break' statement. Mistake
                                  on my part :(</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">The crash fix
                                  null check is in thrid switch case.</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">As a good
                                  practice of making the method robust,
                                  I added the null check in first switch
                                  case as well. </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">The null checks
                                  have been added in method:</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">static jstring
                                  jstringFromSTRRET(JNIEnv* env,
                                  LPITEMIDLIST pidl, STRRET* pStrret)</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">This method is
                                  invoked from below methods in
                                  ShellFolder2.cpp</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">getDisplayNameOf()
                                </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">doGetColumnValue()</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">createColumnInfo()</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">The crash was
                                  due to function call
                                  getDisplayNameOf() --- which gets
                                  fixed if we add null check only in
                                  third swich case.</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">The null check
                                  in first switch case is more of making
                                  the code robust against possible
                                  crash.</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">I examined calls
                                  to these native methods
                                  getDisplayNameOf(), doGetColumnValue()
                                  & createColumnInfo() - </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">Only
                                  getDisplayNameOf() return has null
                                  checks on java side. There are no
                                  checks on java side for string being
                                  null for other two methods.</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">These two
                                  methods are invoked from FilePane.java
                                  class, which is used only in
                                  XXXXFileChooserUI classes.</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">I haven't seen
                                  any side effect in my jtreg test runs
                                  of JFileChooser with this fix.</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">The null check
                                  for first switch statement never
                                  existed and there were no crashes
                                  reported. What I have done is more of
                                  preventive check.</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <p class="MsoNormal"><span style="color:#1F497D">Request you to
                                  review.</span></p>
                              <p class="MsoNormal"><span style="font-size:12.0pt"><a href="http://cr.openjdk.java.net/%7Epkbalakr/8179014/webrev.01/" target="_blank">http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.01/</a>
                                </span></p>
                              <p class="MsoNormal"><span style="font-size:12.0pt"> </span></p>
                              <p class="MsoNormal"><span style="font-size:12.0pt">Regards,</span></p>
                              <p class="MsoNormal"><span style="font-size:12.0pt">Prem</span></p>
                              <p class="MsoNormal"><span style="color:#1F497D"> </span></p>
                              <div>
                                <div style="border:none;border-top:solid
                                  #B5C4DF 1.0pt;padding:3.0pt 0in 0in
                                  0in">> 
                                  <p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times
                                      New Roman","serif"">>
                                      </span></p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times
                                      New Roman","serif"">>
                                    </span></p>
                                  <p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext">
                                      Phil Race <br>> 
                                      > > <b>Sent:</b> Saturday,
                                      April 29, 2017 11:00 PM<br>> 
                                      > > <b>To:</b> Kevin
                                      Rushforth<br>> 
                                      > > <b>Cc:</b> Prem
                                      Balakrishnan; Alexander
                                      Scherbatiy; Sergey Bylokhov; <a href="mailto:awt-dev@openjdk.java.net" target="_blank">awt-dev@openjdk.java.net</a><br>> 
                                      > > <b>Subject:</b> Re:
                                      <AWT Dev> Review Request:
                                      JDK-8179014 JFileChooser with
                                      Windows look and feel crashes on
                                      win 10 + jre-8 131</span></p>
                                </div>
                              </div>
                              <p class="MsoNormal"> </p>
                              <div>
                                <p class="MsoNormal">I agree. This
                                  switch statement clearly was written
                                  in the expectation that each case
                                  executed return.</p>
                              </div>
                              <div>
                                <p class="MsoNormal"> </p>
                              </div>
                              <div>
                                <p class="MsoNormal" style="margin-bottom:12.0pt">And as I
                                  said offline pls confirm that you are
                                  confident that all return paths
                                  actually handle that null which may
                                  not have been tested in practice
                                  before now. There seemed to be several
                                  code paths.</p>
                                <div>
                                  <p class="MsoNormal">-Phil.</p>
                                </div>
                              </div>
                              <div>
                                <p class="MsoNormal" style="margin-bottom:12.0pt"><br>> 
                                  > > On Apr 29, 2017, at 5:33 AM,
                                  Kevin Rushforth <<a href="mailto:kevin.rushforth@oracle.com" target="_blank">kevin.rushforth@oracle.com</a>>
                                  wrote:</p>
                              </div>
                              <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
                                <div>
                                  <p class="MsoNormal" style="margin-bottom:12.0pt">I'm not
                                    a "R"eviewer for AWT, but this part
                                    of the change looks wrong to me:</p>
                                  <pre><span style="color:black">         case STRRET_CSTR :</span></pre>
                                  <pre><span class="new"><span style="color:black">+            if (pStrret->cStr != NULL) {</span></span><span style="color:black"></span></pre>
                                  <pre><span style="color:black">             return JNU_NewStringPlatform(env, reinterpret_cast<const char*>(pStrret->cStr));</span></pre>
                                  <pre><span class="new"><span style="color:black">+            }</span></span><span style="color:black"></span></pre>
                                  <pre><span style="color:black">         case STRRET_OFFSET :</span></pre>
                                  <p class="MsoNormal"><br>> 
                                    > > <br>> 
                                    > > Did you really intended
                                    that a NULL pStrret->cStr pointer
                                    should fall through to the next case
                                    (STRRET_OFFSET)? If so, then a
                                    comment is in order because this
                                    seems odd.<br>> 
                                    > > <br>> 
                                    > > -- Kevin<br>> 
                                    > > <br>> 
                                    > > <br>> 
                                    > > Prem Balakrishnan wrote: </p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt">Hi<b>,</b></span></p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt">Please
                                      review fix for JDK 9,</span></p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt"> </span></p>
                                  <p class="MsoNormal"><b><span style="font-size:12.0pt">Bug:</span></b><span style="font-size:12.0pt"> <a href="https://bugs.openjdk.java.net/browse/JDK-8179014" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8179014</a>
                                    </span></p>
                                  <p class="MsoNormal"><b><span style="font-size:12.0pt">Webrev:</span></b><span style="font-size:12.0pt"> <a href="http://cr.openjdk.java.net/%7Epkbalakr/8179014/webrev.00/" target="_blank">http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.00/</a>
                                    </span></p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt"> </span></p>
                                  <p class="MsoNormal"><b><span style="font-size:12.0pt">Issue:</span></b></p>
                                  <p class="MsoNormal">JFileChooser with
                                    Windows LAF crashes on Windows 10.</p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt">This
                                      issue is reproducible only on
                                      Windows 10 (v1703).</span></p>
                                  <p class="MsoNormal">Creating GodMode
                                    directory is the only way we are
                                    able to reproduce this issue.</p>
                                  <p class="MsoNormal"> </p>
                                  <p class="MsoNormal"><b>Cause:</b></p>
                                  <p class="MsoNormal">IShellFolder::GetDisplayNameOf()
                                    method is used to retrieve the
                                    display name of folder/subfolder
                                    using the GUID.</p>
                                  <p class="MsoNormal">This method 
                                    returns NULL for GodMode directory
                                    [i.e., folder created with name:
                                    GodMode.{ED7BA470-8E54-465E-825C-99712043E01C}].</p>
                                  <p class="MsoNormal">In previous
                                    version of windows (tested on v1511)
                                    this call returns name “GodMode”.</p>
                                  <p class="MsoNormal">Hence we get
                                    crash when processing the return
                                    data from the this method.</p>
                                  <p class="MsoNormal"> </p>
                                  <p class="MsoNormal">Why only on
                                    Windows LAF?</p>
                                  <p class="MsoNormal">Unlike other LAFs
                                    in Windows LAF
                                    “useSystemExtensionHiding” is set to
                                    true.</p>
                                  <p class="MsoNormal">This check avoids
                                    the call to
                                    IShellFolder::GetDisplayNameOf()
                                    method in all other LAF’s.</p>
                                  <p class="MsoNormal">Hence crash is
                                    not reported on other LAFs.</p>
                                  <p class="MsoNormal"> </p>
                                  <p class="MsoNormal"><b>Fix:</b></p>
                                  <p class="MsoNormal">Added missing
                                    NULL checks in native method.</p>
                                  <p class="MsoNormal">This results in
                                    native method returning NULL in this
                                    case, which has already been handled
                                    on java side, hence no additional
                                    change is needed.</p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt"> </span></p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt">Regards,</span></p>
                                  <p class="MsoNormal"><span style="font-size:12.0pt">Prem</span></p>
                                  <p class="MsoNormal"><span style="color:windowtext"> </span></p>
                                </div>
                              </blockquote>
                            </blockquote>
                            <p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times
                                New Roman","serif""><br>> 
                                > > </span></p>
                          </div>
                        </div>
                      </div>
                    </div>
                  </div>
                </div>
              </blockquote>
            </div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>> 
  </div>

</div></div></div></body></html>