Revision: 5359
Author: [email protected]
Date: Wed Apr 17 11:49:04 2013
Log: Repair Object.create on Chrome; make SES compatible with no
Object.create(null).
https://codereview.appspot.com/8806045
In Chrome 28.0.1480.0 canary (among other versions),
Object.freeze(Object.prototype) causes __proto__ to lose its magic,
which causes various internal code to fail; most critically,
Object.create(X) produces objects whose [[Prototype]] is
Object.prototype rather than X. Repair this by reimplementing
Object.create from scratch.
The repair cannot support Object.create(null); therefore, kludge
SES-only code into working despite its absence:
* StringMap.js is willing to use {} instead of Object.create(null); safe
because the keys are suffixed.
* Replace string-map uses of Object.create(null) in repairES5 with an
explicit (lightened) StringMap abstraction.
* sharedImports and scope objects will have Object.prototype's
properties as bindings.
* Loosen testProtoMention test to be willing for __proto__ as a global
variable to throw errors.
Also repair Error.prototype.toString whose native implementation is
also broken by lack of __proto__ magic.
Note that there is further fatal-to-Caja misbehavior in the same Chrome
versions, with apparently the same root cause, only if the "Enable
Experimental JavaScript" flag is on (symptom: {}.constructor === Symbol
(itself an experimental feature), rather than Object), and this repair
does not deal with that.
[email protected], [email protected]
http://code.google.com/p/google-caja/source/detail?r=5359
Modified:
/trunk/src/com/google/caja/ses/StringMap.js
/trunk/src/com/google/caja/ses/repairES5.js
/trunk/src/com/google/caja/ses/startSES.js
/trunk/tests/com/google/caja/plugin/es53-test-language-guest.html
=======================================
--- /trunk/src/com/google/caja/ses/StringMap.js Wed Mar 6 20:18:30 2013
+++ /trunk/src/com/google/caja/ses/StringMap.js Wed Apr 17 11:49:04 2013
@@ -17,6 +17,7 @@
*
* @author Mark S. Miller
* @author Jasvir Nagra
+ * @requires ses
* @overrides StringMap
*/
@@ -38,10 +39,19 @@
}
return x;
}
+
+ var createNull;
+ if (ses.es5ProblemReports.FREEZING_BREAKS_PROTOTYPES.beforeFailure) {
+ // Object.create(null) is broken; fall back to ES3-style
implementation
+ // (safe because we suffix keys anyway).
+ createNull = function() { return {}; }
+ } else {
+ createNull = function() { return Object.create(null); }
+ }
StringMap = function StringMap() {
- var objAsMap = create(null);
+ var objAsMap = createNull();
return freeze({
get: constFunc(function(key) {
=======================================
--- /trunk/src/com/google/caja/ses/repairES5.js Mon Apr 15 15:20:52 2013
+++ /trunk/src/com/google/caja/ses/repairES5.js Wed Apr 17 11:49:04 2013
@@ -258,6 +258,30 @@
ses.maxSeverity = severity;
}
};
+
+ /**
+ * Several test/repair routines want string-keyed maps. Unfortunately,
+ * our exported StringMap is not yet available, and our repairs
+ * include one which breaks Object.create(null). So, an ultra-minimal,
+ * ES3-compatible implementation.
+ */
+ function EarlyStringMap() {
+ var objAsMap = {};
+ return {
+ get: function(key) {
+ return objAsMap[key + '$'];
+ },
+ set: function(key, value) {
+ objAsMap[key + '$'] = value;
+ },
+ has: function(key) {
+ return (key + '$') in objAsMap;
+ },
+ 'delete': function(key) {
+ return delete objAsMap[key + '$'];
+ }
+ };
+ }
//////// Prepare for "caller" and "argument" testing and repair /////////
@@ -623,9 +647,9 @@
* freezing Object.prototype breaks Object.create inheritance.
* https://code.google.com/p/v8/issues/detail?id=2565
*/
- function test_FREEZING_BREAKS_CREATE() {
+ function test_FREEZING_BREAKS_PROTOTYPES() {
// This problem is sufficiently problematic that testing for it breaks
the
- // frame, so we create another frame to test in.
+ // frame under some circumstances, so we create another frame to test
in.
var otherObject = inTestFrame(function(window) { return window.Object;
});
if (!otherObject) { return false; }
@@ -1952,6 +1976,10 @@
'stack',
'stacktrace'
];
+ var errorInstanceWhiteMap = new EarlyStringMap();
+ strictForEachFn(errorInstanceWhitelist, function(name) {
+ errorInstanceWhiteMap.set(name, true);
+ });
var errorInstanceBlacklist = [
// seen in a Firebug on FF
@@ -1967,18 +1995,6 @@
'getSourceLine',
'resetSource'
];
-
- /** Return a fresh one so client can mutate freely */
- function freshErrorInstanceWhiteMap() {
- var result = Object.create(null);
- strictForEachFn(errorInstanceWhitelist, function(name) {
- // We cannot yet use StringMap so do it manually
- // We do this naively here assuming we don't need to worry about
- // __proto__
- result[name] = true;
- });
- return result;
- }
/**
* Do Error instances on those platform carry own properties that we
@@ -1996,11 +2012,9 @@
try { null.foo = 3; } catch (err) { errs.push(err); }
var result = false;
- var approvedNames = freshErrorInstanceWhiteMap();
-
strictForEachFn(errs, function(err) {
strictForEachFn(Object.getOwnPropertyNames(err), function(name) {
- if (!(name in approvedNames)) {
+ if (!errorInstanceWhiteMap.has(name)) {
result = 'Unexpected error instance property: ' + name;
// would be good to terminate early
}
@@ -2023,20 +2037,20 @@
try { null.foo = 3; } catch (err) { errors.push(err); }
for (var i = 0; i < errors.length; i++) {
var err = errors[i];
- var found = Object.create(null);
+ var found = new EarlyStringMap();
strictForEachFn(gopn(err), function (prop) {
- found[prop] = true;
+ found.set(prop, true);
});
var j, prop;
for (j = 0; j < errorInstanceWhitelist.length; j++) {
prop = errorInstanceWhitelist[j];
- if (gopd(err, prop) && !found[prop]) {
+ if (gopd(err, prop) && !found.get(prop)) {
return true;
}
}
for (j = 0; j < errorInstanceBlacklist.length; j++) {
prop = errorInstanceBlacklist[j];
- if (gopd(err, prop) && !found[prop]) {
+ if (gopd(err, prop) && !found.get(prop)) {
return true;
}
}
@@ -2097,7 +2111,16 @@
* we only care about the case where we cannot replace it.
*/
function test_NONCONFIGURABLE_OWN_PROTO() {
- var o = Object.create(null);
+ try {
+ var o = Object.create(null);
+ } catch (e) {
+ if (e.message === NO_CREATE_NULL) {
+ // result of repair_FREEZING_BREAKS_PROTOTYPES
+ return false;
+ } else {
+ throw e;
+ }
+ }
var desc = Object.getOwnPropertyDescriptor(o, '__proto__');
if (desc === undefined) { return false; }
if (desc.configurable) { return false; }
@@ -2877,6 +2900,86 @@
});
}
+ // error message is matched elsewhere (for tighter bounds on catch)
+ var NO_CREATE_NULL =
+ 'Repaired Object.create can not support Object.create(null)';
+ function repair_FREEZING_BREAKS_PROTOTYPES() {
+ var baseObject = Object;
+ var baseDefProp = Object.defineProperties;
+
+ // Object.create fails to override [[Prototype]]; reimplement it.
+ Object.defineProperty(Object, 'create', {
+ configurable: true, // attributes per ES5.1 section 15
+ writable: true,
+ value: function repairedObjectCreate(O, Properties) {
+ if (O === null) {
+ // Not ES5 conformant, but hopefully adequate for Caja as ES5/3
also
+ // does not support Object.create(null).
+ throw new TypeError(NO_CREATE_NULL);
+ }
+ // "1. If Type(O) is not Object or Null throw a TypeError
exception."
+ if (O !== Object(O)) {
+ throw new TypeError('Object.create: prototype must be an
object');
+ }
+ // "2. Let obj be the result of creating a new object as if by the
+ // expression new Object() where Object is the standard built-in
+ // constructor with that name"
+ // "3. Set the [[Prototype]] internal property of obj to O."
+ // Cannot redefine [[Prototype]], so we use the .prototype trick
instead
+ function temporaryConstructor() {}
+ temporaryConstructor.prototype = O;
+ var obj = new temporaryConstructor();
+ // "4. If the argument Properties is present and not undefined,
add own
+ // properties to obj as if by calling the standard built-in
function
+ // Object.defineProperties with arguments obj and Properties."
+ if (Properties !== void 0) {
+ baseDefProp(obj, Properties);
+ }
+ // "5. Return obj."
+ return obj;
+ }
+ });
+
+ var baseErrorToString = Error.prototype.toString;
+
+ // Error.prototype.toString fails to use the .name and .message.
+ // This is being repaired not because it is a critical issue but
because
+ // it is more direct than disabling the tests of error taming which
fail.
+ Object.defineProperty(Error.prototype, 'toString', {
+ configurable: true, // attributes per ES5.1 section 15
+ writable: true,
+ value: function repairedErrorToString() {
+ // "1. Let O be the this value."
+ var O = this;
+ // "2. If Type(O) is not Object, throw a TypeError exception."
+ if (O !== Object(O)) {
+ throw new TypeError('Error.prototype.toString: this not an
object');
+ }
+ // "3. Let name be the result of calling the [[Get]] internal
method of
+ // O with argument "name"."
+ var name = O.name;
+ // "4. If name is undefined, then let name be "Error"; else let
name be
+ // ToString(name)."
+ name = name === void 0 ? 'Error' : '' + name;
+ // "5. Let msg be the result of calling the [[Get]] internal
method of O
+ // with argument "message"."
+ var msg = O.message;
+ // "6. If msg is undefined, then let msg be the empty String; else
let
+ // msg be ToString(msg)."
+ msg = msg === void 0 ? '' : '' + msg;
+ // "7. If msg is undefined, then let msg be the empty String; else
let
+ // msg be ToString(msg)."
+ msg = msg === void 0 ? '' : '' + msg;
+ // "8. If name is the empty String, return msg."
+ if (name === '') { return msg; }
+ // "9. If msg is the empty String, return name."
+ if (msg === '') { return name; }
+ // "10. Return the result of concatenating name, ":", a single
space
+ // character, and msg."
+ return name + ': ' + msg;
+ }
+ });
+ }
////////////////////// Kludge Records /////////////////////
//
@@ -2929,17 +3032,6 @@
sections: ['15.2.3.4'],
tests: ['15.2.3.4-0-1']
},
- {
- id: 'FREEZING_BREAKS_CREATE',
- description: 'Freezing Object.prototype breaks Object.create',
- test: test_FREEZING_BREAKS_CREATE,
- repair: void 0,
- preSeverity: severities.NOT_SUPPORTED,
- canRepair: false,
- urls: ['https://code.google.com/p/v8/issues/detail?id=2565'],
- sections: ['15.2.3.5'],
- tests: [] // TODO(kpreid): find test242
- }
];
/**
@@ -3638,7 +3730,18 @@
sections: [], // Not spelled out in spec, according to Brendan Eich
(see
// es-discuss link)
tests: [] // TODO(kpreid): add to test262 once we have a section to
cite
- }
+ },
+ {
+ id: 'FREEZING_BREAKS_PROTOTYPES',
+ description: 'Freezing Object.prototype breaks prototype setting',
+ test: test_FREEZING_BREAKS_PROTOTYPES,
+ repair: repair_FREEZING_BREAKS_PROTOTYPES,
+ preSeverity: severities.UNSAFE_SPEC_VIOLATION,
+ canRepair: true,
+ urls: ['https://code.google.com/p/v8/issues/detail?id=2565'],
+ sections: ['15.2.3.5'],
+ tests: [] // TODO(kpreid): find/add test262
+ },
];
////////////////////// Testing, Repairing, Reporting ///////////
@@ -3758,7 +3861,13 @@
// Made available to allow for later code reusing our diagnoses to work
// around non-repairable problems in application-specific ways.
startSES
// will also expose this on cajaVM for unprivileged code.
- var indexedReports = Object.create(null);
+ var indexedReports;
+ try {
+ indexedReports = Object.create(null);
+ } catch (e) {
+ // repair_FREEZING_BREAKS_PROTOTYPES does not support null
+ indexedReports = {};
+ }
reports.forEach(function (report) {
indexedReports[report.id] = report;
});
=======================================
--- /trunk/src/com/google/caja/ses/startSES.js Wed Apr 10 18:29:53 2013
+++ /trunk/src/com/google/caja/ses/startSES.js Wed Apr 17 11:49:04 2013
@@ -253,6 +253,20 @@
var keys = Object.keys;
var freeze = Object.freeze;
var create = Object.create;
+
+ /**
+ * repairES5 repair_FREEZING_BREAKS_PROTOTYPES causes
Object.create(null) to
+ * be impossible. This falls back to a regular object. Each use of it
+ * should be accompanied by an explanation of why it is sufficiently
+ * safe.
+ */
+ function createNullIfPossible() {
+ try {
+ return create(null);
+ } catch (e) {
+ return {};
+ }
+ }
/**
* The function ses.mitigateGotchas, if defined, is a function which
@@ -407,7 +421,11 @@
* this {@code imports} should first be initialized with a copy of the
* properties of {@code sharedImports}, but nothing enforces this.
*/
- var sharedImports = create(null);
+ var sharedImports = createNullIfPossible();
+ // createNullIfPossible safety: If not possible, the imports will include
+ // Object.prototype's properties. This has no effect on Caja use, because
+ // we make the global object be the Window which inherits
Object.prototype,
+ // and is not a security risk since the properties are ambiently
available.
var MAX_NAT = Math.pow(2, 53);
function Nat(allegedNum) {
@@ -494,7 +512,8 @@
* property copying.
*/
function makeImports() {
- var imports = create(null);
+ var imports = createNullIfPossible();
+ // createNullIfPossible safety: similar to comments about
sharedImports.
copyToImports(imports, sharedImports);
return imports;
}
@@ -539,7 +558,11 @@
* {@code imports}.
*/
function makeScopeObject(imports, freeNames) {
- var scopeObject = create(null);
+ var scopeObject = createNullIfPossible();
+ // createNullIfPossible safety: The inherited properties should
+ // always be shadowed by defined properties if they are relevant
+ // (that is, if they occur in freeNames).
+
// Note: Although this loop is a bottleneck on some platforms,
// it does not help to turn it into a for(;;) loop, since we
// still need an enclosing function per accessor property
=======================================
--- /trunk/tests/com/google/caja/plugin/es53-test-language-guest.html Mon
Apr 15 14:31:05 2013
+++ /trunk/tests/com/google/caja/plugin/es53-test-language-guest.html Wed
Apr 17 11:49:04 2013
@@ -718,38 +718,56 @@
jsunitRegisterIf(inES5Mode,
'testProtoMention',
function testProtoMention() {
- // Buried in an eval to avoid causing static failures in ES5/3
- // Split identifier to avoid causing the same failure in this block
- var test = cajaVM.compileModule(
- // Specifically is a 'global' variable
- 'var __pr'+'oto__;' +
+ function doProtoTest(global) {
+ // Buried in an eval to avoid causing static failures in ES5/3
+ // Split identifier to avoid causing the same failure in this block
+ var test = cajaVM.compileModule(
+ (global ? '' : '(function() {') +
+ 'var __pr'+'oto__;' +
- 'var a = __pr'+'oto__;' +
- '__pr'+'oto__ = 1;' + // If __proto__ magic exists then this fails
- // silently since the value isn't an object.
- 'var b = __pr'+'oto__;' +
- 'testProtoMention_callback(a, b);');
- var envProto = {};
- var env = Object.create(envProto);
- envProto.toString = function() { return '<envProto>'; };
- env.toString = function() { return '<env>'; };
- env.testProtoMention_callback = jsunitCallback(function(a, b) {
- // Two different permissible behaviors: either __proto__ is
consistently
- // the prototype property of the supplied environment object,
- var looksLikeProto = a === envProto;
- // or it is consistently just a declared variable.
- var looksLikeVar = a === undefined;
- assertTrue('initial value is not leaky: ' + a,
- looksLikeProto || looksLikeVar);
- if (looksLikeProto) {
- // not using assertEquals so as to not get the taming membrane
involved
- assertTrue('assignment is consistently ignored: ' + b, b ===
envProto);
+ 'var a = __pr'+'oto__;' +
+ '__pr'+'oto__ = 1;' + // If you-know-what magic exists then this
+ // fails silently since the value isn't an object.
+ 'var b = __pr'+'oto__;' +
+ 'testProtoMention_callback(a, b);' +
+ (global ? '' : '}());'));
+ var envProto = {};
+ var env = Object.create(envProto);
+ envProto.toString = function() { return '<envProto>'; };
+ env.toString = function() { return '<env>'; };
+ env.testProtoMention_callback = jsunitCallback(function(a, b) {
+ // Two different permissible behaviors: either you-know-what is
+ // consistently the prototype property of the supplied environment
+ // object,
+ var looksLikeProto = a === envProto;
+ // or it is consistently just a declared variable.
+ var looksLikeVar = a === undefined;
+ assertTrue('initial value is not leaky: ' + a,
+ looksLikeProto || looksLikeVar);
+ if (looksLikeProto) {
+ // not using assertEquals so as to not get the taming membrane
+ // involved
+ assertTrue('assignment is consistently ignored: ' + b, b ===
envProto);
+ } else {
+ assertTrue('assignment works: ' + b, b === 1);
+ }
+ pass('testProtoMention');
+ });
+ test(env);
+ }
+ doProtoTest(false);
+ try {
+ doProtoTest(true);
+ } catch (e) {
+ if (/Cannot assign to read only
property '__proto__'/.test(e.message)) {
+ // Other permissible behavior: __proto__ fails loudly at being a
global
+ // but it should still work as a lexical var. TODO(kpreid): Quick
kludge
+ // to deal with FREEZING_BREAKS_PROTOTYPES. Refactor this test for
+ // cleaner control flow and no error-wording-dependency.
} else {
- assertTrue('assignment works: ' + b, b === 1);
+ throw e;
}
- pass('testProtoMention');
- });
- test(env);
+ }
});
</script>
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
