Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline

[Mandy Chung]

> On May 2, 2017, at 2:22 PM, Kevin Rushforth  
> wrote:
> 
> Here is the message:
> 
> IllegalAccessException: class com.sun.javafx.property.MethodHelper cannot 
> access class com.foo (in module foo.app) because module foo.app does not open 
> com.foo to javafx.base 
> 

It would be better to emit a more informative message e.g. “javafx.base cannot 
access class com.foo (in module foo.app).  Either exports com.foo unqualifiedly 
or open com.foo to javafx.base”.  Also in MethodUtil::invoke
  61if (!module.isExported(packageName)) {
You can do this check only if module.isNamed.

> It is roughly the same message that any similar illegal access would generate 
> (e.g., we get similar error messages when FXML tries to call setAccessible 
> for a field annotated with @FXML if the module is not "open" to javafx.fxml).
> 
>> javafx.base/src/main/java/com/sun/javafx/property/MethodHelper.java
>> javafx.fxml/src/main/java/com/sun/javafx/fxml/MethodHelper.java
>> javafx.web/src/main/java/com/sun/webkit/MethodHelper.java
>>   45 public static Object invoke(Method m, Object obj, Object[] params)
>> 
>> To avoid 3 ModuleHelper classes, the invoke method can take
>> the callerModule argument to replace this line: 
>>   56 final Module thisModule = MethodHelper.class.getModule();
>>   
> 
> I'm fairly certain that won't work. Module::addOpens is caller sensitive and 
> will only work when called from the module in question.
> 

You are right.  Wont’t work.
>> javafx.base/src/main/java/com/sun/javafx/reflect/MethodUtil.java
>>   There are a few other public methods which I think JavaFX doesn’t
>>   need and can be removed.
>>   
> 
> Yes, I could do this to reduce the public footprint of the class. To minimize 
> the diffs between the original and our copy, I might just comment out the 
> "public". That would also make it easier to add them back in a future version 
> (e.g., to eventually get rid of all dependency on sun.reflect.misc). Thoughts?

I will leave it up to you.
Mandy

Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline

[Mandy Chung]

Hi Kevin,

> On May 1, 2017, at 5:21 PM, Kevin Rushforth  
> wrote:
> 
> This review is being cross-posted to both openjfx-dev and jigsaw-dev.
> 
> Please review the proposed fix for:
> 
> https://bugs.openjdk.java.net/browse/JDK-8177566
> http://cr.openjdk.java.net/~kcr/8177566/webrev.00/complete-webrev/

First pass of comment:

javafx.base/src/main/java/com/sun/javafx/property/PropertyReference.java
 196 try {
 197 return 
(ReadOnlyProperty)MethodHelper.invoke(propertyGetter, bean, (Object[])null);

 198 } catch (Exception ex) {
 199 throw new RuntimeException(ex);
 200 }

Do you have an example exception thrown if the package is not
open to javafx.base?  IAE is thrown by MethodHelper.invoke.
Are you detecting this and throw an exception with friendlier
message?

javafx.base/src/main/java/com/sun/javafx/property/MethodHelper.java
javafx.fxml/src/main/java/com/sun/javafx/fxml/MethodHelper.java
javafx.web/src/main/java/com/sun/webkit/MethodHelper.java
  45 public static Object invoke(Method m, Object obj, Object[] params)

To avoid 3 ModuleHelper classes, the invoke method can take
the callerModule argument to replace this line: 
  56 final Module thisModule = MethodHelper.class.getModule();

javafx.base/src/main/java/com/sun/javafx/reflect/MethodUtil.java
  There are a few other public methods which I think JavaFX doesn’t
  need and can be removed.

Mandy

Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline

[Kevin Rushforth]

Is this any better? It adds an example module-info.java file to 
JavaBeanXXX as is done in the FXML class.


--
NOTE: If the Java Bean class is in a named module, then it must be 
reflectively accessible to the javafx.base module. A class is 
reflectively accessible if the module "opens" the containing package to 
at least the javafx.base module. For example, if com.foo.MyBeanClass is 
in the foo.app module, the module-info.java might look like this:


module foo.app {
   opens com.foo to javafx.base;
}

Alternatively, a class is also reflectively accessible if the module 
"exports" the containing package unconditionally.

--

Comments?

-- Kevin


Kevin Rushforth wrote:



David Hill wrote:


Current:
   All classes and properties used in a select-binding have to be 
declared public. Additionally, if any class is in a named module, 
then the module must |open| 
 
the containing package for that class to at least the |javafx.base| 
module (or |export| 
 
the containing package unconditionally).


Suggestion:

   All classes and properties used in a select-binding have to be 
declared public. If any class is in a named module, then the module 
or package containing the class needs to be open or include 
|javafx.base in the export list.|


any better ?



It might read better, but is now incorrect. It is not sufficient for a 
module to *export* the package to javafx.base. Nor is it required that 
the module be *open* unconditionally. Rather it must *open* the 
package to javafx.base. The alternative is to export it 
unconditionally as public API of the app:


WORKS:  module foo.app { opens com.foo to javafx.base; }
WORKS:  module foo.app { exports com.foo; }
WORKS (but overkill):  module foo.app { opens com.foo; }

FAILS:  module foo.app { exports com.foo to javafx.base; }
FAILS:  module foo.app { /* does not export or open com.foo */ }


-- Kevin









On 5/2/17, 9:04 AM, Alan Bateman wrote:

On 02/05/2017 01:21, Kevin Rushforth wrote:


This review is being cross-posted to both openjfx-dev and jigsaw-dev.

Please review the proposed fix for:

https://bugs.openjdk.java.net/browse/JDK-8177566
http://cr.openjdk.java.net/~kcr/8177566/webrev.00/complete-webrev/

Details of the fix as well as notes to reviewers are in the bug 
report [1] (e.g., I've also generated separate webrevs for the fix 
itself, the doc changes, and the test changes).
Overall I think the approach is okay but I wonder how easy it will 
be for joe developer to understand. By this I mean the description 
in the javadoc is lengthy and I wonder if it might be better to 
split up the two "solutions" so that it the choice is clearer. 
Related is that the IAE thrown by MethodHelper.invoke could make it 
clear that the package is not exported.


-Alan.



--
David Hill 
Java Embedded Development

"A man's feet should be planted in his country, but his eyes should 
survey the world."

-- George Santayana (1863 - 1952)