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

[Adam Domurad]

On 05/17/2013 11:48 AM, Jiri Vanek wrote:
> On 05/17/2013 05:10 PM, Adam Domurad wrote:
>> 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).
> Hmhm. No issue with reproducer of course. At first I had 10s and it sounded to me like an ages...
>
> Do you really wont such an long timeout?
>
> Although I see your concern.... my bargain is going to 20s.. Ouch I would love 10.. but it is to low
> under your offer :)
>
> The vote against the "monstrous Javascript functions out there" :)) is that js->applet->js should
> not be such an evil construction,  Well - js->applet or applet-> js can be... but... Well Its the
> same loop isnt it?
> Then your minute will probably win.
>
> (reproducer need update now, but after your reply)
>


If you can detect re-entry, you can shorten the time.
But this is a general code path, so I think we should tread carefully
with one minute. (It'll make the test a bit long, but probably the
lesser evil).

> * 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.
> As a separate changese unless you insist.
> Refactoring should always happen as separate changeset. Ok?

Sure.

>
>

OK to push with one minute+ changed reproducer IMO.
-Adam