RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Roger Riggs]

Hi,

A few higher level issues should be considered, though the details of 
the webrev
captured my immediate attention.

Is this the right feature and is this the right level of implementation 
(C++/native)?

 From a security perspective, adding field names to exceptions exposes
information to callers that they could not otherwise get and that breaks 
encapsulation.
That needs to be evaluated.


I think there are ways to make this easier to implement and be easier to 
maintain
and perform better.

NullPointerException:

28: unused import of Field

64: The lazyComputeMessage boolean should be inverted so it does not require
    initialization, false is the default, anything else adds overhead.
    And it may not be needed.

91: Why do you think synchonization is necessary?  It is a performance hit.
    It is highly unlikely to be called from multiple threads and the 
value will
    be the same whether it is computed once or multiple times by 
different threads.

99: extendedMessage will never be null (so says the javadoc of 
getExtendedNPEMessage)
   Bug: If it does return null, then null is returned from getMessage
   regardless of the value of super.getMessage().

121: Simplify getExtendedNPEMessage by making it only responsible for 
the detail
   and not concatenation with an existing message.  Put that in 
getMessage().

   Its not strictly necessary to change the updated message with 
setMessage().
   Avoiding that will avoid complexity and a performance hit.
   The message will be computed each time it is needed, and that happens 
infrequently.
   (Even in the serialization case, it will be re-computed from the 
attached stack frames).

   And it avoids adding setMessage() to Throwable, that's significant 
change since
   the current implementation only allows the message to be initialized 
not updated.
   Immutable is an important characteristic to maintain.

   That makes the writeReplace() unnecessary too.

Additional command line arguments are an unnecessary complexity,
documentation, and maintenance overhead.  If the feature is as valuable as
you suggest, it should be enabled all the time.

I'm assuming there are no tests broken by the modification of the message.
It is likely that somewhere an application or test looks at the message 
itself.
Has that been verified?

I can't speak for the hotspot code, but that seems to add a lot of code 
to support
only this convenience feature.  That's a maintenance overhead and burden.

The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a 
question
about how useful the information is,  developers should not need to know
about "slot 1".

The test output of NullPointerExceptionTest shows 
"java.lang.Object.hashCode(()I)".
Why is the argument type for Integer shown as "()I";  that's not the 
conventional
representation of a primitive int.

Generally, the explanations are too verbose.
Method names and field names would be easier to recognize if they were
quoted, as is done with 'this'.  Argument numbers should be digits,
not English words (first, second, etc) to make them easier to pick out.
And has it limits that do not read easily, e.g. "nr. 9".
I would not describe 'this' as a local variable.

Tests, especially new tests should compile without warnings.
NullPointerExceptionTest.java:125 newInstance has been deprecated.

The messages can be easier to understand, e.g.
'field a1 is null, trying to invoke a1.hashCode...' instead of:

"while trying to invoke the method java.lang.Object.hashCode(()I) on a 
null reference loaded from local variable 'a1'"

It will easier to understand if it looks more like the code.
BTW, what does this look like for fully optimized code?
Does it bear any resemblance to the source code at all?
Or does it have to be deoptimized to come up with a sensible source 
reference.

How much of this can be done in Java code with StackWalker and other 
java APIs?
It would be a shame to add this much native code if there was a more robust
way to implement it using APIs with more leverage.

Regards, Roger


On 02/08/2019 09:13 AM, Langer, Christoph wrote:
> Hi Goetz,
>
>> ok, so here a new webrev just adding a setter for the field:
>> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/02
>>
>> ... and incorporating the other comments.
> Looks better ��
>
> I have a few additions to src/java.base/share/classes/java/lang/NullPointerException.java:
>
> 28 import java.lang.reflect.Field;
> 29
> -> can be removed now.
>
> 91         synchronized (this) {
> -> I think this is not needed here. The transient modifier for lazyComputeMessage already implies the lock on the Object, I think.
>
> 108         return extendedMessage;
> -> I guess it would be more correct if you returned super.getMessage() here.
>
> Thanks
> Christoph