JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

[Martin Buchholz]

In the hotspot sources I find
      // An instance field can be constant if it's a final static field or
if
      // it's a final non-static field of a trusted class (classes in
      // java.lang.invoke and sun.invoke packages and subpackages).
BUT
you can't trust MethodHandle.form!
Yes, it's
        final LambdaForm form;
but it's also frobbed via Unsafe, and in an unusual unsafe way:
        UNSAFE.putObject(this, FORM_OFFSET, newForm);
        UNSAFE.fullFence();


On Mon, Jan 8, 2018 at 10:50 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:

> Hi Martin
>
> [Back from holiday]
>
> Can you reproduce using the debugger? If so do you have a more up to date
> stack trace?
>
> That change looks ok. The form field is final, and in the j.l.invoke
> package such fields are also stable so once C2 gets at it it will likely
> treat it as a constant. In general the updates to the field will only be
> visible to interpreted code or recompiled code.
>
> I suspect the problematic field may well be that of
> LambdaForm.transformCache.
>
> Paul.
>
> > On 8 Jan 2018, at 18:41, Martin Buchholz <martinrb at google.com> wrote:
> >
> > No takers? ... let's try again.  Despite uniform failure to reproduce
> this
> > except under a debugger, let's apply the simple obviously correct fix,
> > namely:
> >
> >     /** Craft a LambdaForm customized for this particular MethodHandle */
> >     /*non-public*/
> >     void customize() {
> > +        final LambdaForm form = this.form;
> >         if (form.customized == null) {
> >             LambdaForm newForm = form.customize(this);
> >             updateForm(newForm);
> >
> >
> >
> >
> > On Wed, Jan 3, 2018 at 1:51 PM, Martin Buchholz <martinrb at google.com>
> wrote:
> >
> >> Let's try again without mail bounces ...
> >>
> >>
> >> On Wed, Jan 3, 2018 at 1:40 PM, Martin Buchholz <martinrb at google.com>
> >> wrote:
> >>
> >>> Here at Google we tried to fix JDK-8145371 because it looked like it
> was
> >>> causing rare problems in production.  But after looking at the code,
> I'm
> >>> not sure...  Anyways,
> >>>
> >>> http://cr.openjdk.java.net/~martin/webrevs/jdk/MethodHandle.form/
> >>> https://bugs.openjdk.java.net/browse/JDK-8145371
> >>> Copying to a local in customize() must be an improvement.
> >>> The UNSAFE publication in updateForm looks fishy, as does that comment
> >>> in MethodHandleImpl.java.  Is there something the fullFence() is
> supposed
> >>> to do that isn't taken care of by putObjectVolatile ?
> >>>
> >>> Feel free to take ownership of this bug from me.
> >>>
> >>> --- a/src/java.base/share/classes/java/lang/invoke/MethodHandle.java
> >>> +++ b/src/java.base/share/classes/java/lang/invoke/MethodHandle.java
> >>> @@ -1660,13 +1660,13 @@
> >>>         assert(newForm.customized == null || newForm.customized ==
> this);
> >>>         if (form == newForm)  return;
> >>>         newForm.prepare();  // as in MethodHandle.<init>
> >>> -        UNSAFE.putObject(this, FORM_OFFSET, newForm);
> >>> -        UNSAFE.fullFence();
> >>> +        UNSAFE.putObjectVolatile(this, FORM_OFFSET, newForm);
> >>>     }
> >>>
> >>>     /** Craft a LambdaForm customized for this particular MethodHandle
> */
> >>>     /*non-public*/
> >>>     void customize() {
> >>> +        final LambdaForm form = this.form;
> >>>         if (form.customized == null) {
> >>>             LambdaForm newForm = form.customize(this);
> >>>             updateForm(newForm);
> >>> diff --git a/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
> b/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
> >>> --- a/src/java.base/share/classes/java/lang/invoke/
> MethodHandleImpl.java
> >>> +++ b/src/java.base/share/classes/java/lang/invoke/
> MethodHandleImpl.java
> >>> @@ -909,7 +909,7 @@
> >>>             int c = count;
> >>>             maybeCustomizeTarget();
> >>>             if (c <= 1) {
> >>> -                // Try to limit number of updates.
> MethodHandle.updateForm() doesn't guarantee LF update visibility.
> >>> +                // Try to limit number of updates.
> >>>                 if (isCounting) {
> >>>                     isCounting = false;
> >>>                     return true;
> >>>
> >>>
> >>>
> >>
>
>