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

[Adam Domurad]

On 04/24/2013 02:22 AM, Jiri Vanek wrote:
> 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.

I am unable to reproduce outside of my laptop. However it was clear, 
once it entered the deadlock, why it had done so.

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

Was it ? :-))

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

Sorry, where do you want this description, and of what 'removal code' ?

>
> Anyway.. push when you feel ....

If you have any reservations, don't hesitate to bring them up.

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