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

Adam Domurad adomurad at redhat.com
Fri May 17 07:59:49 PDT 2013


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: Reproducer only, see other email for fix

> diff -r 1b1e547ccb4a 
> tests/reproducers/simple/AppletJsAppletDeadlock/resources/AppletJsAppletDeadlock.html
> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
> +++ 
> b/tests/reproducers/simple/AppletJsAppletDeadlock/resources/AppletJsAppletDeadlock.html 
>  Fri May 17 13:02:36 2013 +0200
> [..license snip..]
> +
> +-->
> +<html><script>
> +    function callApplet_real() {
> + document.getElementById('theApplet').jsCallback(document.location);
> +    }
> +
> +    function callApplet_wrap() {
> +        setTimeout(callApplet_real, 1000);
> +    }

You don't use this, do you ?

> +
> +    function callApplet() {
> +        callApplet_real();
> +        //callApplet_wrap();

Don't you just need one function, 'callApplet'?

> +    }
> +    </script><body>
> +        <p><applet id="theApplet" type="application/x-java-applet"
> +               codebase="." archive="AppletJsAppletDeadlock.jar" 
> code="AppletJsAppletDeadlock.class" width="800" height="250">
> +    </applet></p>
> +</body></html>
> diff -r 1b1e547ccb4a 
> tests/reproducers/simple/AppletJsAppletDeadlock/srcs/AppletJsAppletDeadlock.java
> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
> +++ 
> b/tests/reproducers/simple/AppletJsAppletDeadlock/srcs/AppletJsAppletDeadlock.java 
>  Fri May 17 13:02:36 2013 +0200
> @@ -0,0 +1,89 @@
> +[..license snip..]
> +
> +public class AppletJsAppletDeadlock extends java.applet.Applet {
> +
> +    private TextArea outputText = null;
> +
> +    public void printOutput(String msg) {
> +        System.out.println(msg);
> +        outputText.setText(outputText.getText() + msg + "\n");
> +    }
> +
> +    public void jsCallback(String location) {
> +        printOutput("Callback function called");
> +
> +        // try requesting the page
> +        try {
> +            URL url = new URL(location);
> +            URLConnection con = url.openConnection();
> +            BufferedReader br = new BufferedReader(new InputStreamReader(
> +                    con.getInputStream()));
> +
> +            String line;
> +            while ((line = br.readLine()) != null) {
> +                System.err.println(line);
> +            }

Why do you request the page ? I can't quite see how it's related to the 
test. It seems like it could just be a simple print statement.

> +
> +            printOutput("Succesfully connected to " + location);
> +        } catch (Exception e) {
> +            printOutput("Failed to connect to '" + location + "': " + 
> e.getMessage());
> +        }
> +
> +    }
> +
> +    @Override
> +    public void start() {
> +//    public void init() {

Drop comment line

> +        outputText = new TextArea("", 12, 95);

This is just for visual information right ? It makes the test slightly 
more complicated than it needs to be, but OK if you think it helps.

> +        this.add(outputText);
> +
> +        printOutput("AppletJsAppletDeadlock started");
> +
> +        JSObject win = JSObject.getWindow(this);
> +        win.eval("callApplet();");
> +
> +        printOutput("JS call finished");
> +    }
> +}
> diff -r 1b1e547ccb4a 
> tests/reproducers/simple/AppletJsAppletDeadlock/testcases/AppletJsAppletDeadlockTest.java
> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
> +++ 
> b/tests/reproducers/simple/AppletJsAppletDeadlock/testcases/AppletJsAppletDeadlockTest.java 
>  Fri May 17 13:02:36 2013 +0200
> @@ -0,0 +1,67 @@
> +[..license snip..]
> +
> +public class AppletJsAppletDeadlockTest extends BrowserTest {
> +
> +    private static final String called = "Callback function called";
> +    private static final String started = "AppletJsAppletDeadlock 
> started";
> +    private static final String finished = "JS call finished";
> +
> +    private static final RulesFolowingClosingListener.ContainsRule 
> calledRule = new RulesFolowingClosingListener.ContainsRule(called);
> +    private static final RulesFolowingClosingListener.ContainsRule 
> startedRule =  new RulesFolowingClosingListener.ContainsRule(started);
> +    private static final RulesFolowingClosingListener.ContainsRule 
> finishedRule=  new RulesFolowingClosingListener.ContainsRule(finished);

Pretty hard to read. Simpler please. I doubt it's better than just 
making an assertContains method.

> +
> +    @Test
> +    @NeedsDisplay
> +    @TestInBrowsers(testIn = Browsers.one)
> +    public void callAppletJsAppletNotDeadlock() throws Exception {
> +        ProcessResult processResult = 
> server.executeBrowser("AppletJsAppletDeadlock.html", new 
> RulesFolowingClosingListener(finishedRule), null);
> +        Assert.assertTrue(startedRule.toPassingString(), 
> startedRule.evaluate(processResult.stdout));
> +        Assert.assertTrue(finishedRule.toPassingString(), 
> finishedRule.evaluate(processResult.stdout));

Pretty hard to read. It looks like a lot more than a simple string match.

> +        //this is representing another error, not sure now it is 
> worthy to be fixed

Why is this not occurring ? Is it because of the timeout you introduced 
? IMHO it should be a known-to-fail case. I don't think you should give 
the impression that your 'fix' is the last word here.

> + //Assert.assertTrue(calledRule.toPassingString(), 
> calledRuleRule.evaluate(processResult.stdout));
> +    }
> +}

Happy hacking,
-Adam



More information about the distro-pkg-dev mailing list