[rfc][icedtea-web] Fix for PR1251: AppContext has wrong context classloader

Tue Apr 23 04:03:16 PDT 2013

On 04/18/2013 07:44 PM, Adam Domurad wrote:
> On 04/08/2013 09:17 AM, Jiri Vanek wrote:
>> On 04/05/2013 08:08 PM, Adam Domurad wrote:
>>> On 04/05/2013 12:18 PM, Jiri Vanek wrote:
>>>> On 01/15/2013 05:53 PM, Adam Domurad wrote:
>>>>> This fixes one of the (numerous) issues in ADOMII, and fixes SweetHome3D furniture dragging.
>>>>>
>>>>> I'll preface by saying, I'm not particularly fond of the changes in this patch, but they have to
>>>>> be done to fix JNLP applications that load resources on the Swing thread, like eg SweetHome3D.
>>>>>
>>>>> The patch adds a flag to JNLPClassLoader that specifies whether it has been initialized. The
>>>>> default for getInstance remains the same, it grabs an initialized JNLPClassLoader. However an
>>>>> alternate getInstance was added to be able to grab JNLPClassLoader that hasn't been fully
>>>>> initialized.
>>>>>
>>>>> The problem with initialization:
>>>>> - To create AppContext we need JNLPClassLoader object constructed
>>>>> - To construct JNLPClassLoader we must initialize it
>>>>> - Initializing JNLPClassLoader implies downloading all resources, and updating Swing components
>>>>>
>>>>> However, we cannot update swing components before we create our AppContext, or the result will be
>>>>> a mess (eg unclickable popups).
>>>>> In essence, it's a bootstrapping problem. So the patch makes it possible to get a constructed
>>>>> JNLPClassLoader, which is initialized in a separate step.
>>>>>
>>>>> What else was tried ?
>>>>> The only other option is to set the context classloader to some other classloader. A proxy
>>>>> classloader can be created that initially points to the system classloader, but can be set to
>>>>> JNLPClassLoader after it is created. Determined too much of a hack, and getParent cannot be
>>>>> properly proxied.
>>>>>
>>>>> Basically with this patch, the life-time of the context classloaders for each thread is:
>>>>> 1. By default system classloader is context classloader.
>>>>> 2. Create new AppContext, capture JNLPClassLoader that isn't yet valid. Ensure we keep using
>>>>> system classloader as context classloader for all threads.
>>>>> 3. Initialize JNLPClassLoader. Before start of applet, ensure all threads context classloaders
>>>>> are set to (now initialized) JNLPClassLoader.
>>>>>
>>>>> Recommendations welcome -- it was a lot of deliberation before I decided to take this route. If
>>>>> someone has a simpler/cleaner solution than by all means suggest it!
>>>>>
>>>>> Reproducer changelog:
>>>>> 2013-XX-XX Adam Domurad <adomurad at redhat.com>
>>>>>
>>>>> * tests/reproducers/signed/AppContextHasJNLPClassLoader/resources/AppContextHasJNLPClassLoader.html:
>>>>> Test AppContext context classloader from HTML applet
>>>>> * tests/reproducers/signed/AppContextHasJNLPClassLoader/resources/AppContextHasJNLPClassLoader.jnlp:
>>>>> Test AppContext context classloader from JNLP application
>>>>> *
>>>>> tests/reproducers/signed/AppContextHasJNLPClassLoader/resources/AppContextHasJNLPClassLoaderForJNLPApplet.jnlp:
>>>>>
>>>>> Test AppContext context classloader from JNLP applet
>>>>> * tests/reproducers/signed/AppContextHasJNLPClassLoader/srcs/AppContextHasJNLPClassLoader.java:
>>>>> Print out context classloader for thread & AppContext, for
>>>>> current thread & Swing thread.
>>>>> *
>>>>> tests/reproducers/signed/AppContextHasJNLPClassLoader/testcases/AppContextHasJNLPClassLoaderTest.java:
>>>>>
>>>>> Test runner for AppContextHasJNLPClassLoader
>>>>>
>>>>> REPRODUCER NOTES:
>>>>> This exposes an additional bug in JNLP applets. start/init incorrectly runs on the Swing thread,
>>>>> and invokeAndWait hangs forever. I'm not sure how this wasn't caught, but the JNLP applet
>>>>> reproducer is broken pending a separate bug fix.
>>>>>
>>>>> Fixes changelog:
>>>>> 2013-XX-XX Adam Domurad <adomurad at redhat.com>
>>>>>
>>>>> Fix PR1251: AppContext has wrong context classloader
>>>>> * netx/net/sourceforge/jnlp/Launcher.java:
>>>>> Remove unused context flag. Make new AppContext capture not-yet-
>>>>> initialized JNLPClassLoader.
>>>>> * netx/net/sourceforge/jnlp/NetxPanel.java:
>>>>> Move PluginBridge initialization into constructor.
>>>>> Make new AppContext capture not-yet-initialized JNLPClassLoader.
>>>>> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
>>>>> Add initialization flag. Move part of constructor into initialize
>>>>> method. Modify getInstance to allow temporarily bypassing
>>>>> initialization.
>>>>> * netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
>>>>> (createAppContext): New, creates an AppContext that captures the given
>>>>> (not necessarily yet valid)
>>>>>
>>>>> -Adam
>>>>
>>>>
>>>> The reproducer have to go in with your new patch.
>>>> It should go both to 1.4 and to head (with proper known to fail).
>>>> But needs fixing - missing @TestInBrowser annotation is quite crucial :)
>>>>
>>>> Without patch, all three failed (but thebrowser test because of unset browser)
>>>>
>>>> with patch (after fixing of annotation)
>>>> Passed: AppContextHasJNLPClassLoaderTest.testJNLPApplicationAppContext
>>>> FAILED: testJNLPAppletAppContext(AppContextHasJNLPClassLoaderTest) stdout should contains
>>>> 'main-thread: thread context classloader == JNLPClassLoader', but did not
>>>> - This test is known to fail
>>>
>>> Fixed
>>>
>>>> Passed: AppContextHasJNLPClassLoaderTest.testAppletAppContext - google-chrome
>>>>
>>>>
>>>> Should the jnlp fial? ( i guess yes as it is mark as known), but have faile dto early:
>>>
>>> See http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1253 for why it is known-to-fail
>>>
>>>>
>>>> /*
>>>> Connecting /home/jvanek/icedtea-web-image/bin/javaws
>>>> http://localhost:56174/AppContextHasJNLPClassLoaderJNLPApplet.jnlp
>>>>
>>>> Fri Apr 05 18:10:40 CEST 2013
>>>>
>>>> net.sourceforge.jnlp.ServerAccess.executeProcessUponURL(ServerAccess.java:680)
>>>> net.sourceforge.jnlp.ServerAccess.executeJavaws(ServerAccess.java:589)
>>>> net.sourceforge.jnlp.ServerAccess.executeJavaws(ServerAccess.java:550)
>>>> AppContextHasJNLPClassLoaderTest.testJNLPAppletAppContext(AppContextHasJNLPClassLoaderTest.java:86)
>>>> sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>>> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
>>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>>> java.lang.reflect.Method.invoke(Method.java:616)
>>>> org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
>>>> org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
>>>> org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
>>>> org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
>>>> net.sourceforge.jnlp.browsertesting.BrowserTestRunner.runLeaf(BrowserTestRunner.java:162)
>>>> net.sourceforge.jnlp.browsertesting.BrowserTestRunner.runChildX(BrowserTestRunner.java:137)
>>>> net.sourceforge.jnlp.browsertesting.BrowserTestRunner.runChild(BrowserTestRunner.java:106)
>>>> net.sourceforge.jnlp.browsertesting.BrowserTestRunner.runChild(BrowserTestRunner.java:58)
>>>> org.junit.runners.ParentRunner$3.run(ParentRunner.java:193)
>>>> org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52)
>>>> org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191)
>>>> org.junit.runners.ParentRunner.access$000(ParentRunner.java:42)
>>>> org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184)
>>>> org.junit.runners.ParentRunner.run(ParentRunner.java:236)
>>>> org.junit.runners.Suite.runChild(Suite.java:128)
>>>> org.junit.runners.Suite.runChild(Suite.java:24)
>>>> org.junit.runners.ParentRunner$3.run(ParentRunner.java:193)
>>>> org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52)
>>>> org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191)
>>>> org.junit.runners.ParentRunner.access$000(ParentRunner.java:42)
>>>> org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184)
>>>> org.junit.runners.ParentRunner.run(ParentRunner.java:236)
>>>> org.junit.runner.JUnitCore.run(JUnitCore.java:157)
>>>> org.junit.runner.JUnitCore.run(JUnitCore.java:136)
>>>> org.junit.runner.JUnitCore.run(JUnitCore.java:117)
>>>> CommandLine.runMain(CommandLine.java:49)
>>>> CommandLine.runMainAndExit(CommandLine.java:28)
>>>> CommandLine.main(CommandLine.java:24)
>>>>
>>>> and now the important one: */
>>>>
>>>> Attempted to download http://localhost:56174/AppContextHasJNLPClassLoaderJNLPApplet.jnlp, but
>>>> failed to connect!
>>>> netx: Read Error: Could not read or parse the JNLP file.
>>>> (http://localhost:56174/AppContextHasJNLPClassLoaderJNLPApplet.jnlp)
>>>>
>>>> reproducers_test_server_deploydir]$ ll AppContextHasJNLPClassLoader*
>>>> -rw-rw-r-- 1 jvanek jvanek 2457 Apr 5 18:10 AppContextHasJNLPClassLoaderForJNLPApplet.jnlp
>>>> -rw-rw-r-- 1 jvanek jvanek 1851 Apr 5 18:10 AppContextHasJNLPClassLoader.html
>>>> -rw-rw-r-- 1 jvanek jvanek 3238 Apr 5 18:08 AppContextHasJNLPClassLoader.jar
>>>> -rw-rw-r-- 1 jvanek jvanek 2355 Apr 5 18:10 AppConte
>>
>> xtHasJNLPClassLoader.jnlp
>>>>
>>>> Wrong name perhaps?
>>>
>>> Fixed
>>>
>>>>
>>>> J.
>>>>
>>>>
>>>> Anyway why was the review of initial patch[1] abandoned?
>>>> [1] [rfc][icedtea-web] Fix for PR1251: AppContext has wrong context classloader - in january 2013
>>>
>>> I spent a while on it struggling with the correct design before I started working on other things. I
>>> still intend to get back to it, but the initial patch & approach I worked on was rejected. I still
>>> need to come up with a good way to approach this; it isn't trivial.
>>>
>>
>> Ach, one more improvemnt - sorry for late notice. Can you add closing listeners for browser tests for case of success?
>
> It already has one, unless I misunderstand you. (It has an auto-closer, anyway.)
>
>> With it it can go to 1.4 with your sweet home patch. But for head should be all methods as knonwToFial. You can add link to @Bug to the 1.4 hack push to keep line between head and 1.4.
>>
>> Please send to one more round both for 1.4 and head when time will come.
>>
>> J
>
> I'll add @KnownToFail before I push.
>

Ok then. hank you.
> Happy hacking,
> -Adam