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>
*Cc:* Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com>;
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
