Fixed a potential dealloc race caused by values that escape from sync…

…hronization

1. Add retain + autorelease to values that are returned from within a synchronized block or method. 2. Upgrade local variables to RetainedLocal upon entering a synchronized block and downgrade upon exit

	Change on 2017/08/18 by zgao <zgao@google.com>

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=165740092

translator/src/main/java/com/google/devtools/j2objc/translate/OperatorRewriter.java

@@ -25,12 +25,15 @@
 import com.google.devtools.j2objc.ast.FieldAccess;
 import com.google.devtools.j2objc.ast.FunctionInvocation;
 import com.google.devtools.j2objc.ast.InfixExpression;
+import com.google.devtools.j2objc.ast.MethodDeclaration;
 import com.google.devtools.j2objc.ast.NumberLiteral;
 import com.google.devtools.j2objc.ast.PrefixExpression;
 import com.google.devtools.j2objc.ast.QualifiedName;
+import com.google.devtools.j2objc.ast.ReturnStatement;
 import com.google.devtools.j2objc.ast.SimpleName;
 import com.google.devtools.j2objc.ast.StringLiteral;
 import com.google.devtools.j2objc.ast.SuperFieldAccess;
+import com.google.devtools.j2objc.ast.SynchronizedStatement;
 import com.google.devtools.j2objc.ast.ThisExpression;
 import com.google.devtools.j2objc.ast.TreeNode;
 import com.google.devtools.j2objc.ast.TreeUtil;
@@ -46,9 +49,14 @@
 import com.google.devtools.j2objc.util.TypeUtil;
 import com.google.devtools.j2objc.util.UnicodeUtils;
 import com.google.j2objc.annotations.RetainedLocalRef;
+import java.lang.reflect.Modifier;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedList;
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
 import javax.lang.model.element.VariableElement;
 import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
@@ -61,6 +69,10 @@
  */
 public class OperatorRewriter extends UnitTreeVisitor {
 
+  private final LinkedList<Set<VariableElement>> retainedLocalCandidateStack = new LinkedList<>();
+  private Set<VariableElement> retainedLocalCandidates = new HashSet<>();
+  private boolean isSynchronizedMethod = false;
+
   public OperatorRewriter(CompilationUnit unit) {
     super(unit);
   }
@@ -118,6 +130,46 @@ public void endVisit(InfixExpression node) {
     }
   }
 
