Mitigate top-level vars and typeof differences between ES5 and ES5/3 with rewriting (issue 6827077)

[jas...@gmail.com]

Reviewers: metaweta,

Description:
Uses the acorn parser and escodegen serializer to rewrite:
* top level var decls to assignments on the global object
* typeof of undeclared variables to return the string "undefined"

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

Affected files:
M build.xml
M src/com/google/caja/ses/hookupSES.js
M src/com/google/caja/ses/hookupSESPlus.js
A src/com/google/caja/ses/mitigateGotchas.js
M src/com/google/caja/ses/startSES.js
M tests/com/google/caja/plugin/GeneralBrowserTest.java
A tests/com/google/caja/plugin/es53-test-gotchas-guest.html
M tests/com/google/caja/plugin/test-index.js
A third_party/js/acorn/acorn.js
A third_party/js/escodegen/escodegen.js
A third_party/js/escodegen/estraverse.js

[eri...@gmail.com]

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/startSES.js
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/startSES.js#newcode646
src/com/google/caja/ses/startSES.js:646: exprSrc =
mitigateGotchas(exprSrc);
Here, you're mitigating gotchas on exprSrc *after* the old exprSrc has
been used to create wrapperSrc. It is the unmitigated wrapperSrc which
is evaluated. The mitigated exprSrc is used only to calculate the
freeVarNames. Is this what you intend?

Note that your compileModule mitigates the input *before* it is wrapped.
This looks more correct to me.

https://codereview.appspot.com/6827077/

[eri...@gmail.com]

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/startSES.js
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/startSES.js#newcode633
src/com/google/caja/ses/startSES.js:633: * analogous {@code
compileProgram} function that accepts a
Now that we're parsing and transforming, can we get rid of the
artificial compileExpr vs compileModule kludge and provide just
compileProgram? It shouldn't matter for this purpose that the
parser/transformer is outside the TCB.

https://codereview.appspot.com/6827077/

[eri...@gmail.com]

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/mitigateGotchas.js
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/mitigateGotchas.js#newcode18
src/com/google/caja/ses/mitigateGotchas.js:18: * outside the TCB.
Please put in a "see" link to
http://code.google.com/p/google-caja/wiki/SES#Source-SES_vs_Target-SES .
How do the mitigations in this CL relate to the mitigations explained
there? Can we put in a TODO for the mitigations explained there but not
yet implemented here?

Do we know of any other needed mitigations besides those explained
there?

https://codereview.appspot.com/6827077/

[kpreid.sw...@gmail.com]

https://codereview.appspot.com/6827077/diff/1/build.xml
File build.xml (right):

https://codereview.appspot.com/6827077/diff/1/build.xml#newcode990
build.xml:990: <input file="${third_party}/js/escodegen/escodegen.js"
jslint="false"/>
Since these third-party files support the 'exports' pattern, we should
use it in order to avoid crufting up the global environment. Place files
before and after this group which create an 'exports' object (saving the
old variable), let them load, then transfer the exports into the 'ses'
module and restore the original 'exports' if any.

https://codereview.appspot.com/6827077/diff/1/build.xml#newcode1016
build.xml:1016: <input file="${src.caja}/ses/mitigateGotchas.js"/>
Ditto.

https://codereview.appspot.com/6827077/diff/1/tests/com/google/caja/plugin/es53-test-gotchas-guest.html
File tests/com/google/caja/plugin/es53-test-gotchas-guest.html (right):

https://codereview.appspot.com/6827077/diff/1/tests/com/google/caja/plugin/es53-test-gotchas-guest.html#newcode17
tests/com/google/caja/plugin/es53-test-gotchas-guest.html:17: <div
class="testcontainer" id="testTopLevel"
There are already some tests for "assignments show up on window which is
the global environment" (disabled in ES5 mode) in
es53-test-domado-dom-guest.html. We shouldn't have tests in two places.

Perhaps these tests and those should be moved to
es53-test-language-guest.html?

https://codereview.appspot.com/6827077/diff/1/tests/com/google/caja/plugin/es53-test-gotchas-guest.html#newcode36
tests/com/google/caja/plugin/es53-test-gotchas-guest.html:36: debugger;
cruft

https://codereview.appspot.com/6827077/diff/1/tests/com/google/caja/plugin/es53-test-gotchas-guest.html#newcode53
tests/com/google/caja/plugin/es53-test-gotchas-guest.html:53:
jsunitRegisterIf(inES5Mode,
no testcontainer to match this test

https://codereview.appspot.com/6827077/

[meta...@gmail.com]

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/mitigateGotchas.js
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/mitigateGotchas.js#newcode20
src/com/google/caja/ses/mitigateGotchas.js:20: * Note the parse tree
manipulate in this file uses the SpiderMonkey
That sentence doesn't parse.

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/mitigateGotchas.js#newcode48
src/com/google/caja/ses/mitigateGotchas.js:48: * window.x = window.x,
window.y = 2, window.z = window.z
Under what conditions do we throw ReferenceErrors after such a
translation?

https://codereview.appspot.com/6827077/diff/1/src/com/google/caja/ses/mitigateGotchas.js#newcode81
src/com/google/caja/ses/mitigateGotchas.js:81: * (function() { try {
typeof x } catch (e) { return "undefined" } })()
typeof (function(){throw Error()})(); // Should be rethrown

typeof (function(){throw ReferenceError()})(); // Should be rethrown,
but is unavoidably eaten; document as gotcha

Perhaps you only want to rewrite the case where the argument to typeof
is a bare variable?

https://codereview.appspot.com/6827077/