<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 12 (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: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]--></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='color:#1F497D'>Yes GodMode/ControlPanel(All Tasks) works on Windows 7,8 and 10.<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'>I have removed the OS version check.<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><a href="http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.02/">http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.02/</a> <o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='color:#1F497D'>Regards,<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'>Prem<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></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> awt-dev@openjdk.java.net; 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<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><div><p class=MsoNormal>I had an impression that this mode works on windows 7/8/10, no?<o:p></o:p></p><div><p class=MsoNormal><br>----- <a href="mailto:philip.race@oracle.com">philip.race@oracle.com</a> wrote: <o:p></o:p></p><div><div id=AppleMailSignature><p class=MsoNormal>> Not reproducible only because of the fix.<o:p></o:p></p></div><div id=AppleMailSignature><p class=MsoNormal>> So it is a valid question.<o:p></o:p></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.<o:p></o:p></p></div><div id=AppleMailSignature><p class=MsoNormal>> Do it is fine as is I think.<o:p></o:p></p></div><div id=AppleMailSignature><p class=MsoNormal><br>> <br>> -Phil.<o:p></o:p></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>> <o:p></o:p></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"'>> <o:p></o:p></span></p><p class=MsoNormal>Thank you for the review.<o:p></o:p></p><p class=MsoNormal> <o:p></o:p></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?<o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></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><o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>Regards,</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>Prem</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></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"'>> <o:p></o:p></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><o:p></o:p></p></div></div><p class=MsoNormal> <o:p></o:p></p><div><p class=MsoNormal>The fix looks fine.<o:p></o:p></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?<o:p></o:p></p><div><p class=MsoNormal> <o:p></o:p></p><div><p class=MsoNormal>----- <a href="mailto:philip.race@oracle.com" target="_blank">philip.race@oracle.com</a> wrote: <br>> > <o:p></o:p></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>> > <o:p></o:p></p><div><p class=MsoNormal>> On 05/04/2017 02:55 AM, Prem Balakrishnan wrote:<br>> > <o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>Thanks Kevin and Phil for the review.</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>I shouldn't have missed the 'break' statement. Mistake on my part :(</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>The crash fix null check is in thrid switch case.</span><o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>The null checks have been added in method:</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>static jstring jstringFromSTRRET(JNIEnv* env, LPITEMIDLIST pidl, STRRET* pStrret)</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>This method is invoked from below methods in ShellFolder2.cpp</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>getDisplayNameOf() </span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>doGetColumnValue()</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>createColumnInfo()</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></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><o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>I examined calls to these native methods getDisplayNameOf(), doGetColumnValue() & createColumnInfo() - </span><o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></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><o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>Request you to review.</span><o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'> </span><o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'>Regards,</span><o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'>Prem</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></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"'>> <o:p></o:p></span></p><p class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New Roman","serif"'>> </span><o:p></o:p></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><o:p></o:p></p></div></div><p class=MsoNormal> <o:p></o:p></p><div><p class=MsoNormal>I agree. This switch statement clearly was written in the expectation that each case executed return.<o:p></o:p></p></div><div><p class=MsoNormal> <o:p></o:p></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.<o:p></o:p></p><div><p class=MsoNormal>-Phil.<o:p></o:p></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:<o:p></o:p></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:<o:p></o:p></p><pre><span style='color:black'>         case STRRET_CSTR :<o:p></o:p></span></pre><pre><span class=new><span style='color:black'>+            if (pStrret->cStr != NULL) {</span></span><span style='color:black'><o:p></o:p></span></pre><pre><span style='color:black'>             return JNU_NewStringPlatform(env, reinterpret_cast<const char*>(pStrret->cStr));<o:p></o:p></span></pre><pre><span class=new><span style='color:black'>+            }</span></span><span style='color:black'><o:p></o:p></span></pre><pre><span style='color:black'>         case STRRET_OFFSET :<o:p></o:p></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: <o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'>Hi<b>,</b></span><o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'>Please review fix for JDK 9,</span><o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'> </span><o:p></o:p></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><o:p></o:p></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><o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'> </span><o:p></o:p></p><p class=MsoNormal><b><span style='font-size:12.0pt'>Issue:</span></b><o:p></o:p></p><p class=MsoNormal>JFileChooser with Windows LAF crashes on Windows 10.<o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'>This issue is reproducible only on Windows 10 (v1703).</span><o:p></o:p></p><p class=MsoNormal>Creating GodMode directory is the only way we are able to reproduce this issue.<o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p class=MsoNormal><b>Cause:</b><o:p></o:p></p><p class=MsoNormal>IShellFolder::GetDisplayNameOf() method is used to retrieve the display name of folder/subfolder using the GUID.<o:p></o:p></p><p class=MsoNormal>This method  returns NULL for GodMode directory [i.e., folder created with name: GodMode.{ED7BA470-8E54-465E-825C-99712043E01C}].<o:p></o:p></p><p class=MsoNormal>In previous version of windows (tested on v1511) this call returns name “GodMode”.<o:p></o:p></p><p class=MsoNormal>Hence we get crash when processing the return data from the this method.<o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p class=MsoNormal>Why only on Windows LAF?<o:p></o:p></p><p class=MsoNormal>Unlike other LAFs in Windows LAF “useSystemExtensionHiding” is set to true.<o:p></o:p></p><p class=MsoNormal>This check avoids the call to IShellFolder::GetDisplayNameOf() method in all other LAF’s.<o:p></o:p></p><p class=MsoNormal>Hence crash is not reported on other LAFs.<o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p class=MsoNormal><b>Fix:</b><o:p></o:p></p><p class=MsoNormal>Added missing NULL checks in native method.<o:p></o:p></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.<o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'> </span><o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'>Regards,</span><o:p></o:p></p><p class=MsoNormal><span style='font-size:12.0pt'>Prem</span><o:p></o:p></p><p class=MsoNormal><span style='color:windowtext'> </span><o:p></o:p></p></div></blockquote></blockquote><p class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New Roman","serif"'><br>> > </span><o:p></o:p></p></div></div></div></div></div></div></blockquote></div></div></div></div></body></html>