Patch: issue RT-14177
Richard Bair
richard.bair at oracle.com
Fri Jun 1 09:58:22 PDT 2012
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
More information about the openjfx-dev
mailing list