+  @Override
+  public boolean visit(MethodDeclaration node) {
+    isSynchronizedMethod = Modifier.isSynchronized(node.getModifiers());
+    retainedLocalCandidates.addAll(
+        node.getParameters()
+            .stream()
+            .map(v -> v.getVariableElement())
+            .filter(v -> !v.asType().getKind().isPrimitive())
+            .collect(Collectors.toList()));
+    return true;
+  }
+
+  @Override
+  public void endVisit(MethodDeclaration node) {
+    retainedLocalCandidateStack.clear();
+    retainedLocalCandidates.clear();
+    isSynchronizedMethod = false;
+  }
+
+  @Override
+  public boolean visit(SynchronizedStatement node) {
+    retainedLocalCandidateStack.add(retainedLocalCandidates);
+    retainedLocalCandidates = new HashSet<>();
+    return true;
+  }
+
+  @Override
+  public void endVisit(SynchronizedStatement node) {
+    retainedLocalCandidates = retainedLocalCandidateStack.removeLast();
+  }
+
+  @Override
+  public void endVisit(ReturnStatement node) {
+    Expression expr = node.getExpression();
+    if ((isSynchronizedMethod || !retainedLocalCandidateStack.isEmpty()) && expr != null
+        && !expr.getTypeMirror().getKind().isPrimitive()) {
+      rewriteRetainedLocal(expr);
+    }
+  }
+
   @Override
   public boolean visit(FieldAccess node) {
     rewriteVolatileLoad(node);
@@ -147,21 +199,44 @@ public boolean visit(SimpleName node) {
   public boolean visit(VariableDeclarationFragment node) {
     // Skip name so that it doesn't get mistaken for a variable load.
     Expression initializer = node.getInitializer();
+    VariableElement var = node.getVariableElement();
     if (initializer != null) {
       initializer.accept(this);
-      handleRetainedLocal(node.getVariableElement(), node.getInitializer());
+      handleRetainedLocal(var, node.getInitializer());
+    }
+    if (!var.asType().getKind().isPrimitive()) {
+      retainedLocalCandidates.add(var);
+    }
+    return false;
+  }
+
+  private boolean isRetainedLocal(VariableElement var) {
+    if (ElementUtil.isLocalVariable(var)
+        && ElementUtil.hasAnnotation(var, RetainedLocalRef.class)) {
+      return true;
+    }
+    for (Set<VariableElement> candidates : retainedLocalCandidateStack) {
+      if (candidates.contains(var)) {
+        return true;
+      }
     }
     return false;
   }
 
+  private static void rewriteRetainedLocal(Expression expr) {
+    if (expr.getKind() == TreeNode.Kind.STRING_LITERAL) {
+      return;
+    }
+    FunctionElement element =
+        new FunctionElement("JreRetainedLocalValue", TypeUtil.ID_TYPE, null);
+    FunctionInvocation invocation = new FunctionInvocation(element, expr.getTypeMirror());
+    expr.replaceWith(invocation);
+    invocation.addArgument(expr);
+  }
+
   private void handleRetainedLocal(VariableElement var, Expression rhs) {
-    if (ElementUtil.isLocalVariable(var) && ElementUtil.hasAnnotation(var, RetainedLocalRef.class)
-        && options.useReferenceCounting()) {
-      FunctionElement element =
-          new FunctionElement("JreRetainedLocalValue", TypeUtil.ID_TYPE, null);
-      FunctionInvocation invocation = new FunctionInvocation(element, rhs.getTypeMirror());
-      rhs.replaceWith(invocation);
-      invocation.addArgument(rhs);
+    if (options.useReferenceCounting() && isRetainedLocal(var)) {
+      rewriteRetainedLocal(rhs);
     }
   }
 

translator/src/test/java/com/google/devtools/j2objc/translate/OperatorRewriterTest.java

@@ -215,4 +215,60 @@ public void testLazyInitFields() throws IOException {
         "FOUNDATION_EXPORT volatile_id Test_lazyStaticStr;",
         "J2OBJC_STATIC_FIELD_OBJ_VOLATILE(Test, lazyStaticStr, NSString *)");
   }
+
+  public void testRetaineLocal_synchronizedBlock() throws IOException {
+    String translation = translateSourceFile(
+        "class Test {"
+        + "  class Foo {}"
+        + "  Foo f = new Foo();"
+        + "  void test(String s1, char c1) {"
+        + "    Foo f1 = new Foo(), f2 = f;"
+        + "    synchronized(f1) {"
+        + "      f1 = f2;"
+        + "      Foo f3 = f2;"
+        + "      s1 = \"foo\";"
+        + "      c1 = 'a';"
+        + "      synchronized(f3) {"
+        + "        f3 = f1;"
+        + "      }"
+        + "      synchronized(f3) {"
+        + "        f3 = f2;"
+        + "      }"
+        + "    }"
+        + "    f2 = f1;"
+        + "  }"
+        + "}", "Test", "Test.m");
+    assertTranslation(translation, "Test_Foo *f2 = f_;");
+    assertTranslation(translation, "f1 = JreRetainedLocalValue(f2);");
+    assertTranslation(translation, "Test_Foo *f3 = f2;");
+    assertTranslation(translation, "s1 = @\"foo\";");
+    assertTranslation(translation, "c1 = 'a';");
+    assertTranslation(translation, "f3 = JreRetainedLocalValue(f1);");
+    assertTranslation(translation, "f3 = JreRetainedLocalValue(f2);");
+    assertTranslation(translation, "f2 = f1;");
+  }
+
+  public void testRetainedLocal_returnWithinSynchronizedMethodOrBlock() throws IOException {
+    String translation = translateSourceFile(
+        "class Test {"
+        + "  class Foo {}"
+        + "  Foo f = new Foo();"
+        + "  String test1(String s1, char c1) {"
+        + "    synchronized(s1) {"
+        + "      return s1;"
+        + "    }"
+        + "  }"
+        + "  synchronized Foo test2() {"
+        + "    Foo f1 = f;"
+        + "    return f1;"
+        + "  }"
+        + "  synchronized int test3() {"
+        + "    int val = 1;"
+        + "    return val;"
+        + "  }"
+        + "}", "Test", "Test.m");
+    assertTranslation(translation, "return JreRetainedLocalValue(s1);");
+    assertTranslation(translation, "return JreRetainedLocalValue(f1)");
+    assertTranslation(translation, "return val;");
+  }
 }