Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotating an interface method with @JsProperty can produce runtime errors if jsinterop exports are not enabled #9976

Open
niloc132 opened this issue Jul 8, 2024 · 1 comment

Comments

@niloc132
Copy link
Contributor

niloc132 commented Jul 8, 2024

GWT version: 2.10
Browser (with version): Any
Operating System: Any


Description

Annotating a method as JsProperty with jsinterop exports disabled (in general or for that member) still emits a Object.defineProperties call, and calls to that method within other GWT code end up being written as a property access rather than a method call.

However, since the property isn't actually being exported, obfuscation still kicks in, but apparently the reference name ends up being different.

A few ways to fix it seem to present themselves:

  • Don't emit the Object.defineProperties at all when the method (as a property) won't be exported. This will also decrease generated code size.
  • Always consistently name methods annotated with JsProperty, even if they are not marked for export (but don't treat them as entrypoints, so they aren't truly "exported")
Steps to reproduce

This is easier to reproduce quickly with draft mode, but still possible with a production compile. First, an example with draft mode enabled and no jsinterop exports:

public class Foo {
    @JsProperty
    public String getSomeString() {
        // easy to find method in JS output
        return Window.prompt("", "");
    }
}

To reproduce the bug then, attempt to use that property:

Window.alert(new Foo().getSomeString())

This results in the following alert in the generated JS (note that the property name is not correct for an exported member):

cgguc.alert_0((new ceac.Foo).getSomeString);

and this class (with a differently-inconsistent name for the defined member):

defineClass(75, 1, {1:1}, ceac.Foo);
_.$init_1 = function $init_1(){
}
;
defineProperties(_, {getSomeString__Ljava_lang_String_2:{'get':function getSomeString(){
  return cgguc.prompt_0('', '');
}
}});
cggl.Lcom_example_app_client_Foo_2_classLit = createForClass('com.example.app.client', 'Foo', 75, cggl.Ljava_lang_Object_2_classLit);

In OBF, this compiles out to roughly:

uyb.ux((new gyb.gb).F);

and

Ev(_,{getSomeString__Ljava_lang_String_2:{'get':function hb(){return uyb.vx('','')}}});

demonstrating that this isn't just an issue caused by PRETTY.


To reproduce this with optimized output, we need a bit more indirection - it appears to be sufficient to extra an interface from Foo, and reference that instead:

public interface IFoo {
    @JsProperty
    String getSomeString();
}

Then introduce a subclass of Foo and instantiate that instead

public class Bar extends Foo {
}

...And finally create the instance in a way that the compiler can't statically figure out that we always call the same method.

    private static native IFoo getFoo() /*-{
        return @com.example.app.client.Bar::new()();
    }-*/;

It might be possible to skip one or more of these steps through a larger, un-inlineable method body, but I didn't explore deeply here. In the original code, there is an interface and a class that implements it, then another subclass that overrides unrelated methods.

Prod, OBF output:

tc((new o).l);
mc(_,{getSomeString__Ljava_lang_String_2:{'get':function n(){return $wnd.prompt('','')}}});
Known workarounds

This use case came about as part of writing test cases for a GWT based JS library - the library always has the exports enabled, but the unit tests have no need for those. Leaving those exports disabled exposed the issue, and enabling exports on these specific types does work around the issue.

@niloc132
Copy link
Contributor Author

It is possible that this is related to #9623 - there are a few commonalities here. There is a stale review for that issue, should be tested with this case to see if it resolves both.

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

No branches or pull requests

1 participant