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

[Peter Levart]

Hi Kevin,

On 05/10/2017 03:19 AM, Kevin Rushforth wrote:

inline

Peter Levart wrote:

Hi Kevin,

On 05/02/2017 02:21 AM, 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).


-- Kevin

[1] 
https://bugs.openjdk.java.net/browse/JDK-8177566?focusedCommentId=14074243&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14074243




I think it is very unusual to handle unqualified exports as something 
special, different from qualified exports. I know what the reasoning 
is: if a package is exported unconditionally (to everyone) then it is 
part of public API and so the trampoline may access members of that 
package on behalf of anyone. But such reasoning is just a consequence 
of the lack of a finer-grained (per-module) access support in JavaFX. 
I know it is too much to ask for JDK 9, but could JavaFX in say JDK 
10, given current API, somehow determine on whose behalf it is making 
the trampoline access? If it could, then the trampoline could allow 
qualified exports to be effective too.


The primary remedy is for the application to use a qualified "opens" 
to the appropriate javafx module. For example, to allow the 
JavaBeanXXXProperty classes the ability to access a class in your 
module, your module needs to "opens my.package to javafx.base". The 
only difference between what you propose and what was implemented is 
qualified exports versus qualified opens, which really shouldn't be 
too much of an issue for applications (such applications already need 
to use qualified opens to allow access to their FXML controller class).


The only reason we mention unconditional exports as an alternative is 
for the benefit of application that happen to already have their 
package exported unconditionally.


-- Kevin


I was thinking more in the direction of who the "real" accessor is when 
some JavaBeanXXXProperty is being used to access the bean getter/setter 
methods. Could it be the one invoking the 
JavaBeanXXXProperty.get()/.set() methods? Could it be the one invoking 
the JavaBeanXXXProperty.bind(ObservableValue) ?


JavaBeanXXXProperty is a kind of "reflection" API with additional 
features. Classical Java reflection, for example, uses the "real" caller 
(the one invoking Method.invoke or Field.get/.set) to base access 
decisions on. Would this be the right approach for JavaBeanXXXProperty 
too? (injections with @FXML are a different story).


Say for example, that module A has some Java bean classes that it would 
like to expose solely to module B and module B would like to bind their 
properties to some observables. Now module A would like to expose those 
bean classes to B with simple qualified exports so that no other module 
but B could bind or access A's bean properties.


Does this make sense so far?

If qualified "opens" to the appropriate javafx module is enough for 
JavaBeanXXXProperty to access bean properties in so opened packages, 
then JavaBeanXXXProperty provides a means for anyone to access those 
getters/setters. In my view this represents an elevation of privilege. A 
qualified opens to javafx then means more than just that. It means 
getters/setters are open to anyone who dares to use JavaBeanXXXProperty 
API instead of classic reflection API.


What do you think?

Regards, Peter

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

[Kevin Rushforth]

inline

Peter Levart wrote:

Hi Kevin,

On 05/02/2017 02:21 AM, 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).


-- Kevin

[1] 
https://bugs.openjdk.java.net/browse/JDK-8177566?focusedCommentId=14074243&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14074243




I think it is very unusual to handle unqualified exports as something 
special, different from qualified exports. I know what the reasoning 
is: if a package is exported unconditionally (to everyone) then it is 
part of public API and so the trampoline may access members of that 
package on behalf of anyone. But such reasoning is just a consequence 
of the lack of a finer-grained (per-module) access support in JavaFX. 
I know it is too much to ask for JDK 9, but could JavaFX in say JDK 
10, given current API, somehow determine on whose behalf it is making 
the trampoline access? If it could, then the trampoline could allow 
qualified exports to be effective too.


The primary remedy is for the application to use a qualified "opens" to 
the appropriate javafx module. For example, to allow the 
JavaBeanXXXProperty classes the ability to access a class in your 
module, your module needs to "opens my.package to javafx.base". The only 
difference between what you propose and what was implemented is 
qualified exports versus qualified opens, which really shouldn't be too 
much of an issue for applications (such applications already need to use 
qualified opens to allow access to their FXML controller class).


The only reason we mention unconditional exports as an alternative is 
for the benefit of application that happen to already have their package 
exported unconditionally.


-- Kevin



Regards, Peter

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

