Skip to content

jsii kernel: method lookup logic is extremely sus #5028

Description

@rix0rrr

Describe the bug

I'm pretty sure the effect of this bug will be:

  • The wrong method gets called when a JavaScript method exists both on a class, and it's also defineProperty'ed directly on the instance (or simply assigned), and it gets called form Java.
  • The wrong method gets called when two methods are overridden on a Java subclass inheriting from a JavaScript subclass, and a this.theOtherMethod() call is done (calls the JavaScript base method instead of the Java overridden one).

In both cases this means: behavior will be different between Java and JavaScript in hard-to-debug ways.

Full explanation from a PR. See https://github.com/aws/jsii/blame/main/packages/@jsii/kernel/src/kernel.ts#L980

    // always first look up the method in the prototype. this practically bypasses
    // any methods overridden by derived classes (which are by definition native
    // methods). this serves to allow native call to invoke "super.method()" when
    // overriding the method.
    // if we didn't find the method on the prototype, it could be a literal object
    // that implements an interface, so we look if we have the method on the object
    // itself. if we do, we invoke it.
    //
    //--------------------------------------------------------------
    //
    // huijbers@ (2026) -- I'm pretty sure the above logic is wrong. It reads like
    // looking up methods from the prototype is intended to prevent cyclic calls
    // when subclassing JS classes from a jsii language. We don't want:
    //
    // ```java
    // class MyClass extends JavaScriptClass {
    //   public void myMethod() {
    //     super.myMethod();  <-- should call myMethod on base class
    //   }
    // }
    // ```
    //
    // To call the same `myMethod` again and infinitely recurse, which a naive `invoke(this, 'myMethod')`
    // would do. In order to work around this we seem to be default-ignoring functions
    // that live directly on an object, *unless* we otherwise can't find it on the class.
    //
    // But `#findInvokeTarget()` is used for *all* invokes, and this now finds the wrong
    // method in situations where both a class and the instance have a method, for whatever
    // reason; maybe the JS object got patched or something.
    //
    // At least one case where I ran into this is when calling `toString()` on an anonymous
    // object. Because 'Object.prototype' already has an implementation for `toString`, we
    // never call the one on the anonymous object but always the built-in one which returns
    // "[object Object]".
    //
    // I'm tempted to reverse the logic: look up the method on the instance, *unless* we
    // have reasons to think the object we're looking at is a proxy for a jsii-client object
    // in which case we look up on the parent. But even that is wrong because it would also
    // do the wrong thing in this case:
    //
    // ```java
    // class MyClass extends JavaScriptClass {
    //   @Override public void myMethod1() {
    //     this.myMethod2();  <-- should call myMethod2 below, not from parent
    //   }
    //   @Override public void myMethod2() {
    //   }
    // }
    // ```
    //
    // Pretty sure this is wrong and needs attention, but for now I'll just make an exception
    // for the one case I need to get working: `toString`.
    let fn = instance.constructor.prototype[methodName];
    if (!fn) {
      fn = instance[methodName];
      if (!fn) {
        throw new JsiiFault(`Cannot find ${methodName} on object`);
      }
    }
    return { ti, obj: instance, fn };

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugThis issue is a bug.p1

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions