[rfc][icedtea-web] Fix for PR1198, JSObject passed incorrectly to Javascript
Adam Domurad
adomurad at redhat.com
Fri Jan 4 11:36:05 PST 2013
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jsfix3.patch
Type: text/x-patch
Size: 26866 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130104/2509b0a9/jsfix3.patch
More information about the distro-pkg-dev
mailing list