Ok then .. +1
-phil.
On 05/09/2017 01:46 AM, Prem Balakrishnan wrote:
Yes GodMode/ControlPanel(All Tasks) works on Windows 7,8 and 10.
I have removed the OS version check.
http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.02/
<http://cr.openjdk.java.net/%7Epkbalakr/8179014/webrev.02/>
Regards,
Prem
*From:*Sergey Bylokhov
*Sent:* Tuesday, May 09, 2017 1:21 AM
*To:* Philip Race
*Cc:* awt-dev@openjdk.java.net; Prem Balakrishnan; Alexander
Scherbatiy; Kevin Rushforth
*Subject:* Re: <AWT Dev> Review Request: JDK-8179014 JFileChooser with
Windows look and feel crashes on win 10 + jre-8 131
I had an impression that this mode works on windows 7/8/10, no?
----- philip.r...@oracle.com <mailto:philip.r...@oracle.com> wrote:
> Not reproducible only because of the fix.
> So it is a valid question.
> 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.
> Do it is fine as is I think.
>
> -Phil.
> On May 6, 2017, at 1:34 AM, Prem Balakrishnan
<prem.balakrish...@oracle.com <mailto:prem.balakrish...@oracle.com>>
wrote:
>
>
>
Thank you for the review.
>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?
This crash is due to a Windows 10 method returning null in the
latest update(v1703).
since both null and not null cases are handled, with upcoming
windows updates/releases this crash will not be reproducible.
Regards,
Prem
>
*From:*Sergey Bylokhov
> *Sent:* Saturday, May 06, 2017 7:06 AM
> *To:* Philip Race
> *Cc:* awt-dev@openjdk.java.net
<mailto:awt-dev@openjdk.java.net>; Prem Balakrishnan; Alexander
Scherbatiy; Kevin Rushforth
> *Subject:* Re: <AWT Dev> Review Request: JDK-8179014
JFileChooser with Windows look and feel crashes on win 10 + jre-8 131
The fix looks fine.
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?
----- philip.r...@oracle.com <mailto:philip.r...@oracle.com> wrote:
> >
OK +1 to this fix. Please follow RDP2 procedures to request this
fix in 9 ..
> >
> > -phil.
> >
> >
> >
> On 05/04/2017 02:55 AM, Prem Balakrishnan wrote:
> >
>
Thanks Kevin and Phil for the review.
I shouldn't have missed the 'break' statement. Mistake on my
part :(
The crash fix null check is in thrid switch case.
As a good practice of making the method robust, I added the
null check in first switch case as well.
The null checks have been added in method:
static jstring jstringFromSTRRET(JNIEnv* env, LPITEMIDLIST
pidl, STRRET* pStrret)
This method is invoked from below methods in ShellFolder2.cpp
getDisplayNameOf()
doGetColumnValue()
createColumnInfo()
The crash was due to function call getDisplayNameOf() ---
which gets fixed if we add null check only in third swich case.
The null check in first switch case is more of making the code
robust against possible crash.
I examined calls to these native methods getDisplayNameOf(),
doGetColumnValue() & createColumnInfo() -
Only getDisplayNameOf() return has null checks on java side.
There are no checks on java side for string being null for
other two methods.
These two methods are invoked from FilePane.java class, which
is used only in XXXXFileChooserUI classes.
I haven't seen any side effect in my jtreg test runs of
JFileChooser with this fix.
The null check for first switch statement never existed and
there were no crashes reported. What I have done is more of
preventive check.
Request you to review.
http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.01/
<http://cr.openjdk.java.net/%7Epkbalakr/8179014/webrev.01/>
Regards,
Prem
>
>
*From:*Phil Race
> > *Sent:* Saturday, April 29, 2017 11:00 PM
> > *To:* Kevin Rushforth
> > *Cc:* Prem Balakrishnan; Alexander Scherbatiy; Sergey
Bylokhov; awt-dev@openjdk.java.net
<mailto:awt-dev@openjdk.java.net>
> > *Subject:* Re: <AWT Dev> Review Request: JDK-8179014
JFileChooser with Windows look and feel crashes on win 10 +
jre-8 131
I agree. This switch statement clearly was written in the
expectation that each case executed return.
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.
-Phil.
> > On Apr 29, 2017, at 5:33 AM, Kevin Rushforth
<kevin.rushfo...@oracle.com
<mailto:kevin.rushfo...@oracle.com>> wrote:
I'm not a "R"eviewer for AWT, but this part of the change
looks wrong to me:
case STRRET_CSTR :
+ if (pStrret->cStr != NULL) {
return JNU_NewStringPlatform(env,
reinterpret_cast<const char*>(pStrret->cStr));
+ }
case STRRET_OFFSET :
> >
> > 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.
> >
> > -- Kevin
> >
> >
> > Prem Balakrishnan wrote:
Hi*,*
Please review fix for JDK 9,
*Bug:*https://bugs.openjdk.java.net/browse/JDK-8179014
*Webrev:*http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.00/
<http://cr.openjdk.java.net/%7Epkbalakr/8179014/webrev.00/>
*Issue:*
JFileChooser with Windows LAF crashes on Windows 10.
This issue is reproducible only on Windows 10 (v1703).
Creating GodMode directory is the only way we are able to
reproduce this issue.
*Cause:*
IShellFolder::GetDisplayNameOf() method is used to
retrieve the display name of folder/subfolder using the GUID.
This method returns NULL for GodMode directory [i.e.,
folder created with name:
GodMode.{ED7BA470-8E54-465E-825C-99712043E01C}].
In previous version of windows (tested on v1511) this call
returns name “GodMode”.
Hence we get crash when processing the return data from
the this method.
Why only on Windows LAF?
Unlike other LAFs in Windows LAF
“useSystemExtensionHiding” is set to true.
This check avoids the call to
IShellFolder::GetDisplayNameOf() method in all other LAF’s.
Hence crash is not reported on other LAFs.
*Fix:*
Added missing NULL checks in native method.
This results in native method returning NULL in this case,
which has already been handled on java side, hence no
additional change is needed.
Regards,
Prem
> >
