Add the Cajita taming API to ES5/3.

Removes the unneeded caching of primordials.

Fixes lots of taming/caching bugs:
- Adds in the cajita taming API as used by domita.
- Fixes wrapping of recursive functions.
- Tames the call property of function instances.
- Whitelists the prototype property of function instances.
- Tames RegExp instances.
- Begin using get/set for methods that domita depends on
- Makes "this" at the global scope refer to IMPORTS___

Makes globals properties of IMPORTS___.
Fixes renderer so it puts parens around the constructor:
   new IMPORTS___.v___('RegExp').new___()
treats IMPORTS___.v___ as the constructor, which isn't
what we want.

Fixes the rendering of new expr when expr contains
operations with higher precedence than new.

[MarkM, 2010-08-14 01:39:18]

Posting what I've got so far. I'm going sequentially and have not yet dived into
the new RegExp taming code.

http://codereview.appspot.com/1955042/diff/13005/6006
File src/com/google/caja/es53.js (left):

http://codereview.appspot.com/1955042/diff/13005/6006#oldcode1743
src/com/google/caja/es53.js:1743: function readImport(module_imports, name,
opt_permitsUsed) {
Don't we still need the static path optimization?

http://codereview.appspot.com/1955042/diff/13005/6006#oldcode2892
src/com/google/caja/es53.js:2892: * end in __ and freezes it.  For internal use
only.
Is this no longer for internal use only? Or is this simply an inappropriate
place to comment on that?

http://codereview.appspot.com/1955042/diff/13005/6006
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/1955042/diff/13005/6006#newcode870
src/com/google/caja/es53.js:870: // Convert to object form so we can look up the
non-extensibility.
Ok, now that I know what this is about, I see that it's correct but
unnecessarily complex. How about

  function isExtensible(obj) {
    return Type(obj) === 'Object' && obj.ne___ === obj;
  }

?

http://codereview.appspot.com/1955042/diff/13005/6006#newcode1141
src/com/google/caja/es53.js:1141: * Enforces {@code typeof(specimen) ===
typename}, in which case
The primitive typeof doesn't need to parenthesize its argument.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode1670
src/com/google/caja/es53.js:1670: // Looping again over the same untrusted
stamps alleged-array is safe
(I suspect the following may be my oversight from a previous CL. But in any
case, now's a good time to fix this)

The comment here dates from before we were using Array.slice___ to copy the
stamps alleged array into a genuine array. Now that we are, this is safe and the
comment can be removed.

We should ensure that an attacker-installed access on a numerically-named or
'length' property on Array.prototype can't interfere. For full ES5/3, it's
plausible to me that we might want to initialize Array.prototype.length to a
non-writable, non-configurable 0, even if we don't freeze all of
Array.prototype.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode1740
src/com/google/caja/es53.js:1740: switch (typeof(ex)) {
The primitive typeof doesn't need to parenthesize its argument.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode2553
src/com/google/caja/es53.js:2553: Array.prototype.shift___.call___(result);
Wow, good catch! I think I missed this bug on an earlier CL. And it's a bad one:
mutating the argument, which is usually a genuine ES3 arguments object -- a
dangerous thing to mutate!

http://codereview.appspot.com/1955042/diff/13005/6006#newcode2580
src/com/google/caja/es53.js:2580: * Returns a function object with {@code call,
bind}, and {@code apply}
The comment diagrees with the code -- wrap does not create a
per-function-instance bind method. I suspect it should not and the comment is
simply stale.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode2986
src/com/google/caja/es53.js:2986: // TODO: report the CLASS___ property
I'd guess this TODO is rather urgent.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode2997
src/com/google/caja/es53.js:2997: this.toString = markFunc(function (var_args) {
Don't you need to first check that this is not frozen?

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3017
src/com/google/caja/es53.js:3017: this.valueOf = markFunc(function (var_args) {
Don't you need to first check that this is not frozen?

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3033
src/com/google/caja/es53.js:3033: set: markFunc(function (V) { return hop =
asFirstClass(V); }),
This has two problems:

* I thought we wanted to special case Object.prototype to be
non-monkey-patchable, even in full ES5/3.

* But we still need a setter, since this access property will be inherited by
objects that don't override 'hasOwnProperty'. If (this !== Object.prototype) and
!isFrozen(this), then we want to simulate a simple assignment to
this.hasOwnProperty.

The 2nd point above requires sufficient complexity that I'm not leaning to the
hasOwnProperty___ approach rather than the access property approach for guest
override whenever possible.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3041
src/com/google/caja/es53.js:3041: throw new Error('isPrototypeOf is not
supported.');
I understand why Object.getPrototypeOf is hard. But AFAICT, the built-in
isPrototypeOf should work as is. The guest and host views of the prototype chain
should be identical.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3069
src/com/google/caja/es53.js:3069: // NOT extensible under ES5/3
Aha! Thought so.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3507
src/com/google/caja/es53.js:3507: set: markFunc(function (V) { return replace =
asFirstClass(V); }),
Although practically less urgent, this fails for the same reason the naive
setter on hasOwnProperty fails. Although it's perverse, anyone can make an
object, O, that inherits from String.prototype. If someone then assigns to
O.replace, that will invoke this inherited setter which will do the wrong thing.

Also, again you forgot to check whether String.prototype was frozen before
allowing the mutate.

I'm becoming yet more enthusiastic about the replace___ approach. Sorry. I do
remember that it had its own set of complexities.

[MarkM, 2010-08-14 02:45:15]

Comments only on your latest delta.

http://codereview.appspot.com/1955042/diff/13005/6006
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3745
src/com/google/caja/es53.js:3745: Date.prototype.DefineOwnProperty___('now', {
Why did you pull this? It looked good to me.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode913
src/com/google/caja/es53.js:913: if (m) { result.push(m[1]); }
This seems too restrictive. It will exclude everything but own data properties.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode927
src/com/google/caja/es53.js:927: if (m) { result.push(m[1]); }
This seems too restrictive. It will exclude everything but own data properties.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4048
src/com/google/caja/es53.js:4048: 
I'm almost certain that "exception.stack" is not portable.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4049
src/com/google/caja/es53.js:4049: var message = 'unknown';
The whole HTML5 onerror thing is complex and I don't understand it anymore. I'm
just noting this as something I need to come back to review before LGTMing.

[MarkM, 2010-08-14 03:23:29]

Still not done with new/old taming methods. In the meantime...

http://codereview.appspot.com/1955042/diff/14001/15001
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/1955042/diff/14001/15001#newcode3785
src/com/google/caja/es53.js:3785: if
(Object.prototype.toString___.call___(pattern) === '[object RegExp]'
Where is "pattern" defined?

http://codereview.appspot.com/1955042/diff/14001/15001#newcode3786
src/com/google/caja/es53.js:3786: && flags === void 0) {
Where is "flags" defined?

http://codereview.appspot.com/1955042/diff/14001/15001#newcode3821
src/com/google/caja/es53.js:3821: writable: instanceProps[i] === 'lastIndex',
Perhaps a bit too clever. Although it's more verbose, I suggest a separate call
to DefineOwnProperty___ to define 'lastIndex'.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode3839
src/com/google/caja/es53.js:3839: var props = ['constructor', 'exec', 'test'];
You cannot simply whitelist the built-in exec and test because of issue 528. See
the taming of these in cajita.js and
http://code.google.com/p/es-lab/source/browse/trunk/src/ses/initSES.js#117

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4073
src/com/google/caja/es53.js:4073: return module.instantiate(___, imports);
Nice!

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4122
src/com/google/caja/es53.js:4122: function grantFunc(obj, name) {
Shouldn't this also do a grantRead(obj, name)?

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4137
src/com/google/caja/es53.js:4137: * can't violate invariants, like a "peek"
method on stacks that returns
Please choose an actual example instead of peek.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4299
src/com/google/caja/es53.js:4299: es53 = whitelistAll({
We need to come up with a name for this object that doesn't bake the version
into the name. Perhaps we continue to call it "cajita", so old cajita code can
continue to work when it should?

[MarkM, 2010-08-15 00:28:07]

http://codereview.appspot.com/1955042/diff/14001/15001
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4158
src/com/google/caja/es53.js:4158: * In order for this fault handler to get
control, it's important
This now objectively replaces the method as it should, so the comment is very
stale.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4161
src/com/google/caja/es53.js:4161: * FIXME(ben): also fastpath?
Comment also stale.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4174
src/com/google/caja/es53.js:4174: return original.apply(this,
Array.slice___(arguments, 0));
Should this be apply___ ?

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4185
src/com/google/caja/es53.js:4185: if ('___' in obj) { return USELESS; }
I no longer remember: Is this the right way to treat the global object, or did
we intend to throw an Error on this case?

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4213
src/com/google/caja/es53.js:4213: var feralResult = original.apply(feralThis,
feralArgs);
I'm not sure if this one should be "apply" or "apply___". Thoughts?

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4269
src/com/google/caja/es53.js:4269: configurable: true,
I suspect this should be "configurable: false" so you don't accidentally step on
the host's obj[name].

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4285
src/com/google/caja/es53.js:4285: configurable: true,
Ah, right, I see the issue. If we make useGetHandler & useSetHandler both use
"configurable: false", then we can't install these one at a time. Nevertheless,
I think "configurable: false" is right, and we should refactor the taming code
accordingly. But this can wait for the next round -- if you postpone it, please
leave an issue or TODO.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4397
src/com/google/caja/es53.js:4397: isES53: true,
We should avoid baking the version name into the property name. It would be
better to have some reusable property name such as "version:" or something, and
then put the specific version in the value.

http://codereview.appspot.com/1955042/diff/14001/15002
File src/com/google/caja/parser/quasiliteral/ES53Rewriter.java (right):

http://codereview.appspot.com/1955042/diff/14001/15002#newcode1000
src/com/google/caja/parser/quasiliteral/ES53Rewriter.java:1000:
substitutes="IMPORTS___.w___('@v', void 0)")
This is only a good translation when IMPORTS___[@v] doesn't already exist. When
it does, a non-initializing declaration should have no effect. In particular, it
should not wipe out its previous value.

http://codereview.appspot.com/1955042/diff/14001/15002#newcode1654
src/com/google/caja/parser/quasiliteral/ES53Rewriter.java:1654:
substitutes="@fnameRef = ___.wrap(function @fname(dis___, @ps*) {\n"
This seems wrong. It brings @fname into scope within the body of the expanded
function with @fname bound to the expanded function. Only the wrapped function
is a faithful emulation of the original, so @fname should be bound to it
instead.

http://codereview.appspot.com/1955042/diff/14001/15002#newcode1699
src/com/google/caja/parser/quasiliteral/ES53Rewriter.java:1699:
substitutes="IMPORTS___.w___(@rf, ___.wrap(function @fname(dis___, @ps*) {\n"
Are you treating this as global? From the rest of this rule, it does not seem to
be about global function declarations.

http://codereview.appspot.com/1955042/diff/14001/15002#newcode1719
src/com/google/caja/parser/quasiliteral/ES53Rewriter.java:1719:
"IMPORTS___.w___(@rf, ___.wrap(function @fname(dis___, @ps*) {\n"
That global confusion again.

[metaweta, 2010-08-16 23:15:27]

http://codereview.appspot.com/1955042/diff/13005/6006
File src/com/google/caja/es53.js (left):

http://codereview.appspot.com/1955042/diff/13005/6006#oldcode1743
src/com/google/caja/es53.js:1743: function readImport(module_imports, name,
opt_permitsUsed) {
On 2010/08/14 01:39:18, MarkM wrote:
> Don't we still need the static path optimization?

Let's talk about this one; readImports may come back, since I need some way to
trigger a ReferenceError when attempting to read an undefined global.

http://codereview.appspot.com/1955042/diff/13005/6006#oldcode2892
src/com/google/caja/es53.js:2892: * end in __ and freezes it.  For internal use
only.
On 2010/08/14 01:39:18, MarkM wrote:
> Is this no longer for internal use only? Or is this simply an inappropriate
> place to comment on that?

It's no longer for internal use only; I needed to expose it so that the hostpage
could use it.

http://codereview.appspot.com/1955042/diff/13005/6006
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/1955042/diff/13005/6006#newcode870
src/com/google/caja/es53.js:870: // Convert to object form so we can look up the
non-extensibility.
On 2010/08/14 01:39:18, MarkM wrote:
> Ok, now that I know what this is about, I see that it's correct but
> unnecessarily complex. How about
> 
>   function isExtensible(obj) {
>     return Type(obj) === 'Object' && obj.ne___ === obj;
>   }
> 
> ?

Done.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode1141
src/com/google/caja/es53.js:1141: * Enforces {@code typeof(specimen) ===
typename}, in which case
On 2010/08/14 01:39:18, MarkM wrote:
> The primitive typeof doesn't need to parenthesize its argument.

Fixed here and in tameException.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode1670
src/com/google/caja/es53.js:1670: // Looping again over the same untrusted
stamps alleged-array is safe
On 2010/08/14 01:39:18, MarkM wrote:
> (I suspect the following may be my oversight from a previous CL. But in any
> case, now's a good time to fix this)
> 
> The comment here dates from before we were using Array.slice___ to copy the
> stamps alleged array into a genuine array. Now that we are, this is safe and
the
> comment can be removed.
> 
> We should ensure that an attacker-installed access on a numerically-named or
> 'length' property on Array.prototype can't interfere. For full ES5/3, it's
> plausible to me that we might want to initialize Array.prototype.length to a
> non-writable, non-configurable 0, even if we don't freeze all of
> Array.prototype.
> 

Array instances have an own length property that masks the prorotype's length
property.  The array produced by slice___ is guaranteed to have own indices for
0 up to length-1 and to have a length property, so this loop can't be corrupted
by changing Array.prototype's properties, and the prototype itself is
nonconfigurable, so it can't be replaced.  The length will also be a positive
integer, so the ">>> 0" is also unnecessary.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode1740
src/com/google/caja/es53.js:1740: switch (typeof(ex)) {
On 2010/08/14 01:39:18, MarkM wrote:
> The primitive typeof doesn't need to parenthesize its argument.

Done.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode2553
src/com/google/caja/es53.js:2553: Array.prototype.shift___.call___(result);
On 2010/08/14 01:39:18, MarkM wrote:
> Wow, good catch! I think I missed this bug on an earlier CL. And it's a bad
one:
> mutating the argument, which is usually a genuine ES3 arguments object -- a
> dangerous thing to mutate!

Bugs are easier to catch while they're biting me!

http://codereview.appspot.com/1955042/diff/13005/6006#newcode2580
src/com/google/caja/es53.js:2580: * Returns a function object with {@code call,
bind}, and {@code apply}
On 2010/08/14 01:39:18, MarkM wrote:
> The comment diagrees with the code -- wrap does not create a
> per-function-instance bind method. I suspect it should not and the comment is
> simply stale.

Are we reading different comments?  I don't see any mention of
per-function-instance...

http://codereview.appspot.com/1955042/diff/13005/6006#newcode2997
src/com/google/caja/es53.js:2997: this.toString = markFunc(function (var_args) {
On 2010/08/14 01:39:18, MarkM wrote:
> Don't you need to first check that this is not frozen?

Not when using getters and setters.  I suppose we should choose to do so, but it
doesn't violate ES5 semantics.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3017
src/com/google/caja/es53.js:3017: this.valueOf = markFunc(function (var_args) {
On 2010/08/14 01:39:18, MarkM wrote:
> Don't you need to first check that this is not frozen?

Done.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3033
src/com/google/caja/es53.js:3033: set: markFunc(function (V) { return hop =
asFirstClass(V); }),
On 2010/08/14 01:39:18, MarkM wrote:
> This has two problems:
> 
> * I thought we wanted to special case Object.prototype to be
> non-monkey-patchable, even in full ES5/3.
> 
> * But we still need a setter, since this access property will be inherited by
> objects that don't override 'hasOwnProperty'. If (this !== Object.prototype)
and
> !isFrozen(this), then we want to simulate a simple assignment to
> this.hasOwnProperty.
> 
> The 2nd point above requires sufficient complexity that I'm not leaning to the
> hasOwnProperty___ approach rather than the access property approach for guest
> override whenever possible.

Now checks to see if the object is frozen before setting the property.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3041
src/com/google/caja/es53.js:3041: throw new Error('isPrototypeOf is not
supported.');
On 2010/08/14 01:39:18, MarkM wrote:
> I understand why Object.getPrototypeOf is hard. But AFAICT, the built-in
> isPrototypeOf should work as is. The guest and host views of the prototype
chain
> should be identical.

I didn't realize this was supported in ES3 browsers.  I've whitelisted it.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3069
src/com/google/caja/es53.js:3069: // NOT extensible under ES5/3
On 2010/08/14 01:39:18, MarkM wrote:
> Aha! Thought so.

Yep.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3507
src/com/google/caja/es53.js:3507: set: markFunc(function (V) { return replace =
asFirstClass(V); }),
On 2010/08/14 01:39:18, MarkM wrote:
> Although practically less urgent, this fails for the same reason the naive
> setter on hasOwnProperty fails. Although it's perverse, anyone can make an
> object, O, that inherits from String.prototype. If someone then assigns to
> O.replace, that will invoke this inherited setter which will do the wrong
thing.

Unless they use Object.defineProperties API to set it or declare it in an object
literal.  

> 
> Also, again you forgot to check whether String.prototype was frozen before
> allowing the mutate.

Again, it's not a violation of frozenness, just a design decision.

> I'm becoming yet more enthusiastic about the replace___ approach. Sorry. I do
> remember that it had its own set of complexities.

Let's talk about this: I'm realizing there are major problems with innocent code
unless we use get/set.

http://codereview.appspot.com/1955042/diff/13005/6006#newcode3745
src/com/google/caja/es53.js:3745: Date.prototype.DefineOwnProperty___('now', {
On 2010/08/14 02:45:15, MarkM wrote:
> Why did you pull this? It looked good to me.

Because there is no Date.prototype.now, only Date.now.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode913
src/com/google/caja/es53.js:913: if (m) { result.push(m[1]); }
On 2010/08/14 02:45:15, MarkM wrote:
> This seems too restrictive. It will exclude everything but own data
properties.

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode927
src/com/google/caja/es53.js:927: if (m) { result.push(m[1]); }
On 2010/08/14 02:45:15, MarkM wrote:
> This seems too restrictive. It will exclude everything but own data
properties.

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode3785
src/com/google/caja/es53.js:3785: writable: true,
On 2010/08/14 03:23:29, MarkM wrote:
> Where is "pattern" defined?

Added 

var pattern = as[0], flags = as[1];

http://codereview.appspot.com/1955042/diff/14001/15001#newcode3786
src/com/google/caja/es53.js:3786: enumerable: false,
On 2010/08/14 03:23:29, MarkM wrote:
> Where is "flags" defined?

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode3821
src/com/google/caja/es53.js:3821: }
On 2010/08/14 03:23:29, MarkM wrote:
> Perhaps a bit too clever. Although it's more verbose, I suggest a separate
call
> to DefineOwnProperty___ to define 'lastIndex'.

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode3839
src/com/google/caja/es53.js:3839: 
On 2010/08/14 03:23:29, MarkM wrote:
> You cannot simply whitelist the built-in exec and test because of issue 528.
See
> the taming of these in cajita.js and
> http://code.google.com/p/es-lab/source/browse/trunk/src/ses/initSES.js#117

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4048
src/com/google/caja/es53.js:4048: 
On 2010/08/14 02:45:15, MarkM wrote:
> I'm almost certain that "exception.stack" is not portable.

Oops, debugging cruft.  Removed.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4122
src/com/google/caja/es53.js:4122: return myNewModuleHandler.m___('handle',
[module]);
On 2010/08/14 03:23:29, MarkM wrote:
> Shouldn't this also do a grantRead(obj, name)?

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4137
src/com/google/caja/es53.js:4137: value: obj[name],
On 2010/08/14 03:23:29, MarkM wrote:
> Please choose an actual example instead of peek.

Removed, as this API is used neither in domita nor in shindig.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4158
src/com/google/caja/es53.js:4158: 
On 2010/08/15 00:28:07, MarkM wrote:
> This now objectively replaces the method as it should, so the comment is very
> stale.

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4161
src/com/google/caja/es53.js:4161: * {@code this} inherits from proto.
On 2010/08/15 00:28:07, MarkM wrote:
> Comment also stale.

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4174
src/com/google/caja/es53.js:4174: var original = proto[name];
On 2010/08/15 00:28:07, MarkM wrote:
> Should this be apply___ ?

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4185
src/com/google/caja/es53.js:4185: enumerable: false,
On 2010/08/15 00:28:07, MarkM wrote:
> I no longer remember: Is this the right way to treat the global object, or did
> we intend to throw an Error on this case?

I've replaced this with safeDis, which throws an error.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4213
src/com/google/caja/es53.js:4213: function grantInnocentMethod(proto, name) {
On 2010/08/15 00:28:07, MarkM wrote:
> I'm not sure if this one should be "apply" or "apply___". Thoughts?

The original is always a feral function, so I think we should use apply___.

And slice should definitely be slice___.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4285
src/com/google/caja/es53.js:4285: obj.DefineOwnProperty___(name, desc);
On 2010/08/15 00:28:07, MarkM wrote:
> Ah, right, I see the issue. If we make useGetHandler & useSetHandler both use
> "configurable: false", then we can't install these one at a time.
Nevertheless,
> I think "configurable: false" is right, and we should refactor the taming code
> accordingly. But this can wait for the next round -- if you postpone it,
please
> leave an issue or TODO.

Done.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4299
src/com/google/caja/es53.js:4299: desc.set = setHandler;
On 2010/08/14 03:23:29, MarkM wrote:
> We need to come up with a name for this object that doesn't bake the version
> into the name.  Perhaps we continue to call it "cajita", so old cajita code
can
> continue to work when it should?

There's not enough legacy code to matter, I think.  Anything written in Cajita
ought to migrate to ES5 strict mode.

http://codereview.appspot.com/1955042/diff/14001/15001#newcode4397
src/com/google/caja/es53.js:4397: // which is truthy iff it's a data property
On 2010/08/15 00:28:07, MarkM wrote:
> We should avoid baking the version name into the property name. It would be
> better to have some reusable property name such as "version:" or something,
and
> then put the specific version in the value.

Removed, since nothing's using it now that we have the keeper API again.

http://codereview.appspot.com/1955042/diff/14001/15002
File src/com/google/caja/parser/quasiliteral/ES53Rewriter.java (right):

http://codereview.appspot.com/1955042/diff/14001/15002#newcode1000
src/com/google/caja/parser/quasiliteral/ES53Rewriter.java:1000:
substitutes="IMPORTS___.w___('@v', void 0)")
On 2010/08/15 00:28:07, MarkM wrote:
> This is only a good translation when IMPORTS___[@v] doesn't already exist.
When
> it does, a non-initializing declaration should have no effect. In particular,
it
> should not wipe out its previous value.

Hmm, true.  And I'll need a different translation anyway because reading a
nonexistent variable doesn't currently throw a ReferenceError.

http://codereview.appspot.com/1955042/diff/14001/15002#newcode1654
src/com/google/caja/parser/quasiliteral/ES53Rewriter.java:1654:
substitutes="@fnameRef = ___.wrap(function @fname(dis___, @ps*) {\n"
On 2010/08/15 00:28:07, MarkM wrote:
> This seems wrong. It brings @fname into scope within the body of the expanded
> function with @fname bound to the expanded function. Only the wrapped function
> is a faithful emulation of the original, so @fname should be bound to it
> instead.

Yeah; what do you think of the new translation?

http://codereview.appspot.com/1955042/diff/14001/15002#newcode1699
src/com/google/caja/parser/quasiliteral/ES53Rewriter.java:1699:
substitutes="IMPORTS___.w___(@rf, ___.wrap(function @fname(dis___, @ps*) {\n"
On 2010/08/15 00:28:07, MarkM wrote:
> Are you treating this as global? From the rest of this rule, it does not seem
to
> be about global function declarations.

Yes, this only fires in the global scope due to the way the bindings are
constructed.

[MarkM, 2010-08-17 07:04:20]

http://codereview.appspot.com/1955042/diff/22001/23001
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/1955042/diff/22001/23001#newcode3882
src/com/google/caja/es53.js:3882: set: markFunc(function (V) { exec =
asFirstClass(V); }),
Another instance of the same setter vs inheritance problem. By using a
non-configurable accessor property to emulate a data property, you must emulate
the behavior of assigning to the 'exec' property of an object that inherits from
RegExp.prototype. This setter does not.

http://codereview.appspot.com/1955042/diff/22001/23001#newcode3895
src/com/google/caja/es53.js:3895: set: markFunc(function (V) { test =
asFirstClass(V); }),
Setter vs inheritance again.

http://codereview.appspot.com/1955042/diff/22001/23001#newcode4333
src/com/google/caja/es53.js:4333: es53 = whitelistAll({
But we still shouldn't call this "es53".

[MarkM, 2010-08-18 19:13:29]

LGTM modulo comments (including verbal)

http://codereview.appspot.com/1955042/diff/26001/27001
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/1955042/diff/26001/27001#newcode2698
src/com/google/caja/es53.js:2698: Object.defineProperties(obj, opt_Properties);
Just call a local named function so we don't need to virtualize

http://codereview.appspot.com/1955042/diff/26001/27001#newcode3153
src/com/google/caja/es53.js:3153: function guardedVirtualize(obj, name,
opt_attrs) {
opt_attrs?

http://codereview.appspot.com/1955042/diff/26001/27001#newcode4280
src/com/google/caja/es53.js:4280: if (guestHasOwnProperty(imports, name)) {
strike "Own" perhaps. Please check ES5 spec.

[MarkM, 2010-08-18 19:15:00]

The remaining (verbal) issue is that virtualize should use configurable: false,
and the configurability check in DefineOwnProperty should be restored.

[metaweta, 2010-08-20 22:37:02]

> The remaining (verbal) issue is that virtualize should
> use configurable: false, and the configurability check
> in DefineOwnProperty should be restored.

Done.

http://codereview.appspot.com/1955042/diff/26001/27001
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/1955042/diff/26001/27001#newcode2698
src/com/google/caja/es53.js:2698: Object.defineProperties(obj, opt_Properties);
On 2010/08/18 19:13:29, MarkM wrote:
> Just call a local named function so we don't need to virtualize

Done.

http://codereview.appspot.com/1955042/diff/26001/27001#newcode3153
src/com/google/caja/es53.js:3153: function guardedVirtualize(obj, name,
opt_attrs) {
On 2010/08/18 19:13:29, MarkM wrote:
> opt_attrs?

Done.

http://codereview.appspot.com/1955042/diff/26001/27001#newcode4280
src/com/google/caja/es53.js:4280: if (guestHasOwnProperty(imports, name)) {
On 2010/08/18 19:13:29, MarkM wrote:
> strike "Own" perhaps. Please check ES5 spec.

I think the relevant clause is 10.2.1.2.1; done.