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

[Adam Domurad]

On 01/31/2013 05: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.
>
> Fixed in dont-interrupt-workers.patch:
>
> 3.) Applets were being interrupted while shutting down -- a major 
> cause of spurious exceptions being printed out (causing even more 
> noise in reproducer system). This interrupt facility was too 
> heavy-weight for the desired task. Instead Java's builtin wait & 
> notify facilities are now used to perform the same operation on a much 
> finer scale.
>
> ** Note that I looked into the matter and saw that the method 
> notifyWorkerIsFree did not actually perform the desired task and was 
> removed as misleading. In fact the consumer thread spins around 
> looking for free workers and did not wait for an interruption. This 
> behaviour has not changed, and the method is not needed (nor was it 
> needed how the code was before).
>
> 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
>
> dont-interrupt-workers.patch changes:
> 2013-XX-XX  Adam Domurad  <adomurad at redhat.com>
>
>     Don't interrupt worker/consumer threads (can prevent shutdown code 
> from
>     executing); instead use Object wait/notify methods.
>     * plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
>     (notifyHasWork): Replacement for thread interruption
>     (waitForWork): Replacement for thread sleeping
>     (run): Use waitForWork instead of Thread.sleep
>     (notifyWorkerIsFree): Removed -- misleading method.
>     * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>     (message): Make volatile, as it should have always been.
>     (notifyHasWork): Replacement for thread interruption
>     (waitForWork): Replacement for thread sleeping
>     (run): Use waitForWork instead of Thread.sleep
>     (getPermissions): avoid potential NPE if code source location is
>     missing
>     (free): Remove reference to notifyWorkerIsFree.

Ping ? (for dont-interrupt-workers.patch) This is IMO quite an 
improvement for stability. It's also bugging me seeing this avoidable 
InterruptedException over and over.

>
>
> I am excited to see these changes get in. I will see soon if there's 
> any effect on the amount of noise in the test system.
>
> Happy hacking,
> -Adam

-Adam