[RFC][icedtea-web] Fix regression in broken AppletTest (Was Re: Broken applet test)

[Jiri Vanek]

On 02/08/2012 01:34 PM, Jiri Vanek wrote:
> On 02/07/2012 12:00 AM, Danesh Dadachanji wrote:
>> Hi,
>>
>> The attached patch should prevent the regression occurring when running AppletTest.
>>
>> It regressed because I did not set the applet var for AppletEnvironment after creating an AppletInstance with a null applet param. I've added this now and gone through wherever AppletEnvironment has been called in icedtea-web. I didn't find anywhere else it was being called so I hope that's the last of this fault. Please comment if you think of anywhere I may have missed! I've also added comments to the reasoning behind the original changeset since it was in no way obvious why that code was refactored.
>>
>> The intent of the original changeset that brought in the regression was to let applets run ServiceManager code in the constructor. I did not realize I could test it at the time using javaws, I thought it could only be done through the plugin. I've made a test for that changeset now. I realized though that it was not the right kind of test (needs access to netx.jar) so I'll rewrite it tomorrow and post it for review separately.
>>
>  >
> Patch is definitely fixing the regression and looks correct.
> Thanx for fast hack, and more thanks to future test:)
> Few hints below.
>

I have noticed that applet, after page is reloaded never starts. Can this be caused by  "Applet can only be set once." ???

>
>> ChangeLog
>> +2012-02-06 Danesh Dadachanji <ddadacha at redhat.com>
>> +
>> + Fixed regression in running webstart applets from JNLP files.
>> + * netx/net/sourceforge/jnlp/Launcher.java (createApplet): Added call to
>> + set applet variable in the AppletInstance's AppletEnvironment.
>> + * netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
>> + (setApplet): New method, set AppletEnvironment's applet variable only once.
>> +
>>
>>
> There are spaces in "diff" but i think your tabs were changed during email processing.
>
>> Cheers,
>> Danesh
>>
>> On 06/02/12 11:47 AM, Danesh Dadachanji wrote:
>>> Hi Jiri,
>>>
>>> My apologies for this regression! =( Commenting below.
>>>
>>> On 05/02/12 08:01 AM, Jiri Vanek wrote:
>>>> This commit -
>>>> http://icedtea.classpath.org/hg/icedtea-web/rev/221174bcd4ec ,
>>>> especially this hunk:
>>>
>>> [snip]
>>>
>>>> have broken applet test. I have verified manually. With this patch
>>>> applet lunched via jnlp (but I have not tested embedded applets - they
>>>> can be affected to!) is causing null pointer exception (only in verbose
>>>> mode, without it just died)
>>>> When the "bad hunk" is reverted, applet works as expected.
>>>
>>> I noticed this problem before when testing some other jnlp_href stuff.
>>> The exception I found was in AppletEnvironment, I assume this is the
>>> same one that you noticed? I missed setting up AppletEnvironment
>>> properly after AppletInstance is setup. I thought any reference to the
>>> applet var would be from AppletInstance, I didn't realize
>>> AppletEnvironment had its own reference too.
>>>
>>> I'm testing some code right now that assigns AppletEnvironment.applet
>>> after AppletInstance is initialized, I'll post a RFC by the end of the
>>> day. I want to make sure I didn't miss anything this time around!
>>>
>>> Regards,
>>> Danesh
>>
>> applet-test-regression-fix-01.patch
>>
>>
>> diff --git a/netx/net/sourceforge/jnlp/Launcher.java b/netx/net/sourceforge/jnlp/Launcher.java
>> --- a/netx/net/sourceforge/jnlp/Launcher.java
>> +++ b/netx/net/sourceforge/jnlp/Launcher.java
>> @@ -708,6 +708,9 @@ public class Launcher {
>>
>> ThreadGroup group = Thread.currentThread().getThreadGroup();
>>
>> + // appletInstance is needed by ServiceManager when looking up
>> + // services. This could potentially be done in applet constructor
>> + // so initialize appletInstance before creating applet.
>> AppletInstance appletInstance;
>> if (cont == null)
>> appletInstance = new AppletInstance(file, group, loader, null);
>> @@ -716,10 +719,14 @@ public class Launcher {
>>
>> loader.setApplication(appletInstance);
>>
>> + // Initialize applet now that ServiceManager has access to its
>> + // appletInstance.
>> String appletName = file.getApplet().getMainClass();
>> Class appletClass = loader.loadClass(appletName);
>> Applet applet = (Applet) appletClass.newInstance();
>> + // Finish setting up appletInstance.
>> appletInstance.setApplet(applet);
>> + appletInstance.getAppletEnvironment().setApplet(applet);
>>
>> setContextClassLoaderForAllThreads(appletInstance.getThreadGroup(), appletInstance.getClassLoader());
>>
>> diff --git a/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java b/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
>> --- a/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
>> +++ b/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
>> @@ -207,6 +207,20 @@ public class AppletEnvironment implement
>> }
>>
>> /**
>> + * Set the applet of this environment; can only be called once.
>> + */
>> + public void setApplet(Applet applet) {
>> + if (this.applet != null) {
>> + if (JNLPRuntime.isDebug()) {
>> + Exception ex = new IllegalStateException("Applet can only be set once.");
>> + ex.printStackTrace();
>
> Well this is more then interesting approach of debug message!
>
> Although personally I have nothing against this (and I was surprised it is working , and I started to like it:) ) it is not common stile in icedetea-web sources and just printed out and never thrown exception can be confusing....
>
> So...
> 1)Shouldn't this exception really be thrown?
> 2)In case that not, then is this really something what need so screaming message? Isn't just "INFO - Somebody is trying setting applet second times
> 3) how worthy is to have this only in debug mode (It looks like it can be serious considering "loudness" of printStackTrace).
>
> a)If this is really harmless that somebody is trying to replace once setted applet, then I think (2) will be enough (and keep it in isDebug branch)
> b)If this is nearly harmless that somebody is trying to replace once setted applet but is important to know about it eg because some crash can be expected in users (not in debug mode) after this happens, then some info like (2) but without isDebug branching will be most correct.
> c)If it is really bad think which should never happens, then throwing this exception also out of debug mode is correct.
>
>  From my current point of view a) is most correct, but feel free to disagree.
> Unless somebody will rise his voice i think the patch is correct if you agree with replacement of exception by plain stderr/out "Applet can only be set once." you can push to head and 1.2.
>
>> + }
>> + return;
>> + }
>> + this.applet = applet;
>> + }
>> +
>> + /**
>> * Returns an enumeration that contains only the applet
>> * from the JNLP file.
>> */
>>
>
>
> Best Regards
>
> J.