Revision: 5713
Author: [email protected]
Date: Thu Feb 26 21:05:22 2015 UTC
Log: Fix optimization of function calls to not leak this.
https://codereview.appspot.com/202360043
The optimizer was propagating an isFn bit improperly when folding
expressions
which caused (0, a.b)() to simplify to (a.b()) which leaks (a.b) as
the "this"
value for the call.
There was another problem in folding which caused an infinite loop on the
test
cases added to JsOptimizerTest. That was fixed in Operation.fold.
[email protected]
https://code.google.com/p/google-caja/source/detail?r=5713
Modified:
/trunk/src/com/google/caja/ancillary/opt/StatementSimplifier.java
/trunk/src/com/google/caja/parser/js/Operation.java
/trunk/tests/com/google/caja/ancillary/opt/JsOptimizerTest.java
=======================================
--- /trunk/src/com/google/caja/ancillary/opt/StatementSimplifier.java Tue
Sep 24 20:06:05 2013 UTC
+++ /trunk/src/com/google/caja/ancillary/opt/StatementSimplifier.java Thu
Feb 26 21:05:22 2015 UTC
@@ -188,14 +188,19 @@
return new ReturnStmt(rs.getFilePosition(), optReturnValue);
}
return rs;
+ } else if (n instanceof Expression) {
+ // We need to recurse into it to find function constructor bodies,
+ // and we want to fold the result, but we don't want to fold
recursively
+ // with isFn=false, so we separate the recursion through the
expression
+ // for statements out.
+ return optimizeEmbeddedExpressions((Expression) n, false);
} else {
List<? extends ParseTreeNode> children = n.children();
int nChildren = children.size();
List<ParseTreeNode> newChildren = null;
boolean childNeedsBlock = (
- n instanceof FunctionConstructor || n instanceof TryStmt
- || n instanceof CatchStmt || n instanceof FinallyStmt
- || n instanceof SwitchCase);
+ n instanceof TryStmt || n instanceof CatchStmt
+ || n instanceof FinallyStmt || n instanceof SwitchCase);
for (int i = 0; i < nChildren; ++i) {
ParseTreeNode child = children.get(i);
ParseTreeNode newChild = optimize(child, childNeedsBlock);
@@ -262,9 +267,40 @@
n = ParseTreeNodes.newNodeInstance(
n.getClass(), n.getFilePosition(), n.getValue(), newChildren);
}
- return n instanceof Expression ? ((Expression) n).fold(false) : n;
+ return n;
}
}
+
+ private Expression optimizeEmbeddedExpressions(Expression e, boolean
isFn) {
+ List<? extends ParseTreeNode> children = e.children();
+ int nChildren = children.size();
+ List<ParseTreeNode> newChildren = null;
+ for (int i = 0; i < nChildren; ++i) {
+ ParseTreeNode child = children.get(i);
+ ParseTreeNode newChild;
+ if (child instanceof Expression) {
+ boolean childIsFn = i == 0 && Operation.is(e,
Operator.FUNCTION_CALL);
+ newChild = optimizeEmbeddedExpressions((Expression) child,
childIsFn);
+ } else if (child instanceof Statement) {
+ newChild = optimize(child, e instanceof FunctionConstructor);
+ } else {
+ newChild = child;
+ }
+ if (child != newChild) {
+ if (newChildren == null) {
+ newChildren = Lists.newArrayListWithCapacity(nChildren);
+ }
+ newChildren.addAll(children.subList(newChildren.size(), i));
+ newChildren.add(newChild);
+ }
+ }
+ if (newChildren != null) {
+ newChildren.addAll(children.subList(newChildren.size(), nChildren));
+ e = (Expression) ParseTreeNodes.newNodeInstance(
+ e.getClass(), e.getFilePosition(), e.getValue(), newChildren);
+ }
+ return e.fold(isFn);
+ }
/**
* <code>{ a; { b; c; } ; ; d }</code> -> <code>{ a; b; c; d; }</code>
@@ -592,7 +628,7 @@
}
}
// TODO(mikesamuel): if c is simple and not a global reference,
optimize
- // out he common head as well.
+ // out the common head as well.
CommaCommonalities opt = commaCommonalities(x, y);
if (opt != null) {
// Both reduced sides can't be null since we checked above whether
=======================================
--- /trunk/src/com/google/caja/parser/js/Operation.java Mon Feb 24 22:34:37
2014 UTC
+++ /trunk/src/com/google/caja/parser/js/Operation.java Thu Feb 26 21:05:22
2015 UTC
@@ -519,9 +519,18 @@
case 1: folded = foldUnaryOp(); break;
case 2: folded = foldBinaryOp(); break;
case 3: folded = foldTernaryOp(); break;
- default: return this;
+ default: folded = this;
}
if (isFn && isFnSpecialForm(folded) && !isFnSpecialForm(this)) {
+ if (is(this, Operator.COMMA)) {
+ List<? extends Expression> operands = children();
+ Expression left = operands.get(0);
+ Expression right = operands.get(1);
+ if (right == folded
+ && left instanceof IntegerLiteral &&
left.getValue().equals(0L)) {
+ return this;
+ }
+ }
FilePosition pos = getFilePosition();
folded = Operation.create(
pos, Operator.COMMA,
@@ -739,7 +748,7 @@
}
}
} else if (is(fn, Operator.MEMBER_ACCESS)
- && fn.children().get(0) instanceof StringLiteral) {
+ && fn.children().get(0) instanceof StringLiteral) {
StringLiteral sl = (StringLiteral) fn.children().get(0);
String methodName = ((Reference) fn.children().get(1))
.getIdentifierName();
=======================================
--- /trunk/tests/com/google/caja/ancillary/opt/JsOptimizerTest.java Sun
Jun 3 19:18:42 2012 UTC
+++ /trunk/tests/com/google/caja/ancillary/opt/JsOptimizerTest.java Thu Feb
26 21:05:22 2015 UTC
@@ -83,6 +83,26 @@
js(fromString("alert(function(){ if (a) { foo(); bar(); return
baz(); }"
+ "else return boo(); }());")));
}
+
+ public final void testThisNotUnintentionallyPassed1() throws Exception {
+ // Test that 0 is not preserved when not necessary as in the two
+ // similar tests below.
+ assertOptimized(
+ js(fromString("var x = a.b")),
+ js(fromString("var x = (0,a.b)")));
+ }
+
+ public final void testThisNotUnintentionallyPassed2() throws Exception {
+ assertOptimized(
+ js(fromString("x=(0,a.b)()")), // Does not pass a.b as this
+ js(fromString("x = (1,a.b)()")));
+ }
+
+ public final void testThisNotUnintentionallyPassed3() throws Exception {
+ assertOptimized(
+ js(fromString("var x=(0,a.b)()")), // Does not pass a.b as this
+ js(fromString("var x = (1,a.b)()")));
+ }
public final void testIssue1348() throws Exception {
String input = Join.join(
--
---
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
