[OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Fri Feb 15 06:40:51 UTC 2019
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.veerabhadraiah at oracle.com>
>> *Cc:* Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com>;
>> 2d-dev at 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.veerabhadraiah at oracle.com>
>> <mailto:shashidhara.veerabhadraiah at oracle.com>
>> *Cc:* Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com>
>> <mailto:prasanta.sadhukhan at oracle.com>; 2d-dev at openjdk.java.net
>> <mailto:2d-dev at 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.race at oracle.com>
>> <mailto:philip.race at oracle.com>
>> *Cc:* Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com>
>> <mailto:prasanta.sadhukhan at oracle.com>;
>> 2d-dev at openjdk.java.net <mailto:2d-dev at 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.veerabhadraiah at oracle.com
>> <mailto:shashidhara.veerabhadraiah at oracle.com>>
>> *Cc:* Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com
>> <mailto:prasanta.sadhukhan at oracle.com>>;
>> 2d-dev at openjdk.java.net <mailto:2d-dev at 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.veerabhadraiah at oracle.com>
>> <mailto:shashidhara.veerabhadraiah at oracle.com>; Prasanta
>> Sadhukhan <prasanta.sadhukhan at oracle.com>
>> <mailto:prasanta.sadhukhan at oracle.com>;
>> 2d-dev at openjdk.java.net <mailto:2d-dev at 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.veerabhadraiah at oracle.com>
>> <mailto:shashidhara.veerabhadraiah at oracle.com>;
>> Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com>
>> <mailto:prasanta.sadhukhan at oracle.com>;
>> 2d-dev at openjdk.java.net <mailto:2d-dev at 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.sadhukhan at oracle.com>
>> <mailto:prasanta.sadhukhan at oracle.com>;
>> Shashidhara Veerabhadraiah
>> <shashidhara.veerabhadraiah at oracle.com>
>> <mailto:shashidhara.veerabhadraiah at oracle.com>;
>> 2d-dev at openjdk.java.net
>> <mailto:2d-dev at 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.veerabhadraiah at oracle.com>
>> <mailto:shashidhara.veerabhadraiah at oracle.com>;
>> swing-dev at openjdk.java.net
>> <mailto:swing-dev at openjdk.java.net>;
>> 2d-dev at openjdk.java.net
>> <mailto:2d-dev at 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.veerabhadraiah at oracle.com
>> <mailto:shashidhara.veerabhadraiah at 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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20190215/602a9122/attachment-0001.html>
More information about the 2d-dev
mailing list