RFR: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

[Daniel Fuchs]

Hi Mandy,

Thanks for all the suggestions!

On 07/02/17 20:03, Mandy Chung wrote:
>> Please find below a new webrev that incorporates your feedback.
>> http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.03/
>>
>
> I think this sentence is not needed.
>  216          * <p>A {@code StackWalker} with this {@code SHOW_REFLECT_FRAMES} option
>  217          * will show all reflection frames.

I disagree: the text starts by saying:
"By default reflection frames are hidden. The reflection
  frames include [...]"
but the sentence at lines 216-217 is the only place where we say what
the option actually does: it shows all reflection frames.
I think it's needed.
Possibly the sentence at lines 216-217 could be moved to become
the first sentence in the doc comment, but I'd rather not change
it now that the CCC has approved the doc changes as in webrev.03.


> Basic.java test - the @bug already includes the bug number.  line 67-69 can be removed.

Done.

> Minor comments: StackFrame.getClassName() can replace StackFrame.getDeclaringClass().getName().  Or if you want to compare package name, StackFrame.getDeclaringClass().getPackageName is an alternative.

Oh - good remark. StackFrame.getClassName() it is.

> Nit: what about renaming SimpleObj to ConstructorNewInstance?
> SimpleObj::found should probably name as `framesInUnnamedOrReflectionPackage`.

SimpleObj => ConstructorNewInstance
found => testFramesOrReflectionFrames
          (frames in the unnamed package are test frames)

I added some comments as well in the accept() method.

> Nit: ReflectionFrames.java test - the test is a bit hard maybe SimpleObj can be renamed to TestHelper and `found` be `result`.   Just trying to make the test easier to read and understand.  You may have better idea.

SimpleObj => StackInspector (it calls walk & getCallerClass
              to inspect the stack from its constructor)
found => collectedFrames

Also added some comments to better explain the purposes of
StackInspector and StackInspector.Caller.

Here is the new webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.04/

best regards,

-- daniel
>
> Otherwise looks good.
>
> Mandy
>