<AWT Dev> Review Request: JDK-8179014 JFileChooser with Windows look and feel crashes on win 10 + jre-8 131
Phil Race
philip.race at oracle.com
Tue May 9 19:23:05 UTC 2017
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 at 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.race at oracle.com <mailto:philip.race at 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.balakrishnan at oracle.com <mailto:prem.balakrishnan at 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 at openjdk.java.net
> <mailto:awt-dev at 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.race at oracle.com <mailto:philip.race at 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 at openjdk.java.net
> <mailto:awt-dev at 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.rushforth at oracle.com
> <mailto:kevin.rushforth at 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
>
>
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20170509/4d2a2d80/attachment-0001.html>
More information about the awt-dev
mailing list