atLeastFreeVarNames must parse \u escapes.

atLeastFreeVarNames must parse \u escapes.

Fix by MarkM, tests by kpreid.

Superseded by https://codereview.appspot.com/283510043/ committed as 739a368f287d119c263428367df1dffd98906d0f .

[MarkM, 2016-02-01 22:24:40]

https://codereview.appspot.com/285330043/diff/1/tests/com/google/caja/ses/tes...
File tests/com/google/caja/ses/test-ses-parts.js (right):

https://codereview.appspot.com/285330043/diff/1/tests/com/google/caja/ses/tes...
tests/com/google/caja/ses/test-ses-parts.js:105: result = eval(testProgram);
Comment to make clear that this is the original unsafe eval, rather than the ses
eval.

Looking at startSES.js, this assumption seems to hinge on TAME_GLOBAL_EVAL.
Please make the test somehow independent of that flag, or check that the eval
you're invoking is the original unsafe eval you are assuming.

Since the case we're worried about involves only strict code, this should either
test the strict case, or test both. Perhaps even test with both eval functions?

https://codereview.appspot.com/285330043/diff/1/tests/com/google/caja/ses/tes...
tests/com/google/caja/ses/test-ses-parts.js:117: assertEquals(syntaxErrors, 1);
Seems fragile to assume that if you get 3 safes and 1 syntaxError, that they are
the ones you expect. Better to check specifically.

[kpreid_google, 2016-02-01 22:48:31]

Fix by MarkM, tests by kpreid.

[kpreid_google, 2016-02-01 22:49:40]

Fix by MarkM, tests by kpreid.

[kpreid_google, 2016-02-01 22:51:20]

https://codereview.appspot.com/285330043/diff/1/tests/com/google/caja/ses/tes...
File tests/com/google/caja/ses/test-ses-parts.js (right):

https://codereview.appspot.com/285330043/diff/1/tests/com/google/caja/ses/tes...
tests/com/google/caja/ses/test-ses-parts.js:105: result = eval(testProgram);
Now tests with both evaluators, and is independent of TAME_GLOBAL_EVAL. Note
that I used Function()() rather than eval() to get consistent return value
handling.

https://codereview.appspot.com/285330043/diff/1/tests/com/google/caja/ses/tes...
tests/com/google/caja/ses/test-ses-parts.js:117: assertEquals(syntaxErrors, 1);
On 2016/02/01 22:24:40, MarkM wrote:
> Seems fragile to assume that if you get 3 safes and 1 syntaxError, that they
are
> the ones you expect. Better to check specifically.

Done.

[Jasvir, 2016-02-01 23:01:14]

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

https://codereview.appspot.com/285330043/diff/1/src/com/google/caja/ses/atLea...
src/com/google/caja/ses/atLeastFreeVarNames.js:153: name = JSON.parse('"' + name
+ '"');
I found myself hand testing names that had \u007b\u007d, \u0022 and \u0020 to be
sure those corner cases were handled right - recommend adding those to the tests
as a result.

[MarkM, 2016-02-01 23:07:19]

Modulo Jasvir's suggestion, LGTM

[MarkM, 2016-02-01 23:08:56]

To be clear, once you implement Jasvir's suggestion, LGTM without any need to
rereview.

[kpreid_google, 2016-02-01 23:34:54]

Fix by MarkM, tests by kpreid.

[kpreid_google, 2016-02-01 23:36:21]

MarkM, please take a look — testing found another bug and I had to change
atLeastFreeVarNames again.

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

https://codereview.appspot.com/285330043/diff/1/src/com/google/caja/ses/atLea...
src/com/google/caja/ses/atLeastFreeVarNames.js:153: name = JSON.parse('"' + name
+ '"');
On 2016/02/01 23:01:14, Jasvir wrote:
> I found myself hand testing names that had \u007b\u007d, \u0022 and \u0020
> to be sure those corner cases were handled right - recommend adding those
> to the tests as a result.

It's not clear to me what combinations make good test cases, because they all
end up as syntax errors in eval anyway. Added some — please let me know what you
think.

Also, I found another fatal bug in atLeastFreeVarNames! Its regexp pattern was
matching decimal digits instead of hex digits, so it could miss an escape and
therefore miss a name. Fixed and added a test case.

This now feels kind of fragile, but I don't have great ideas to improve it.

[MarkM, 2016-02-01 23:38:54]

https://codereview.appspot.com/285330043/diff/60001/src/com/google/caja/ses/a...
File src/com/google/caja/ses/atLeastFreeVarNames.js (right):

https://codereview.appspot.com/285330043/diff/60001/src/com/google/caja/ses/a...
src/com/google/caja/ses/atLeastFreeVarNames.js:119: return
/(\w|\\u[0-9a-fA-F]{4}|\$)+/g;
Oops. I should have caught this in review since I was focused on it.

[MarkM, 2016-02-01 23:41:37]

I agree that we should figure out how to improve this. But remaining
improvements can happen after this CL. LGTM.

I am embarrassed about the \d{4} bug, but it looks fine now. Yes, it is a shame
this was not somehow caught automatically.

[Jasvir, 2016-02-02 01:58:05]

LGTM

On 2016/02/01 23:41:37, MarkM wrote:
> I agree that we should figure out how to improve this. But remaining
> improvements can happen after this CL. LGTM.
> 
> I am embarrassed about the \d{4} bug, but it looks fine now. Yes, it is a
shame
> this was not somehow caught automatically.