On Fri, 3 Sep 2021 12:51:13 GMT, Peter Levart <plev...@openjdk.org> wrote:
>> Vladimir Ivanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments
>
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 877:
>
>> 875: }
>> 876: if (asTypeSoftCache != null) {
>> 877: atc = asTypeSoftCache.get();
>
> NPE is possible here too! asTypeSoftCache is a non-volatile field which is
> read twice. First time in the if (...) condition, 2nd time in the line that
> de-references it to call .get(). This is a data-race since concurrent thread
> may be setting this field from null to non-null. Those two reads may get
> reordered. 1st read may return non-null while 2nd may return null. This can
> be avoided if the field is read just once by introducing a local variable to
> store its value.
Fixed.
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 878:
>
>> 876: if (asTypeSoftCache != null) {
>> 877: atc = asTypeSoftCache.get();
>> 878: if (newType == atc.type) {
>
> NPE is possible here! act can be null as it is a result of SoftReference::get
Good catch! Fixed.
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 933:
>
>> 931: }
>> 932:
>> 933: /* Returns true when {@code loader} keeps {@code mt} either
>> directly or indirectly through the loader delegation chain. */
>
> Well, to be precise, loader can't keep mt alive. It would be better to say
> "keeps mt components alive" ...
Fixed.
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 948:
>
>> 946: if (isBuiltinLoader(defLoader)) {
>> 947: return true; // built-in loaders are always reachable
>> 948: }
>
> No need for special case here. isAncestorLoaderOf(defLoader, loader) already
> handles this case.
Though the check is redundant, I find the current version clearer.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5246
