[OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
Phil Race
philip.race at oracle.com
Thu Feb 14 21:18:40 UTC 2019
+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/
>
> 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/20190214/ec30f1a4/attachment-0001.html>
More information about the 2d-dev
mailing list