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

[Jiri Vanek]

On 02/05/2013 05:46 PM, Adam Domurad wrote:
> 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.

I was afraid there will be reply like this :(

If it have sense to push alone, please go on. The part two still eed confirmation from Pavel.
>
>>
>>>                   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