Thoughts on adding getElementClass() method to StackTraceElement?

[Nick Williams]

On Jun 16, 2013, at 9:29 AM, Nick Williams wrote:

> 
> On Jun 16, 2013, at 1:25 AM, Jeroen Frijters wrote:
> 
>> Nick Williams wrote:
>>> I'm going to stick by my original assessment that I'm not convinced
>>> there's a security issue. It's possible that getClassContext() filters
>>> out classes the caller can't access, but nothing in the documentation
>>> indicates that's the case, so I'm operating under the assumption that it
>>> doesn't.
>> 
>> SecurityManager.getClassContext() is not available to unpriviliged callers, so I don't think this a valid argument.
> 
> Can you define what "is not available to unprivileged callers" means a little better? We use it in Log4j 2 when getCallerClass() isn't available, and it works just fine. I took a look at the Java code, and it's native, so there's no check there. (You can't instantiate a SecurityManager unless there's not currently a SecurityManager installed _OR_ the installed SecurityManager allows the creation (installation privilege isn't checked) of another SecurityManager. Is that what you're talking about?) I took a look at the native code, and I don't see anything there that would represent a privilege check that I recognized, but if I understood what you meant a little better I might see where I'm wrong.
> 
>> Given the security implications, the serialization issue and the need for weak references, it seems to me that adding this to Throwable would be way too expensive. 
> 
> The only expense I'm seeing here is the security implications, and I'm still not convinced that they exist. We have all agreed now that making this transient is okay, and adding the transient keyword to a field only takes 5 seconds. And I certainly don't believe that declaring the field as "WeakReference<Class<?>> elementClass" instead of "Class<?> elementClass" takes more than 30 seconds longer, but maybe I'm missing something, so that's not expensive either.
> 
> Also, to be clear, I'm not suggesting adding it to Throwable specifically, I'm suggesting adding it to StackTraceElement (which would apply anywhere that a stack trace is filled in, including Throwable). A stack trace is filled in with one native code method, so updating that method will make this work in Throwable, Thread, and anywhere else that you can get a stack trace. I know this is a nitpicky distinction, but it could be important.
> 
>> 
>> Adding a new API to collect a stack trace seems like a far better approach.
>> 
>> Here's my off the cuff proposal:
>> 
>> public final class StackFrame {
>> public Executable method();
>> public String getFileName();
>> public int getLineNumber();
>> 
>> public static StackFrame[] capture(int skipFrames, int maxLength, boolean includeSourceInfo);
>> }
>> 
> 
> I'm not opposed to the StackFrame class as suggested, it just doesn't meet my needs. At all. I can already successfully get this information using the stack trace and getClassContext(), so I wouldn't be compelled to switch. I need to efficiently get the Class from a stack trace generated by a Throwable, which this does not solve. Right now I'm having to resort to getting that Class using inefficient methods.

What if we also added a getStackFrames() method to Throwable? That would meet my needs but it would also satisfy what I'm observing is a desire to have a new API for this (StackFrame) instead of adding it to StackTraceElement. I'm very open to how it's implemented, as long as it satisfies my use case. :-)

The stack trace of a Throwable can be "filled in" on demand when getStackTrace() is called the first time, so that the overhead isn't incurred when creating and throwing the exception. Presumably, we would need to do something similar with getStackFrames(), especially since calling it would be less common.

Thoughts on this?