Patch: issue RT-14177
David Grieve
david.grieve at oracle.com
Fri Jun 1 10:20:37 PDT 2012
This should be tested from an applet or with applet-like java.policy, no? I mean, if it's called from user code by way of Control or PopupControl, will it throw a SecurityException?
Also, loadSkinClass is a bottle neck and I fear this will make it worse. We may need to think about caching.
On Jun 1, 2012, at 12:58 PM, Richard Bair wrote:
> Thanks guys! Thanks Jonathan & Tom for pointing out the other location in PopupControl. I've uploaded rt-14177.6.patch, which moves the code into com.sun.javafx.Utils and then has PopupControl and Control use it. As such the logic was rearranged a bit (for example, the methods just return the looked up class or fail with an exception, instead of assigning to a variable).
>
> Florian if you are still online and can test it out that would be great. Another code review would be appreciated.
>
> Thanks
> Richard
>
> On Jun 1, 2012, at 9:21 AM, Tom Schindl wrote:
>
>> Ok - that makes sense. Classloader B only sees public packages from A.
>>
>> Need to leave for a weekend after work beer. So I'm fine with the last
>> patch from Florian.
>>
>> Tom
>>
>> Am 01.06.12 18:10, schrieb Florian Brunner:
>>> Hi Tom,
>>>
>>> I have the following bundles:
>>>
>>> Bundle A:
>>> SomeBaseControl with a custom skin in a different (most commonly private) package
>>>
>>> Bundle B
>>> SomeControl extends SomeBaseControl
>>>
>>> Bundle C
>>> Instantiates SomeControl
>>>
>>> Just using:
>>>
>>> getClass().getClassLoader().loadClass(className);
>>>
>>> in Control resulted in a ClassNotFoundException (here: getClass() == SomeControl.class; className=skin class name for SomeBaseControl).
>>> So I started to loop over the super-types to get a class which is in the same bundle as the custom skin class. That worked.
>>>
>>> Do you see a better way without changing the API?
>>>
>>> Regards,
>>> Florian
>>>
>>>
>>> Am Freitag 01 Juni 2012, 16:44:24 schrieb Tom Schindl:
>>>> Hi Florian,
>>>>
>>>> Good. I can you tell me why the super-type loop is used - I think it was
>>>> already in your first patch - wondering what this is good for.
>>>>
>>>> Tom
>>>>
>>>> Am 01.06.12 16:38, schrieb Florian Brunner:
>>>>> I tested rt-14177.3.patch and Tom is right, the ClassNotFoundException has to be catched otherwise the new code won't be executed.
>>>>>
>>>>> Unfortunatly, hg import failed for 5.patch for some reason.
>>>>>
>>>>> I created and tested a new patch (with "hg diff --git" as recommended by Kevin (see attachements, I don't have the necessary rights in JIRA).
>>>>>
>>>>> It works fine for my use case.
>>>>>
>>>>> I also renamed ex2 to ex3 and ex3 to ex2 and fixed some formatting issues.
>>>>>
>>>>> Note however that now the method contains around 100 lines of code. You might want to refactor it into several smaller methods for maintainability reasons.
>>>>>
>>>>> Regards,
>>>>> Florian
>>>>>
>>>>>
>>>>> Am Freitag 01 Juni 2012, 11:04:09 schrieb Tom Schindl:
>>>>>> Hi,
>>>>>>
>>>>>> I've uploaded a fixed version of the patch! I think Richards patch
>>>>>> missed to catch the CNE for the context classloader.
>>>>>>
>>>>>> I can not comment on other areas of the control system - just did a
>>>>>> short Class.forName-scan and can confirm you are finding about
>>>>>> PopControl - it even has the same fix for RT-17525 so extracing the code
>>>>>> and moveing it to a library makes sense.
>>>>>>
>>>>>> CSS might have similar problems (but in case of classloading but they
>>>>>> are harder to fix because one has to infer the correct classloader from
>>>>>> the CSS-URI or even the target they are applied to!).
>>>>>>
>>>>>> FXML has fixed all iusses (beside one small remaining issue I'm just
>>>>>> discussing with Greg on this list) because it allows to pass custom
>>>>>> classloaders and hence stuff loaded through FXML will directly profit
>>>>>> from the Control enhancement.
>>>>>>
>>>>>> FXML is IMHO different to CSS/Controls because there's a defined
>>>>>> lifecycle and so controlling the classloader there is much easier than
>>>>>> it is in CSS/Control space where the classloader access is happening in
>>>>>> an async fashion.
>>>>>>
>>>>>> I currently have no local build so I have to rely on Florian to give
>>>>>> feedback if it fixes the problem. From a source introspection it looks
>>>>>> good to me with my patch 5.
>>>>>>
>>>>>> Tom
>>>>>>
>>>>>> Am 01.06.12 05:22, schrieb Jonathan Giles:
>>>>>>> +1 on the patch, although I do wonder whether we are being consistent in
>>>>>>> the other places where classloading comes into play. Is this something
>>>>>>> that should be extracted and used as an internal library?
>>>>>>>
>>>>>>> For example, there is much the same code in PopupControl that needs to
>>>>>>> be fixed at the same time as the patch you are submitting here, but more
>>>>>>> abstractly, should we have a library rather than duplicate similar code
>>>>>>> in FXML and CSS as well?
>>>>>>>
>>>>>>> -- Jonathan
>>>>>>>
>>>>>>>
>>>>>>> On 1/06/2012 3:14 p.m., Richard Bair wrote:
>>>>>>>> Florian / Tom,
>>>>>>>>
>>>>>>>> I've uploaded two patches to the issue:
>>>>>>>> rt-14177.2.patch
>>>>>>>> rt-14177.3.patch
>>>>>>>>
>>>>>>>> The .3 patch is my preferred one. Can you take a look / apply it and
>>>>>>>> see if it solves your issue? If the .3 patch works, then I think we've
>>>>>>>> got a clean patch which will produce no additional performance
>>>>>>>> overhead to a correctly functioning JavaFX application today, but will
>>>>>>>> fix your issue (at least as Tom said in the issue itself for 95% of
>>>>>>>> the folks).
>>>>>>>>
>>>>>>>> We can then file a new issue for the remaining 5% cases which will
>>>>>>>> need more time (since it involves some form of new API) whereas in
>>>>>>>> this case it should "just work".
>>>>>>>>
>>>>>>>> David / Jonathan, can you guys give a quick code review? I'm pretty
>>>>>>>> sure the code does what I want it to do, and hopefully Florian / Tom
>>>>>>>> will tell us if what I want it to do is actually the right thing to do
>>>>>>>> (ie: solves the issue) :-).
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Richard
>>>>>>>>
>>>>>>>> On May 31, 2012, at 5:45 PM, Richard Bair wrote:
>>>>>>>>
>>>>>>>>> Hi Florian,
>>>>>>>>>
>>>>>>>>> We'll get this fixed ASAP.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Richard
>>>>>>>>>
>>>>>>>>> On May 31, 2012, at 4:45 PM, Kevin Rushforth wrote:
>>>>>>>>>
>>>>>>>>>> Hi Florian,
>>>>>>>>>>
>>>>>>>>>> This JIRA issue is targeted for 3.0 which is just in the initial
>>>>>>>>>> planning stages, so no one has looked at it yet.
>>>>>>>>>>
>>>>>>>>>> The patch as attached to the issue deals with one case of CSS
>>>>>>>>>> loading a skin, but when the controls team evaluated it, their take
>>>>>>>>>> was that it a more general platform issue, although it could be
>>>>>>>>>> dealt with in isolation. In any case, even if I were to assign this
>>>>>>>>>> back to someone on the controls team, this code would need to be
>>>>>>>>>> evaluated for potential bugs and to ensure that there are no
>>>>>>>>>> security issues (probably not an issue, but since it would be using
>>>>>>>>>> a system class loader we need to make sure it is never called from a
>>>>>>>>>> privileged context), which is unlikely to happen for the current
>>>>>>>>>> release.
>>>>>>>>>>
>>>>>>>>>> -- Kevin
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Florian Brunner wrote:
>>>>>>>>>>> Hi Kevin,
>>>>>>>>>>>
>>>>>>>>>>> What is the status of my patch?
>>>>>>>>>>> As mentioned in JIRA, this issue is becoming a blocker! Please
>>>>>>>>>>> increase the priority! (Nobody from Oracle is responding on JIRA).
>>>>>>>>>>>
>>>>>>>>>>> Can we agree then to integrate the provided patch? It improves the
>>>>>>>>>>> situation but doesn't introduce any API additions and we can
>>>>>>>>>>> replace/ complete it later with other solutions if needed.
>>>>>>>>>>> Please tell me if I should change something in the patch to get it
>>>>>>>>>>> approved.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Florian
>>>>>>>>>>>
>>>>>>>>>>> Am Freitag 06 April 2012, 23:58:13 schrieb Kevin Rushforth:
>>>>>>>>>>>
>>>>>>>>>>>> I attached it to the JIRA issue.
>>>>>>>>>>>>
>>>>>>>>>>>> -- Kevin
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Florian Brunner wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Here the patch created with "hg diff --git" and zipped.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>> Florian
>>>>>>>>>>>>>
>>>>>>>>>>>>> Am Freitag 06 April 2012, 18:31:23 schrieb Kevin Rushforth:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Right. Since you didn't file the issue and are not the assignee,
>>>>>>>>>>>>>> you can't add an attachment.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Copying Brian Beck in case this is something we can (and want
>>>>>>>>>>>>>> to) fix.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In the mean time, you can e-mail me the patch and I'll attach it
>>>>>>>>>>>>>> to the bug report.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -- Kevin
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Florian Brunner wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Kevin,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for your response.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Unfortunatly, I don't see a way to attach something to the JIRA
>>>>>>>>>>>>>>> issue.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Either I'm missing something or maybe I need additional rights?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>> Florian
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Am Donnerstag 05 April 2012, 18:54:38 schrieben Sie:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Great. Please attach the patch as a zip file to the JIRA issue
>>>>>>>>>>>>>>>> (preferably generated with either webrev or "hg diff --git")
>>>>>>>>>>>>>>>> so that it can be evaluated.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> -- Kevin
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Florian Brunner wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Let me introduce myself: my name is Florian Brunner and I'm a
>>>>>>>>>>>>>>>>> Senior Software Engineer from Switzerland.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I started to use JavaFX in an OSGi environment. Now I
>>>>>>>>>>>>>>>>> stumbled over the following issue, which blocks me:
>>>>>>>>>>>>>>>>> http://javafx-jira.kenai.com/browse/RT-14177
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I have written a patch now. It probably doesn't solve the
>>>>>>>>>>>>>>>>> whole issue, but at least it unblocks me for now.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> It would be great if you could integrate it.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Note that I contributed to OpenJDK before (Swing) and have
>>>>>>>>>>>>>>>>> signed the Sun Contributor Agreement:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://www.oracle.com/technetwork/community/oca-486395.html#b
>>>>>>>>>>>>>>>>> Florian Brunner - java.net - puce
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>>> Florian
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>> --
>> B e s t S o l u t i o n . a t EDV Systemhaus GmbH
>> ------------------------------------------------------------------------
>> tom schindl geschäftsführer/CEO
>> ------------------------------------------------------------------------
>> eduard-bodem-gasse 5-7/1 A-6020 innsbruck fax ++43 512 935833
>> http://www.BestSolution.at phone ++43 512 935834
>
David Grieve | Principal Member of Technical Staff
Mobile: +16033121013
Oracle Java Client UI and Tools
Durham, NH 03824
Oracle is committed to developing practices and products that help protect the environment
More information about the openjfx-dev
mailing list