Fixes Issue #631.
Avoid collisions with pre-defined properties in JavaScript objects,
either by prepending a ":" where we have control over accesses to the
underlying object, or by using Object.hasOwnProperty for ones that we
don't. Added RPC test for String arrays with values to verify that
strings with property names are properly handled.
Patch by: sandymac, jat
Review by: knorton, mmendez
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@931 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/i18n/client/Dictionary.java b/user/src/com/google/gwt/i18n/client/Dictionary.java
index f89d028..a541023 100644
--- a/user/src/com/google/gwt/i18n/client/Dictionary.java
+++ b/user/src/com/google/gwt/i18n/client/Dictionary.java
@@ -1,5 +1,5 @@
/*
- * Copyright 2006 Google Inc.
+ * Copyright 2007 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
@@ -127,14 +127,20 @@
/**
* Get the value associated with the given Dictionary key.
*
+ * We have to call Object.hasOwnProperty to verify that the value is
+ * defined on this object, rather than a superclass, since normal Object
+ * properties are also visible on this object.
+ *
* @param key to lookup
* @return the value
* @throws MissingResourceException if the value is not found
*/
public native String get(String key) /*-{
var value = this.@com.google.gwt.i18n.client.Dictionary::dict[key];
- if (value == null) {
- this.@com.google.gwt.i18n.client.Dictionary::resourceError(Ljava/lang/String;)(key);
+ if (value == null || !Object.prototype.hasOwnProperty.call(
+ this.@com.google.gwt.i18n.client.Dictionary::dict, key))
+ {
+ this.@com.google.gwt.i18n.client.Dictionary::resourceError(Ljava/lang/String;)(key);
}
return String(value);
}-*/;
diff --git a/user/src/com/google/gwt/json/client/JSONObject.java b/user/src/com/google/gwt/json/client/JSONObject.java
index 6221085..9874b41 100644
--- a/user/src/com/google/gwt/json/client/JSONObject.java
+++ b/user/src/com/google/gwt/json/client/JSONObject.java
@@ -1,5 +1,5 @@
/*
- * Copyright 2006 Google Inc.
+ * Copyright 2007 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
@@ -43,34 +43,52 @@
/**
* Tests whether or not this JSONObject contains the specified key.
*
+ * We use Object.hasOwnProperty here to verify that a given key is
+ * specified on this object rather than a superclass (such as standard
+ * properties defined on Object).
+ *
* @param key the key to search for
* @return <code>true</code> if the JSONObject contains the specified key
*/
public native boolean containsKey(String key) /*-{
- return this.@com.google.gwt.json.client.JSONObject::backStore[key] !== undefined
- || this.@com.google.gwt.json.client.JSONObject::frontStore[key] !== undefined;
+ return Object.prototype.hasOwnProperty.call(
+ this.@com.google.gwt.json.client.JSONObject::backStore, key)
+ || Object.prototype.hasOwnProperty.call(
+ this.@com.google.gwt.json.client.JSONObject::frontStore, key);
}-*/;
/**
* Gets the JSONValue associated with the specified key.
*
+ * We use Object.hasOwnProperty here to verify that a given key is
+ * specified on this object rather than a superclass (such as standard
+ * properties defined on Object).
+ *
* @param key the key to search for
* @return if found, the value associated with the specified key, or
* <code>null</code> otherwise
*/
public native JSONValue get(String key) /*-{
- if (this.@com.google.gwt.json.client.JSONObject::backStore[key] !== undefined) {
- var x=this.@com.google.gwt.json.client.JSONObject::backStore[key];
- if (typeof x == 'number' || typeof x == 'string' || typeof x == 'array' || typeof x == 'boolean') {
+ if (Object.prototype.hasOwnProperty.call(
+ this.@com.google.gwt.json.client.JSONObject::backStore, key))
+ {
+ var x = this.@com.google.gwt.json.client.JSONObject::backStore[key];
+ if (typeof x == 'number' || typeof x == 'string' || typeof x == 'array'
+ || typeof x == 'boolean')
+ {
x = Object(x);
}
this.@com.google.gwt.json.client.JSONObject::frontStore[key]=
- // don't linebreak the next line
- @com.google.gwt.json.client.JSONParser::buildValue(Lcom/google/gwt/core/client/JavaScriptObject;)(x);
+ // don't linebreak the next line
+ @com.google.gwt.json.client.JSONParser::buildValue(Lcom/google/gwt/core/client/JavaScriptObject;)(x);
delete this.@com.google.gwt.json.client.JSONObject::backStore[key];
}
- var out = this.@com.google.gwt.json.client.JSONObject::frontStore[key];
- return (out == null) ? null : out;
+ if (Object.prototype.hasOwnProperty.call(
+ this.@com.google.gwt.json.client.JSONObject::frontStore, key))
+ {
+ return this.@com.google.gwt.json.client.JSONObject::frontStore[key];
+ }
+ return null;
}-*/;
/**
@@ -102,7 +120,8 @@
* <code>null</code> otherwise
*/
public native JSONValue put(String key, JSONValue jsonValue) /*-{
- var out = this.@com.google.gwt.json.client.JSONObject::get(Ljava/lang/String;)(key);
+ var out = // can't break next line
+ this.@com.google.gwt.json.client.JSONObject::get(Ljava/lang/String;)(key);
this.@com.google.gwt.json.client.JSONObject::frontStore[key] = jsonValue;
return out;
}-*/;
diff --git a/user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamWriter.java b/user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamWriter.java
index ebaa8bc..89fb7d6 100644
--- a/user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamWriter.java
+++ b/user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamWriter.java
@@ -37,9 +37,18 @@
return {};
}-*/;
- JavaScriptObject objectMap;
+ /*
+ * Accessed from JSNI code, so ignore unused warning.
+ */
+ private JavaScriptObject objectMap;
- JavaScriptObject stringMap;
+ /*
+ * Accesses need to be prefixed with ':' to prevent conflict with built-in
+ * JavaScript properties.
+ *
+ * Accessed from JSNI code, so ignore unused warning.
+ */
+ private JavaScriptObject stringMap;
private StringBuffer encodeBuffer;
@@ -123,8 +132,9 @@
return (result == null) ? -1 : result;
}-*/;
+ // prefix needed to prevent conflict with built-in JavaScript properties.
private native int getIntForString(String key) /*-{
- var result = this.@com.google.gwt.user.client.rpc.impl.ClientSerializationStreamWriter::stringMap[key];
+ var result = this.@com.google.gwt.user.client.rpc.impl.ClientSerializationStreamWriter::stringMap[':' + key];
return (result == null) ? 0 : result;
}-*/;
@@ -132,8 +142,9 @@
this.@com.google.gwt.user.client.rpc.impl.ClientSerializationStreamWriter::objectMap[key] = value;
}-*/;
+ // prefix needed to prevent conflict with built-in JavaScript properties.
private native void setIntForString(String key, int value) /*-{
- this.@com.google.gwt.user.client.rpc.impl.ClientSerializationStreamWriter::stringMap[key] = value;
+ this.@com.google.gwt.user.client.rpc.impl.ClientSerializationStreamWriter::stringMap[':' + key] = value;
}-*/;
private void writeHeader(StringBuffer buffer) {
diff --git a/user/src/com/google/gwt/user/client/ui/FastStringMap.java b/user/src/com/google/gwt/user/client/ui/FastStringMap.java
index 1e4fe9e..885faad 100644
--- a/user/src/com/google/gwt/user/client/ui/FastStringMap.java
+++ b/user/src/com/google/gwt/user/client/ui/FastStringMap.java
@@ -1,5 +1,5 @@
/*
- * Copyright 2006 Google Inc.
+ * Copyright 2007 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
@@ -57,6 +57,7 @@
return false;
}
+ // strip prefix from key
public Object getKey() {
return key;
}
@@ -94,7 +95,11 @@
}
}
- JavaScriptObject map;
+ /*
+ * Accesses need to be prefixed with ':' to prevent conflict with built-in
+ * JavaScript properties.
+ */
+ private JavaScriptObject map;
public FastStringMap() {
init();
@@ -157,14 +162,12 @@
return get(keyMustBeString(key));
}
+ // Prepend ':' to avoid conflicts with built-in Object properties.
public native Object get(String key) /*-{
- var value = this.@com.google.gwt.user.client.ui.FastStringMap::map[key];
- if(value == null){
- return null;
- } else{
- return value;
- }
- }-*/;
+ var value
+ = this.@com.google.gwt.user.client.ui.FastStringMap::map[':' + key];
+ return (value == null) ? null : value;
+ }-*/;
public boolean isEmpty() {
return size() == 0;
@@ -192,15 +195,13 @@
return put(keyMustBeString(key), widget);
}
+ // Prepend ':' to avoid conflicts with built-in Object properties.
public native Object put(String key, Object widget) /*-{
- var previous = this.@com.google.gwt.user.client.ui.FastStringMap::map[key];
- this.@com.google.gwt.user.client.ui.FastStringMap::map[key] = widget;
- if(previous == null){
- return null;
- } else{
- return previous;
- }
- }-*/;
+ var previous
+ = this.@com.google.gwt.user.client.ui.FastStringMap::map[':' + key];
+ this.@com.google.gwt.user.client.ui.FastStringMap::map[':' + key] = widget;
+ return (previous == null) ? null : previous;
+ }-*/;
public void putAll(Map arg0) {
Iterator iter = arg0.entrySet().iterator();
@@ -214,14 +215,15 @@
return remove(keyMustBeString(key));
}
+ // only count keys with ':' prefix
public native int size() /*-{
- var value = this.@com.google.gwt.user.client.ui.FastStringMap::map;
- var count = 0;
- for(var key in value){
- ++count;
- }
- return count;
- }-*/;
+ var value = this.@com.google.gwt.user.client.ui.FastStringMap::map;
+ var count = 0;
+ for(var key in value) {
+ if (key.charAt(0) == ':') ++count;
+ }
+ return count;
+ }-*/;
public Collection values() {
List values = new ArrayList();
@@ -229,28 +231,33 @@
return values;
}
+ // only count keys with ':' prefix
private native void addAllKeysFromJavascriptObject(Collection s,
JavaScriptObject javaScriptObject) /*-{
- for(var key in javaScriptObject) {
- s.@java.util.Collection::add(Ljava/lang/Object;)(key);
- }
- }-*/;
+ for(var key in javaScriptObject) {
+ if (key.charAt(0) != ':') continue;
+ s.@java.util.Collection::add(Ljava/lang/Object;)(key.substring(1));
+ }
+ }-*/;
+ // only count keys with ':' prefix
private native void addAllValuesFromJavascriptObject(Collection s,
JavaScriptObject javaScriptObject) /*-{
- for(var key in javaScriptObject) {
- var value = javaScriptObject[key];
- s.@java.util.Collection::add(Ljava/lang/Object;)(value);
- }
- }-*/;
+ for(var key in javaScriptObject) {
+ if (key.charAt(0) != ':') continue;
+ var value = javaScriptObject[key];
+ s.@java.util.Collection::add(Ljava/lang/Object;)(value);
+ }
+ }-*/;
+ // Prepend ':' to avoid conflicts with built-in Object properties.
private native boolean containsKey(String key, JavaScriptObject obj)/*-{
- return obj[key] !== undefined;
- }-*/;
+ return obj[':' + key] !== undefined;
+ }-*/;
private native void init() /*-{
- this.@com.google.gwt.user.client.ui.FastStringMap::map = [];
- }-*/;
+ this.@com.google.gwt.user.client.ui.FastStringMap::map = [];
+ }-*/;
private String keyMustBeString(Object key) {
if (key instanceof String) {
@@ -261,14 +268,11 @@
}
}
+ // Prepend ':' to avoid conflicts with built-in Object properties.
private native Object remove(String key) /*-{
- var previous = this.@com.google.gwt.user.client.ui.FastStringMap::map[key];
- delete this.@com.google.gwt.user.client.ui.FastStringMap::map[key];
- if(previous == null){
- return null;
- } else{
- return previous;
- }
- }-*/;
-
+ var previous
+ = this.@com.google.gwt.user.client.ui.FastStringMap::map[':' + key];
+ delete this.@com.google.gwt.user.client.ui.FastStringMap::map[':' + key];
+ return (previous == null) ? null : previous;
+ }-*/;
}
diff --git a/user/super/com/google/gwt/emul/java/lang/String.java b/user/super/com/google/gwt/emul/java/lang/String.java
index 1b41554..e1cecdc 100644
--- a/user/super/com/google/gwt/emul/java/lang/String.java
+++ b/user/super/com/google/gwt/emul/java/lang/String.java
@@ -1,5 +1,5 @@
/*
- * Copyright 2006 Google Inc.
+ * Copyright 2007 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
@@ -22,6 +22,8 @@
*/
package java.lang;
+import com.google.gwt.core.client.JavaScriptObject;
+
/**
* Intrinsic string class.
*/
@@ -30,9 +32,10 @@
// CHECKSTYLE_OFF: This class has special needs.
/**
+ * accesses need to be prefixed with ':' to prevent conflict with built-in JavaScript properties.
* @skip
*/
- protected static Object hashCache;
+ private static JavaScriptObject hashCache;
public static native String valueOf(boolean x) /*-{ return x ? "true" : "false"; }-*/;
@@ -128,8 +131,9 @@
return me.toString() == other;
}-*/;
+ // Prefix needed to prevent conflict with built-in JavaScript properties.
private static native int __hashCode(String me) /*-{
- var hashCode = @java.lang.String::hashCache[me];
+ var hashCode = @java.lang.String::hashCache[':' + me];
if (hashCode) {
return hashCode;
}
@@ -141,7 +145,7 @@
hashCode <<= 1;
hashCode += me.charCodeAt(i);
}
- @java.lang.String::hashCache[me] = hashCode;
+ @java.lang.String::hashCache[':' + me] = hashCode;
return hashCode;
}-*/;
diff --git a/user/test/com/google/gwt/emultest/java/lang/StringTest.java b/user/test/com/google/gwt/emultest/java/lang/StringTest.java
index 7088a74..95e0360 100644
--- a/user/test/com/google/gwt/emultest/java/lang/StringTest.java
+++ b/user/test/com/google/gwt/emultest/java/lang/StringTest.java
@@ -101,6 +101,41 @@
assertEquals(haystack.indexOf(""), 0);
}
+ /**
+ * Tests hashing with strings.
+ *
+ * The specific strings used in this test used to trigger failures
+ * because we use a JavaScript object as a hash map to cache the
+ * computed hash codes. This conflicts with built-in properties
+ * defined on objects -- see issue #631.
+ *
+ */
+ public void testHashCode() {
+ String[] testStrings = {
+ "watch", "unwatch", "toString", "toSource", "eval", "valueOf",
+ "constructor", "__proto__"
+ };
+ int[] savedHash = new int[testStrings.length];
+ for (int i = 0; i < testStrings.length; ++i ) {
+ savedHash[i] = testStrings[i].hashCode();
+
+ /*
+ * Verify that the resulting hash code is numeric, since this is not
+ * enforced in web mode.
+ */
+ String str = Integer.toString(savedHash[i]);
+ for (int j = 0; j < str.length(); ++j) {
+ char ch = str.charAt(j);
+ assertTrue("Bad character '" + ch + "' (U+0" + Integer.toHexString(ch)
+ + ")", ch == '-' || ch == ' ' || Character.isDigit(ch));
+ }
+ }
+ // verify the hash codes are constant for a given string
+ for (int i = 0; i < testStrings.length; ++i ) {
+ assertEquals(savedHash[i], testStrings[i].hashCode());
+ }
+ }
+
/** tests lastIndexOf */
public void testLastIndexOf() {
String x = "abcdeabcdef";
diff --git a/user/test/com/google/gwt/user/client/rpc/CollectionsTest.java b/user/test/com/google/gwt/user/client/rpc/CollectionsTest.java
index 201d1a6..1e90f2b 100644
--- a/user/test/com/google/gwt/user/client/rpc/CollectionsTest.java
+++ b/user/test/com/google/gwt/user/client/rpc/CollectionsTest.java
@@ -373,6 +373,24 @@
});
}
+ public void testStringArray() {
+ delayTestFinish(TEST_DELAY);
+
+ CollectionsTestServiceAsync service = getServiceAsync();
+ final String[] expected = TestSetFactory.createStringArray();
+ service.echo(expected, new AsyncCallback() {
+ public void onFailure(Throwable caught) {
+ fail(caught.toString());
+ }
+
+ public void onSuccess(Object result) {
+ assertNotNull(result);
+ assertTrue(TestSetValidator.equals(expected, (String[]) result));
+ finishTest();
+ }
+ });
+ }
+
public void testShortArray() {
delayTestFinish(TEST_DELAY);
diff --git a/user/test/com/google/gwt/user/client/rpc/TestSetFactory.java b/user/test/com/google/gwt/user/client/rpc/TestSetFactory.java
index 6350ac0..2ab7811 100644
--- a/user/test/com/google/gwt/user/client/rpc/TestSetFactory.java
+++ b/user/test/com/google/gwt/user/client/rpc/TestSetFactory.java
@@ -300,8 +300,13 @@
new Short(Short.MAX_VALUE), new Short(Short.MIN_VALUE)};
}
+ /*
+ * Check names that collide with JS properties inherited from Object.prototype
+ * to make sure they are handled properly.
+ */
public static String[] createStringArray() {
- return new String[] {null, "", "one", "two"};
+ return new String[] {null, "", "one", "two", "toString", "watch",
+ "prototype", "eval", "valueOf", "constructor", "__proto__" };
}
public static Vector createVector() {
diff --git a/user/test/com/google/gwt/user/client/ui/FastStringMapTest.java b/user/test/com/google/gwt/user/client/ui/FastStringMapTest.java
index 8c0e624..d620897 100644
--- a/user/test/com/google/gwt/user/client/ui/FastStringMapTest.java
+++ b/user/test/com/google/gwt/user/client/ui/FastStringMapTest.java
@@ -17,7 +17,9 @@
import com.google.gwt.junit.client.GWTTestCase;
+import java.util.Collection;
import java.util.Map;
+import java.util.Set;
/**
* Tests <code>FastStringMap</code>Right now, no tests are directly run here,
@@ -43,5 +45,38 @@
// Only FastStringMap specific tests should go here. Look in
// com.google.gwt.user.maptests.FastStringMapTest for all apache Map tests.
}
+
+ /*
+ * Test for collisions between stored strings and JavaScript Object
+ * properties.
+ */
+ public void testJSOCollision() {
+ Map map = makeEmptyMap();
+ assertEquals(0, map.size());
+ map.put("k1", "v1");
+ assertEquals(1, map.size());
+ assertEquals("v1", (String) map.get("k1"));
+ map.put("toString", "toStringVal");
+ assertEquals(2, map.size());
+ assertEquals("toStringVal", (String) map.get("toString"));
+ map.put("watch", "watchVal");
+ Set keys = map.keySet();
+ assertEquals(3, keys.size());
+ map.put("__proto__", "__proto__Val");
+ assertEquals(4 ,map.size());
+ assertEquals("__proto__Val", (String)map.get("__proto__"));
+ map.put("k1", "v1b");
+ keys = map.keySet();
+ assertEquals(4, keys.size());
+ Collection values = map.values();
+ assertEquals(4, values.size());
+ map.put("k2", "v1b");
+ values = map.values();
+ assertEquals(5, values.size());
+ map.put("","empty");
+ assertEquals("empty", (String) map.get(""));
+ map.remove("k2");
+ assertEquals(5, values.size());
+ }
}
