Thank you Prashanta. This bug(NPE) is already a regression for an
earlier bug and do not wish to cause one more in future for a poorly
creation of the function definition especially when it is not implied
by the function definition. That's my intent. The null check
expression should not even take a single cpu cycle per me and running
it in 4 mins is definitely fine I feel rather than creating a path for
a future NPE(it's very easy to overlook per me when not implied).
So any other volunteers for reviewing this bug.
Thanks and regards,
Shashi
*From:*Prasanta Sadhukhan
*Sent:* Monday, February 18, 2019 5:39 PM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>; Philip Race
<philip.r...@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 think if the overhead can be fixed with a small change, we should do
it as I think we need to keep the code optimized with whatever code is
present now.
If in future, if somebody changes the function order, like
doCompare(prev..., curr..) to doCompare(curr..., prev..) [what you are
trying to imply]
then it needs to be done with proper reasoning (which I am not sure
what it can be and then we can do these checks). That's my take. It
other reviewers feel the present changes are ok, you can commit the
changes with their approval.
Regards
Prasanta
On 18-Feb-19 12:26 PM, Shashidhara Veerabhadraiah wrote:
Hi Prasanta, I hope I have answered your questions satisfactorily.
Please let me know if you have any more questions.
Thanks and regards,
Shashi
*From:*Shashidhara Veerabhadraiah
*Sent:* Friday, February 15, 2019 12:37 PM
*To:* Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>; Philip Race
<philip.r...@oracle.com> <mailto:philip.r...@oracle.com>
*Cc:* 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 Prasanta, The parameters for the function does not tells the
order and is not implied by the way function definition is and
more over this function is called in an interval of minimum
refresh time(4 mins). Rather than adding overhead of confusion
since it is not implied by the function I think it is safe to keep
it that way. Depending on reviewer's comments is also not the
right way to depend on. The current definition avoids that but
with a small overhead and I think is not too much of a burden to bear.
Thanks and regards,
Shashi
*From:*Prasanta Sadhukhan
*Sent:* Friday, February 15, 2019 12:28 PM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com
<mailto:shashidhara.veerabhadra...@oracle.com>>; Philip Race
<philip.r...@oracle.com <mailto:philip.r...@oracle.com>>
*Cc:* 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 Shashi,
In this case, we know doCompare() is called from one location only
[and also is not a public method to be called by user] and the
check I mentioned is redundant. It's in while(true) loop so any
optimzation we can do to avoid unnecessary checks will be good in
my opinion.
If someone changed the doCompare() call without seeing the
implication or giving thought, then he has to face the
repurcussions, but I guess reviewers would be able to catch that
beforehand.
Regards
Prasanta
On 15-Feb-19 12:17 PM, Shashidhara Veerabhadraiah wrote:
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>
<mailto:philip.r...@oracle.com>; Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>
*Cc:* 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 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 returnenv->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
