[OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
Philip Race
philip.race at oracle.com
Sat Feb 23 23:12:59 UTC 2019
I think the code as it is, is correct and safe.
The proposed optimisation would have very little measurable benefit
for something that creates such a dependency on knowing the implementation
of some other piece of code.
So we should push it as it is now.
-phil.
On 2/19/19, 9:15 PM, Shashidhara Veerabhadraiah wrote:
>
> Thank you Prashanta. This bug(NPE) is already a regression for an
> earlier bug and do not wish to cause one more in future for a poorly
> creation of the function definition especially when it is not implied
> by the function definition. That's my intent. The null check
> expression should not even take a single cpu cycle per me and running
> it in 4 mins is definitely fine I feel rather than creating a path for
> a future NPE(it's very easy to overlook per me when not implied).
>
> So any other volunteers for reviewing this bug.
>
> Thanks and regards,
>
> Shashi
>
> *From:*Prasanta Sadhukhan
> *Sent:* Monday, February 18, 2019 5:39 PM
> *To:* Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>; Philip Race
> <philip.race at oracle.com>
> *Cc:* 2d-dev at openjdk.java.net
> *Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print
> tests after JDK-8153732
>
> Hi Shashi,
>
> I think if the overhead can be fixed with a small change, we should do
> it as I think we need to keep the code optimized with whatever code is
> present now.
> If in future, if somebody changes the function order, like
> doCompare(prev..., curr..) to doCompare(curr..., prev..) [what you are
> trying to imply]
> then it needs to be done with proper reasoning (which I am not sure
> what it can be and then we can do these checks). That's my take. It
> other reviewers feel the present changes are ok, you can commit the
> changes with their approval.
>
> Regards
> Prasanta
>
> On 18-Feb-19 12:26 PM, Shashidhara Veerabhadraiah wrote:
>
> 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 <prasanta.sadhukhan at oracle.com>
> <mailto:prasanta.sadhukhan at oracle.com>; Philip Race
> <philip.race at oracle.com> <mailto:philip.race at oracle.com>
> *Cc:* 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 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
> <shashidhara.veerabhadraiah at oracle.com
> <mailto:shashidhara.veerabhadraiah at oracle.com>>; Philip Race
> <philip.race at oracle.com <mailto:philip.race at oracle.com>>
> *Cc:* 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 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 <philip.race at oracle.com>
> <mailto:philip.race at oracle.com>; Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>
> <mailto:shashidhara.veerabhadraiah at oracle.com>
> *Cc:* 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 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>
> <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
>
>
> 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 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/20190223/fb486af4/attachment-0001.html>
More information about the 2d-dev
mailing list