[rfc][icedtea-web] Fix for PR1198, JSObject passed incorrectly to Javascript
Adam Domurad
adomurad at redhat.com
Fri Jan 4 09:19:41 PST 2013
On 01/04/2013 04:43 AM, Jiri Vanek wrote:
> On 12/03/2012 08:03 PM, Adam Domurad wrote:
>> On 11/30/2012 04:08 PM, Adam Domurad wrote:
>>> Hi all. Attached is a fix for PR1198.
>>>
>>> I still plan to write unit tests for at least the new method
>>> PluginAppletViewer.toObjectIDString, but I wanted to get this out
>>> before leaving for the weekend.
>>>
>>> One tricky thing with this patch was that it had to consolidate _a
>>> lot_ of duplicated functionality (actually I found some subtle
>>> differences in handling, this should be more consistent). Once that
>>> was done the actual patch was fairly straight forward.
>>>
>>> The basic issue was that JSObject was being passed as if it were a
>>> normal Java object, when the liveconnect spec specifies that it
>>> should be returned as the underlying javascript object.
>>>
>>> A method was added to JSObject to get the underlying reference. This
>>> was kept package-private to not pollute its public interface. As
>>> well, a new permission was added for accessing this method. To
>>> access this outside of the package, a utility was created in JSUtil.
>>>
>>> This patch adds a special case to Java->JS communication when
>>> sending objects, to indicate that a Javascript object is being passed.
>>>
>>> With patch applied, JSObjectFromEval should pass in all browsers.
>>>
>>> 2012-11-30 Adam Domurad <adomurad at redhat.com>
>>>
>>> Fix PR1198: JSObject passed incorrectly to Javascript
>>> * plugin/icedteanp/IcedTeaJavaRequestProcessor.cc: Pass extra
>>> data for
>>> 'jsobject' object result messages.
>>> * plugin/icedteanp/IcedTeaPluginRequestProcessor.cc: Same.
>>> * plugin/icedteanp/IcedTeaPluginUtils.cc: Add special casing of
>>> javascript references passed from java.
>>> *
>>> plugin/icedteanp/java/netscape/javascript/JSObjectUnboxPermission.java:
>>> New permission for unboxing a JSObject's internal reference.
>>> * plugin/icedteanp/java/netscape/javascript/JSObject.java
>>> (getInternalReference): New, package-private, retrieves internal
>>> reference (Must have proper permission).
>>> * plugin/icedteanp/java/netscape/javascript/JSUtil.java
>>> (getJSObjectInternalReference) New, utility for accessing
>>> JSObject#getInternalReference from outside the package.
>>> *
>>> plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
>>> (toObjectIDString): New, creates a string that precisely
>>> identifies a
>>> Java object.
>>> (handleMessage): Replace a lot of duplicated functionality with
>>> 'toObjectIDString'.
>>> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java: Replace
>>> duplicated functionality with 'toObjectIDString'.
>>> *
>>> tests/reproducers/simple/JSObjectFromEval/srcs/JSObjectFromEval.java:
>>> Don't print out type passed (differs from browser to browser).
>>> *
>>> tests/reproducers/simple/JSObjectFromEval/testcases/JSObjectFromEvalTest.java:
>>> Don't check type passed (differs from browser to browser). Remove
>>> known-to-fail. Reformat.
>>
>> As promised attached is the unit-test for the newly introduced
>> Java->JS function (which encapsulates logic that was duplicated in
>> many areas)
>>
>> Note that this patch contains a Makefile patch for unit testing
>> sun.applet package, however this will be committed with another
>> now-approved patch so I have left it out of the ChangeLog.
>>
>> ChangeLog:
>> 2012-12-XX Adam Domurad <adomurad at redhat.com>
>>
>> Unit test for PluginAppletSecurityContext#toObjectIDString. Make
>> PluginAppletSecurityContext more unit-testable.
>> *
>> plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
>> Don't initialize security manager in constructor. Fix a few
>> Java->JS
>> corner cases.
>> * plugin/icedteanp/java/sun/applet/PluginMain.java: Initialize
>> SecurityManager before creating PluginAppletSecurityContext.
>> * tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java:
>> Unit test for all the corner cases of converting a Java object to a
>> string that can be precisely identified.
>>
>> Happy hacking,
>> -Adam
>>
>>
>
> Tests first. A lot of questions....
>
>> jsfix-unittest.patch
>>
>>
>> diff --git a/Makefile.am b/Makefile.am
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -969,7 +969,7 @@ stamps/netx-unit-tests-compile.stamp: st
>> mkdir -p $(NETX_UNIT_TEST_DIR)&& \
>> $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
>> -d $(NETX_UNIT_TEST_DIR) \
>> - -classpath
>> $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar:$(TEST_EXTENSIONS_DIR) \
>> + -classpath
>> $(JUNIT_JAR):$(DESTDIR)$(datadir)/$(PACKAGE_NAME)/plugin.jar:$(NETX_DIR)/lib/classes.jar:$(TEST_EXTENSIONS_DIR)
>> \
>> @netx-unit-tests-source-files.txt&& \
>> mkdir -p stamps&& \
>> touch $@
>> @@ -999,7 +999,7 @@ stamps/run-netx-unit-tests.stamp: stamps
>> done ; \
>> cd $(NETX_UNIT_TEST_DIR) ; \
>> class_names=`cat $(UNIT_CLASS_NAMES)` ; \
>> -
>> CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):$(TEST_EXTENSIONS_DIR):.
>> \
>> +
>> CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(DESTDIR)$(datadir)/$(PACKAGE_NAME)/plugin.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):$(TEST_EXTENSIONS_DIR):.
>> \
>
> Just for confirmation, I remember both those changes already in,
> little bit better (without installed path)
This is already in, yes
>
>> $(BOOT_DIR)/bin/java -Xbootclasspath:$(RUNTIME) CommandLine
>> $$class_names
>> if WITH_XSLTPROC
>> $(XSLTPROC) --stringparam logs logs_unit.html
>> $(TESTS_SRCDIR)/$(REPORT_STYLES_DIRNAME)/jreport.xsl
>> $(NETX_UNIT_TEST_DIR)/tests-output.xml> $(TESTS_DIR)/index_unit.html
>> diff --git
>> a/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
>> b/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
>> --- a/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
>> @@ -53,7 +53,6 @@ import java.security.Permissions;
>> import java.security.PrivilegedAction;
>> import java.security.ProtectionDomain;
>> import java.util.ArrayList;
>> -import java.util.Arrays;
>> import java.util.Hashtable;
>> import java.util.List;
>> import java.util.Map;
>> @@ -241,16 +240,6 @@ public class PluginAppletSecurityContext
>> public PluginAppletSecurityContext(int identifier) {
>> this.identifier = identifier;
>>
>> - // We need a security manager.. and since there is a good
>> chance that
>> - // an applet will be loaded at some point, we should make it
>> the SM
>> - // that JNLPRuntime will try to install
>> - if (System.getSecurityManager() == null) {
>> - JNLPRuntime.initialize(/* isApplication */false);
>> - JNLPRuntime.setDefaultLaunchHandler(new
>> DefaultLaunchHandler(System.err));
>> - }
>> -
>> - JNLPRuntime.disableExit();
>> -
>
> Can't the moving of this initiliazaton cause some hole?
This code is invoked exactly once, and I figured this type of thing
shouldn't be in a constructor.
It was causing problems for my unit tests because of the security
manager it installed. A different solution is welcome.
The problem is I need access to PluginAppletSecurityContext
#toObjectIDString. It requires a security context instance. IMO this is
an indicator that this class 'does too much' (a class called
SecurityContext handling all the liveconnect logic??).
If you feel I should do it, I can move liveconnect logic into a
different class and then testing will be fine.
> What corner cases it fixed? Can't they be fixed by better way?
> Anyway - it should be in different changeset.
>
>> URL u = null;
>> try {
>> u = new URL("file://");
>> @@ -348,16 +337,16 @@ public class PluginAppletSecurityContext
>> */
>> public String toObjectIDString(Object obj, Class<?> type,
>> boolean unboxPrimitives) {
>>
>> + /* Void (can occur from declared return type), pass special
>> "void" string: */
>> + if (type == Void.TYPE) {
>> + return "literalreturn void";
>> + }
>> +
>> /* Null, pass special "null" string: */
>> if (obj == null) {
>> return "literalreturn null";
>> }
>>
>> - /* Void (can occur from declared return type), pass special
>> "void" string: */
>> - if (type == Void.TYPE) {
>> - return "literalreturn void";
>> - }
>> -
>> /* Primitive, accurately represented by its toString()
>> form: */
>> boolean returnAsString = ( type == Boolean.TYPE
>> || type == Byte.TYPE
>> @@ -378,7 +367,7 @@ public class PluginAppletSecurityContext
>>
>> /* Floating point number, we ensure we give enough
>> precision: */
>> if ( type == Float.TYPE || type == Double.TYPE ||
>> - ( unboxPrimitives&& (type == Double.class || type
>> == Double.class) )) {
>> + ( unboxPrimitives&& (type == Float.class || type ==
>> Double.class) )) {
>
> Why this? Also Shouldn't it be in different changeset?
I will move this into the fix itself, they were bugs discovered during
unit testing :-)
[.. rest snipped..]
More information about the distro-pkg-dev
mailing list