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