AMDLoader in SES and other SES refactoring (issue 5648043)

1 view
Skip to first unread message

jas...@gmail.com

unread,
Feb 9, 2012, 2:17:27 AM2/9/12
to fel...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com
Reviewers: felix8a,

Description:
Taking over from erights CL

* refactored startSES.js to separate wrapping from compilation, enabling
other
ways to recombine them, like script injection (see below).

* compileExprLater.js -- Implemented a compileExprLater abstraction,
which is
like cajaVM.compileExpr, except that it returns a promise which
eventually becomes the compiled expr function. This abstraction enables
future
interoperation with ES5/3, where server-side translation
requires an asynchrony barrier. The compileExprLater, if it finds itself
in a
browser environment, uses script injection rather than "eval",
avoiding yet more stratification bugs in Chrome's debugger.

* refactored explicit.html to use compileExprLater, to load
makeSimpleAMDLoader
outside the SES TCB. It is therefore now also multiply
instantiable and debuggable.

* refactored makeSimpleAMDLoader.js to make use of compileExprLater to
load AMD
modules, making them also debuggable under the
Chrome debugger.

* repairES5.js and startSES.js -- The big additional feature in this CL
is to
"support override by assignment in SES", enabling SES to accept
far more legacy best practice JS code.

* repairES5.js -- More repairs

* debug.js -- A start on being able to report stack traces for errors
while
still denying untrusted code access to these errors, all without
translation. All platforms should be safe, but the stack capture works
only on
Chrome.

* refactored startSES.js so all freezing of primordials is gathered
together, as
a step toward supporting confined SES (sacrificing
defensiveness in exchange for greater legacy compatibility).

Please review this at http://codereview.appspot.com/5648043/

Affected files:
M build.xml
M src/com/google/caja/ses/StringMap.js
M src/com/google/caja/ses/WeakMap.js
M src/com/google/caja/ses/amdTest.js
M src/com/google/caja/ses/amdTest1.js
M src/com/google/caja/ses/amdTest3.js
M src/com/google/caja/ses/amdTestDir/amdTest2.js
M src/com/google/caja/ses/atLeastFreeVarNames.js
A src/com/google/caja/ses/compileExprLater.js
A src/com/google/caja/ses/debug.js
M src/com/google/caja/ses/detect.js
M src/com/google/caja/ses/ejectorsGuardsTrademarks.js
M src/com/google/caja/ses/explicit.html
M src/com/google/caja/ses/hookupSES.js
M src/com/google/caja/ses/hookupSESPlus.js
M src/com/google/caja/ses/logger.js
M src/com/google/caja/ses/makeFarResourceMaker.js
M src/com/google/caja/ses/makeQ.js
M src/com/google/caja/ses/makeSimpleAMDLoader.js
M src/com/google/caja/ses/repairES5.js
M src/com/google/caja/ses/startSES.js
M src/com/google/caja/ses/useHTMLLogger.js
M src/com/google/caja/ses/whitelist.js


jas...@gmail.com

unread,
Feb 9, 2012, 3:50:49 PM2/9/12
to fel...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/compileExprLater.js
File src/com/google/caja/ses/compileExprLater.js (right):

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/compileExprLater.js#newcode105
src/com/google/caja/ses/compileExprLater.js:105: scriptSrc = '/* from '
+ opt_sourceUrl + ' */ ' + scriptSrc;
Note to self. This isn't sufficient to escape opt_sourceUrl.

http://codereview.appspot.com/5648043/

fel...@gmail.com

unread,
Feb 11, 2012, 3:36:58 AM2/11/12
to jas...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com
a few nits but otherwise lgtm


http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/debug.js
File src/com/google/caja/ses/debug.js (right):

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/debug.js#newcode44
src/com/google/caja/ses/debug.js:44: * make it otheriwse unreachable.
Replace it with a reachable
typo "otheriwse"

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js
File src/com/google/caja/ses/repairES5.js (right):

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode356
src/com/google/caja/ses/repairES5.js:356: * unifortunately specified
that a simple assignment to a
typo "unifortunately"

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode363
src/com/google/caja/ses/repairES5.js:363: * best practices, of this
previous code used assignment to
"of" should be "if"?

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode368
src/com/google/caja/ses/repairES5.js:368: * "constructor" is a function
whose "prototype" is this obj. If so,
this definition of "prototypical", while precise, is not very helpful.
it took me a while to figure out an example of a prototypical object.
how about mention something like: Object.prototype and
Function.prototype are prototypical.

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode475
src/com/google/caja/ses/repairES5.js:475: *
<p>"makeDelayedTamperProof()" must only be called once.
how about throw an error if it's called more than once?

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode1642
src/com/google/caja/ses/repairES5.js:1642: * Do Error instances on thos
platform carry own properties that we
typo "thos"

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/startSES.js
File src/com/google/caja/ses/startSES.js (right):

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/startSES.js#newcode258
src/com/google/caja/ses/startSES.js:258: func.prototype = null;
what's the rationale for setting prototype to null?

http://codereview.appspot.com/5648043/

eri...@gmail.com

unread,
Feb 11, 2012, 12:09:40 PM2/11/12
to jas...@gmail.com, fel...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/startSES.js#newcode258
src/com/google/caja/ses/startSES.js:258: func.prototype = null;

On 2012/02/11 08:36:58, felix8a wrote:
> what's the rationale for setting prototype to null?

Only to avoid the cost of freezing an empty prototype object, as well as
pointlessly keeping around that unique empty prototype object. This
doesn't prevent the function from being used as a constructor, and in
fact has the same effect as setting its .prototype to Object.prototype.
But this representation is closer to the representation of the built-in
non-constructor functions in Ch15 which have no .prototype or
[[Construct]] internal method at all, better indicating intent.

http://codereview.appspot.com/5648043/

jas...@gmail.com

unread,
Feb 11, 2012, 6:48:11 PM2/11/12
to fel...@gmail.com, eri...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/debug.js#newcode44
src/com/google/caja/ses/debug.js:44: * make it otheriwse unreachable.
Replace it with a reachable

On 2012/02/11 08:36:58, felix8a wrote:

> typo "otheriwse"

Done.

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode356
src/com/google/caja/ses/repairES5.js:356: * unifortunately specified
that a simple assignment to a

On 2012/02/11 08:36:58, felix8a wrote:

> typo "unifortunately"

Done.

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode363
src/com/google/caja/ses/repairES5.js:363: * best practices, of this
previous code used assignment to

On 2012/02/11 08:36:58, felix8a wrote:

> "of" should be "if"?

Done.

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode368
src/com/google/caja/ses/repairES5.js:368: * "constructor" is a function
whose "prototype" is this obj. If so,

On 2012/02/11 08:36:58, felix8a wrote:

> this definition of "prototypical", while precise, is not very helpful.
it took
> me a while to figure out an example of a prototypical object. how
about mention
> something like: Object.prototype and Function.prototype are
prototypical.

Done.

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode475
src/com/google/caja/ses/repairES5.js:475: *
<p>"makeDelayedTamperProof()" must only be called once.

On 2012/02/11 08:36:58, felix8a wrote:

> how about throw an error if it's called more than once?

Done.

http://codereview.appspot.com/5648043/diff/3001/src/com/google/caja/ses/repairES5.js#newcode1642
src/com/google/caja/ses/repairES5.js:1642: * Do Error instances on thos
platform carry own properties that we

On 2012/02/11 08:36:58, felix8a wrote:

> typo "thos"

Done.

http://codereview.appspot.com/5648043/

Reply all
Reply to author
Forward
0 new messages