Code Review Request: JEP 140 Limited doPrivileged

[Mandy Chung]

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.

    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?

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

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

    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.

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

Mandy



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/
>