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

[Jiri Vanek]

On 04/24/2013 04:00 PM, Adam Domurad wrote:
> 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 ? :-))

yah.. the original implementation was quite unclear...
>
>>>
>>> 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' ?

"740-751 are fairly redundant because line 760 (==
 >>> 'waitForAppletInit(panel)') waits for complete initialization anyway."

/s/removal/removed

>
>>
>> Anyway.. push when you feel ....
>
> If you have any reservations, don't hesitate to bring them up.

I wish I have. The fact that I'm  unable to reproduce is put me to strange situation.. but Ilike it...
>
>>> 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