hg: jdk8/tl/jdk: 7103570: AtomicIntegerFieldUpdater does not work when SecurityManager is installed

[Ulf Zibis]

Am 08.05.2012 12:30, schrieb David Holmes:
> On 8/05/2012 8:22 PM, David Holmes wrote:
>> <restricting to core-libs-dev>
>>
>> Hi Ulf,
>>
>> On 8/05/2012 8:02 PM, Ulf Zibis wrote:
>>> Hi all,
>>>
>>> I'm a little bit late, but I just have seen:
>>> (1) some indentations in the patch are broken
>>
>> <sigh> I missed a couple of indent-2 that should be indent-4.
>>
>> And something went awry with indentation in ensureProtectedAccess.
>> Unfortunately the webrev didn't show this and it isn't code that was
>> functionally modified (I had a tab-to-space conversion issue, which is
>> how this got touched at all).
Did you also noticed it in test class ?

>>
>>> (2) following code snipped is repeated many times:
>>
>> 4 times.
>>
>>> + ClassLoader cl = tclass.getClassLoader();
>>> + ClassLoader ccl = caller.getClassLoader();
>>> + if ((ccl != null) && (ccl != cl) &&
>>> + ((cl == null) || !isAncestor(cl, ccl))) {
>>> + sun.reflect.misc.ReflectUtil.checkPackageAccess(tclass);
>>> + }
>>> Wouldn't it be better, to move it in a method, maybe in
>>> sun.reflect.misc.ReflectUtil ?
>>
>> It didn't seem quite worth the effort of factoring out.
>>
>> The isAncestor method is also repeated but again was not considered a
>> worthwhile factorization.
>>
>> Now that I'm writing that I'm having my doubts, but at the time it was
>> expedient. Looking forward this code will likely have to change again to
>> suit a modular world.
>
> I was forgetting there's another major consideration here too, and that is that this code sync's 
> up with Doug Lea's JSR-166 CVS repository. The changes I made are independent of the JDK used, but 
> if I refactored things into ReflectUtil then the code would only be valid on a JDK with that 
> update - which means Doug would need to maintain a different version of this fix for older JDKs.
Class ReflectUtil was just a suggestion. as it will be loaded anyway.
Looking in detail maybe whole AtomicXyzFieldUpdaterImpl constructors could be factored out.

Same for the code snippet:
     if (obj == null || obj.getClass() != tclass || cclass != null) fullCheck(obj);

-Ulf