Port Domita to ECMAScript 5 as "Domado".

Domado is a straight port of Domita, but written in pure ECMAScript 5 code; it is run cajoled under ES5/3. It is designed to also be capable of running in a pure SES environment, using strictly ES5 features to defend itself.

In order to support being cajoled and not having privileged access, Domado does not use any ___ properties internally (replacing them with WeakMaps) or externally (replacing them with separate objects providing privileged access). HtmlEmitter and Bridal have necessarily been ported in the same way.

[MarkM, 2011-06-06 06:22:21]

By no means comprehensive. Just some things I spotted.

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
File src/com/google/caja/es5-guards.js (right):

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
src/com/google/caja/es5-guards.js:40: toString: Object.freeze(function
tokenToString() {
fotgot to freeze tokenToString.prototype. This comes up below too. Should
probably just use SES's cajaVM.def.

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
src/com/google/caja/es5-guards.js:100: var failFunc = opt_failFunc || function
(x) { return x; };
This will evaluate the function expression to a new identity function on every
call to callWithEjector. Better to factor out.

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
src/com/google/caja/es5-guards.js:103: // token.UNCATCHABLE___ = true; //
TODO(kpreid): can't do in ES5?
Correct. The whole uncatchable notion cannot be supported on a
lite-verification-only strategy.

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
src/com/google/caja/es5-guards.js:152: // newTable as well, but perhaps we
should.
Yes, I think we should be using WeakMap-based permissions amplification rather
than MarcS's pattern of authority amplification.

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
src/com/google/caja/es5-guards.js:181: this.seal = seal;
Using "this" in defensive code is generally a really bad idea. This should be
creating and returning a new object instead.

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
src/com/google/caja/es5-guards.js:208: _es5g_mark:
markSealing.seal(Object.freeze(function(obj) { // TODO(kpreid): make this
nonenumerable
please stay within 80 columns

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es53.js#n...
src/com/google/caja/es53.js:2033: // Tokens
Are you planning to remove the parts of es53.js that are redundant with
es5-guards.js? (I hope so)

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/ses/white...
File src/com/google/caja/ses/whitelist.js (right):

http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/ses/white...
src/com/google/caja/ses/whitelist.js:115: WeakMap: t,
"WeakMap: {..." appears below, so this seems redundant.

[kpreid1, 2011-06-06 14:02:07]

On Jun 5, 2011, at 23:22, erights@gmail.com wrote:

> By no means comprehensive. Just some things I spotted.

As a general remark, I can see that I should have noted that es5-guards.js is,
ah, not exactly polished and thoughtfully designed. It is merely the repository
of everything I needed to support Domado which was a preexisting API.


>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
> File src/com/google/caja/es5-guards.js (right):
> 
>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
> src/com/google/caja/es5-guards.js:40: toString: Object.freeze(function
> tokenToString() {
> fotgot to freeze tokenToString.prototype. This comes up below too.
> Should probably just use SES's cajaVM.def.

I had not previously noticed that SES provided a cajaVM object, and assumed it
was strictly an ES5/3 feature. I will examine it for relevant facilities.

>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
> src/com/google/caja/es5-guards.js:100: var failFunc = opt_failFunc ||
> function (x) { return x; };
> This will evaluate the function expression to a new identity function on
> every call to callWithEjector. Better to factor out.
> 
>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
> src/com/google/caja/es5-guards.js:103: // token.UNCATCHABLE___ = true;
> // TODO(kpreid): can't do in ES5?
> Correct. The whole uncatchable notion cannot be supported on a
> lite-verification-only strategy.

Right; the question is really whether it matters.

For example: given that we cannot have uncatchables for our ejector
implementation, should we perhaps define the thrown object so that it makes more
sense to outside code which observes it?

>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
> src/com/google/caja/es5-guards.js:152: // newTable as well, but perhaps
> we should.
> Yes, I think we should be using WeakMap-based permissions amplification
> rather than MarcS's pattern of authority amplification.

Agreed; I will change this code. Most of es53-guards was hacked up before I
moved to actually using WeakMaps anywhere.

>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
> src/com/google/caja/es5-guards.js:181: this.seal = seal;
> Using "this" in defensive code is generally a really bad idea. This
> should be creating and returning a new object instead.

Why does it matter? This constructor is using 'this' just as a convenient bundle
for the three functions; the 'this' object is not used further by the code. That
said, I can see that it ought to freeze the prototype and constructor for
defensive consistency.

>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guard...
> src/com/google/caja/es5-guards.js:208: _es5g_mark:
> markSealing.seal(Object.freeze(function(obj) { // TODO(kpreid): make
> this nonenumerable
> please stay within 80 columns

I have bothered to ensure 80 columns only on those lines which I expect to make
it into the final commit.

> http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es53.js
> File src/com/google/caja/es53.js (right):
> 
>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es53.js#n...
> src/com/google/caja/es53.js:2033: // Tokens
> Are you planning to remove the parts of es53.js that are redundant with
> es5-guards.js? (I hope so)

I had not considered modifying es53.js, but I agree it would be good to have
only one implementation of these facilities.

Supposing that this were to happen, what would be a good name for the file? It
is not all about guards, after all.

>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/ses/white...
> File src/com/google/caja/ses/whitelist.js (right):
> 
>
http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/ses/white...
> src/com/google/caja/ses/whitelist.js:115: WeakMap: t,
> "WeakMap: {..." appears below, so this seems redundant.

My error; I was poking around trying to get the WeakMap constructor to show up
in the actual runtime environment. Removed.

> http://codereview.appspot.com/4535118/

-- 
Kevin Reid                                  <http://switchb.org/kpreid/>

[MikeSamuel, 2011-06-24 18:21:09]

Some initial comments but still much more to look at.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/es53.js#...
src/com/google/caja/es53.js:2339: * {@code typename} is used for toString and
{@code errorMessage} is used
Maybe @param typename

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/b...
File src/com/google/caja/plugin/bridal.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/b...
src/com/google/caja/plugin/bridal.js:29: // TODO(kpreid): These were previously
gotten from the taming frame, not the host frame; is this OK?
Getting them from the host frame allows the host frame to easily
remove/wrap/monkey patch these which may be useful.

Getting them from the taming frame means more work is required to do any of
those.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/b...
src/com/google/caja/plugin/bridal.js:744: }
;

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:328: * TODO(kpreid): document this.
Yes please.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:332: if (typeof o !== 'undefined' && o !==
null) {
Isn't the below only effective for objects and functions.  Are we expecting
functions to be passed to this function?  Can the test become ('object' ===
typeof o)?

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:339: if (!(typeof p === 'number' || ('' +
(+p)) === p) && !(/__$/).test(p)) {
How can typeof p be number when you've just converted it to a string above?

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:340: if (typeof o[p] === 'function') {
Should we read o[p] once instead of here and again below before applying.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:347: o.HasProperty___ = function (P) { return
P in this; };
P -> p

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1: // Copyright (C) 2008-2011 Google Inc.
Is this based on domita.js via svn mv in your local client?
When you diff via svn diff --notice_ancestry, do you get smaller diffs?

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/ses/es5s...
File src/com/google/caja/ses/es5shim.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:677:
!desc.ses_ignoreBug_propertyWillAppearAsOwn) {
Could you comment on this?

[kpreid2, 2011-06-25 00:32:20]

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/es53.js#...
src/com/google/caja/es53.js:2339: * {@code typename} is used for toString and
{@code errorMessage} is used
On 2011/06/24 18:21:09, MikeSamuel wrote:
> Maybe @param typename

Done.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/b...
File src/com/google/caja/plugin/bridal.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/b...
src/com/google/caja/plugin/bridal.js:29: // TODO(kpreid): These were previously
gotten from the taming frame, not the host frame; is this OK?
On 2011/06/24 18:21:09, MikeSamuel wrote:
> Getting them from the host frame allows the host frame to easily
> remove/wrap/monkey patch these which may be useful.

OK, I will leave this as is and remove this comment. (In theory, bridal simply
provides tools for the one document and doesn't know anything about taming vs.
host...)

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/b...
src/com/google/caja/plugin/bridal.js:744: }
On 2011/06/24 18:21:09, MikeSamuel wrote:
> ;

Done.

On the subject of this file, do you have a suggestion for where to provide
getWindow, as putting it on something named "fooMaker" is clunky?

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:328: * TODO(kpreid): document this.
On 2011/06/24 18:21:09, MikeSamuel wrote:
> Yes please.

Done.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:332: if (typeof o !== 'undefined' && o !==
null) {
On 2011/06/24 18:21:09, MikeSamuel wrote:
> Isn't the below only effective for objects and functions.  Are we expecting
> functions to be passed to this function?  Can the test become ('object' ===
> typeof o)?

I've changed it to (typeof o === 'object' && o !== null && !('v___' in o)). I
don't expect functions or value primitives, but I *do* expect undefined or null.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:339: if (!(typeof p === 'number' || ('' +
(+p)) === p) && !(/__$/).test(p)) {
On 2011/06/24 18:21:09, MikeSamuel wrote:
> How can typeof p be number when you've just converted it to a string above?

This was a inlining of isNumericName from es53 without any attempt at
optimization. Fixed.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:340: if (typeof o[p] === 'function') {
On 2011/06/24 18:21:09, MikeSamuel wrote:
> Should we read o[p] once instead of here and again below before applying.

Done, but shouldn't the same change then be made to es53.js?

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:347: o.HasProperty___ = function (P) { return
P in this; };
On 2011/06/24 18:21:09, MikeSamuel wrote:
> P -> p

Done. This is what was in es53.js, but consistency here seems a fine
alternative.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1: // Copyright (C) 2008-2011 Google Inc.
On 2011/06/24 18:21:09, MikeSamuel wrote:
> Is this based on domita.js via svn mv in your local client?
> When you diff via svn diff --notice_ancestry, do you get smaller diffs?

It was but isn't at the moment. I haven't tried playing with the diffing; but I
think a diff-against-copied-file already gets the best result. IIRC Rietveld
doesn't diff-against-copy-source even if it's in svn that way.

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/ses/es5s...
File src/com/google/caja/ses/es5shim.js (right):

http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:677:
!desc.ses_ignoreBug_propertyWillAppearAsOwn) {
On 2011/06/24 18:21:09, MikeSamuel wrote:
> Could you comment on this?

Done.

[MikeSamuel, 2011-07-06 20:48:27]

I'm holding off on reviewing the files in the draft and experimental directories
since I don't know what they're for.  If they're demo then they're exempt from
review.

I haven't yet started on domado.js but i am going to start working through that
next.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/demos/pl...
File src/com/google/caja/demos/playground/es53.html (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/demos/pl...
src/com/google/caja/demos/playground/es53.html:57: function (n) {return n;},
div, imports.document);
What does using the identity function for makeDOMAccessible mean?  Does it mean
we're not membraning?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
File src/com/google/caja/es5-guards.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:20: * TODO(kpreid): Straighten out exactly
what this exports
Please explain the relationship of this file to es53.js which still seems to
contain trademarks and sealer unsealer pairs.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:30: // SES.
Mike Stay, can you comment on this?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:32: var gCajaVM;
Should gCajaVM be a read-only global?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:102: var failFunc = opt_failFunc || function
(x) { return x; };
Should this failFunc be frozen?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:105: // token.UNCATCHABLE___ = true; //
TODO(kpreid): can't do in ES5?
MarkM, can you comment on this?

Is this meant to stop it from being intercepted via

try {
...
} catch (ex) {}

and

do {
  try {
    ...
  } finally { break; }
} while (0);

I chatted with Mark and he says it is not a goal to support uncatchable
exceptions in SES.  I am filing an issue to audit all uses of callWithEjector in
the current codebase to see whether any security properties depend on ejectors
being reliable in the face of exception interception.

It may be that the efficient runtime JS lexer proposed earlier on the list will
allow uncatchable exceptions via a token-level-only transformation.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:182: }
Should SealerUnsealerPair fail if called without new?
if (!(this instanceof SealerUnsealerPair)) { throw new TypeError(); }
As written, it looks like this leaks the authority to freeze the global object
to someone who has no access to Object.freeze.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/b...
File src/com/google/caja/plugin/bridal.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/b...
src/com/google/caja/plugin/bridal.js:27: var bridalMaker = function
(makeDOMAccessible, document) {
Can you add comments to help maintainers understand when makeDOMAccessible
should be called and when it shouldn't.  If there is documentation elsewhere on
this question, a pointer is sufficient.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/b...
src/com/google/caja/plugin/bridal.js:770: doc.body.appendChild(s);
Is this supposed to happen synchronously?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:341: * code.
How are we going to verify that this function is only called on objects that are
not given to untrusted code?  This seems to require static inside-membrane
determinations.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:345: * wrappers. (Domado is not.)
I don't understand this.  Does it mean that bridal makes fewer assumptions, and
might receive a makeDOMAccessible parameter that unwraps, whereas html-emitter
always receives one that returns its argument.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:358: p = '' + p;
Ok.  Coerces p to a string.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:359: if (('' + (+p)) !== p &&
!(/__$/).test(p)) {
Tests that p is not a canonicalized numeric property and does not end in __.

Why is the canonicalized property significant?  Why does passing in '1' result
in a runtime error, but passing in '1.0' does not?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:365: throw new TypeError('Not a function: ' +
p);
Misleading when p === '1.0' || p == 'x__'

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:382: } // else let primitives go normally
Ok.  Nothing that supports typeof [] === 'array' is in the A-grade set.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/capture-domado.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/capture-domado.js:34: ["log", "warn", "error",
"debug"].forEach(function (v) {
Is this a minimal set that you need for debugging?  Why these methods and not
group/ungroup?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:30: /**
Please document @param makeDOMAccessible

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:32: * @param opt_domadoTools the
tools of the domado instance that will receive a
Tools is an obscure term that does nothing to convey the contracts it or its
clients have to meet.  Is there a set of interfaces that the HTML DOM spec
defines that tools have to implement?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:41: makeDOMAccessible(base);
Return value discarded here and below.  In the only place I've seen it used thus
far, it was the identity function, not a no-op and bridal.js uses the return
value.

What happens if makeDOMAccessible returns a value other than its input?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:63: // name conflicts.
Maybe add a note that prerequisite passes such as SafeHTMLMaker are responsible
for ensuring that IDs that end with __ are allowed.

Also, the prohibition is on 2 underscores, not 3.

[metaweta, 2011-07-06 22:45:43]

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
File src/com/google/caja/es5-guards.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:30: // SES.
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Mike Stay, can you comment on this?

To have read-only globals in ES53 from within host code, you say something like

IMPORTS___.DefineOwnProperty___('myGlobal', { value: 1 });

To do it from within guest code, you say

Object.defineProperty(this, 'myGlobal', { value: 1 });

at the global scope.  In both cases, I've left off the writable and configurable
flag so they default to false.

[kpreid2, 2011-07-13 00:24:09]

Oops! I thought I had published these comments long ago!

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/demos/pl...
File src/com/google/caja/demos/playground/es53.html (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/demos/pl...
src/com/google/caja/demos/playground/es53.html:57: function (n) {return n;},
div, imports.document);
On 2011/07/06 20:48:27, MikeSamuel wrote:
> What does using the identity function for makeDOMAccessible mean?  Does it
mean
> we're not membraning?

It means that there is no ES5/3 taming membrane that we need to work despite.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
File src/com/google/caja/es5-guards.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:20: * TODO(kpreid): Straighten out exactly
what this exports
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Please explain the relationship of this file to es53.js which still seems to
> contain trademarks and sealer unsealer pairs.

This file is only loaded when es53.js is not. In the final version, this will be
turned into an optional component of SES, such that cajaVM (possibly renamed
'ses' per discussion with MarkM) has a larger subset identical on both platforms
(ES5/3 and SES).

Perhaps that should be a separate changeset coming first?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:32: var gCajaVM;
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Should gCajaVM be a read-only global?

See above; I hope to fold it into the real cajaVM.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:102: var failFunc = opt_failFunc || function
(x) { return x; };
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Should this failFunc be frozen?

It is only ever called, not passed, so freezing would not make a difference.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guar...
src/com/google/caja/es5-guards.js:182: }
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Should SealerUnsealerPair fail if called without new?
> if (!(this instanceof SealerUnsealerPair)) { throw new TypeError(); }
> As written, it looks like this leaks the authority to freeze the global object
> to someone who has no access to Object.freeze.

Done. This may just go away seeing as Confidence is nearly always a better
abstraction -- except insofar as this file should have feature parity with
es53.js's cajaVM.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/b...
File src/com/google/caja/plugin/bridal.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/b...
src/com/google/caja/plugin/bridal.js:27: var bridalMaker = function
(makeDOMAccessible, document) {
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Can you add comments to help maintainers understand when makeDOMAccessible
> should be called and when it shouldn't.  If there is documentation elsewhere
on
> this question, a pointer is sufficient.

Done.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/b...
src/com/google/caja/plugin/bridal.js:770: doc.body.appendChild(s);
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Is this supposed to happen synchronously?

This code is unmodified, just moved (line 227 on the left).

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:341: * code.
On 2011/07/06 20:48:27, MikeSamuel wrote:
> How are we going to verify that this function is only called on objects that
are
> not given to untrusted code?  This seems to require static inside-membrane
> determinations.

This function is only ever called on host DOM objects which aren't safe to give
to untrusted code anyway.

The alternative which Ihab and I discussed was to have a "perform arbitrary
operations on arbitrary objects" authority given to Domado/Bridal/etc.; we
decided this, "allow arbitrary operations to be performed on this designated
object", was safer.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:345: * wrappers. (Domado is not.)
On 2011/07/06 20:48:27, MikeSamuel wrote:
> I don't understand this.  Does it mean that bridal makes fewer assumptions,
and
> might receive a makeDOMAccessible parameter that unwraps, whereas html-emitter
> always receives one that returns its argument.

Yes. There is no current example of a situation where makeDOMAccessible would
need to wrap or unwrap, but it was easy to let bridal support that, so I did.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:359: if (('' + (+p)) !== p &&
!(/__$/).test(p)) {
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Tests that p is not a canonicalized numeric property and does not end in __.
> 
> Why is the canonicalized property significant?  Why does passing in '1' result
> in a runtime error, but passing in '1.0' does not?

This code is intended to be identical to es53.js's (!isNumericName(p) &&
!endsWith__.test(p)). I have no knowledge of why it should be one way or the
other.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/capture-domado.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/capture-domado.js:34: ["log", "warn", "error",
"debug"].forEach(function (v) {
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Is this a minimal set that you need for debugging?  Why these methods and not
> group/ungroup?

It's a minimal set for debugging and presenting unusual events to the end
developer, and also the set of methods which have identical interfaces to the
one I needed.

Note that IIRC this is unsafe in that passing guest objects into the Firebug
console can cause invocation of their toString. For hostile uses this needs to
coerce all its arguments to string. I've made a note to work on that.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:32: * @param opt_domadoTools the
tools of the domado instance that will receive a
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Tools is an obscure term that does nothing to convey the contracts it or its
> clients have to meet.  Is there a set of interfaces that the HTML DOM spec
> defines that tools have to implement?

Tools is Domado-specific authorities to the document which used to be accessible
via privileged access to foo___ properties (i.e. amplification) on the
tameDocument. It actually only needs signalLoaded and sanitizeAttrs.

I welcome suggestions of more generic interfaces that work given that
HtmlEmitter is now unprivileged.

On the other hand, tools is now the object that is returned by
attachDocumentStub2. It should probably therefore have a more explanatory name.
I'd call it a 'Domado instance' except that there is a Domado() constructor
already, which returns, not 'tools', but the object of which attachDocumentStub2
is an instance. (I should also rename attachDocumentStub2; the '2' is a relic.)

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:41: makeDOMAccessible(base);
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Return value discarded here and below.  In the only place I've seen it used
thus
> far, it was the identity function, not a no-op and bridal.js uses the return
> value.

In the actual membrane case (caja.js sets it up), makeDOMAccessible is a
side-effecting identity function.

> What happens if makeDOMAccessible returns a value other than its input?

bridal.js will function in that case; HtmlEmitter and Domado won't.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:63: // name conflicts.
On 2011/07/06 20:48:27, MikeSamuel wrote:
> Maybe add a note that prerequisite passes such as SafeHTMLMaker are
responsible
> for ensuring that IDs that end with __ are allowed.
> 
> Also, the prohibition is on 2 underscores, not 3.

Name conflicts by themselves are sufficient reason; I've made the comment more
generic.

[MikeSamuel, 2011-07-18 21:57:00]

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:60: var PLACEHOLDER_theTools; //
TODO(kpreid): This is a placeholder for something to replace the pluginId
mechanism, making it into a global singleton instead. 'tools' is the replacement
for ___ properties on 'imports'.
Isn't tools also akin to a view?  html-emitter accesses tools.document.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:182: * the right thing is.
"overriden" -> "overridden"?

Does "non-writable property cannot be overridden" imply (propName in
object.prototype && !Object.hasOwnProperty.call(object, propName)) before the
attempt to set?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:192: enumerable:
domitaModules.getPropertyDescriptor(object, propName)
propName might be coerced to two different strings above if propName can come
from an untrusted source.
Maybe do propName += ''; at the start.

Does getPropertyDescriptor not return a nullish value when propName is blank? 
Should properties default to non-enumerable if not overriding?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:229: * objects).
Mark, can you please review this?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:318: * @param {Object} target The tame node
to forward methods to.
Ok.  After discussion, there is a 3-layered onion.  The feral node is the core,
a middle tame DOM node which reinterprets messages meant for the feral node, and
an outer layer that implements expandos for any messages that cannot be handled
by the middle tier.

So the target is both tame and an inner layer.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:320: * @param {Object} key The object to
index the expando properties under.
key seems an odd name here.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:330: return this;
Is this necessary?

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:352: };
Restart here.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:50: * @overrides domitaModules
This is a really impressive piece of work.

Are there ways we could use naming conventions to make it easier for
reviewers/maintainers to triage inputs, e.g. into (untrusted value, tame value,
statically unclear provenance).  That's something that is out of scope for this
CL, but thought I'd raise the idea.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:160: if (i === props.length) throw
StopIteration;
I know this is commented out, but is it a memory leak to not null out elements
of props that are < i?
Probably only an issue if large strings are used as keys with long-lived
iterators.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:204: function (aCallback) {
Do the callbacks tend to receive an argument?

Is it worth doing an arity check?

Do we need to check that the argument is callable by untrusted code or is that
not ES53-ish?

Is eval a valid callback?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:226: * ability for one object to access its
the private state of other similar
"its the" -> "the"

Is this sibling amplification similar to the way that an instance of a class C
in java can access the privates of other instances of a class C?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:282: guard.coerce(object);  // borrow
failure
Does this always fail, or will this return undefined instead of an empty object
sometimes?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:348:
ExpandoProxyHandler.prototype.getOwnPropertyDescriptor = function (name) {
Are these name parameters guaranteed to be coerced to strings or numbers before
reaching the proxy handler?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:515: var TameXHRConf = new Confidence();
Missing typename?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:520: TameXHRConf.confide(this);
If someone forgets a `new` then we might confide in the global object.  Maybe a
simple instanceof check at the top of constructors that confide in this would
add a layer of protection.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:521: var xhr = p(this).feral =
Ok, so p(this) gets the private of this and that has to be done to the object
that backs the proxy, not the proxy object.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:530: }
This self idiom seems to occur several times with subtle variations.  Can we
reduce the differences so someone who has to find and change uses of this idiom
has an easier time operating on all of them?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:652: var evt = { target: this };
Do we use this, not the proxy because an event handler wrapper dispatches to the
proxy if proxy self != this?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:833: function
definePropertiesAwesomely(object, propDescs) {
Is this part of Bill and Ted's excellent reflection?

Does this threads the property name through to things like
myNode.style.backgroundColor where the handler can be generic over all CSS
property names if it just figure out which CSS property is being set?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:850: return extGet.call(this, prop);
What is `this` here and at 855?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:892: return array.push.apply(array, rest);
This seems to be trusting array.push which is OK I guess if it is only called on
listener lists we control, but if you don't need the negative index handling,
isn't it easier to just do array.splice(from, to - from) or the negative index
handling equivalent : array.splice((from + array.length) % array.length, (to +
array.length) % array.length - (from + array.length) % array.length)?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:912: var h = String(htmlFragment || '');
If htmlFragment is 0 or NaN this might cause strangeness.  Probably a problem
with the original code, so I don't think it's a huge problem, but maybe
String(htmlFragment == null ? '' : htmlFragment) where the == is intentionally
not ===.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:922: :
html.escapeAttrib(String(htmlFragment || ''));
Same problem as above.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1061: * map is never used with more than
one version of setTimeout.
Cool.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1266: *     object is known as a
"domicile".
Thanks for the docs.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1279: var window =
bridalMaker.getWindow(pseudoBodyNode);
Ah, so this uses the trick with the inserted script node.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1282: domicile.isProcessingEvent = false;
Ok, so this is used to elevate permission when the user is interacting with an
element.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1286: }
Maybe move this check to the top.  The closer it is to either succeeding or ding
nothing to document and window, the better.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1345: // unfrozen so that assignments to
expando fields work.
ok, so this is just an extra safety net that is available when proxy nets are
available.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1635: * predicate restricting the value)
have the value the original node
"and" before "have the value"?

Maybe make it clear that the predicate is not applied on read.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1665: extendedAccessors: true,
Is this necessary?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1781: makeDOMAccessible(node);
Should this move below the sanity check below?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1836: makeDOMAccessible(node);
Should this move below the sanity check and early out below?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2389: });
Ok.  So bridal.dispatch event here gets the ferals, and is responsible for all
the membrane maintenance.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2507: get: nodeMethod(function () { return
np(this).geometryDelegate.clientWidth; })
Can these be defined in terms of one awesome property getter that gets the
property name?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2703: innerText: textAccessor,
Ok.  Since this only applies to text nodes, the difference between innerText and
nodeValue can be ignored.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3123: var construct = record.construct ||
function (self) { return self; };
|| identity?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3146: });
Cool

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3146: });
Cool

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3927: src: NP.filter(false, identity, true,
identity)
How does the identity setter with an attr getter work here?  This turns into a
call to setAttribute("src") which presumably invokes the URL policy?

Where is the policy code that prevents adding text nodes to a script element?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4052: function fromInt(x) { return '' + (x
| 0); }  // coerce null and false to 0
So if x is NaN, it is coerced to 0, and if it is "NaN" it is coerced to NaN. 
Strange, but probably fine in practice, (and quite possibly my fault).

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5178: //'HTMLScriptElement',
Why are these commented out?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5224: event = makeDOMAccessible(event ||
bridalMaker.getWindow(thisNode).event);
ok

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5251: fn =
domicile.imports.window[handler];
Much nicer.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5267: //}
I do not know yet.  I think it is good to leave this catch block around though
to guard against IE discarding finally blocks.

[kpreid2, 2011-07-20 00:26:47]

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:60: var PLACEHOLDER_theTools; //
TODO(kpreid): This is a placeholder for something to replace the pluginId
mechanism, making it into a global singleton instead. 'tools' is the replacement
for ___ properties on 'imports'.
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Isn't tools also akin to a view?  html-emitter accesses tools.document.

'tools' has been renamed 'domicile'.

I don't know what you mean by "view" here. The View concept in DOM? If so, that
doesn't seem to say much. I suppose it could be seen as a
bundle-of-authorities-plus-a-document as the browser Window is, but in this case
it contains both the (tame) document and window objects.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:182: * the right thing is.
On 2011/07/18 21:57:01, MikeSamuel wrote:
> "overriden" -> "overridden"?

Done.

> Does "non-writable property cannot be overridden" imply (propName in
> object.prototype && !Object.hasOwnProperty.call(object, propName)) before the
> attempt to set?

Yes.

Note that at least some of this is due to an ES5/3 bug recently fixed; the
latest patch set has fewer uses of setOwn due to that.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:192: enumerable:
domitaModules.getPropertyDescriptor(object, propName)
On 2011/07/18 21:57:01, MikeSamuel wrote:
> propName might be coerced to two different strings above if propName can come
> from an untrusted source.
> Maybe do propName += ''; at the start.

That case shouldn't arise, but done, just for robustness.

> Does getPropertyDescriptor not return a nullish value when propName is blank? 
> Should properties default to non-enumerable if not overriding?

This is only ever used for overriding since the problem it is a workaround for
cannot occur otherwise.

In the long run I should look into the ES5 semantics for whether this is a bug
workaround or something that is actually needed.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:320: * @param {Object} key The object to
index the expando properties under.
On 2011/07/18 21:57:01, MikeSamuel wrote:
> key seems an odd name here.

It's called that because it is the key in the weak-key table. For example, when
implementing expando properties on a DOM node, the key is the feral node; but
ExpandoProxyHandler doesn't use the key for any other purpose. It's *just* a
key.

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:330: return this;
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Is this necessary?

No. Removed.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:50: * @overrides domitaModules
On 2011/07/18 21:57:01, MikeSamuel wrote:
> This is a really impressive piece of work.
> 
> Are there ways we could use naming conventions to make it easier for
> reviewers/maintainers to triage inputs, e.g. into (untrusted value, tame 
> value, statically unclear provenance).  That's something that is out of scope
> for this CL, but thought I'd raise the idea.

Worthwhile to think about, but out of scope for this summer :)

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:160: if (i === props.length) throw
StopIteration;
On 2011/07/18 21:57:01, MikeSamuel wrote:
> I know this is commented out, but is it a memory leak to not null out elements
> of props that are < i?
> Probably only an issue if large strings are used as keys with long-lived
> iterators.

This code is from the ES Wiki and I have left it as unmodified as I could. Take
it up with them!

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:204: function (aCallback) {
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Do the callbacks tend to receive an argument?
> 
> Is it worth doing an arity check?
> 
> Do we need to check that the argument is callable by untrusted code or is that
> not ES53-ish?
> 
> Is eval a valid callback?

No idea. This function is a hacked-up version of the one from Domita, removing
the Cajita/Valija elements; I suggest you compare them and tell me what this
function *should* be doing.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:226: * ability for one object to access its
the private state of other similar
On 2011/07/18 21:57:01, MikeSamuel wrote:
> "its the" -> "the"

Done.

> Is this sibling amplification similar to the way that an instance of a class C
> in java can access the privates of other instances of a class C?

Yes, exactly, replacing "class C" with everything that has the "p" member.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:282: guard.coerce(object);  // borrow
failure
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Does this always fail, or will this return undefined instead of an empty
object
> sometimes?

Always fails. A guard is fail-stop; returning undefined would be coercing to
undefined.

I'll add a backstop throw just for robustness/casual readers/static analyzers.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:348:
ExpandoProxyHandler.prototype.getOwnPropertyDescriptor = function (name) {
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Are these name parameters guaranteed to be coerced to strings or numbers
before
> reaching the proxy handler?

I would expect so. ES5 semantics have the property name in an ordinary foo[bar]
coerced to string well before any internal methods of foo are invoked. But the
final arbiter would have to be the semantics of proxies, which don't seem to be
written down on ES Wiki.

MarkM would be the person to consult for this.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:515: var TameXHRConf = new Confidence();
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Missing typename?

Done.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:520: TameXHRConf.confide(this);
On 2011/07/18 21:57:01, MikeSamuel wrote:
> If someone forgets a `new` then we might confide in the global object.  Maybe
a
> simple instanceof check at the top of constructors that confide in this would
> add a layer of protection.

This is not a risk in strict mode; 'this' will be undefined for function calls.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:521: var xhr = p(this).feral =
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Ok, so p(this) gets the private of this and that has to be done to the object
> that backs the proxy, not the proxy object.

In this specific case, yes. In the case of DOM nodes, we also confide in the
proxy (using the second argument to confide), so that if given a tame node we
don't care whether it's a proxy or not.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:530: }
On 2011/07/18 21:57:01, MikeSamuel wrote:
> This self idiom seems to occur several times with subtle variations.  Can we
> reduce the differences so someone who has to find and change uses of this
> idiom has an easier time operating on all of them?

Done. I've changed all "return proxy;" inside of if statements to use this "var
self; ... return self;" pattern instead. Now whenever a constructor may return
an object other than this, it is "return self;" at the end.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:652: var evt = { target: this };
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Do we use this, not the proxy because an event handler wrapper dispatches
> to the proxy if proxy self != this?

There aren't any event handler wrappers here (see the definition of
onreadystatechange above); this is just a bug.

In fact, I don't even have access to the proxy here! (No reference to it is
retained from this or p(this).) But, since TameXHR always creates (rather than
taking an existing) feral XHR, we don't actually need an expando proxy. I
therefore propose to just throw out the proxy logic here and let clients set
properties on this object (as we do on non-proxy-supporting environments for all
of our objects).

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:833: function
definePropertiesAwesomely(object, propDescs) {
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Is this part of Bill and Ted's excellent reflection?

Haven't seen it, can't say. The name just reflects the "Now we can just say
'someDomProperty: P_WHATEVER' and it's mapped without having to repeat
ourselves! Awesome!" aspect of this design. I also named this early on before I
knew that the *only* thing it would do is add the property name parameters.

> Does this threads the property name through to things like
> myNode.style.backgroundColor where the handler can be generic over all CSS
> property names if it just figure out which CSS property is being set?

You have the idea right, except that the style objects in particular happen not
to use this. It's mainly used on Element subclasses for passing through
properties which map to attributes (or don't).

(A later refactoring will be to have the style properties' accessors do less
runtime lookups, so they will actually close over the CSS property name
corresponding to the JS property, which can't be done here since the closure is
baked into dPA.)

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:850: return extGet.call(this, prop);
On 2011/07/18 21:57:01, MikeSamuel wrote:
> What is `this` here and at 855?

Accessors are invoked like methods on the object. Given that foo.bar is an
accessor (not data) property:

foo.bar = baz;
    ⇓
Object.getOwnPropertyDescriptor(foo, "bar").set.call(foo, baz);

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:892: return array.push.apply(array, rest);
On 2011/07/18 21:57:01, MikeSamuel wrote:
> This seems to be trusting array.push which is OK I guess if it is only called
> on listener lists we control, but if you don't need the negative index
handling,
> isn't it easier to just do array.splice(from, to - from) or the negative index
> handling equivalent : array.splice((from + array.length) % array.length, (to +
> array.length) % array.length - (from + array.length) % array.length)?

This code is directly from Domita; I prefer not to change it now.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:912: var h = String(htmlFragment || '');
On 2011/07/18 21:57:01, MikeSamuel wrote:
> If htmlFragment is 0 or NaN this might cause strangeness.  Probably a problem
> with the original code, so I don't think it's a huge problem, but maybe
> String(htmlFragment == null ? '' : htmlFragment) where the == is intentionally
> not ===.

Done.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:922: :
html.escapeAttrib(String(htmlFragment || ''));
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Same problem as above.

Done.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1061: * map is never used with more than
one version of setTimeout.
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Cool.

Talking to yourself? :) The properties described in that comment are all from
the original Domita code; all I did was refactor it so that setTimeout and
setInterval use the same code (and to use a WeakMap instead of a trademark and
timeoutId___ property, which has the same behavior as a result).

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1282: domicile.isProcessingEvent = false;
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Ok, so this is used to elevate permission when the user is interacting with an
> element.

If you say so. I haven't studied that component in detail, just ported it.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1286: }
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Maybe move this check to the top.  The closer it is to either succeeding or
> ding nothing to document and window, the better.

Done. I also added the function name to the error message, moved the init of
optPseudoWindowLocation next to it, and put isProcessingEvent in the initializer
for domicile, just to make things less scattered about.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1635: * predicate restricting the value)
have the value the original node
On 2011/07/18 21:57:01, MikeSamuel wrote:
> "and" before "have the value"?
> 
> Maybe make it clear that the predicate is not applied on read.

Done. I've reworded and updated all of the PropertyTaming docs to say "feral
object" instead of "original node" per the more general current use.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1665: extendedAccessors: true,
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Is this necessary?

No. What I wonder about is whether, if extendedAccessors were changed later to
have additional semantics, this would better be true. Probably this would need
to be changed anyway. Maybe 'extendedAccessors' should be replaced with a flag
specific to the "pass property names" semantics?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1781: makeDOMAccessible(node);
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Should this move below the sanity check below?

Done.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1836: makeDOMAccessible(node);
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Should this move below the sanity check and early out below?

Done.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2386:
TameBackedNode.prototype.dispatchEvent = nodeMethod(function dispatchEvent(evt)
{
Why is this function not anonymous? I just realized I've introduced a name
confusion with this vs. what used to be plugin_dispatchEvent___ below. I'm going
to rename the latter for clarity, but what is the reason for naming this
function? (And can we bury it in a (function() {})()?)

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2389: });
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Ok.  So bridal.dispatch event here gets the ferals, and is responsible for all
> the membrane maintenance.

This is Domita code. It appears to me that bridal.dispatchEvent works in ferals
and does not know anything about the membrane (nor does it need to, since it
touches no tame/guest objects).

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2507: get: nodeMethod(function () { return
np(this).geometryDelegate.clientWidth; })
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Can these be defined in terms of one awesome property getter that gets the
> property name?

Done.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3123: var construct = record.construct ||
function (self) { return self; };
On 2011/07/18 21:57:01, MikeSamuel wrote:
> || identity?

Done.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3927: src: NP.filter(false, identity, true,
identity)
On 2011/07/18 21:57:01, MikeSamuel wrote:
> How does the identity setter with an attr getter work here?  This turns into a
> call to setAttribute("src") which presumably invokes the URL policy?

This is a property *getter* and attribute *setter*, and this is copied from
Domita.

> Where is the policy code that prevents adding text nodes to a script element?

This was lost in refactoring; fixed.

I also found out that the test cases checking for this were disabled in r3802 by
you due to crashes on IE. I think we ought to re-enable these tests at least for
everything-but-IE; I may prepare a separate changeset for that.

I'm working on fixing the bugs discovered by enabling those tests, but it's
hairy enough I'm deferring that until after this round of itsy bitsy code review
changes.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5178: //'HTMLScriptElement',
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Why are these commented out?

Because they actually have classes, and the previous code wasn't checking for
conflicts so it was silently overwriting the non-default constructor with the
default.

I propose to change this so that it is a list of *all* HTML element classes, and
change the logic so it uses the default only if there is a non-default. Then 
the only place default/non-default is decided is in the defineElement uses
above. Good?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5267: //}
On 2011/07/18 21:57:01, MikeSamuel wrote:
> I do not know yet.  I think it is good to leave this catch block around though
> to guard against IE discarding finally blocks.

Can you provide the text of a comment explaining this?

Should we just keep this error reporting but lose the cajitaStack___ logic? I'm
not sure what cajitaStack___ means.

[MikeSamuel, 2011-07-21 18:16:16]

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:60: var PLACEHOLDER_theTools; //
TODO(kpreid): This is a placeholder for something to replace the pluginId
mechanism, making it into a global singleton instead. 'tools' is the replacement
for ___ properties on 'imports'.
On 2011/07/20 00:26:47, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > Isn't tools also akin to a view?  html-emitter accesses tools.document.
> 
> 'tools' has been renamed 'domicile'.
> 
> I don't know what you mean by "view" here. The View concept in DOM? If so,
that
> doesn't seem to say much. I suppose it could be seen as a
> bundle-of-authorities-plus-a-document as the browser Window is, but in this
case
> it contains both the (tame) document and window objects.

ok.  By "view" I meant
http://www.w3.org/TR/DOM-Level-2-Views/views.html#Views-AbstractView

http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:192: enumerable:
domitaModules.getPropertyDescriptor(object, propName)
On 2011/07/20 00:26:47, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > propName might be coerced to two different strings above if propName can
come
> > from an untrusted source.
> > Maybe do propName += ''; at the start.
> 
> That case shouldn't arise, but done, just for robustness.
> 
> > Does getPropertyDescriptor not return a nullish value when propName is
blank? 
> > Should properties default to non-enumerable if not overriding?
> 
> This is only ever used for overriding since the problem it is a workaround for
> cannot occur otherwise.
> 
> In the long run I should look into the ES5 semantics for whether this is a bug
> workaround or something that is actually needed.

k

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:204: function (aCallback) {
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > Do the callbacks tend to receive an argument?
> > 
> > Is it worth doing an arity check?
> > 
> > Do we need to check that the argument is callable by untrusted code or is
that
> > not ES53-ish?
> > 
> > Is eval a valid callback?
> 
> No idea. This function is a hacked-up version of the one from Domita, removing
> the Cajita/Valija elements; I suggest you compare them and tell me what this
> function *should* be doing.

I'll chat with Mike Stay.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:348:
ExpandoProxyHandler.prototype.getOwnPropertyDescriptor = function (name) {
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > Are these name parameters guaranteed to be coerced to strings or numbers
> before
> > reaching the proxy handler?
> 
> I would expect so. ES5 semantics have the property name in an ordinary
foo[bar]
> coerced to string well before any internal methods of foo are invoked. But the
> final arbiter would have to be the semantics of proxies, which don't seem to
be
> written down on ES Wiki.
> 
> MarkM would be the person to consult for this.

Mark, is that correct?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:520: TameXHRConf.confide(this);
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > If someone forgets a `new` then we might confide in the global object. 
Maybe
> a
> > simple instanceof check at the top of constructors that confide in this
would
> > add a layer of protection.
> 
> This is not a risk in strict mode; 'this' will be undefined for function
calls.

I'm confused.  Is domado.js running inside ES5/3 on an ES3 interpreter?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:530: }
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > This self idiom seems to occur several times with subtle variations.  Can we
> > reduce the differences so someone who has to find and change uses of this
> > idiom has an easier time operating on all of them?
> 
> Done. I've changed all "return proxy;" inside of if statements to use this
"var
> self; ... return self;" pattern instead. Now whenever a constructor may return
> an object other than this, it is "return self;" at the end.

thx

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:652: var evt = { target: this };
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > Do we use this, not the proxy because an event handler wrapper dispatches
> > to the proxy if proxy self != this?
> 
> There aren't any event handler wrappers here (see the definition of
> onreadystatechange above); this is just a bug.
> 
> In fact, I don't even have access to the proxy here! (No reference to it is
> retained from this or p(this).) But, since TameXHR always creates (rather than
> taking an existing) feral XHR, we don't actually need an expando proxy. I
> therefore propose to just throw out the proxy logic here and let clients set
> properties on this object (as we do on non-proxy-supporting environments for
all
> of our objects).

That would simplify a lot of the code above?

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:892: return array.push.apply(array, rest);
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > This seems to be trusting array.push which is OK I guess if it is only
called
> > on listener lists we control, but if you don't need the negative index
> handling,
> > isn't it easier to just do array.splice(from, to - from) or the negative
index
> > handling equivalent : array.splice((from + array.length) % array.length, (to
+
> > array.length) % array.length - (from + array.length) % array.length)?
> 
> This code is directly from Domita; I prefer not to change it now.

k

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1665: extendedAccessors: true,
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > Is this necessary?
> 
> No. What I wonder about is whether, if extendedAccessors were changed later to
> have additional semantics, this would better be true. Probably this would need
> to be changed anyway. Maybe 'extendedAccessors' should be replaced with a flag
> specific to the "pass property names" semantics?

Future planning for many flavors of accessors without knowing how often one
extended feature predicts the need of other features is problematic.

I could go either way on the name as long as it's something easily auditable --
a simple grep should pull out all uses to it and not pull out other stuff in
non-malicious code.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2386:
TameBackedNode.prototype.dispatchEvent = nodeMethod(function dispatchEvent(evt)
{
On 2011/07/20 00:26:48, kpreid2 wrote:
> Why is this function not anonymous? I just realized I've introduced a name
> confusion with this vs. what used to be plugin_dispatchEvent___ below. I'm
going
> to rename the latter for clarity, but what is the reason for naming this
> function? (And can we bury it in a (function() {})()?)

The link above,
http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventTarget , defines
the dispatchEvent method as part of the EventTarget interface which HTMLElements
implement.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3927: src: NP.filter(false, identity, true,
identity)
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > How does the identity setter with an attr getter work here?  This turns into
a
> > call to setAttribute("src") which presumably invokes the URL policy?
> 
> This is a property *getter* and attribute *setter*, and this is copied from
> Domita.
> 
> > Where is the policy code that prevents adding text nodes to a script
element?
> 
> This was lost in refactoring; fixed.
> 
> I also found out that the test cases checking for this were disabled in r3802
by
> you due to crashes on IE. I think we ought to re-enable these tests at least
for
> everything-but-IE; I may prepare a separate changeset for that.

Ack.  Definitely.

> I'm working on fixing the bugs discovered by enabling those tests, but it's
> hairy enough I'm deferring that until after this round of itsy bitsy code
review
> changes.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5178: //'HTMLScriptElement',
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > Why are these commented out?
> 
> Because they actually have classes, and the previous code wasn't checking for
> conflicts so it was silently overwriting the non-default constructor with the
> default.
> 
> I propose to change this so that it is a list of *all* HTML element classes,
and
> change the logic so it uses the default only if there is a non-default. Then 
> the only place default/non-default is decided is in the defineElement uses
> above. Good?

Please do.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5267: //}
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > I do not know yet.  I think it is good to leave this catch block around
though
> > to guard against IE discarding finally blocks.
> 
> Can you provide the text of a comment explaining this?
> 
> Should we just keep this error reporting but lose the cajitaStack___ logic?
I'm
> not sure what cajitaStack___ means.

Actually, it looks like cajitaStack___ is only mentioned in domita.js so I think
this code is definitely obsolete.

It used to be hooked into by an extra layer of rewriting that maintained a call
stack mapping back to original line numbers but that's dead now.

[kpreid2, 2011-07-21 19:31:05]

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:520: TameXHRConf.confide(this);
On 2011/07/21 18:16:17, MikeSamuel wrote:
> I'm confused.  Is domado.js running inside ES5/3 on an ES3 interpreter?

Yes. Or inside SES on an ES5 interpreter. Domado fundamentally requires ES5.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:652: var evt = { target: this };
On 2011/07/21 18:16:17, MikeSamuel wrote:
> On 2011/07/20 00:26:48, kpreid2 wrote:
> > I therefore propose to just throw out the proxy logic here and let clients
> > set properties on this object (as we do on non-proxy-supporting environments
> > for all of our objects).

Done.

> That would simplify a lot of the code above?

Not especially; it would just mean dropping the proxy creation from the
constructor.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2386:
TameBackedNode.prototype.dispatchEvent = nodeMethod(function dispatchEvent(evt)
{
On 2011/07/21 18:16:17, MikeSamuel wrote:
> On 2011/07/20 00:26:48, kpreid2 wrote:
> > Why is this function not anonymous? I just realized I've introduced a name
> > confusion with this vs. what used to be plugin_dispatchEvent___ below. I'm
> going
> > to rename the latter for clarity, but what is the reason for naming this
> > function? (And can we bury it in a (function() {})()?)
> 
> The link above,
> http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventTarget ,
defines
> the dispatchEvent method as part of the EventTarget interface which
HTMLElements
> implement.

I agree that this is a method named dispatchEvent. I'm asking why the function
expression is not anonymous (function(evt) ...) as most other method
declarations are.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5178: //'HTMLScriptElement',
On 2011/07/21 18:16:17, MikeSamuel wrote:
> On 2011/07/20 00:26:48, kpreid2 wrote:
> > On 2011/07/18 21:57:01, MikeSamuel wrote:
> > > Why are these commented out?
> > 
> > Because they actually have classes, and the previous code wasn't checking
for
> > conflicts so it was silently overwriting the non-default constructor with
the
> > default.
> > 
> > I propose to change this so that it is a list of *all* HTML element classes,
> and
> > change the logic so it uses the default only if there is a non-default. Then

> > the only place default/non-default is decided is in the defineElement uses
> > above. Good?
> 
> Please do.

Done.

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:5267: //}
On 2011/07/21 18:16:17, MikeSamuel wrote:
> Actually, it looks like cajitaStack___ is only mentioned in domita.js so I
think
> this code is definitely obsolete.

I have deleted everything but the catch-and-throw with a comment that it's for
IE.

[MikeSamuel, 2011-07-21 19:54:46]

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:1061: * map is never used with more than
one version of setTimeout.
On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > Cool.
> 
> Talking to yourself? :) The properties described in that comment are all from
> the original Domita code; all I did was refactor it so that setTimeout and
> setInterval use the same code (and to use a WeakMap instead of a trademark and
> timeoutId___ property, which has the same behavior as a result).

It's only a sign of madness if I answer myself.



D'OH!

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2386:
TameBackedNode.prototype.dispatchEvent = nodeMethod(function dispatchEvent(evt)
{
On 2011/07/21 19:31:05, kpreid2 wrote:
> On 2011/07/21 18:16:17, MikeSamuel wrote:
> > On 2011/07/20 00:26:48, kpreid2 wrote:
> > > Why is this function not anonymous? I just realized I've introduced a name
> > > confusion with this vs. what used to be plugin_dispatchEvent___ below. I'm
> > going
> > > to rename the latter for clarity, but what is the reason for naming this
> > > function? (And can we bury it in a (function() {})()?)
> > 
> > The link above,
> > http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventTarget ,
> defines
> > the dispatchEvent method as part of the EventTarget interface which
> HTMLElements
> > implement.
> 
> I agree that this is a method named dispatchEvent. I'm asking why the function
> expression is not anonymous (function(evt) ...) as most other method
> declarations are.

No reasons.  I may have added it during debugging so that it shows up in stack
traces.

http://codereview.appspot.com/4535118/diff/48001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/48001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:912: var h = String(htmlFragment == null ?
'' : htmlFragment);
Maybe add a comment that the == is intentional.
Otherwise people are liable to "correct" it to ===.

http://codereview.appspot.com/4535118/diff/48001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:922: :
html.escapeAttrib(String(htmlFragment == null ? '' : htmlFragment));
ditto.

[kpreid2, 2011-07-21 20:21:46]

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2386:
TameBackedNode.prototype.dispatchEvent = nodeMethod(function dispatchEvent(evt)
{
On 2011/07/21 19:54:46, MikeSamuel wrote:
> No reasons.  I may have added it during debugging so that it shows up in
> stack traces.

Removed.

http://codereview.appspot.com/4535118/diff/48001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/48001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:912: var h = String(htmlFragment == null ?
'' : htmlFragment);
On 2011/07/21 19:54:46, MikeSamuel wrote:
> Maybe add a comment that the == is intentional.
> Otherwise people are liable to "correct" it to ===.

Done.

http://codereview.appspot.com/4535118/diff/48001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:922: :
html.escapeAttrib(String(htmlFragment == null ? '' : htmlFragment));
On 2011/07/21 19:54:46, MikeSamuel wrote:
> ditto.

Done.

[Mark S. Miller, 2011-07-21 20:47:07]

[+tvcutsem]

On Thu, Jul 21, 2011 at 11:16 AM, <mikesamuel@gmail.com> wrote:

>
> http://codereview.appspot.com/**4535118/diff/44001/src/com/**
>
google/caja/plugin/domado.js#**newcode348<http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode348>
> src/com/google/caja/plugin/**domado.js:348:
> ExpandoProxyHandler.prototype.**getOwnPropertyDescriptor = function
> (name) {
> On 2011/07/20 00:26:48, kpreid2 wrote:
>
>> On 2011/07/18 21:57:01, MikeSamuel wrote:
>> > Are these name parameters guaranteed to be coerced to strings or numbers
>
> > before reaching the proxy handler?
>
>
>  I would expect so. ES5 semantics have the property name in an
>> ordinary foo[bar]
>
>  coerced to string well before any internal methods of foo are invoked. But
>> the
>
>  final arbiter would have to be the semantics of proxies, which don't seem
>> to be
>
>  written down on ES Wiki.
>>
>
>  MarkM would be the person to consult for this.
>>
>
> Mark, is that correct?


It's correct. I suspect it is captured somewhere on the proxies semantics
pages, but will check later. We have talked about changing it and rejected
that plan, at least for now, which is why I'm fairly confident that it's
always a string.

cc'ed Tom. Tom?

(Tom is on vacation and hard to reach right now, so don't be surprised by
lack of answer.)


-- 
    Cheers,
    --MarkM

[MikeSamuel, 2011-07-25 15:38:47]

I looked over the test changes again.  I'm pretty happy where this is and think
we should try to get it in before packing more into this CL.  Let's chat about
how to go about committing this in a way that lets us push stable patches for
existing clients given that there will inevitably by some fallout that needs to
settle.

So, LGTM, tentatively pending discussions at today's meeting.

http://codereview.appspot.com/4535118/diff/54036/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domita-dom-guest.html (right):

http://codereview.appspot.com/4535118/diff/54036/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domita-dom-guest.html:410: //
TODO(kpreid): The above remark may be obsolete.
It is.  We have property names in the schema.  The only part that looks
problematic is the {assertEquals(void 0, d.foo);} after {d.setAttribute('foo',
'hi');} so maybe change this to a TODO to look into that.

http://codereview.appspot.com/4535118/diff/54036/third_party/js/jqueryjs/runt...
File third_party/js/jqueryjs/runtest/env.js (right):

http://codereview.appspot.com/4535118/diff/54036/third_party/js/jqueryjs/runt...
third_party/js/jqueryjs/runtest/env.js:228: },
Yeah.  I don't know why that was that way.  HTML5 has wisely just redefined
defaultView to be a Window instead of some amorphous view thingy.

[kpreid2, 2011-07-26 01:15:46]

The latest snapshot also includes the <script> security fixes and adjustments
for compatibility with ES5/3's proxy implementation. In particular, the 'return
self;' pattern that you mentioned before has been entirely replaced and
finishNode now does a lot more work. However, this code is not tested as there
are problems — which *seem* like bugs in ES5/3 — preventing Domado from working
with proxies under ES5/3; I'm going to consult Mike Stay on this ASAP.

http://codereview.appspot.com/4535118/diff/54036/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domita-dom-guest.html (right):

http://codereview.appspot.com/4535118/diff/54036/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domita-dom-guest.html:410: //
TODO(kpreid): The above remark may be obsolete.
On 2011/07/25 15:38:47, MikeSamuel wrote:
> It is.  We have property names in the schema.

Deleted.

> The only part that looks
> problematic is the {assertEquals(void 0, d.foo);} after {d.setAttribute('foo',
> 'hi');} so maybe change this to a TODO to look into that.

I've added a TODO(mikesamuel) for that. :)

[MikeSamuel, 2011-07-26 12:55:41]

LGTM

[MikeSamuel, 2011-07-29 00:26:24]

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:66: // autoswitches as needed, remapping to
the (name[, value], proxy) order.
Can we not change to be consistent?

If so, maybe add a TODO for whomever is likely to fix that.

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:88: var result = "can't see this";
Why is this initial value here?

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:104: return result;
Maybe structure it like this
    var needToSwap = false;
    if (domitaModules.proxiesAvailable) {
      // feature test to determine needToSwap
    }
    if (needToSwap) {
      return <inline value of swapper here>
    } else {
      return <inline value of straight here>
    }
instead of creating twice as many objects as needed even in the case where
!domitaModules.proxiesAvailable.

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2281: traceStartup("DT: about to set
toString for TameNode");
Is this meant to be submitted?

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4610: throw new Error('document.write not
provided for this document');
write -> writeln

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4613: domicile.writeHook.call(undefined,
'');
What is this extra call supposed to do?

Can't this just become

   var args = Array.slice.call(arguments, 0);
   args.push("\n");
   domicile.writeHook.apply(undefined, args);

?

[kpreid2, 2011-07-29 00:41:56]

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:66: // autoswitches as needed, remapping to
the (name[, value], proxy) order.
On 2011/07/29 00:26:38, MikeSamuel wrote:
> Can we not change to be consistent?
> 
> If so, maybe add a TODO for whomever is likely to fix that.

My understanding is that Firefox is the one expected to change; else we would
not have done it differently. Stay or MarkM can comment further.

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:88: var result = "can't see this";
On 2011/07/29 00:26:38, MikeSamuel wrote:
> Why is this initial value here?

Because the linter complained about a possibly uninitialized value. I've added a
comment.

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:104: return result;
On 2011/07/29 00:26:38, MikeSamuel wrote:
> Maybe structure it like this
>     var needToSwap = false;
>     if (domitaModules.proxiesAvailable) {
>       // feature test to determine needToSwap
>     }
>     if (needToSwap) {
>       return <inline value of swapper here>
>     } else {
>       return <inline value of straight here>
>     }
> instead of creating twice as many objects as needed even in the case where
> !domitaModules.proxiesAvailable.

Done.

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2281: traceStartup("DT: about to set
toString for TameNode");
On 2011/07/29 00:26:38, MikeSamuel wrote:
> Is this meant to be submitted?

traceStartup is statically a no-op above. I have found these debugging lines
sufficiently repeatedly useful for testing on Firefox (where Firebug gives
misleading stack traces for errors during Domado startup) that I would rather
not have to re-add them in the future, but if you think they are too unsightly I
would go ahead and do so.

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4610: throw new Error('document.write not
provided for this document');
On 2011/07/29 00:26:38, MikeSamuel wrote:
> write -> writeln

Done.

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4613: domicile.writeHook.call(undefined,
'');
On 2011/07/29 00:26:38, MikeSamuel wrote:
> What is this extra call supposed to do?
> 
> Can't this just become
> 
>    var args = Array.slice.call(arguments, 0);
>    args.push("\n");
>    domicile.writeHook.apply(undefined, args);
> 
> ?

The extra call was supposed to have a '\n'. The intent was to avoid copying the
arguments array.

[Mark S. Miller, 2011-07-29 01:46:33]

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:66: // autoswitches as needed, remapping to
the (name[, value], proxy) order.
On 2011/07/29 00:41:56, kpreid2 wrote:
> On 2011/07/29 00:26:38, MikeSamuel wrote:
> > Can we not change to be consistent?
> > 
> > If so, maybe add a TODO for whomever is likely to fix that.
> 
> My understanding is that Firefox is the one expected to change; else we would
> not have done it differently. Stay or MarkM can comment further.

That's correct. Or it was before the end-of-July meeting which just concluded.
Tom was on vacation, so I presented to proposal to add proxy as a last argument
to all traps. However, we were all able to generate reasons why we didn't need
to. I suspect we're forgetting something, so we're postponing this until the
next meeting when Tom will be there.

However, the proposal to drop the receiver from the get/set traps was accepted,
so the basic swap logic here is sound if you leave out the "c === proxy"
conjunct.

[MikeSamuel, 2011-07-29 17:56:13]

As discussed verbally, will punt on fleshing out writeln testcases to another
CL.

LGTM.

[kpreid2, 2011-07-29 18:27:20]

Committed as r4576.

[tvcutsem_vub.ac.be, 2011-08-11 11:20:56]

> [+tvcutsem]
> 
> On Thu, Jul 21, 2011 at 11:16 AM, <mikesamuel@gmail.com> wrote:
> 
>
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/d...
> src/com/google/caja/plugin/domado.js:348:
> ExpandoProxyHandler.prototype.getOwnPropertyDescriptor = function (name) {
> On 2011/07/20 00:26:48, kpreid2 wrote:
> On 2011/07/18 21:57:01, MikeSamuel wrote:
> > Are these name parameters guaranteed to be coerced to strings or numbers
> > before reaching the proxy handler?
> 
> I would expect so. ES5 semantics have the property name in an ordinary
foo[bar]
> coerced to string well before any internal methods of foo are invoked. But the
> final arbiter would have to be the semantics of proxies, which don't seem to
be
> written down on ES Wiki.
> 
> MarkM would be the person to consult for this.
> 
> Mark, is that correct?
> 
> It's correct. I suspect it is captured somewhere on the proxies semantics
pages, but will check later. We have talked about changing it and rejected that
plan, at least for now, which is why I'm fairly confident that it's always a
string.
> 
> cc'ed Tom. Tom?

Yes, name parameters are coerced to strings (not numbers) before the trap is
invoked. It's implicit in the Proxies spec, by means of the [[Get]] built-in
method, which can only take a property name as an argument.

Details at
<http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics#detailed_sem...>

Kind regards,
Tom