Repair test breakage. Update RegExp whitelist. (issue 251670043 by erights@gmail.com)

[re...@codereview-hr.appspotmail.com]

Reviewers: kpreid_google,

Description:
While running the Caja regression tests ("ant runtests") the testOk
test fails in test_GLOBAL_LEAKS_FROM_ARRAY_METHODS due to a bug on FF
39 we have yet to diagnose or report: When this code tries to define a
global property to its current state, defineProperty throws an
error. Since guest code cannot cause SES to attempt to defineProperty
a global property, simply skipping the defineProperty in this case is
safe.

For several properties that were data properties on RegExp instances
in ES5, ES6 (ECMAScript 2015) instead makes them accessor properties
on RegExp.prototype. See the note at
http://www.ecma-international.org/ecma-262/6.0/index.html#sec-properties-of-regexp-instances
This CL updates the whitelist to reflect this change. Prior to this
CL, this discrepancy was causing several test failures on FF 39.

Please review this at https://codereview.appspot.com/251670043/

Affected files (+29, -8 lines):
M src/com/google/caja/ses/repairES5.js
M src/com/google/caja/ses/whitelist.js


Index: src/com/google/caja/ses/repairES5.js
diff --git a/src/com/google/caja/ses/repairES5.js
b/src/com/google/caja/ses/repairES5.js
index
44afacd1f0348eb66268828a5159d3bd9d472ec0..0fa0300e75fe945f5c330afb24f0ea6a43534445
100644
--- a/src/com/google/caja/ses/repairES5.js
+++ b/src/com/google/caja/ses/repairES5.js
@@ -1555,10 +1555,29 @@ var ses;
);
} finally {
saved.forEach(function(record) {
- if (record[1]) {
- Object.defineProperty(global, record[0], record[1]);
+ var name = record[0];
+ var oldDesc = record[1];
+ if (oldDesc) {
+ var newDesc = Object.getOwnPropertyDescriptor(global, name);
+ if (!is(oldDesc.value, newDesc.value) ||
+ oldDesc.writable !== newDesc.writable ||
+ oldDesc.enumerable !== newDesc.enumerable) {
+ // See the comments on freezeGlobalProp in startSES.js for
+ // why we delete oldDesc.configurable. Given that we do,
+ // the following two lines of code should succeed even if
+ // the condition above is false. Thus, the condition
+ // should not be needed. However, when running the Caja
+ // regression tests, the testOk test tries to define a
+ // global property to itself, which fails on FF 39 for
+ // undiagnosed reasons.
+ //
+ // TODO(erights): Diagnose why testOk on FF 39 fails if we
+ // do the following lines unconditionally, and report.
+ delete oldDesc.configurable;
+ Object.defineProperty(global, name, oldDesc);
+ }
} else {
- delete global[record[0]];
+ delete global[name];
}
});
}
Index: src/com/google/caja/ses/whitelist.js
diff --git a/src/com/google/caja/ses/whitelist.js
b/src/com/google/caja/ses/whitelist.js
index
6f07570a39333eb25cd435f962372657644ddb8c..6c96928651b6d73c1a44573fcb91efdb4fd36d9c
100644
--- a/src/com/google/caja/ses/whitelist.js
+++ b/src/com/google/caja/ses/whitelist.js
@@ -493,13 +493,15 @@ var ses;
prototype: {
exec: t,
test: t,
- source: '*',
- global: '*',
- ignoreCase: '*',
- multiline: '*',
+ source: 'maybeAccessor',
+ global: 'maybeAccessor',
+ ignoreCase: 'maybeAccessor',
+ multiline: 'maybeAccessor',
+ flags: 'maybeAccessor',
+ unicode: 'maybeAccessor',
lastIndex: '*',
options: '*', // non-std
- sticky: '*' // non-std
+ sticky: 'maybeAccessor' // non-std
}
},
Error: {

[kpr...@google.com]

LGTM

https://codereview.appspot.com/251670043/