Hi Prasanta, doCompare(str1, str2) is a different function and one
should not define a function based on how it is going to be called!!
What if someone changed the caller to this:
doCompare(currentRemotePrinters, prevRemotePrinters). There is no
restriction if one calls like this. Here the function is taking 2
strings and it does not say which one should be passed at what
position. Probably if the function takes different parameters then it
sets an automatic rule on which parameter needs to be passed at which
position but otherwise function definition should take care this.
Hope you agree with me.
Thanks and regards,
Shashi
*From:*Prasanta Sadhukhan
*Sent:* Friday, February 15, 2019 12:11 PM
*To:* Phil Race <philip.r...@oracle.com>; Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
*Cc:* 2d-dev@openjdk.java.net
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print
tests after JDK-8153732
Hi Shashi,
I have one comment:
doCompare(prevRemotePrinters, currentRemotePrinters) is only called
from run() method when "prevRemotePrinters" is already checked to be
not null [*if (prevRemotePrinters != null && prevRemotePrinters.length
> 0) ]*
so I see no point in checking "str1" [which is prevRemotePrinters] to
be null in doCompare(). I guess instead of
*413 if (str1 == null && str2 == null) {*
*414 return false;*
*415 } else if (str1 == null || str2 == null) {*
*416 return true;*
*417 }*
you can have
if (str2 == null)
return true;
Regards
Prasanta
On 15-Feb-19 2:48 AM, Phil Race wrote:
+1 .. remember to use the current bug synopsis in the push comment
ie : "[Windows] Exception if no printers are installed."
-phil.
On 2/11/19 12:39 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the new Webrev fixing those comments:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.06/
<http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.06/>
Thanks and regards,
Shashi
*From:*Philip Race
*Sent:* Saturday, February 9, 2019 2:25 AM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>
*Cc:* Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>;
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
413 if (str1 == null && str2 == null) {
414 return false;
415 } else if ((str1 == null && str2 != null) ||
(str2 == null && str1 != null)) {
416 return true;
417 }
When we get to 415 we already know at least one of str1 or
str2 is non-null so 415 can just be
} else if (str1 == null || str2 == null) {
-phil.
On 2/6/19, 12:31 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the updated Webrev fixing those comments:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.05/
<http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.05/>
Thanks and regards,
Shashi
*From:*Phil Race
*Sent:* Thursday, January 31, 2019 4:36 AM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>
*Cc:* Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>;
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
Sorry .. this got lost ..
I don't understand this line :
253 jobjectArray emptyArray = env->NewObjectArray(1,
clazz, NULL);
This is returning an array of length 1 and element 0 is NULL.
I think you want
env->NewObjectArray(0, clazz, NULL);
and I don't see why you need to create it there
instead you can just have
304 if (nameArray != NULL) {
305 return nameArray;
306 } else {
307 return env->NewObjectArray(0, clazz, NULL);
308 }
449 if (prevRemotePrinters != null) {
shouldn't this be
449 if (prevRemotePrinters != null &&
prevRemotePrinters.length > 0) {
?
-phil.
On 12/10/18 10:19 PM, Shashidhara Veerabhadraiah wrote:
Hi Phil, I have updated with small code updates:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.04/
<http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.04/>
Thanks and regards,
Shashi
*From:*Shashidhara Veerabhadraiah
*Sent:* Monday, December 10, 2018 3:19 PM
*To:* Philip Race <philip.r...@oracle.com>
<mailto:philip.r...@oracle.com>
*Cc:* Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>;
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
Hi Phil, Please find the updated Webrev.
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.03/
<http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.03/>
Thanks and regards,
Shashi
*From:*Philip Race
*Sent:* Friday, December 7, 2018 8:54 PM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com
<mailto:shashidhara.veerabhadra...@oracle.com>>
*Cc:* Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com
<mailto:prasanta.sadhuk...@oracle.com>>;
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
First off, I think the high order bit is that if a
non-null array reference is returned there are NO
null String entries in it. Whether a zero length or
null array is returned when there are no printers
is the less important issue.
However an empty array also tells you there are no
printers, and you are less likely to get an NPE from that.
It is arguably easier for the caller, if they don't
need the extra null check first.
FWIW the javax.print public APIs return zero length
arrays instead of null and applications seem to
survive :-)
I don't know what you mean by :
> (And anyway we need to handle the first null string
reference)?
If you are referring to a small matter of coding in
the native layer, then that is not an insurmountable
problem.
Basically if there are problems getting names or
whatever in the native layer, handle it THERE, don't make
everyone else have to deal with it.
One caveat: JNI calls can theoretically fail due to
OOME .. in such a case we are doomed and
the main goal is to not crash in native and to obey
all JNI rules. What is returned in that case
can be a null array reference and an NPE in a
Java-level user of that reference in such a case is
not a big deal.
-phil
On 12/6/18, 8:10 PM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Is it advisable to return zero length
array from native? The null return from native is
telling the caller that there are no printers
connected to the system. To tell this should we
allocate a zero length array containing null back
to the caller(And anyway we need to handle the
first null string reference)? Handling the null on
the caller isn’t an easier option?
Thanks and regards,
Shashi
*From:*Phil Race
*Sent:* Thursday, December 6, 2018 2:35 AM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>;
Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>;
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
But what I am saying is we should not return a
NULL string reference
from native. You are still allowing that and then
having to handle
it in Java code.
And FWIW you can return a zero length array as
well so there
isn't a NULL array reference to deal with either.
-phil.
On 12/3/18 8:29 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, There were 2 problems earlier. One is
that, from the native it is possible to have
no printers being associated with the
system(causing to return null reference) and
other problem in the implementation was that
there may be no network printers and still
return a valid array reference containing a
null string. The first problem is fixed by
handling null references returned from this
function and other problem was that earlier
there were different base conditions, for
updating the reference and use it to create
the printer name array which could produce a
valid reference pointing to null string. Here
is the updated Webrev which fixes these problems:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.02/
<http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.02/>
The big problem was that I was not able to
reproduce this problem neither at my end nor
at the systems used for PIT testing. Only
Sergey had produced this NPE till now.
Thanks and regards,
Shashi
*From:*Phil Race
*Sent:* Wednesday, November 28, 2018 11:19 PM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>;
Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>;
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
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/
<http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.01/>
Thanks and regards,
Shashi
*From:*Phil Race
*Sent:* Tuesday, November 27, 2018 1:52 AM
*To:* Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>;
Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>;
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
[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.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>;
swing-...@openjdk.java.net
<mailto:swing-...@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.veerabhadra...@oracle.com
<mailto:shashidhara.veerabhadra...@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
