[OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
Philip Race
philip.race at oracle.com
Fri Feb 8 20:54:45 UTC 2019
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>
> *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
>
> 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.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/20190208/84a79ad9/attachment-0001.html>
More information about the 2d-dev
mailing list