[rfc][icedtea-web] A dead-lock / Firefox hanging fix

[Jiri Vanek]

On 04/16/2013 10:15 PM, Adam Domurad wrote:
> Hi all.
>
> Recently on one of my computers I noticed that Firefox was hanging every time an applet failed. While I'm not sure why this was suddenly happening every time, I took it as an opportunity to look into a fix while the bug was in plain sight.

Was this caused by some special version of firefox?

I was unable to reproduce - nowhere.

Anyway I think this cleanup is good thing and is doing the code much more readable

The only torubeling thing is "why it was so complicated??"
>
> The problem area is here:
>
> plugin/icedteanp/java/sun/applet/PluginAppletViewer.java:740 (from HEAD)
>> while (panel == null || !panel.isAlive()) {
>> ^^^^^^^^^^^^^
>> maxTimeToSleep -= waitTillTimeout(panelLock, panelLive,
>> maxTimeToSleep);
>>
>> /* we already waited till timeout, give up here directly,
>> * instead of waiting 180s again in below waitForAppletInit()
>> */
>> if(maxTimeToSleep < 0) {
>> streamhandler.write("instance " + identifier + " reference " + -1 + " fatalError: " + "Initialization timed out");
>> return;
>
> The intent for using panel.isAlive() in the condition here is presumably to ensure that the handler thread is created. NetxPanel is created with appletAlive=true, so the following method:
>
> netx/net/sourceforge/jnlp/NetxPanel.java:196 (from HEAD)
>> public boolean isAlive() {
>> return handler != null && handler.isAlive() && this.appletAlive;
>> }
>
> Effectively tests that the handler thread was created. There's one problem, in the case of an exception path ...
>
> netx/net/sourceforge/jnlp/NetxPanel.java:150 (from HEAD)
>> } catch (Exception e) {
>> this.appletAlive = false;
>> ^^^^^^^^^^^^^^^^^^
>> e.printStackTrace();
>> replaceSplash(SplashUtils.getErrorSplashScreen(getWidth(), getHeight(), e));
>
> This unfortunately causes isAlive() to permanently return false from that point onwards. Clearly if isAlive() ever gets false in the condition at PluginAppletViewer:740 we will loop forever. The C++ side of the plugin can do nothing but wait. Additionally, since icedtea-web is run in-process unlike other plugins, this hangs firefox completely.
>
> One thing to notice is, PluginAppletViewer:740-751 are fairly redundant because line 760 (== 'waitForAppletInit(panel)') waits for complete initialization anyway.
> The patch therefore axes these lines, which were mostly identical to waitForAppletInit. There is no need to separately wait for the handler thread to initialize. It's also worth noting that waiting on panel == null makes little sense because 'panel' is set *only* in the constructor. Therefore if it were ever null we would be spinning forever anyway.
>
> The other part of the fix was making sure the C++ side of the plugin gets a response.
>
> plugin/icedteanp/java/sun/applet/PluginAppletViewer.java:764 (from HEAD)
>> if (panel.getApplet() == null) {
>> streamhandler.write("instance " + identifier + " reference " + -1 + " fatalError: " + "Initialization failed");
>> return;
>> }
>
> This lacks a response to the message being handled, meaning the C++ side will wait until timeout. Adding 'streamhandler.write("context 0 reference " + reference + " Error");' solves the problem neatly.
>
>
> ChangeLog:

Myabe little but more detailed description of the removal code (similar as above)

Anyway.. push when you feel ....
> 2013-XX-XX Adam Domurad <adomurad at redhat.com>
>
> Fix a dead-lock that can cause (namely) Firefox to hang.
> * netx/net/sourceforge/jnlp/NetxPanel.java
> (appletAlive): Remove flag.
> (isAlive): Remove getter.
> (initialized): New, explicit initialization flag.
> (isInitialized): New, getter.
> (runLoader): Set initialization flag when done (whether errored or not).
> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> (waitForAppletInit): Wait on initialization flag from NetxPanel.
> (handleMessage): Remove redundant waiting for init. Respond properly to
> GetJavaObject in case of error/time-out.
>
>
> Happy hacking,
> -Adam