[Peter Levart]

Hi Kevin,

On 05/02/2017 02:21 AM, 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).


-- Kevin

[1] 
https://bugs.openjdk.java.net/browse/JDK-8177566?focusedCommentId=14074243&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14074243




I think it is very unusual to handle unqualified exports as something 
special, different from qualified exports. I know what the reasoning is: 
if a package is exported unconditionally (to everyone) then it is part 
of public API and so the trampoline may access members of that package 
on behalf of anyone. But such reasoning is just a consequence of the 
lack of a finer-grained (per-module) access support in JavaFX. I know it 
is too much to ask for JDK 9, but could JavaFX in say JDK 10, given 
current API, somehow determine on whose behalf it is making the 
trampoline access? If it could, then the trampoline could allow 
qualified exports to be effective too.


Regards, Peter

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

[Kevin Rushforth]

This seems like something that could be useful, although at this point 
in the release we would more likely do it for JDK 10. I do note that 
including the class that made the illegal access is generally a good 
idea when that class is attempting the access on its own behalf. For 
frameworks such as JavaFX beans or FXML, we are making the request on 
behalf of an application, it wasn't as helpful to have the class itself 
be in the error message (which was Mandy's main point). So there are at 
least two cases to consider if we end up creating such a utility method.


Thanks.

-- Kevin


Gregg Wonderly wrote:

If there is not already such an exception, it would seem like a good idea to 
have an exception that formats such a message from constructor parameters 
providing the details so that it’s the same everywhere, and so that it can be 
changed in once place if needed.

Gregg

  

On May 3, 2017, at 9:48 AM, Kevin Rushforth  wrote:

OK, I'll create a more informative message. I think it will be more clear in the message 
to just say that it needs to "open" the package to javafx.base, since that 
would be the recommendation for a package that isn't already exported unconditionally. 
I'll send out a new webrev this morning with all feedback incorporated.

-- Kevin


Mandy Chung wrote:


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

[Gregg Wonderly]

If there is not already such an exception, it would seem like a good idea to 
have an exception that formats such a message from constructor parameters 
providing the details so that it’s the same everywhere, and so that it can be 
changed in once place if needed.

Gregg

> On May 3, 2017, at 9:48 AM, Kevin Rushforth  
> wrote:
> 
> OK, I'll create a more informative message. I think it will be more clear in 
> the message to just say that it needs to "open" the package to javafx.base, 
> since that would be the recommendation for a package that isn't already 
> exported unconditionally. I'll send out a new webrev this morning with all 
> feedback incorporated.
> 
> -- Kevin
> 
> 
> Mandy Chung wrote:
>>> 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

[Kevin Rushforth]

Mandy Chung wrote:

Looks good.
  


Thank you for your help on this and for your review.


"Deploying an Application as a Module” section is duplicated in several
JavaBean*Property classes.  One alternative is to move it to the package
summary.  I have no objection to leave it as is. 
  


I think I'll keep it as is for the JavaBean*Property classes. Especially 
since we don't have anything currently in the package summary. There 
might be other detailed information that could go there, too.


-- Kevin




Mandy


  

On May 3, 2017, at 4:30 PM, Kevin Rushforth  wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8177566

Here is the updated webrev with (I hope) all comments addressed:

http://cr.openjdk.java.net/~kcr/8177566/webrev.01/complete-webrev/

For those who reviewed the earlier webrev, I have prepared delta webrevs.

* Delta webrev for the fix itself (just a slight change to the error message, 
plus I hid the unused public methods in MethodUtil) :

http://cr.openjdk.java.net/~kcr/8177566/webrev.01/delta-fix-only.00/

* No changes in the tests

* Delta webrev for the doc changes:

http://cr.openjdk.java.net/~kcr/8177566/webrev.01/delta-doc-only.00/

* The sparse javadocs are also updated here:

http://cr.openjdk.java.net/~kcr/8177566/webrev.01/javadoc/

-- Kevin


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).

-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8177566?focusedCommentId=14074243&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14074243 

  


  

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

[Mandy Chung]

Looks good.

"Deploying an Application as a Module” section is duplicated in several
JavaBean*Property classes.  One alternative is to move it to the package
summary.  I have no objection to leave it as is. 

Mandy


