[rfc][icedtea-web] Fix for PR1198, JSObject passed incorrectly to Javascript
Adam Domurad
adomurad at redhat.com
Mon Jan 7 07:54:24 PST 2013
On 01/07/2013 10:07 AM, Jiri Vanek wrote:
> 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.
>
> ty :)
>
>>
>>>>
>>>> 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.
> yap
>
>>
>>>
>>>> +
>>>> +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.
>
> oook.
>>
>>>> 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
>> [..snip..]
>> import net.sourceforge.jnlp.ProcessResult;
>> import net.sourceforge.jnlp.ServerAccess.AutoClose;
>> import net.sourceforge.jnlp.annotations.Bug;
>> @@ -62,7 +60,6 @@ public class JSObjectFromEvalTest extend
>> @Test
>> @TestInBrowsers(testIn = { Browsers.all })
>> @NeedsDisplay
>> - @KnownToFail
>> @Bug(id = { "PR1198" })
>> public void testJSObjectSetMemberIsSet() throws Exception {
>> ProcessResult pr =
>> server.executeBrowser("/JSObjectFromEval.html",
>>
>
>
> I'm now ok with the patch except missing unit tests. Also I'm missing
> the prommised :
> " ( unboxPrimitives&& (type == Float.class || type == Double.class) "
> fixes.
Missing how ?
From patch:
+ if ( type == Float.TYPE || type == Double.TYPE ||
+ ( unboxPrimitives && (type == Float.class || type ==
Double.class) )) {
>
> So lets push after those two are solved. Thank you!
> J.
Answer to PS:
>
> One forgotten question to above chunk:
>
> You told that there was one important change in tests - well I have
> noted this one, it does not look so dramatic:) Was there something else??
>
> J.
Sorry, it was pushed as 'cosmetic change' because it would have been
painful to separate :). It was removal of a test assert that explicitly
checked that the return type was java.lang.Integer, but some JS engines
actually returned java.lang.Double. The fix is in HEAD now.
I have also posted new iteration of unit tests.
Thanks for the review!
-Adam
More information about the distro-pkg-dev
mailing list