Mitigate (o.p++) and (o.p += 1) not respecting frozenness in Chrome

Mitigate (o.p++) and (o.p += 1) not respecting frozenness in Chrome

[ihab.awad, 2013-07-29 14:22:41]

Not yet ready for review -- need to add tests.

Many files are simply renaming to remove "es53" from all our test filenames.

[ihab.awad, 2013-07-29 14:26:08]

Still not ready for review, but carved away all the spam renamings to make this
CL manageable. Need to add tests for SES then actually test the additions in
this CL.

[ihab.awad, 2013-07-29 23:49:49]

Please review now, thanks!

[MarkM, 2013-07-30 01:05:59]

Needs more work.

How does this enforce that this mitigation is mandatory on those platforms
suffering from this bug? I see no logic which enforces this.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:31: * xxxx requires console, JSON
"xxxx"?

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:114: var body =
scope.path[scope.path.length - 2].body;
Can we factor out this idiom for obtaining a parent from a scope? As is, the new
code is vastly less clear than the old code. How is a reader supposed to figure
out that this is navigating to the parent?

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:141: var parent =
scope.path[scope.path.length - 2];
Better. But factoring this into an abstraction would be better yet.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:271: function
rewriteStaticKeyMemberExpression(scope, node) {
Did we ever get a confirmation from the v8 folks that this rewrite is adequate
for the actual problem? I am unwilling to sign off on this based just on black
box testing.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:324: } else if
((scope.options.rewritePropertyCompoundAssignmentExpr || true) &&
80 columns

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:388: debugger;
Remove "debugger;"

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/pat...
File src/com/google/caja/ses/patterns.js (right):

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/pat...
src/com/google/caja/ses/patterns.js:68: ses.patterns {
Is this code actually used? If not, please remove this file from the CL. If it
is, I don't understand how this line even parses. It seems like a syntax error
to me.

[ihab.awad, 2013-07-30 02:07:10]

On 2013/07/30 01:05:59, MarkM wrote:
> How does this enforce that this mitigation is mandatory on those platforms
> suffering from this bug? I see no logic which enforces this.

Sorry, I submitted in haste after finishing the test cases but you're right,
this is still required, as is the removal of some obsolete and/or debug cruft.
Will fix (and do the couple of changes you mentioned) then re-ping. Thanks for
the review so far.

[MarkM, 2013-07-30 18:19:32]

https://codereview.appspot.com/12029043/diff/15002/src/com/google/caja/ses/mi...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/12029043/diff/15002/src/com/google/caja/ses/mi...
src/com/google/caja/ses/mitigateGotchas.js:136: var parent = parent(scope);
Does this run? The scope collision on "parent" should prevent it from running.

[ihab.awad, 2013-07-31 05:24:12]

Ready for re-review.

[ihab.awad, 2013-07-31 05:26:17]

Thanks for the comments so far. The CL should be far more clean and complete
now.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:31: * xxxx requires console, JSON
On 2013/07/30 01:05:59, MarkM wrote:
> "xxxx"?

Done.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:114: var body =
scope.path[scope.path.length - 2].body;
On 2013/07/30 01:05:59, MarkM wrote:
> Can we factor out this idiom for obtaining a parent from a scope? As is, the
new
> code is vastly less clear than the old code. How is a reader supposed to
figure
> out that this is navigating to the parent?

Done.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:141: var parent =
scope.path[scope.path.length - 2];
On 2013/07/30 01:05:59, MarkM wrote:
> Better. But factoring this into an abstraction would be better yet.

Done.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:271: function
rewriteStaticKeyMemberExpression(scope, node) {
On 2013/07/30 01:05:59, MarkM wrote:
> Did we ever get a confirmation from the v8 folks that this rewrite is adequate
> for the actual problem? I am unwilling to sign off on this based just on black
> box testing.

Yes, see https://code.google.com/p/v8/issues/detail?id=2779#c12

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:324: } else if
((scope.options.rewritePropertyCompoundAssignmentExpr || true) &&
On 2013/07/30 01:05:59, MarkM wrote:
> 80 columns

Done.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:388: debugger;
On 2013/07/30 01:05:59, MarkM wrote:
> Remove "debugger;"

Done.

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/pat...
File src/com/google/caja/ses/patterns.js (right):

https://codereview.appspot.com/12029043/diff/9001/src/com/google/caja/ses/pat...
src/com/google/caja/ses/patterns.js:68: ses.patterns {
On 2013/07/30 01:05:59, MarkM wrote:
> Is this code actually used? If not, please remove this file from the CL. If it
> is, I don't understand how this line even parses. It seems like a syntax error
> to me.

Done.

https://codereview.appspot.com/12029043/diff/15002/src/com/google/caja/ses/mi...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/12029043/diff/15002/src/com/google/caja/ses/mi...
src/com/google/caja/ses/mitigateGotchas.js:136: var parent = parent(scope);
On 2013/07/30 18:19:32, MarkM wrote:
> Does this run? The scope collision on "parent" should prevent it from running.

Done.

[MarkM, 2013-07-31 05:36:22]

https://codereview.appspot.com/12029043/diff/31001/src/com/google/caja/ses/re...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/12029043/diff/31001/src/com/google/caja/ses/re...
src/com/google/caja/ses/repairES5.js:4107: // TODO(ihab.awad): Build a better
system to record problems of fatal
Wouldn't it be better to use ses.acceptableProblems for this?

https://codereview.appspot.com/12029043/diff/31001/src/com/google/caja/ses/re...
src/com/google/caja/ses/repairES5.js:4110: canRepair: true,
true?

[ihab.awad, 2013-07-31 05:39:11]

https://codereview.appspot.com/12029043/diff/31001/src/com/google/caja/ses/re...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/12029043/diff/31001/src/com/google/caja/ses/re...
src/com/google/caja/ses/repairES5.js:4107: // TODO(ihab.awad): Build a better
system to record problems of fatal
On 2013/07/31 05:36:23, MarkM wrote:
> Wouldn't it be better to use ses.acceptableProblems for this?

Yes it would -- if only for the reason that it would halt safely if
mitigateGotchas is not prsent! Ok, I'll change and send a new snapshot.

https://codereview.appspot.com/12029043/diff/31001/src/com/google/caja/ses/re...
src/com/google/caja/ses/repairES5.js:4110: canRepair: true,
On 2013/07/31 05:36:23, MarkM wrote:
> true?

Done.

[ihab.awad, 2013-07-31 06:01:33]

Ok so it's not clear to me how to make the change just discussed.

startSES.js contains the (somewhat jury-rigged) logic to detect problems and
mitigate them when possible.

On the other hand, ses.acceptableProblems is set by caja.js, which is a layer
above the machinery of startSES.js. From the viewpoint of caja.js, the problem
is not "acceptable" because, at its level of abstraction, the problem does not
"exist" at all (it is mitigated away by startSES).

Does that all makes sense? What am I missing?

[ihab.awad, 2013-07-31 06:11:21]

Ok, more of the puzzle has become clear to me.

I now see that caja.js is the one doing the wiring between the "mitigation
frame" and the SES frame. So I guess caja.js can (a) say that problem X is
"acceptable; and (b) wrap mitigateSrcGotchas in a closure that force-sets the
options that turn on the necessary rewriting fixes for problem X. So far so
good.

But how does caja.js *know* that these problems are indeed the case? I guess it
should look at --

  tamingWin.ses.es5ProblemReports.INCREMENT_IGNORES_FROZEN.afterFailure

? ... But that would mean that, when SES is initialized ...

  tamingWin.ses.ok()

is false. So caja.js would have to have logic that says, ok() is false, but
that's ok.

     *   *   *

Should we perhaps just leave this CL as-is, and defer to a future time the
construction of a clean solution whereby mitigateGotchas can be once and for all
put into the TCB of SES?

[MarkM, 2013-07-31 14:35:33]

On 2013/07/31 06:11:21, ihab.awad wrote:
> Ok, more of the puzzle has become clear to me.
> 
> I now see that caja.js is the one doing the wiring between the "mitigation
> frame" and the SES frame. So I guess caja.js can (a) say that problem X is
> "acceptable; and (b) wrap mitigateSrcGotchas in a closure that force-sets the
> options that turn on the necessary rewriting fixes for problem X. So far so
> good.
> 
> But how does caja.js *know* that these problems are indeed the case? I guess
it
> should look at --
> 
>   tamingWin.ses.es5ProblemReports.INCREMENT_IGNORES_FROZEN.afterFailure
> 
> ? ... But that would mean that, when SES is initialized ...
> 
>   tamingWin.ses.ok()
> 
> is false. So caja.js would have to have logic that says, ok() is false, but
> that's ok.
> 
>      *   *   *
> 
> Should we perhaps just leave this CL as-is,

+1. LGTM

> and defer to a future time the
> construction of a clean solution whereby mitigateGotchas can be once and for
all
> put into the TCB of SES?

-1. I still hope to avoid a future in which mitigateGotchas is necessary. But it
is necessary now, so I admit this question is a bit academic at the moment.

[ihab.awad, 2013-08-14 17:29:31]

@5514