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

[Jiri Vanek]

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.

>                   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);
>