Testing, fixing, and working around recent JS and browser bugs.

Testing, fixing, and working around recent JS and browser bugs.

[Mark S. Miller, 2013-03-04 05:22:59]

You can try it out at
https://es-lab.googlecode.com/svn/trunk/src/ses/explicit.html
This works on Chrome Version 26.0.1410.19 beta on Linux but not yet on Chrome
Version 27.0.1428.0 canary because it doesn't yet repair
https://code.google.com/p/v8/issues/detail?id=2565


On Sun, Mar 3, 2013 at 9:07 PM, <erights@gmail.com> wrote:

> Reviewers: Jasvir Nagra,
>
> Description:
> Testing, fixing, and working around recent JS and browser bugs.
>
> Please review this at
https://codereview.appspot.**com/7433048/<https://codereview.appspot.com/7433...
>
> Affected files:
>   M     src/com/google/caja/ses/**StringMap.js
>   M     src/com/google/caja/ses/**WeakMap.js
>   M     src/com/google/caja/ses/**atLeastFreeVarNames.js
>   M     src/com/google/caja/ses/**compileExprLater.js
>   M     src/com/google/caja/ses/debug.**js
>   M     src/com/google/caja/ses/**explicit.html
>   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
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "Google Caja Discuss" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
google-caja-discuss+**unsubscribe@googlegroups.com<google-caja-discuss%2Bunsu...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>


-- 
    Cheers,
    --MarkM

[kpreid2, 2013-03-04 17:38:38]

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/StringM...
File src/com/google/caja/ses/StringMap.js (right):

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/StringM...
src/com/google/caja/ses/StringMap.js:37: throw new TypeError('Not a string: ' +
x);
For my information, is this an improvement/bugfix or is it simply removing
redundancy?

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/atLeast...
File src/com/google/caja/ses/atLeastFreeVarNames.js (right):

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/atLeast...
src/com/google/caja/ses/atLeastFreeVarNames.js:127: programSrc = ''+programSrc;
Similar question. In this case I find String(foo) more aesthetic than ''+foo; is
there a strong reason to prefer the latter?

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

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/debug.j...
src/com/google/caja/ses/debug.js:83: ses.logger.error('Error while initializing
debug module', err);
Maybe comment here that we expect the following call to fail, or explicitly
throw here?

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/repairE...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:2802: function F(){}
F could be moved out of the function so we reuse it rather than constructing a
new F each call. I don't know whether that would be a performance improvement.

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

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:409: var MAX_NAT = Math.pow(2,52); // Is
this right?
No, you want Math.pow(2, 53).

The maximum exact integer is 2^53, at which point we've just hit the exponent
region where the next representable number is 2^53+2.

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:412: throw new RangeError("not a number");
single quotes, here and below

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:414: if (allegedNum !== allegedNum) { throw
new RangeError("NaN not natural"); }
Why not use isFinite?

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:470: verifyStrictProgram('(( ' + exprSrc +
'\n));');
How does this help?

[MarkM, 2013-03-04 19:15:30]

More responses tonight or tomorrow.

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/StringM...
File src/com/google/caja/ses/StringMap.js (right):

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/StringM...
src/com/google/caja/ses/StringMap.js:37: throw new TypeError('Not a string: ' +
x);
On 2013/03/04 17:38:38, kpreid2 wrote:
> For my information, is this an improvement/bugfix or is it simply removing
> redundancy?

More of a stylistic consistency issue than a redundancy issue, but both in this
case.

IIRC, in ES5/3 and SES we found that String(obj) was significantly slower than
(''+obj), so we decided to use the latter even in cases where the ES5 spec
requires the former. In other cases as above, where it is not a spec conformance
issue, (''+obj) is still both faster and smaller.

Note that we haven't tried timing these for a long time, and that we may not
have tried timing across a variety of browsers -- I don't remember.

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/atLeast...
File src/com/google/caja/ses/atLeastFreeVarNames.js (right):

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/atLeast...
src/com/google/caja/ses/atLeastFreeVarNames.js:127: programSrc = ''+programSrc;
On 2013/03/04 17:38:38, kpreid2 wrote:
> Similar question. In this case I find String(foo) more aesthetic than ''+foo;
is
> there a strong reason to prefer the latter?

Stylistic consistency with a choice that had been made originally for speed and
size.

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/repairE...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:2802: function F(){}
On 2013/03/04 17:38:38, kpreid2 wrote:
> F could be moved out of the function so we reuse it rather than constructing a
> new F each call. I don't know whether that would be a performance improvement.

Since this issue looks like it is about to go away as of the next canary, I
won't worry about this performance issue for now.

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

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:470: verifyStrictProgram('(( ' + exprSrc +
'\n));');
On 2013/03/04 17:38:38, kpreid2 wrote:
> How does this help?

When exprSrc is an anonymous function expression or an object literal,

    exprSrc + ';'

may not parse as a Program.

[MarkM, 2013-03-04 19:24:49]

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

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/debug.j...
src/com/google/caja/ses/debug.js:86: return void 0;
Yang Guo points out that this terminal "return void 0;" is redundant and should
be removed.

[MarkM, 2013-03-05 04:25:50]

Nevermind Patch set 2 -- it was a mistake.

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

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/debug.j...
src/com/google/caja/ses/debug.js:83: ses.logger.error('Error while initializing
debug module', err);
On 2013/03/04 17:38:38, kpreid2 wrote:
> Maybe comment here that we expect the following call to fail, 

Done

> or explicitly throw here?

Done

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/debug.j...
src/com/google/caja/ses/debug.js:86: return void 0;
On 2013/03/04 19:24:49, MarkM wrote:
> Yang Guo points out that this terminal "return void 0;" is redundant and
should
> be removed.

Done.

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

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:409: var MAX_NAT = Math.pow(2,52); // Is
this right?
On 2013/03/04 17:38:38, kpreid2 wrote:
> No, you want Math.pow(2, 53).
> 
> The maximum exact integer is 2^53, at which point we've just hit the exponent
> region where the next representable number is 2^53+2.

Done.

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:409: var MAX_NAT = Math.pow(2,52); // Is
this right?
On 2013/03/04 17:38:38, kpreid2 wrote:
> No, you want Math.pow(2, 53).
> 
> The maximum exact integer is 2^53, at which point we've just hit the exponent
> region where the next representable number is 2^53+2.

Done and verified.

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:412: throw new RangeError("not a number");
On 2013/03/04 17:38:38, kpreid2 wrote:
> single quotes, here and below

Done.

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:414: if (allegedNum !== allegedNum) { throw
new RangeError("NaN not natural"); }
On 2013/03/04 17:38:38, kpreid2 wrote:
> Why not use isFinite?

> isFinite('')
true
> isNaN('x')
true

Although neither could happen here since we've already done a typeof check, this
coercion behavior makes these functions too hazardous to contemplate.

NaN is supposed to be the only non-reflective value, so this test by itself
should be safe. Further, since we've already passed the same typeof test, here
we only need to be confident that NaN is the only non-reflective number.

[MarkM, 2013-03-05 04:37:50]

Someone with access to a running IE10 should test this.

https://codereview.appspot.com/7433048/diff/14001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7433048/diff/14001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:1338: // to allow IE10's
RegExp.prototype.options
Done

[kpreid2, 2013-03-05 18:18:18]

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

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/debug.j...
src/com/google/caja/ses/debug.js:86: return void 0;
This is technically redundant but I feel is good documentation and should be
kept: the pepareStackTrace interface returns a value, so we should be explicit
about what value we are returning.

https://codereview.appspot.com/7433048/diff/21002/src/com/google/caja/ses/deb...
File src/com/google/caja/ses/debug.js (right):

https://codereview.appspot.com/7433048/diff/21002/src/com/google/caja/ses/deb...
src/com/google/caja/ses/debug.js:90: throw err;
This seems wrong, because (per
<http://code.google.com/p/v8/wiki/JavaScriptStackTraceApi>) this is called as a
lazy getter. So throwing here may disrupt an exception's client's error-handling
by throwing inside a catch block. Instead, we should simply return (as well as
logging).

[MarkM, 2013-03-07 03:07:48]

New Snapshot.

Besides responding to comments, I removed the two tests and one repair this CL
would have added: 

test_PROTO_FREEZE_KILLS_CREATE
test_FUNCTION_DOESNT_VERIFY_SYNTAX
repair_PROTO_FREEZE_KILLS_CREATE

postponing them till a later CL.

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

https://codereview.appspot.com/7433048/diff/1/src/com/google/caja/ses/debug.j...
src/com/google/caja/ses/debug.js:86: return void 0;
On 2013/03/05 18:18:18, kpreid2 wrote:
> This is technically redundant but I feel is good documentation and should be
> kept: the pepareStackTrace interface returns a value, so we should be explicit
> about what value we are returning.

Done

https://codereview.appspot.com/7433048/diff/21002/src/com/google/caja/ses/deb...
File src/com/google/caja/ses/debug.js (right):

https://codereview.appspot.com/7433048/diff/21002/src/com/google/caja/ses/deb...
src/com/google/caja/ses/debug.js:90: throw err;
On 2013/03/05 18:18:19, kpreid2 wrote:
> This seems wrong, because (per
> <http://code.google.com/p/v8/wiki/JavaScriptStackTraceApi>) this is called as
a
> lazy getter. So throwing here may disrupt an exception's client's
error-handling
> by throwing inside a catch block. Instead, we should simply return (as well as
> logging).

Done.

[kpreid2, 2013-03-07 03:22:01]

LGTM modulo comments

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/deb...
File src/com/google/caja/ses/debug.js (right):

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/deb...
src/com/google/caja/ses/debug.js:82: if (ssts === void 0) {
This new block is indented with tabs instead of spaces.

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/exp...
File src/com/google/caja/ses/explicit.html (right):

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/exp...
src/com/google/caja/ses/explicit.html:90: // NOTE: do not cajaVM.def(imports)
for this test
Please explain why, so the next person doesn't have to reconstruct the
rationale.

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:3417: tests: [] // ['10.4.3-1-59-s']
Explain why this is commented out.

[MarkM, 2013-03-07 04:11:46]

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/deb...
File src/com/google/caja/ses/debug.js (right):

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/deb...
src/com/google/caja/ses/debug.js:82: if (ssts === void 0) {
On 2013/03/07 03:22:01, kpreid2 wrote:
> This new block is indented with tabs instead of spaces.

Done.

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/exp...
File src/com/google/caja/ses/explicit.html (right):

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/exp...
src/com/google/caja/ses/explicit.html:90: // NOTE: do not cajaVM.def(imports)
for this test
On 2013/03/07 03:22:01, kpreid2 wrote:
> Please explain why, so the next person doesn't have to reconstruct the
> rationale.

Done.

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/7433048/diff/38001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:3417: tests: [] // ['10.4.3-1-59-s']
On 2013/03/07 03:22:01, kpreid2 wrote:
> Explain why this is commented out.

I have no idea why it was commented out. It's uncommented out now, so Done.

[kpreid2, 2013-03-07 04:14:34]

LGTM