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>
*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
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