> On May 3, 2017, at 4:30 PM, Kevin Rushforth  
> wrote:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8177566
> 
> Here is the updated webrev with (I hope) all comments addressed:
> 
> http://cr.openjdk.java.net/~kcr/8177566/webrev.01/complete-webrev/
> 
> For those who reviewed the earlier webrev, I have prepared delta webrevs.
> 
> * Delta webrev for the fix itself (just a slight change to the error message, 
> plus I hid the unused public methods in MethodUtil) :
> 
> http://cr.openjdk.java.net/~kcr/8177566/webrev.01/delta-fix-only.00/
> 
> * No changes in the tests
> 
> * Delta webrev for the doc changes:
> 
> http://cr.openjdk.java.net/~kcr/8177566/webrev.01/delta-doc-only.00/
> 
> * The sparse javadocs are also updated here:
> 
> http://cr.openjdk.java.net/~kcr/8177566/webrev.01/javadoc/
> 
> -- Kevin
> 
> 
> 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).
>> 
>> -- Kevin
>> 
>> [1] 
>> https://bugs.openjdk.java.net/browse/JDK-8177566?focusedCommentId=14074243&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14074243
>>  
>> 

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

[Kevin Rushforth]

JBS: https://bugs.openjdk.java.net/browse/JDK-8177566

Here is the updated webrev with (I hope) all comments addressed:

http://cr.openjdk.java.net/~kcr/8177566/webrev.01/complete-webrev/

For those who reviewed the earlier webrev, I have prepared delta webrevs.

* Delta webrev for the fix itself (just a slight change to the error 
message, plus I hid the unused public methods in MethodUtil) :


http://cr.openjdk.java.net/~kcr/8177566/webrev.01/delta-fix-only.00/

* No changes in the tests

* Delta webrev for the doc changes:

http://cr.openjdk.java.net/~kcr/8177566/webrev.01/delta-doc-only.00/

* The sparse javadocs are also updated here:

http://cr.openjdk.java.net/~kcr/8177566/webrev.01/javadoc/

-- Kevin


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).


-- Kevin

[1] 
https://bugs.openjdk.java.net/browse/JDK-8177566?focusedCommentId=14074243&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14074243 

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

[Kevin Rushforth]

Forgot to respond to this:


Also in MethodUtil::invoke
  61if (!module.isExported(packageName)) {
You can do this check only if module.isNamed. 


No, this check will work fine on an unnamed module. Module::isExported 
and Module::isOpen always return true for Module::unnamed.


-- Kevin



Kevin Rushforth wrote:
OK, I'll create a more informative message. I think it will be more 
clear in the message to just say that it needs to "open" the package 
to javafx.base, since that would be the recommendation for a package 
that isn't already exported unconditionally. I'll send out a new 
webrev this morning with all feedback incorporated.


-- Kevin


Mandy Chung wrote:
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

[Kevin Rushforth]

OK, I'll create a more informative message. I think it will be more 
clear in the message to just say that it needs to "open" the package to 
javafx.base, since that would be the recommendation for a package that 
isn't already exported unconditionally. I'll send out a new webrev this 
morning with all feedback incorporated.


-- Kevin


Mandy Chung wrote:

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]

> 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

[Kevin Rushforth]

inline

Mandy Chung wrote:

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?
  


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 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.



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?


-- Kevin



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)
  

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

[Kevin Rushforth]

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)
  

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

[David Hill]

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 ?


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)

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

[Kevin Rushforth]

I presume you are talking about the short description I added to the 
beans classes and to the select binding methods (and, for that matter, 
the *PropertyValueFactory classes)? I agree that it does seem a bit 
terse, so you are right that developers may have a hard time 
understanding it.


Without duplicating the "deploying an application as a module" section 
from the Introduction to FXML -- which would seem out of place in a 
JavaBeans class description or a select() method description -- 
can you think of a good way to make it more clear?


Btw, the following is the IAE exception that will be thrown:

java.lang.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


This is basically the same IAE message that would happen if we were to 
invoke the method directly.


-- Kevin


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.

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

[Alan Bateman]

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.

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

[Kevin Rushforth]

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).


-- Kevin

[1] 
https://bugs.openjdk.java.net/browse/JDK-8177566?focusedCommentId=14074243&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14074243