[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
From: Phil Race <philip.race () oracle ! com>
Date: 2018-11-28 17:49:28
Message-ID: 20b92921-f7e8-3821-c889-9dbffd1dc53d () oracle ! com
[Download RAW message or body]
I am not understanding you. I thought the problem to be we got an array
of (say) 3 values
(ie printer names) returned from native where some or all of the
*values* were NULL.
And I am saying we should in such a case in the native code, before
returning,
remove from the returned array all values that are NULL.
That could result in an empty (zero length) array being returned from
native but
no null names ..
-phil.
On 11/26/18 10:23 PM, Shashidhara Veerabhadraiah wrote:
>
> The windows function EnumPrinters() returns a value that could be zero
> or greater. If it is zero we have no other option but to return null
> from the function. I do not think there is a way where we can always
> make sure we get a non-zero value considering the various system
> scenarios. So we should handle the null return values as well in the
> caller of this function I think.
>
> Here is the updated Webrev for test fix:
>
> http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.01/
>
> Thanks and regards,
> Shashi
>
> *From:*Phil Race
> *Sent:* Tuesday, November 27, 2018 1:52 AM
> *To:* Prasanta Sadhukhan <prasanta.sadhukhan@oracle.com>; Shashidhara
> Veerabhadraiah <shashidhara.veerabhadraiah@oracle.com>;
> 2d-dev@openjdk.java.net
> *Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print
> tests after JDK-8153732
>
> [Removed swing-dev as this as nothing to do with swing].
>
> You mention in the bug eval that you don't need a new test because this
> is already covered by the test for 8153732. If that is the case then this
> bugid should be added to that test.
> Although it also looks like there are plenty of tests that provoke this ..
> if all you need is a system without any printers then this is a
> serious regression.
>
> I am not sure I am following why doCompare() is the place to fix this.
> If getRemotePrinterNames() is returning NULL strings, then maybe it
> should not !
>
> I suggest to fix it there.
>
> -phil.
>
>
> On 11/26/18 7:51 AM, Prasanta Sadhukhan wrote:
>
> I am not against doCompare() changes. I am just saying run()
> changes are not needed.
>
> Regards
> Prasanta
>
> On 26-Nov-18 9:15 PM, Shashidhara Veerabhadraiah wrote:
>
> There is a possible case and that is the reason for this fix.
> The possible states for prevRemotePinters and
> currentRemotePrinters are null and valid values and they can
> reach those states at any time either because of network
> disconnect or any other OS related changes. With that in mind,
> this fix tries to eliminate the possible NPE cases.
>
> Thanks and regards,
>
> Shashi
>
> *From:*Prasanta Sadhukhan
> *Sent:* Monday, November 26, 2018 8:01 PM
> *To:* Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah@oracle.com>
> <mailto:shashidhara.veerabhadraiah@oracle.com>;
> swing-dev@openjdk.java.net
> <mailto:swing-dev@openjdk.java.net>; 2d-dev@openjdk.java.net
> <mailto:2d-dev@openjdk.java.net>
> *Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the
> print tests after JDK-8153732
>
> On 26-Nov-18 6:51 PM, shashidhara.veerabhadraiah@oracle.com
> <mailto:shashidhara.veerabhadraiah@oracle.com> wrote:
>
> Hi Prasanta, I think we should not create a behavior
> across the functions. doCompare() does only the comparison
> and it may be used for other purposes and is complete with
> respect to the comparison functionality.
>
> run() function has a different behavior as it needs to
> populate the prevRemotePrinters and then the
> currentRemotePrinters and then use the comparison
> functionality. I think this is a good way to do.
>
> Even without the if-else check, it does populates the
> prevRemotePrinters, no?
> if "prevRemotePrinters" is null and currentRemotePrinters is
> null, then no need to update. If currentRemotePrinters is
> null, then what is the point of using getRemotePrintersNames()
> to update prevRemotePrinters as currentRemotePrinters is also
> obtained from getRemotePrintersNames!!
> If "prevRemotePrinters" is null and currentRemotePrinters is
> not null, then doCompare() called from run() will be true and
> prevRemotePrinters will be populated with currentRemotePrinters.
> If "prevRemotePrinters" is not null and currentRemotePrinters
> is null, then also prevRemotePrinters will be populated with
> currentRemotePrinters.
>
> Regards
> Prasanta
>
>
> Thanks and regards,
>
> Shashi
>
> On 26/11/18 6:03 PM, Prasanta Sadhukhan wrote:
>
> Hi Shashi,
>
> I think l437 check of if-else *if (prevRemotePrinters
> != null) {*
>
> is not required. prevRemotePrinters null check is
> addressed in str1==null case in doCompare().
> If prevRemotePrinters is null and
> currentRemotePrinters is not null, then you update
> prevRemotePrinters to currentRemotePrinters as per
> l415 where doCompare returns true.
> Also, If prevRemotePrinters is not null and
> currentRemotePrinters is null, then also you update
> prevRemotePrinters to currentRemotePrinters which is
> the output of getRemotePrintersNames().
>
> Regards
> Prasanta
>
>
> On 26-Nov-18 2:33 PM, Shashidhara Veerabhadraiah wrote:
>
> Hi All, Please review a NPE fix for the below bug.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8212202
>
> Webrev:
> http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.00/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.00/>
>
> Function getRemotePrintersNames() may return null
> values and hence they need to be handled from the
> caller of that function which was missing earlier.
> This fix handles the null return values of the
> said function.
>
> Thanks and regards,
>
> Shashi
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
I am not understanding you. I thought the problem to be we got an
array of (say) 3 values<br>
(ie printer names) returned from native where some or all of the
*values* were NULL.<br>
And I am saying we should in such a case in the native code, before
returning,<br>
remove from the returned array all values that are NULL.<br>
That could result in an empty (zero length) array being returned
from native but<br>
no null names .. <br>
<br>
-phil.<br>
<br>
<div class="moz-cite-prefix">On 11/26/18 10:23 PM, Shashidhara
Veerabhadraiah wrote:<br>
</div>
<blockquote type="cite"
cite="mid:b575ed4e-1d1d-4c19-bfa1-513e17ee285f@default">
<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:PMingLiU;
panose-1:2 1 6 1 0 1 1 1 1 1;}
@font-face
{font-family:Tunga;
panose-1:0 0 4 0 0 0 0 0 0 0;}
@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;}
@font-face
{font-family:"\@PMingLiU";
panose-1:0 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:"Times New Roman \,serif";}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
span.new
{mso-style-name:new;}
span.EmailStyle19
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle20
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle21
{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">The windows
function EnumPrinters() returns a value that could be zero
or greater. If it is zero we have no other option but to
return null from the function. I do not think there is a way
where we can always make sure we get a non-zero value
considering the various system scenarios. So we should
handle the null return values as well in the caller of this
function I think.<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">Here is the
updated Webrev for test fix:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><a
href="http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.01/</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">Thanks and
regards,<br>
Shashi<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"
moz-do-not-send="true"><span style="color:#1F497D"><o:p> \
</o:p></span></a></p> <div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span \
style="color:windowtext">From:</span></b><span style="color:windowtext"> Phil Race \
<br> <b>Sent:</b> Tuesday, November 27, 2018 1:52 AM<br>
<b>To:</b> Prasanta Sadhukhan
<a class="moz-txt-link-rfc2396E" \
href="mailto:prasanta.sadhukhan@oracle.com"><prasanta.sadhukhan@oracle.com></a>; \
Shashidhara Veerabhadraiah
<a class="moz-txt-link-rfc2396E" \
href="mailto:shashidhara.veerabhadraiah@oracle.com"><shashidhara.veerabhadraiah@oracle.com></a>;
<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] [12] JDK-8212202:
NPE in the print tests after JDK-8153732<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal" style="margin-bottom:12.0pt">[Removed
swing-dev as this as nothing to do with swing].<br>
<br>
You mention in the bug eval that you don't need a new test
because this<br>
is already covered by the test for 8153732. If that is the
case then this<br>
bugid should be added to that test.<br>
Although it also looks like there are plenty of tests that
provoke this ..<br>
if all you need is a system without any printers then this is
a serious regression.<br>
<br>
I am not sure I am following why doCompare() is the place to
fix this.<br>
If getRemotePrinterNames() is returning NULL strings, then
maybe it should not !<br>
<br>
I suggest to fix it there.<br>
<br>
-phil.<br>
<br>
<br>
<span style="font-size:12.0pt"><o:p></o:p></span></p>
<div>
<p class="MsoNormal">On 11/26/18 7:51 AM, Prasanta Sadhukhan
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p>I am not against doCompare() changes. I am just saying
run() changes are not needed.<o:p></o:p></p>
<p class="MsoNormal">Regards<br>
Prasanta<o:p></o:p></p>
<div>
<p class="MsoNormal">On 26-Nov-18 9:15 PM, Shashidhara
Veerabhadraiah wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="color:#1F497D">There is a
possible case and that is the reason for this fix. The
possible states for prevRemotePinters and
currentRemotePrinters are null and valid values and they
can reach those states at any time either because of
network disconnect or any other OS related changes. With
that in mind, this fix tries to eliminate the possible
NPE cases.</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">Thanks and
regards,</span><o:p></o:p></p>
<p class="MsoNormal"><span \
style="color:#1F497D">Shashi</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 #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span \
style="color:windowtext">From:</span></b><span style="color:windowtext"> Prasanta \
Sadhukhan <br> <b>Sent:</b> Monday, November 26, 2018 8:01 PM<br>
<b>To:</b> Shashidhara Veerabhadraiah <a
href="mailto:shashidhara.veerabhadraiah@oracle.com"
\
moz-do-not-send="true"><shashidhara.veerabhadraiah@oracle.com></a>; <a \
href="mailto:swing-dev@openjdk.java.net"
moz-do-not-send="true">swing-dev@openjdk.java.net</a>;
<a href="mailto:2d-dev@openjdk.java.net"
moz-do-not-send="true">2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: [OpenJDK 2D-Dev] [12]
JDK-8212202: NPE in the print tests after
JDK-8153732</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p><span style="font-size:12.0pt"> </span><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<div>
<p class="MsoNormal">On 26-Nov-18 6:51 PM, <a
href="mailto:shashidhara.veerabhadraiah@oracle.com"
moz-do-not-send="true">shashidhara.veerabhadraiah@oracle.com</a>
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p>Hi Prasanta, I think we should not create a behavior
across the functions. doCompare() does only the
comparison and it may be used for other purposes and is
complete with respect to the comparison functionality.<o:p></o:p></p>
<p>run() function has a different behavior as it needs to
populate the prevRemotePrinters and then the
currentRemotePrinters and then use the comparison
functionality. I think this is a good way to do.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal">Even without the if-else check, it does
populates the prevRemotePrinters, no? <br>
if "prevRemotePrinters" is null and currentRemotePrinters
is null, then no need to update. If currentRemotePrinters
is null, then what is the point of using
getRemotePrintersNames() to update prevRemotePrinters as
currentRemotePrinters is also obtained from
getRemotePrintersNames!!<br>
If "prevRemotePrinters" is null and currentRemotePrinters
is not null, then doCompare() called from run() will be
true and prevRemotePrinters will be populated with
currentRemotePrinters.<br>
If "prevRemotePrinters" is not null and
currentRemotePrinters is null, then also
prevRemotePrinters will be populated with
currentRemotePrinters.<br>
<br>
Regards<br>
Prasanta<br>
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p>Thanks and regards,<o:p></o:p></p>
<p>Shashi<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<div>
<p class="MsoNormal">On 26/11/18 6:03 PM, Prasanta
Sadhukhan wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p>Hi Shashi,<o:p></o:p></p>
<p class="MsoNormal">I think l437 check of if-else <span
class="new"><b><span
style="font-size:10.0pt;font-family:Consolas;color:blue">if
(prevRemotePrinters != null) \
{</span></b></span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:10.0pt;font-family:Consolas"> \
</span><o:p></o:p></p> <p class="MsoNormal">is not required. prevRemotePrinters
null check is addressed in str1==null case in
doCompare().<br>
If prevRemotePrinters is null and
currentRemotePrinters is not null, then you update
prevRemotePrinters to currentRemotePrinters as per
l415 where doCompare returns true.<br>
Also, If prevRemotePrinters is not null and
currentRemotePrinters is null, then also you update
prevRemotePrinters to currentRemotePrinters which is
the output of getRemotePrintersNames().<br>
<br>
Regards<br>
Prasanta<br>
<br>
<br>
<o:p></o:p></p>
<div>
<p class="MsoNormal">On 26-Nov-18 2:33 PM, Shashidhara
Veerabhadraiah wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal">Hi All, Please review a NPE fix
for the below bug.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212202"
\
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212202</a><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Webrev: <a
\
href="http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.00/"
\
moz-do-not-send="true">http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.00/</a><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Function getRemotePrintersNames()
may return null values and hence they need to be
handled from the caller of that function which was
missing earlier. This fix handles the null return
values of the said function.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Thanks and regards,<o:p></o:p></p>
<p class="MsoNormal">Shashi<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman \,serif""> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman \,serif""> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman \,serif""> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic