Request for review: 6888734, regression test fails when java.security.manager is enabled

[Roman Kennke-5]

Hi there,

Here's a fix for Bug#6888734. One regression test was failing with an
AccessControlException in new FontManager code. We need to move
Class.forName() and newInstance() call into the privileged block. Webrev
here:

http://cr.openjdk.java.net/~rkennke/6888734/webrev.00/

Good? Comments?

/Roman

[Roman Kennke-5]

Hi again,

is this ok for committing?

Thanks, Roman

Am Donnerstag, den 15.10.2009, 15:22 +0200 schrieb Roman Kennke:

> Hi there,
>
> Here's a fix for Bug#6888734. One regression test was failing with an
> AccessControlException in new FontManager code. We need to move
> Class.forName() and newInstance() call into the privileged block. Webrev
> here:
>
> http://cr.openjdk.java.net/~rkennke/6888734/webrev.00/
>
> Good? Comments?
>
> /Roman
>
>

[phil.race]

The fix looks fine to me. One thing. The test is
in closed just because no one got round to moving it to open.
At least I don't see any reason it can't be opened up.
Now would be a good time and it can be included with this
changeset.
Don't forget to tag it with this additional bug ID.

-phil.

Roman Kennke wrote:

> Hi again,
>
> is this ok for committing?
>
> Thanks, Roman
>
> Am Donnerstag, den 15.10.2009, 15:22 +0200 schrieb Roman Kennke:
>> Hi there,
>>
>> Here's a fix for Bug#6888734. One regression test was failing with an
>> AccessControlException in new FontManager code. We need to move
>> Class.forName() and newInstance() call into the privileged block. Webrev
>> here:
>>
>> http://cr.openjdk.java.net/~rkennke/6888734/webrev.00/
>>
>> Good? Comments?
>>
>> /Roman
>>
>>
>
>

[Roman Kennke-5]

Hi Phil,

> The fix looks fine to me.

Good. :-)

>  One thing. The test is
> in closed just because no one got round to moving it to open.
> At least I don't see any reason it can't be opened up.
> Now would be a good time and it can be included with this
> changeset.

I'll do that.

> Don't forget to tag it with this additional bug ID.

Ok.

Is this one review enough for committing?

Could we agree on a slightly more formal reply like 'Go commit', 'Ok,
but wait for one more review', or do we always stick to the 2 reviews
rule?

Thanks, Roman

[phil.race]

 > Could we agree on a slightly more formal reply like 'Go commit', 'Ok,
 > but wait for one more review', or do we always stick to the 2 reviews
 > rule?


I was assuming a second reviewer would pipe up, then you'd go commit.

In general I'd say that
- we do prefer two reviewers but there are cases where one is
more than enough (test fixes, trivial changes to avoid an NPE,
doc changes). This one is borderline.

- unless there's some pressing need (integration deadline?)
   even for a trivial fix I'd wait until it seemed likely
   no one else was going to pipe up.

Saying something in your reviewer request might help, like
"Could Phil & Igor please take a look at this" :-).

-phil.

Roman Kennke wrote:

> Hi Phil,
>
>> The fix looks fine to me.
>
> Good. :-)
>
>>  One thing. The test is
>> in closed just because no one got round to moving it to open.
>> At least I don't see any reason it can't be opened up.
>> Now would be a good time and it can be included with this
>> changeset.
>
> I'll do that.
>
>> Don't forget to tag it with this additional bug ID.
>
> Ok.
>
> Is this one review enough for committing?
>
> Could we agree on a slightly more formal reply like 'Go commit', 'Ok,
> but wait for one more review', or do we always stick to the 2 reviews
> rule?
>
> Thanks, Roman
>
>

[Roman Kennke-5]

I added the (previously closed) testcase, keeping it in the same
relative directory location:

http://cr.openjdk.java.net/~rkennke/6888734/webrev.01/

Is this ok?


/Roman


Am Montag, den 30.11.2009, 11:19 -0800 schrieb Phil Race:

> The fix looks fine to me. One thing. The test is
> in closed just because no one got round to moving it to open.
> At least I don't see any reason it can't be opened up.
> Now would be a good time and it can be included with this
> changeset.
> Don't forget to tag it with this additional bug ID.
>
> -phil.
>
> Roman Kennke wrote:
> > Hi again,
> >
> > is this ok for committing?
> >
> > Thanks, Roman
> >
> > Am Donnerstag, den 15.10.2009, 15:22 +0200 schrieb Roman Kennke:
> >> Hi there,
> >>
> >> Here's a fix for Bug#6888734. One regression test was failing with an
> >> AccessControlException in new FontManager code. We need to move
> >> Class.forName() and newInstance() call into the privileged block. Webrev
> >> here:
> >>
> >> http://cr.openjdk.java.net/~rkennke/6888734/webrev.00/
> >>
> >> Good? Comments?
> >>
> >> /Roman
> >>
> >>
> >
> >

[phil.race]

Roman Kennke wrote:
> I added the (previously closed) testcase, keeping it in the same
> relative directory location:
>
> http://cr.openjdk.java.net/~rkennke/6888734/webrev.01/
>
> Is this ok?
>  

Almost. Reg tests don't have the classpath exception.

And probably the copyright year should be a range, starting
with whatever the existing year is.

-phil.

>
> /Roman
>
>
> Am Montag, den 30.11.2009, 11:19 -0800 schrieb Phil Race:
>  
>> The fix looks fine to me. One thing. The test is
>> in closed just because no one got round to moving it to open.
>> At least I don't see any reason it can't be opened up.
>> Now would be a good time and it can be included with this
>> changeset.
>> Don't forget to tag it with this additional bug ID.
>>
>> -phil.
>>
>> Roman Kennke wrote:
>>    
>>> Hi again,
>>>
>>> is this ok for committing?
>>>
>>> Thanks, Roman
>>>
>>> Am Donnerstag, den 15.10.2009, 15:22 +0200 schrieb Roman Kennke:
>>>      
>>>> Hi there,
>>>>
>>>> Here's a fix for Bug#6888734. One regression test was failing with an
>>>> AccessControlException in new FontManager code. We need to move
>>>> Class.forName() and newInstance() call into the privileged block. Webrev
>>>> here:
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/6888734/webrev.00/
>>>>
>>>> Good? Comments?
>>>>
>>>> /Roman
>>>>
>>>>
>>>>        
>>>      
>
>
>  

[Roman Kennke-5]

Hi Phil,

Am Montag, den 30.11.2009, 13:37 -0800 schrieb Phil Race:

> Roman Kennke wrote:
> > I added the (previously closed) testcase, keeping it in the same
> > relative directory location:
> >
> > http://cr.openjdk.java.net/~rkennke/6888734/webrev.01/
> >
> > Is this ok?
> >  
>
> Almost. Reg tests don't have the classpath exception.

Fixed.

> And probably the copyright year should be a range, starting
> with whatever the existing year is.

This file is added to the closed repository in 2007, with the initial
commit, it doesn't say anything how old this file is. I put it in
OpenJDK in 2009, this is why I say so in the header. What else can I do?

Also, we need one more review, Igor, can you have a look?

http://cr.openjdk.java.net/~rkennke/6888734/webrev.02/

Thanks, Roman

[Dmitri Trembovetski]

   Hi Roman,

   I think you also need to move the 'policy' file from the closed dir along
with the test.

   Thanks,
     Dmitri


Roman Kennke wrote:

> Hi Phil,
>
> Am Montag, den 30.11.2009, 13:37 -0800 schrieb Phil Race:
>> Roman Kennke wrote:
>>> I added the (previously closed) testcase, keeping it in the same
>>> relative directory location:
>>>
>>> http://cr.openjdk.java.net/~rkennke/6888734/webrev.01/
>>>
>>> Is this ok?
>>>  
>> Almost. Reg tests don't have the classpath exception.
>
> Fixed.
>
>> And probably the copyright year should be a range, starting
>> with whatever the existing year is.
>
> This file is added to the closed repository in 2007, with the initial
> commit, it doesn't say anything how old this file is. I put it in
> OpenJDK in 2009, this is why I say so in the header. What else can I do?
>
> Also, we need one more review, Igor, can you have a look?
>
> http://cr.openjdk.java.net/~rkennke/6888734/webrev.02/
>
> Thanks, Roman
>
>
>

[Igor Nekrestyanov]

As Dmitri mentioned you need to move policy file too. Otherwise test
will not work in new location when run with jtreg.

Rest looks good to me.

-igor

On 12/7/09 12:12 PM, Roman Kennke wrote:

> Hi Phil,
>
> Am Montag, den 30.11.2009, 13:37 -0800 schrieb Phil Race:
>    
>> Roman Kennke wrote:
>>      
>>> I added the (previously closed) testcase, keeping it in the same
>>> relative directory location:
>>>
>>> http://cr.openjdk.java.net/~rkennke/6888734/webrev.01/
>>>
>>> Is this ok?
>>>
>>>        
>> Almost. Reg tests don't have the classpath exception.
>>      
> Fixed.
>
>    
>> And probably the copyright year should be a range, starting
>> with whatever the existing year is.
>>      
> This file is added to the closed repository in 2007, with the initial
> commit, it doesn't say anything how old this file is. I put it in
> OpenJDK in 2009, this is why I say so in the header. What else can I do?
>
> Also, we need one more review, Igor, can you have a look?
>
> http://cr.openjdk.java.net/~rkennke/6888734/webrev.02/
>
> Thanks, Roman
>
>
>
>