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

[David Holmes]

On 9/05/2012 1:03 AM, Ulf Zibis wrote:
> Am 08.05.2012 12:30, schrieb David Holmes:
>> On 8/05/2012 8:22 PM, David Holmes wrote:
>>> <restricting to core-libs-dev>
>>>
>>> On 8/05/2012 8:02 PM, Ulf Zibis wrote:
>>>>
>>>> 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 ?

I don't see anything wrong in the test class. Indent is 4. ??

>>>
>>>> (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.

Factored out to where? There's no shared superclass here, so where would 
it get factored to?

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

Not part of my changes.

David
-----

> -Ulf
>