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

[Pavel Tisnovsky]

Hi Adam,

points #2 and #3 looks ok.

Do you want to push this change into HEAD or also into some branch?

Cheers,
Pavel

----- Adam Domurad <adomurad at redhat.com> wrote:
> 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