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

[Adam Domurad]

On 03/28/2013 09:39 AM, Pavel Tisnovsky wrote:
> Hi Adam,
>
> points #2 and #3 looks ok.
>
> Do you want to push this change into HEAD or also into some branch?

I think just HEAD. It can be lived with until 1.4 release, which has 
related fixes.

Thanks for the review!
-Adam

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