http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode60
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/domado.js#newcode182
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/domado.js#newcode192
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/domado.js#newcode320
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/domado.js#newcode330
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/domado.js
File src/com/google/caja/plugin/domado.js (right):
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode50
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/domado.js#newcode160
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/domado.js#newcode204
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/domado.js#newcode226
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/domado.js#newcode282
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/domado.js#newcode348
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/domado.js#newcode515
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/domado.js#newcode520
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/domado.js#newcode521
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/domado.js#newcode530
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/domado.js#newcode652
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/domado.js#newcode833
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/domado.js#newcode850
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/domado.js#newcode892
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/domado.js#newcode912
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/domado.js#newcode922
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/domado.js#newcode1061
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/domado.js#newcode1282
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/domado.js#newcode1286
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/domado.js#newcode1635
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/domado.js#newcode1665
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/domado.js#newcode1781
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/domado.js#newcode1836
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/domado.js#newcode2386
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/domado.js#newcode2389
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/domado.js#newcode2507
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/domado.js#newcode3123
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/domado.js#newcode3927
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/domado.js#newcode5178
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/domado.js#newcode5267
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.
http://codereview.appspot.com/4535118/
