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

Mon Apr 8 06:17:45 PDT 2013

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