[PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8

[Mandy Chung]

Just realize that I missed a few words in a sentence that could mislead 
what I really mean.... correction below.

On 9/3/2013 5:02 PM, Mandy Chung wrote:
> Nick,
>
> I skimmed through the changes.  Congratulations for your first patch 
> making changes in both hotspot and jdk code.
>
> In my mind, the Log4J use case accessing Class instance to obtain 
> additional information for diagnosability is different than the use 
> case of obtaining the caller's class / loader to lookup resources.  
> While the new APIs might be in the same class, I will discuss them 
> individually and keep us focus one at a time.
>
> Ralph has pointed out [1] that Log4j also needs the ability to find an 
> appropriate ClassLoader which is used for logging separation (thank 
> you Ralph). That will be the caller-sensitivity (i.e. obtain caller's 
> class/loader) discussion.
>
> There are a couple of RFEs:
> https://bugs.openjdk.java.net/browse/JDK-4942697
> https://bugs.openjdk.java.net/browse/JDK-6349551
>
> Performance is critical for Log4j to traverse the stack trace and 
> obtain Class information.  I like your patch to speed up the 
> generation of StackTraceElement[] (and StackTraceFrame[] - essentially 
> same code with different type). java.util.logging.LogRecord has 
> workaround the performance overhead and go to a specific frame 
> directly and avoid the cost of generating the entire array. 
> JDK-6349551 requests to lazily instantiate the StackTraceElement 
> object unless that frame is requested.  Does Log4J always walk all 
> frames and log all information?  Do you just log only some max number 
> of frames rather than the entire stack trace?
>
> Class<?> getDeclaringClass() method is the key method you need to 
> enhance the diagnosability.  This method opens up a new way to access 
> a Class instance that untrusted code wouldn't be able in the past.  A 
> permission check is needed as Alan points out early.  Performance as 
> well as logging framework can't access all class loaders are two 
> factors to be considered when defining the permission check.
>
> I saw your other comment about permission check (cut-n-paste below).  
> It seems to me that you don't agree a permission check is needed for 
> the getDeclaringClass() method (regardless of which class it belongs 
> to) and if that's the case, no point to continue.
>
> I want to make it very clear that I have agreed to take this on and 
> provide a replacement of sun.reflect.Reflection.getCallerClass(int) in 
> JDK 8 to address the use cases.  It will take time for the API and 
> security discussion and please be prepared for that (also I am working 
> on other things at the same time).
>

What I really meant is:

I want to make it very clear that I have agreed to take this to come to 
a conclusion and ensure to provide a replacement of 
sun.reflect.Reflection.getCallerClass(int) in JDK 8 to address the use 
cases.  It will take time for the API and security discussion and please 
be prepared for that (also I am working on other things at the same time).

Mandy

> The second comment on your patch is that there are lot of duplicated 
> code in hotspot in order to support two similar but different types 
> (StackTraceFrame and StackTraceElement). Serialization is the reason 
> leading you to have a new StackTraceFrame class.  Maybe some 
> refactoring can help but this is the cost of having the VM directly 
> creating the instance.  One other option, as suggested in the previous 
> thread, is to keep the declaring class in the StackTraceElement as a 
> transient field.  If we add the getDeclaringClass method in the 
> StackTraceElement class, it would need to specify to be optional that 
> it only returns the Class instance when it's available.
>
> There are currently three different ways to get a stack trace:
> 1. Throwable.getStackTrace()
> 2. Thread.getStackTrace() and Thread.getAllStackTraces()
> 3. java.lang.management.ThreadMXBean.getThreadInfo(long id, int 
> maxDepth).getStackTrace() and multiple thread IDs version
>   (a) local (b) remote
>
> Since it's a new StackTraceFrame class, you have to provide a new 
> method replacing #1 and #2.  I don't see any need to provide a new API 
> equivalent to Thread.getAllStackTraces() and java.lang.management.
>
> The proposal I originally have last week was to keep declaring class 
> as transient and add a method in StackTraceElement with a permission 
> check at every call. Tom raises a good question about the cost of 
> permission check.  Would that be a concern to Log4j? Is log4j on 
> bootclasspath or extension directory?  I assume not. So for log4j to 
> work with security manager installed, it would have torequire the 
> application to grant certain permissions - can you confirm?  For 
> example it calls sun.reflect.Reflection.getCallerClass method that 
> will require RuntimePermission("accessClassInPackage.sun.reflect") 
> permission. Calling Class.getProtectionDomain and 
> Class.getClassLoader() requires 
> RuntimePermission("getProtectionDomain") and
> RuntimePermission("getClassLoader") respectively.  That gives me an 
> impression that permission check on individual stack frame might be a 
> non-issue?
>
> Mandy
> [1] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-September/020525.html
>
> On 9/3/13 5:24 AM, Nick Williams wrote:
>> On Sep 3, 2013, at 4:07 AM, Alan Bateman wrote:
>>>> I'm not voicing any objection to any kind of security/permissions 
>>>> checks in these methods. Before I can pass judgement one way or 
>>>> another, I'd want to know 1) specifically what type of permissions 
>>>> check you are looking for, and 2) what you're looking to achieve 
>>>> with said permissions check.
>>> I would say this is TBD and start by asking the question as to 
>>> whether there are concerns about leaking reference to Class objects 
>>> that untrusted code wouldn't normally be able to get a reference to. 
>>> Tom brings up the cost of the permission check and also whether any 
>>> API should be tied to class loader. There are clearly discussion 
>>> points here that could potentially influence the API.
>> As I have said before, there are MANY ways to get a Class object that 
>> aren't security checked. It's when you try to USE that class object 
>> to impersonate it or invoke methods that security checks begin to 
>> happen. As they should!
>>
>> Nick
>
>
>
> On 9/1/13 1:16 AM, Nick Williams wrote:
>> I have completed and am proposing a patch for replacing 
>> sun.reflect.Reflection#getCallerClass(...) with a public API in Java 
>> 8. I saw no point in duplicating an issue, even though it's over 10 
>> years old, so I'm indicating that this is a patch for 4851444 
>> (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4851444).
>>
>> I welcome and solicit support/+1s and a sponsor. Someone about a 
>> month ago had mentioned that they would be willing to be a sponsor if 
>> the patch looked good, but I can't remember who it was and I can't 
>> find the email. I want to say it was someone with RedHat, but my 
>> memory could be faulty, so please don't hold it against me if I'm wrong.
>>
>> *Summary of Changes*
>> Added the new class java.lang.StackTraceFrame. It's a virtual mirror 
>> of StackTraceElement, except that it contains a Class<?> 
>> declaringClass property instead of a String className property. Since 
>> the list members expressed reluctance to add methods to Thread and 
>> Throwable, StackTraceFrame also contains several static methods for 
>> retrieving Classes and StackTraceFrames from the stack. These methods 
>> are as follows:
>>
>> Class<?> getCallerClass(): Retrieves the class of the caller of the 
>> method calling getCallerClass(). This is identical to the new 
>> Reflection#getCallerClass() added in Java 7u25/8.
>>
>> Class<?> getCallerClass(int): Retrieves the class of the caller n 
>> frames down the stack. This is identical to the old 
>> Reflection#getCallerClass(int) that was deprecated in Java 7u25 and 
>> removed in Java 8.
>>
>> StackTraceFrame getCallerFrame(): Retrieves the StackTraceFrame of 
>> the line of code that called the method calling getCallerClass(). 
>> This is similar to the new Reflection#getCallerClass() added in Java 
>> 7u25/8, except that it returns a StackTraceFrame.
>>
>> StackTraceFrame getCallerFrame(int): Retrieves the StackTraceFrame of 
>> the caller n frames down the stack. This is similar to the old 
>> Reflection#getCallerClass(int), except that it returns a 
>> StackTraceFrame.
>>
>> StackTraceFrame[] getCurrentStackTrace(): Functionally equivalent to 
>> Thread#getStackTrace(), except that it returns an array of 
>> StackTraceFrames.
>>
>> StackTraceFrame[] getStackTrace(Throwable throwable): Functionally 
>> equivalent to Throwable#getStackTrace(), except that it returns an 
>> array of StackTraceFrames. It uses the same save point (backtrace) 
>> created when the Throwable is created that Throwable#getStackTrace() 
>> uses when it's first called. It caches the array of StackTraceFrames 
>> in the Throwable just like the array of StackTraceElements are 
>> cached, so that multiple calls for the same Throwable are fast.
>>
>> As a result of this change, sun.reflect.CallerSensitive has been 
>> moved to java.lang.CallerSensitive.
>>
>> I spent considerable time reviewing, revising, considering, and 
>> testing these changes. I included several unit tests that establish 
>> the proper behavior. I also spent considerable time benchmarking the 
>> changes. While benchmarking, I noticed some things. First, 
>> getCallerClass() and getCallerClass(int) are as fast as their 
>> counterparts in sun.reflect.Reflection, and they easily inline when 
>> appropriate. Second, getCallerFrame() and getCallerFrame(int) are 
>> /almost/ as fast as the Class versions, but there is some additional 
>> overhead for the construction of the StackTraceFrame. This is 
>> minuscule (1,000,000 invocations consume around 500 ms total on my 
>> machine). At this point, all of the benchmarking was as expected.
>>
>> However, I then encountered a surprise. The getCurrentStackTrace() 
>> and getStackTrace(Throwable) methods executed (again, 1,000,000 
>> times) in about 70% of the time that Thread#getStackTrace() and 
>> Throwable#getStackTrace() did, respectively. Theoretically, they 
>> should have executed in the same amount of time, not faster. After 
>> extensive analysis, I discovered (what I considered to be) a serious 
>> flaw in how the stack trace is filled in within Throwable (which also 
>> affects how Thread#getStackTrace() works).
>>
>> Instead of simply iterating over the entire save point and filling in 
>> the Throwable stack trace in native code (which is what I did when I 
>> implemented the StackTraceFrame methods), the Java code in Throwable 
>> first called a native method to figure out how deep the stack was, 
>> then called another native method once for every frame in the stack 
>> to retrieve each element individually. This native method that is 
>> called repeatedly iterates over the entire backtrace once for each 
>> call, stopping only when it finds the matching element (so it's O(1) 
>> for the first call, O(2) for the second call, O(3) for the third 
>> call, and so on). While my StackTraceFrame methods were iterating 
>> over the backtrace exactly 1 time (O(n)), the Throwable methods were 
>> iterating over the backtrace 1+(n/2) times (worse than O(nlogn) but 
>> not as bad as O(n^2)). This problem would not have been extremely 
>> apparent over small stack traces (the 30% improvement was a stack 
>> trace of 6 elements), but over a large (200+ elements) stack traces 
>> the performance difference would be huge and noticeable. Seeing a 
>> room for improvement, I refactored the code that fills in the stack 
>> trace for Throwable, improving its performance accordingly to match 
>> the performance of the StackTraceFrame methods.
>>
>> I'm very pleased with how this turned out, and both the unit tests 
>> and my extensive functional testing show that the new class and its 
>> methods are working great. I just need someone willing to review and 
>> sponsor my patch!
>>
>> *The Code Changes*
>> I couldn't get WebRev to run without all kinds of errors. So I ran 
>> `hg diff -g` on every repository in the forest with changes. Here are 
>> the four patch files for the four repositories that had changes (no 
>> other repositories had changes):
>>
>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8.patch
>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_jdk.patch
>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_hotspot.patch
>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_nashorn.patch
>>
>> I believe I have followed all of the procedures as closely as 
>> possible. I await feedback and hope for some support on this, so that 
>> we can get a public replacement for this method in Java 8. Let me 
>> know if you have any questions.
>>
>> Thanks!
>>
>> Nick
>
>