[rfc][icedtea-web] function applet -> js ->applet call deadlock

[Adam Domurad]

On 05/17/2013 07:04 AM, Jiri Vanek wrote:
>
> Here you go:)
>
>
> J.
>
> On 05/15/2013 05:55 PM, Adam Domurad wrote:
>> On 05/15/2013 07:56 AM, Jiri Vanek wrote:
>>> When applet calls JavaScript function, which calls back to appelt,
>>> then *mostly* deadlock occurs.
>>> I made several attempts to fix this more generally, but I was
>>> unsuccessful.
>>>
>>> So this "hack" is preventing deadlock, maybe also the timeout can be
>>> a bit shorter...
>>>
>>> Although this is for both head and 1.4, for head some more
>>> investigations are needed later.
>>>
>>> J.
>>>
>>> ps,reproducer in progress.
>>
>> Hi, thanks for looking into it. Probably good idea to remove
>> dead-lock potential.
>>
>>> diff -r 9f2d8381f5f1
>>> plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>>> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Fri
>>> May 03 16:17:08 2013 +0200
>>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Mon
>>> May 06 16:06:59 2013 +0200
>>> @@ -1305,8 +1305,16 @@
>>> PluginDebug.debug("wait ToString request 1");
>>> synchronized (request) {
>>> PluginDebug.debug("wait ToString request 2");
>>> - while (request.isDone() == false)
>>> - request.wait();
>>> + int counter = 0;
>>> + while (request.isDone() == false){
>>> + // Do not wait indefinitely to avoid the potential of deadlock
>>> + // but this will destroy the intentional recursion ?
>>> + counter++;
>>> + if (counter>10){
>>> + throw new InterruptedException("Possible deadlock, releasing");
>>> + }
>>> + request.wait(1000);
>>> + }
>>> PluginDebug.debug("wait ToString request 3");
>>> }
>>> } catch (InterruptedException e) {
>>>
>>
>> This is more complex than it needs to be.
>> More simple is:
>>
>> if (!request.isDone()) {
>> request.wait(REQUEST_TIMEOUT);
>> }
>> if (!request.isDone()) {
>> // Do not wait indefinitely to avoid the potential of deadlock
>> throw new RuntimeException("Possible deadlock, releasing");
>> }
>>
>> Your message gets tossed aside and a RuntimeException is thrown if
>> you throw InterruptedException,
>> more direct is better.
>>
>> Also please put this in its own method, eg waitForRequestCompletion
>> (probably good to encapsulate
>> the 'catch InterruptedException' here). There are many methods like
>> this that have a wait loop.
>> Though I would like to see the reproducer before commenting more.
>>
>> Thanks,
>> -Adam
>>
>

RE: the newest fix (well, fix for dead-lock)

> diff -r 1b1e547ccb4a
> plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Wed May
> 15 12:14:26 2013 +0200
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Fri May
> 17 13:02:36 2013 +0200
> @@ -181,6 +181,18 @@
>
>
> private SplashPanel splashPanel;
> +
> + private static long REQUEST_TIMEOUT=2000;//2s

I am a little fearful of legitimate requests that take >2 seconds. There
are some monstrous Javascript functions out there; this timeout is a
hard limit on arbitrary Javascript code. I'd sleep easier if it were at
least a minute (I know this would break the reproducer, but you could
set the time-out longer). We're just asking for confusing regressions
otherwise (at least if we *do* cause a regression with Javascript code
that takes a minute, it will be fairly obvious).

> +
> + private static void waitForRequestCompletion(PluginCallRequest
> request) throws RuntimeException, InterruptedException {

You don't need to declare you're throwing RuntimeException, and I don't
think it's in good style.
Also you can encapsulate the 'catch (InterruptedException)' rethrow
logic here.

> + if (!request.isDone()) {
> + request.wait(REQUEST_TIMEOUT);
> + }
> + if (!request.isDone()) {
> + // Do not wait indefinitely to avoid the potential of deadlock
> + throw new RuntimeException("Possible deadlock, releasing");
> + }
> + }
>
> /**
> * Null constructor to allow instantiation via newInstance()
> @@ -1305,8 +1317,7 @@
> PluginDebug.debug("wait ToString request 1");
> synchronized (request) {
> PluginDebug.debug("wait ToString request 2");
> - while (request.isDone() == false)
> - request.wait();
> + waitForRequestCompletion(request);
> PluginDebug.debug("wait ToString request 3");
> }
> } catch (InterruptedException e) {

Surely this method should be used anywhere that 'while (request.isDone()
== false)' was used. Unless you see a reason it shouldn't.

Happy hacking,
-Adam