Code Review Request: JEP 140 Limited doPrivileged

[Jeff Nisewanger]

Comments inline below:

On 5/31/2013 11:53 AM, Mandy Chung wrote:
> Jeff,
>
> The limited doPrivilege is very useful and I'm hoping the libraries
> can begin using the limited doPrivileged once this is in jdk8. It's
> non-trivial stuff and I know that you're working on the
> unit tests and I can look at them when they are available.
>
> It looks fine to me and I don't see anything obviously wrong.
> This extends the wrapper implementation for the combiner
> and the new comments you added make it easier to understand.
> I think it'd be good to extend the javadoc of package-private
> AccessControlContext constructor about "wrapped" ACC
> and some summary on how they will be used in determining limited
> privilege scope.
>
> I don't spot anything obviously wrong.  Just a few minor comments:
>
> AccessController.java
>    You fixed up a couple of @see #doPrivileged in existing
>    methods. Looks like L393-394, 445-446 were copied before
>    the change.  I'm not sure how these @see tags were
>    decided to be included and it seems worth taking a pass
>    and clean that up if appropriate.

Yes, I need to make a pass over the @see listings in the javadoc.
There was one for the PrivilegedExceptionAction version of the basic
doPrivileged() that was inconsistent with the pattern set by the javadoc
on the other methods so I changed it (it looked like a copy & paste
oversight from the original code).


>
>    You can call java.util.Objects.requireNonNull(perms) to
>    replacethe null argument check (e.g. L403-405). Might
>    be good to move the null argument check at the beginning?

Good point. I forgot about the new java.util.Object added in Java 7.

>
> AccessControlContext.java
>    L214-231: I wonder if it might be better to move this
>    validation to AccessController.createWrapperbefore constructing
>    the ACC wrapper.

     I wanted to keep this code in AccessControlContext.java since it is
part of the implementation that is spread over the rest of the class.
Moving it to AccessController.java would be awkward and would
leak the implementation outside of AccessControlContext.java.

>
>    The comments in the combine method e.g. L618-196, 622-623
>    need update after the refactoring.

Good point.

>
>    L740-744: FYI this can be replaced by a simple call to:
>        Objects.equals(this.combiner, that.combiner)
>    Same comment can apply to the equality check for this.permission
>    and this.parent.

True, although the implementation of Object.equals() looks somewhat
non-optimized. I'm curious why there is a long chain of
if/else tests before falling back to obj.equals() rather than a simple
"if obj.getClass().isArray() else". I'll look into that a bit more.

>
>    checkPermission2 is checking the limited privilege scope.
>    Maybe good to rename it to something like checkLimitedPrivilegeScope
>    to make it explicit.

Yes, perhaps.



Jeff

>
>
>
> On 5/28/2013 10:56 PM, Jeff Nisewanger wrote:
>>
>> This is the implementation of the JEP 140 feature for a limited 
>> privilege form of doPrivileged(). A test will be added in an
>> updated webrev within the next day.
>>
>> The JEP is: http://openjdk.java.net/jeps/140
>>
>> The webrev is: http://cr.openjdk.java.net/~jdn/8014097/webrev.0/
>>
>