[rfc][icedtea-web] Fix for PR1198, JSObject passed incorrectly to Javascript
Jana Fabrikova
jfabriko at redhat.com
Fri Jan 11 10:20:29 PST 2013
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
More information about the distro-pkg-dev
mailing list