[rfc][icedtea-web] function applet -> js ->applet call deadlock
Adam Domurad
adomurad at redhat.com
Fri May 17 08:37:17 PDT 2013
On 05/17/2013 11:12 AM, Jiri Vanek wrote:
> On 05/17/2013 04:59 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: 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 ?
>
> I do not. But it is nice to see the difference.
> I think I Insists on this.
Made it harder for me to understand. OK, though.
>
>>
>>> +
>>> + function callApplet() {
>>> + callApplet_real();
>>> + //callApplet_wrap();
>>
>> Don't you just need one function, 'callApplet'?
>
> as above.
>
>>
>>> + }
>>> + </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.
>
> hmhm yeees. This is result of playing with it too much.
>
> If you insists, I will remove.
Please remove, then.
>>
>>> +
>>> + 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
>
> nope.
Sorry if I have missed the value here. Care to explain ? I do not insist
either way.
>>
>>> + 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.
>
> Definitely NOPE.
>
> This is what rules were designed for and this is the cleanest way how
> to avoid doubling the code and still writing the same messages.
Ok, see below.
>
>>
>>> +
>>> + @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.
>
> the same as above :-/////////
Ok, see below.
>
>>
>>> + //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.
>
> Yap. You hit the issue, I'm wondering you have not read it from
> "fix-part".
I have, but the comment 'another error' threw me off.
>
>
> When the timeout bubble out, then the whole stack is halted.
> So when applet>js->applet(deadlock)->js (expectations)
> Then expectation is never executed.
>
> I was thinking about second test, which will do the same, and one
> more assert and will be knowntofail. But As I probably do not want to
> fix it completely, than I do not want to add test testing some
> unfixible thing.
Is it really unfixable ? I think a known-to-fail is deserving
regardless. Even more so if it's not worth trying to fix -- you can
leave comments here for the next poor soul who tries :-))
>
> But I can add if you wont. I was 50/50 with it.
>
> Thanx for review!
>
> J.
>
>
> Ps - Hey you are *real* nitpicker now... Not every line of code have
> to be by your will. And tbh - those rules are exactly for this purpose
> and are cleanest way how to avoid writing to much same/similar code.
Sorry, they won't be insisted and I understand. I do think there are
cleaner solutions with a lot less boilerplate though. It was tricky to
mentally correlate the lines with the strings they match against.
Happy hacking,
-Adam
More information about the distro-pkg-dev
mailing list