[rfc][icedtea-web] Ensure applet shutdown finishes without exceptions - 1 - npe

Adam Domurad adomurad at redhat.com
Tue Feb 5 08:46:40 PST 2013


On 02/01/2013 04:18 AM, Jiri Vanek wrote:
> On 01/31/2013 11:05 PM, Adam Domurad wrote:
>> Hi all. Another round of what I think could be important stability 
>> fixes for icedtea-web.
>>
>> I noticed while doing my unsigned-applet-confirmation patch that 
>> JNLPClassLoader instances did not always reach reference-count == 0 
>> when all their instances should have been destroyed. The first part 
>> of that was the applet synchronization issue, that I have posted a 
>> patch for.
>>
>> 3 additional problems fixed here:
>>
>> Fixed in npe-fix-and-wait-for-init.patch:
>> 1.) There was a sometimes-occurring NPE in 
>> JNLPClassLoader#getPermisions, when a CodeSource did not have an 
>> associated location when an applet was destroyed. This could cause 
>> the destroyApplet code to not finish. This was fixed with a simple 
>> null check guard. This fixes some spurious exceptions being printed 
>> out during shutdown (causing some noise in reproducer system).
>>
>> 2.) Applets can actually be destroyed from one thread *in the middle 
>> of initialization*, while somewhat hard to reproduce, spamming 
>> refresh on a many-applet page inevitably caused it. Luckily a 
>> mechanism was already in place for efficiently waiting for applets to 
>> initialize.
>>
>>
>> npe-fix-and-wait-for-init.patch changes:
>> 2013-XX-XX  Adam Domurad <adomurad at redhat.com>
>>
>>      Ensure applet destruction cannot in the middle of initialization.
>>      Prevent NPE that can sometimes occurs (especially with this 
>> change).
>>      * netx/net/sourceforge/jnlp/NetxPanel.java
>>      (destroyApplet): wait for applet initialization
>>      * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>>      (getPermissions): avoid potential NPE if code source location is
>>      missing
>>
>
>>
>>
>> npe-fix-and-wait-for-init.patch
>>
>>
>> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java 
>> b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> @@ -1144,7 +1144,7 @@ public class JNLPClassLoader extends URL
>>               }
>>
>>               // Class from host X should be allowed to connect to 
>> host X
>> -            if (cs.getLocation().getHost().length()>  0)
>> +            if (cs.getLocation() != null&& 
>> cs.getLocation().getHost().length()>  0)
>
> Here I'm curious,  is NPE  here because location  was not yet 
> initalised,  have no chance to get in, its initialisation was 
> corrupted, or is NPE here because its it wa forgotten to be assigned 
> or its creation was somehow skipped?
>
> It it is first case  then I'm for NPE guard.
> If it is the second then I'm against, because the consequence is 
> healed and the cause will be rotten later. However still the NPE guard 
> can be last and only hope.

I'll admit I don't know the exact situation that causes this, but it 
only occurred during shutdown. Seeing as I can't reproduce it ATM I'm 
fine having this or not, but I don't think it would hide any problems.

>
>>                   result.add(new 
>> SocketPermission(cs.getLocation().getHost(),
>>                           "connect, accept"));
>>
>> diff --git a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java 
>> b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>> @@ -587,6 +587,10 @@ public class PluginAppletViewer extends
>>
>>       private static synchronized void destroyApplet(int identifier) {
>>
>> +        // We should not try to destroy an applet during
>> +        // initialization. It may cause an inconsistent state.
>> +        waitForAppletInit( applets.get(identifier).panel );
>
> Ok, this have sense.
>> +
>>           PluginDebug.debug("DestroyApplet called for ", identifier);
>>
>>           PAV_INIT_STATUS prev = updateStatus(identifier, 
>> PAV_INIT_STATUS.DESTROYED);
>>

-Adam



More information about the distro-pkg-dev mailing list