[rfc][icedtea-web] Fix for PR1198, JSObject passed incorrectly to Javascript

[Jana Fabrikova]

Hello Adam,

the behaviour of icedtea-web with and without patches jsfix3.patch and 
jsfix-unittest3.patch :

- interactive tests for Liveconnect - the same results

- new Adam's version of reproducer JSObjectFromEval on FF and Opera:
   with patches : passed, without patches: failed

- my not yet oficially included reproducers of J->JS communication:
   basicly the same behaviour with and without the patches


Best regards,
Jana


On 01/04/2013 08:36 PM, Adam Domurad wrote:
> On 01/04/2013 05:23 AM, Jiri Vanek wrote:
>> [.. snipped..]
>
> As you suggested, I have separated the cosmetic changes and pushed them.
> It did prove quite tricky, though.
>
>>>
>>> diff --git a/NEWS b/NEWS
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -19,6 +19,7 @@ New in release 1.4 (2012-XX-XX):
>>>   * Plugin
>>>     - PR1106: Buffer overflow in plugin table-
>>>     - PR1166: Embedded JNLP File is not supported in applet tag
>>> +  - PR1198: JSObject is not passed to javascript correctly
>>>   * Common
>>>     - PR1049: Extension jnlp's signed jar with the content of only
>>> META-INF/* is considered
>>>     - PR955: regression: SweetHome3D fails to run
>>> diff --git a/plugin/icedteanp/IcedTeaJavaRequestProcessor.cc
>>> b/plugin/icedteanp/IcedTeaJavaRequestProcessor.cc
>>> --- a/plugin/icedteanp/IcedTeaJavaRequestProcessor.cc
>>> +++ b/plugin/icedteanp/IcedTeaJavaRequestProcessor.cc
>>> @@ -131,7 +131,7 @@ JavaRequestProcessor::newMessageOnBus(co
>>> !message_parts->at(4)->find("GetObjectArrayElement"))
>>>               {
>>>
>>> -                if (!message_parts->at(5)->find("literalreturn"))
>>> +                if (!message_parts->at(5)->find("literalreturn") ||
>>> !message_parts->at(5)->find("jsobject"))
>>>                   {
>>>                       // literal returns don't have a corresponding
>>> jni id
>>>                       result->return_identifier = 0;
>>> diff --git a/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
>>> b/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
>>> --- a/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
>>> +++ b/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
>>> @@ -413,7 +413,7 @@ PluginRequestProcessor::setMember(std::v
>>>       member = (NPVariant*)
>>> (IcedTeaPluginUtilities::stringToJSID(*(message_parts->at(5))));
>>>       propertyNameID = *(message_parts->at(6));
>>>
>>> -    if (*(message_parts->at(7)) == "literalreturn")
>>> +    if (*(message_parts->at(7)) == "literalreturn" ||
>>> *(message_parts->at(7)) == "jsobject" )
>>>       {
>>>           value.append(*(message_parts->at(7)));
>>>           value.append(" ");
>>> diff --git a/plugin/icedteanp/IcedTeaPluginUtils.cc
>>> b/plugin/icedteanp/IcedTeaPluginUtils.cc
>>> --- a/plugin/icedteanp/IcedTeaPluginUtils.cc
>>> +++ b/plugin/icedteanp/IcedTeaPluginUtils.cc
>>> @@ -776,6 +776,14 @@ javaStringResultToNPVariant(const std::s
>>>   }
>>>
>>>   static bool
>>> +javaJSObjectResultToNPVariant(const std::string&  js_id, NPVariant*
>>> variant)
>>> +{
>>> +    NPVariant* result_variant = (NPVariant*)
>>> IcedTeaPluginUtilities::stringToJSID(js_id);
>>> +    *variant = *result_variant;
>>> +    return true;
>>> +}
>>
>> It returns always true?? Maybe void or *variant?
>
> I left this accidentally after it could no longer fail, made it void.
>
>>
>>> +
>>> +static bool
>>>   javaObjectResultToNPVariant(NPP instance, const std::string&
>>> jobject_id, NPVariant* variant)
>>>   {
>>>       // Reference the class object so we can construct an NPObject
>>> with it and the instance
>>> @@ -811,9 +819,14 @@ IcedTeaPluginUtilities::javaResultToNPVa
>>>           std::string* java_value, NPVariant* variant)
>>>   {
>>>       int literal_n = sizeof("literalreturn"); // Accounts for one
>>> space char
>>> +    int jsobject_n = sizeof("jsobject"); // Accounts for one space char
>>> +
>>>       if (strncmp("literalreturn ", java_value->c_str(), literal_n)
>>> == 0)
>>>       {
>>> javaPrimitiveResultToNPVariant(java_value->substr(literal_n), variant);
>>> +    } else if (strncmp("jsobject ", java_value->c_str(), jsobject_n)
>>> == 0)
>>> +    {
>>> + javaJSObjectResultToNPVariant(java_value->substr(jsobject_n),
>>> variant);
>>>       } else
>>>       {
>>>           std::string jobject_id = *java_value;
>>>
> [... long snip...]
>>>       public void setJSMember(JSObject js, String memb, Object val) {
>>> -        String typeName = val.getClass().getName();
>>> -        System.out.println("setJSMember: passed '" + typeName + "'");
>> Not worthy to keep in debug?
>> Anyway - cosmetic change. See my comment lower.
>
> No, this was part of the expected reproducer output. It was removed from
> the checks because the type is dependent on the browser. It was causing
> failures on some browsers. I have pushed it with my cosmetic changes
> anyway.
>
>>>           js.setMember(memb, val);
>>>       }
>>>   }
>>> \ No newline at end of file
>>> diff --git
>>> a/tests/reproducers/simple/JSObjectFromEval/testcases/JSObjectFromEvalTest.java
>>> b/tests/reproducers/simple/JSObjectFromEval/testcases/JSObjectFromEvalTest.java
>>>
>>> ---
>>> a/tests/reproducers/simple/JSObjectFromEval/testcases/JSObjectFromEvalTest.java
>>>
>>> +++
>>> b/tests/reproducers/simple/JSObjectFromEval/testcases/JSObjectFromEvalTest.java
>>>
>>> @@ -40,7 +40,6 @@ import static org.junit.Assert.assertTru
>>>   import net.sourceforge.jnlp.ProcessResult;
>>>   import net.sourceforge.jnlp.ServerAccess.AutoClose;
>>>   import net.sourceforge.jnlp.annotations.Bug;
>>> -import net.sourceforge.jnlp.annotations.KnownToFail;
>>>   import net.sourceforge.jnlp.annotations.NeedsDisplay;
>>>   import net.sourceforge.jnlp.annotations.TestInBrowsers;
>>>   import net.sourceforge.jnlp.browsertesting.BrowserTest;
>>> @@ -51,40 +50,37 @@ import org.junit.Test;
>>>
>>>   public class JSObjectFromEvalTest extends BrowserTest {
>>>
>>> -    private static final String END_STRING =
>>> AutoOkClosingListener.MAGICAL_OK_CLOSING_STRING;
>>> +    private static final String END_STRING =
>>> AutoOkClosingListener.MAGICAL_OK_CLOSING_STRING;
>>>
>>> -    private static final String JAVA_CREATE = "Java create\n";
>>> -    private static final String JS_CREATE = "JS create\n";
>>> -    private static final String JAVA_SET = "Java set\n";
>>> -    private static final String PASSED_INTEGER = "setJSMember:
>>> passed 'java.lang.Integer'\n";
>>> -    private static final String CORRECT_VALUE = "obj.test = 0";
>>> +    private static final String JAVA_CREATE = "Java create\n";
>>> +    private static final String JS_CREATE = "JS create\n";
>>> +    private static final String JAVA_SET = "Java set\n";
>>> +    private static final String CORRECT_VALUE = "obj.test = 0";
>>>
>>> -    @Test
>>> -    @TestInBrowsers(testIn = { Browsers.all })
>>> -    @NeedsDisplay
>>> -    @Bug(id = { "PR1198" })
>>> -    @KnownToFail
>>> -    public void testJSObjectSetMemberIsSet() throws Exception {
>>> -        ProcessResult pr =
>>> server.executeBrowser("/JSObjectFromEval.html",
>>> -                AutoClose.CLOSE_ON_BOTH);
>>> +    @Test
>>> +    @TestInBrowsers(testIn = { Browsers.all })
>>> +    @NeedsDisplay
>>> +    @Bug(id = { "PR1198" })
>>> +    public void testJSObjectSetMemberIsSet() throws Exception {
>>> +        ProcessResult pr =
>>> server.executeBrowser("/JSObjectFromEval.html",
>>> +                AutoClose.CLOSE_ON_BOTH);
>>>
>>> -        String expectedJSCreateOutput = JS_CREATE + JAVA_SET +
>>> PASSED_INTEGER
>>> -                + CORRECT_VALUE;
>>> -        String expectedJavaCreateOutput = JAVA_CREATE + JAVA_SET
>>> -                + PASSED_INTEGER + CORRECT_VALUE;
>>> +        String expectedJSCreateOutput = JS_CREATE + JAVA_SET +
>>> CORRECT_VALUE;
>>> +        String expectedJavaCreateOutput = JAVA_CREATE + JAVA_SET
>>> +                + CORRECT_VALUE;
>>>
>>> -        // No reason JS create should fail, this is mostly a sanity
>>> check:
>>> -        assertTrue("stdout should contain 'JS create [...] " +
>>> CORRECT_VALUE
>>> -                + "' but did not.",
>>> pr.stdout.contains(expectedJSCreateOutput));
>>> +        // No reason JS create should fail, this is mostly a sanity
>>> check:
>>> +        assertTrue("stdout should contain 'JS create [...] " +
>>> CORRECT_VALUE
>>> +                + "' but did not.",
>>> pr.stdout.contains(expectedJSCreateOutput));
>>>
>>> -        // Demonstrates PR1198:
>>> -        assertTrue("stdout should contain 'Java create [...] " +
>>> CORRECT_VALUE
>>> -                + "' but did not.",
>>> -                pr.stdout.contains(expectedJavaCreateOutput));
>>> +        // Demonstrates PR1198:
>>> +        assertTrue("stdout should contain 'Java create [...] " +
>>> CORRECT_VALUE
>>> +                + "' but did not.",
>>> +                pr.stdout.contains(expectedJavaCreateOutput));
>>>
>>> -        // Make sure we got to the end of the script
>>> -        assertTrue("stdout should contain '" + END_STRING + "' but
>>> did not.",
>>> -                pr.stdout.contains(END_STRING));
>>> -    }
>>> +        // Make sure we got to the end of the script
>>> +        assertTrue("stdout should contain '" + END_STRING + "' but
>>> did not.",
>>> +                pr.stdout.contains(END_STRING));
>>> +    }
>>>
>>>   }
>>
>> Cosmetic changes - normlay I'm for smughling small cosmetich changes
>> which were found during coding, especially somnething like
>> > -                    buf
>> > - .append(" "
>> > - + Integer
>> > - .toString(((int) b[i])&  0x0ff, 16));
>> > +                    buf.append(" " + Integer.toString(((int)
>> b[i])&  0x0ff, 16));
>>
>> :))
>>
>> into patch. But there is one whole refacotred file and little bit
>> more. If you will be willing can you separate the changes? You can
>> push the comsetic changes directly.
>>
>> Well and then you will start to hate me with removing of KnowToFail...
>> Thanx for fix.
>>  J.
>>
>>
>> ps: Try to check js<->java automated tests and manual liveConnect
>> testuite. I think there wil be some hddien fixes by this aptch. Adnd
>> With some luck also no regressions;) Maybe jana Can meassure those
>> results for you. She is th eguru of liveConnect testing afterall..
>> pps: Dont take me wrong, this is really nice cleanup and fix.
>
> Will do.
>
> Attached is an updated patch, with some cleanup removed (since it was
> pushed) and the fixes from my unit test patch added.
> See my reply to unit test review though -- I need some additional advice
> on what to do about the security manager problem.
>
> Happy hacking,
> -Adam