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 <[email protected]>; Philip Race
<[email protected]>
*Cc:* [email protected]
*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
<[email protected]
<mailto:[email protected]>>; Philip Race
<[email protected] <mailto:[email protected]>>
*Cc:* [email protected] <mailto:[email protected]>
*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 <[email protected]>
<mailto:[email protected]>; Shashidhara Veerabhadraiah
<[email protected]>
<mailto:[email protected]>
*Cc:* [email protected] <mailto:[email protected]>
*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
<[email protected]>
<mailto:[email protected]>
*Cc:* Prasanta Sadhukhan <[email protected]>
<mailto:[email protected]>;
[email protected] <mailto:[email protected]>
*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
<[email protected]>
<mailto:[email protected]>
*Cc:* Prasanta Sadhukhan
<[email protected]>
<mailto:[email protected]>;
[email protected] <mailto:[email protected]>
*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 <[email protected]>
<mailto:[email protected]>
*Cc:* Prasanta Sadhukhan
<[email protected]>
<mailto:[email protected]>;
[email protected]
<mailto:[email protected]>
*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
<[email protected]
<mailto:[email protected]>>
*Cc:* Prasanta Sadhukhan
<[email protected]
<mailto:[email protected]>>;
[email protected]
<mailto:[email protected]>
*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
<[email protected]>
<mailto:[email protected]>;
Prasanta Sadhukhan
<[email protected]>
<mailto:[email protected]>;
[email protected]
<mailto:[email protected]>
*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
<[email protected]>
<mailto:[email protected]>;
Prasanta Sadhukhan
<[email protected]>
<mailto:[email protected]>;
[email protected]
<mailto:[email protected]>
*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
<[email protected]>
<mailto:[email protected]>;
Shashidhara Veerabhadraiah
<[email protected]>
<mailto:[email protected]>;
[email protected]
<mailto:[email protected]>
*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
<[email protected]>
<mailto:[email protected]>;
[email protected]
<mailto:[email protected]>;
[email protected]
<mailto:[email protected]>
*Subject:* Re: [OpenJDK
2D-Dev] [12] JDK-8212202: NPE
in the print tests after
JDK-8153732
On 26-Nov-18 6:51 PM,
[email protected]
<mailto:[email protected]>
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
