8016235: Use in catch block that may not have been executed in try block...

8016235: Use in catch block that may not have been executed in try block caused illegal byte code to be generated
Reviewed-by: jlaskey, hannesw

src/jdk/nashorn/internal/codegen/Attr.java

@@ -35,6 +35,7 @@ import static jdk.nashorn.internal.codegen.CompilerConstants.SCOPE;
 import static jdk.nashorn.internal.codegen.CompilerConstants.SWITCH_TAG_PREFIX;
 import static jdk.nashorn.internal.codegen.CompilerConstants.THIS;
 import static jdk.nashorn.internal.codegen.CompilerConstants.VARARGS;
+import static jdk.nashorn.internal.ir.Symbol.IS_ALWAYS_DEFINED;
 import static jdk.nashorn.internal.ir.Symbol.IS_CONSTANT;
 import static jdk.nashorn.internal.ir.Symbol.IS_FUNCTION_SELF;
 import static jdk.nashorn.internal.ir.Symbol.IS_GLOBAL;
@@ -128,6 +129,8 @@ final class Attr extends NodeOperatorVisitor<LexicalContext> {
 
     private final Deque<Type> returnTypes;
 
+    private int catchNestingLevel;
+
     private static final DebugLogger LOG   = new DebugLogger("attr");
     private static final boolean     DEBUG = LOG.isEnabled();
 
@@ -169,14 +172,14 @@ final class Attr extends NodeOperatorVisitor<LexicalContext> {
         if (functionNode.isVarArg()) {
             initCompileConstant(VARARGS, body, IS_PARAM | IS_INTERNAL, Type.OBJECT_ARRAY);
             if (functionNode.needsArguments()) {
-                initCompileConstant(ARGUMENTS, body, IS_VAR | IS_INTERNAL, Type.typeFor(ScriptObject.class));
+                initCompileConstant(ARGUMENTS, body, IS_VAR | IS_INTERNAL | IS_ALWAYS_DEFINED, Type.typeFor(ScriptObject.class));
                 addLocalDef(ARGUMENTS.symbolName());
             }
         }
 
         initParameters(functionNode, body);
-        initCompileConstant(SCOPE, body, IS_VAR | IS_INTERNAL, Type.typeFor(ScriptObject.class));
-        initCompileConstant(RETURN, body, IS_VAR | IS_INTERNAL, Type.OBJECT);
+        initCompileConstant(SCOPE, body, IS_VAR | IS_INTERNAL | IS_ALWAYS_DEFINED, Type.typeFor(ScriptObject.class));
+        initCompileConstant(RETURN, body, IS_VAR | IS_INTERNAL | IS_ALWAYS_DEFINED, Type.OBJECT);
     }
 
 
@@ -320,10 +323,12 @@ final class Attr extends NodeOperatorVisitor<LexicalContext> {
         final Block     block     = lc.getCurrentBlock();
 
         start(catchNode);
+        catchNestingLevel++;
 
         // define block-local exception variable
-        final Symbol def = defineSymbol(block, exception.getName(), IS_VAR | IS_LET);
-        newType(def, Type.OBJECT);
+        final Symbol def = defineSymbol(block, exception.getName(), IS_VAR | IS_LET | IS_ALWAYS_DEFINED);
+        newType(def, Type.OBJECT); //we can catch anything, not just ecma exceptions
+
         addLocalDef(exception.getName());
 
         return true;
@@ -334,6 +339,9 @@ final class Attr extends NodeOperatorVisitor<LexicalContext> {
         final IdentNode exception = catchNode.getException();
         final Block  block        = lc.getCurrentBlock();
         final Symbol symbol       = findSymbol(block, exception.getName());
+
+        catchNestingLevel--;
+
         assert symbol != null;
         return end(catchNode.setException((IdentNode)exception.setSymbol(lc, symbol)));
     }
@@ -543,9 +551,19 @@ final class Attr extends NodeOperatorVisitor<LexicalContext> {
                 assert lc.getFunctionBody(functionNode).getExistingSymbol(CALLEE.symbolName()) != null;
                 lc.setFlag(functionNode.getBody(), Block.NEEDS_SELF_SYMBOL);
                 newType(symbol, FunctionNode.FUNCTION_TYPE);
-            } else if (!identNode.isInitializedHere()) { // NASHORN-448
-                // here is a use outside the local def scope
-                if (!isLocalDef(name)) {
+            } else if (!identNode.isInitializedHere()) {
+                /*
+                 * See NASHORN-448, JDK-8016235
+                 *
+                 * Here is a use outside the local def scope
+                 * the inCatch check is a conservative approach to handle things that might have only been
+                 * defined in the try block, but with variable declarations, which due to JavaScript rules
+                 * have to be lifted up into the function scope outside the try block anyway, but as the
+                 * flow can fault at almost any place in the try block and get us to the catch block, all we
+                 * know is that we have a declaration, not a definition. This can be made better and less
+                 * conservative once we superimpose a CFG onto the AST.
+                 */
+                if (!isLocalDef(name) || inCatch()) {
                     newType(symbol, Type.OBJECT);
                     symbol.setCanBeUndefined();
                 }
@@ -572,6 +590,10 @@ final class Attr extends NodeOperatorVisitor<LexicalContext> {
         return end(identNode.setSymbol(lc, symbol));
     }
 
+    private boolean inCatch() {
+        return catchNestingLevel > 0;
+    }
+
     /**
      * If the symbol isn't already a scope symbol, and it is either not local to the current function, or it is being
      * referenced from within a with block, we force it to be a scope symbol.
@@ -584,26 +606,26 @@ final class Attr extends NodeOperatorVisitor<LexicalContext> {
     }
 
     private boolean symbolNeedsToBeScope(Symbol symbol) {
-        if(symbol.isThis() || symbol.isInternal()) {
+        if (symbol.isThis() || symbol.isInternal()) {
             return false;
         }
         boolean previousWasBlock = false;
-        for(final Iterator<LexicalContextNode> it = lc.getAllNodes(); it.hasNext();) {
+        for (final Iterator<LexicalContextNode> it = lc.getAllNodes(); it.hasNext();) {
             final LexicalContextNode node = it.next();
-            if(node instanceof FunctionNode) {
+            if (node instanceof FunctionNode) {
                 // We reached the function boundary without seeing a definition for the symbol - it needs to be in
                 // scope.
                 return true;
-            } else if(node instanceof WithNode) {
-                if(previousWasBlock) {
+            } else if (node instanceof WithNode) {
+                if (previousWasBlock) {
                     // We reached a WithNode; the symbol must be scoped. Note that if the WithNode was not immediately
                     // preceded by a block, this means we're currently processing its expression, not its body,
                     // therefore it doesn't count.
                     return true;
                 }
                 previousWasBlock = false;
-            } else if(node instanceof Block) {
-                if(((Block)node).getExistingSymbol(symbol.getName()) == symbol) {
+            } else if (node instanceof Block) {
+                if (((Block)node).getExistingSymbol(symbol.getName()) == symbol) {
                     // We reached the block that defines the symbol without reaching either the function boundary, or a
                     // WithNode. The symbol need not be scoped.
                     return false;
@@ -1700,8 +1722,8 @@ final class Attr extends NodeOperatorVisitor<LexicalContext> {
     }
 
     private void pushLocalsBlock() {
-        localDefs.push(localDefs.isEmpty() ? new HashSet<String>() : new HashSet<>(localDefs.peek()));
-        localUses.push(localUses.isEmpty() ? new HashSet<String>() : new HashSet<>(localUses.peek()));
+        localDefs.push(new HashSet<>(localDefs.peek()));
+        localUses.push(new HashSet<>(localUses.peek()));
     }
 
     private void popLocals() {

src/jdk/nashorn/internal/ir/Symbol.java

@@ -55,23 +55,25 @@ public final class Symbol implements Comparable<Symbol> {
     public static final int KINDMASK = (1 << 3) - 1; // Kinds are represented by lower three bits
 
     /** Is this scope */
-    public static final int IS_SCOPE         = 1 << 4;
+    public static final int IS_SCOPE             = 1 <<  4;
     /** Is this a this symbol */
-    public static final int IS_THIS          = 1 << 5;
+    public static final int IS_THIS              = 1 <<  5;
     /** Can this symbol ever be undefined */
-    public static final int CAN_BE_UNDEFINED = 1 << 6;
+    public static final int CAN_BE_UNDEFINED     = 1 <<  6;
+    /** Is this symbol always defined? */
+    public static final int IS_ALWAYS_DEFINED    = 1 <<  8;
     /** Can this symbol ever have primitive types */
-    public static final int CAN_BE_PRIMITIVE = 1 << 7;
+    public static final int CAN_BE_PRIMITIVE     = 1 <<  9;
     /** Is this a let */
-    public static final int IS_LET           = 1 << 8;
+    public static final int IS_LET               = 1 << 10;
     /** Is this an internal symbol, never represented explicitly in source code */
-    public static final int IS_INTERNAL      = 1 << 9;
+    public static final int IS_INTERNAL          = 1 << 11;
     /** Is this a function self-reference symbol */
-    public static final int IS_FUNCTION_SELF = 1 << 10;
+    public static final int IS_FUNCTION_SELF     = 1 << 12;
     /** Is this a specialized param? */
-    public static final int IS_SPECIALIZED_PARAM = 1 << 11;
+    public static final int IS_SPECIALIZED_PARAM = 1 << 13;
     /** Is this symbol a shared temporary? */
-    public static final int IS_SHARED = 1 << 12;
+    public static final int IS_SHARED            = 1 << 14;
 
     /** Null or name identifying symbol. */
     private final String name;
@@ -384,7 +386,7 @@ public final class Symbol implements Comparable<Symbol> {
       * Mark this symbol as one being shared by multiple expressions. The symbol must be a temporary.
       */
      public void setIsShared() {
-         if(!isShared()) {
+         if (!isShared()) {
              assert isTemp();
              trace("SET IS SHARED");
              flags |= IS_SHARED;
@@ -416,6 +418,14 @@ public final class Symbol implements Comparable<Symbol> {
         return (flags & KINDMASK) == IS_PARAM;
     }
 
+    /**
+     * Check if this symbol is always defined, which overrides all canBeUndefined tags
+     * @return true if always defined
+     */
+    public boolean isAlwaysDefined() {
+        return isParam() || (flags & IS_ALWAYS_DEFINED) == IS_ALWAYS_DEFINED;
+    }
+
     /**
      * Get the range for this symbol
      * @return range for symbol
@@ -462,7 +472,9 @@ public final class Symbol implements Comparable<Symbol> {
      */
     public void setCanBeUndefined() {
         assert type.isObject() : type;
-        if (!isParam() && !canBeUndefined()) {//parameters are never undefined
+        if (isAlwaysDefined()) {
+            return;
+        } else if (!canBeUndefined()) {
             assert !isShared();
             flags |= CAN_BE_UNDEFINED;
         }

src/jdk/nashorn/internal/parser/JSONParser.java

@@ -149,9 +149,9 @@ public class JSONParser extends AbstractParser {
             @Override
             protected void scanNumber() {
                 // Record beginning of number.
-                final int start = position;
+                final int startPosition = position;
                 // Assume value is a decimal.
-                TokenType type = TokenType.DECIMAL;
+                TokenType valueType = TokenType.DECIMAL;
 
                 // floating point can't start with a "." with no leading digit before
                 if (ch0 == '.') {
@@ -211,11 +211,11 @@ public class JSONParser extends AbstractParser {
                         }
                     }
 
-                    type = TokenType.FLOATING;
+                    valueType = TokenType.FLOATING;
                 }
 
                 // Add number token.
-                add(type, start);
+                add(valueType, startPosition);
             }
 
             // ECMA 15.12.1.1 The JSON Lexical Grammar - JSONEscapeCharacter

src/jdk/nashorn/internal/parser/Lexer.java

@@ -919,6 +919,7 @@ public class Lexer extends Scanner {
 
     /**
      * Scan over a string literal.
+     * @param add true if we nare not just scanning but should actually modify the token stream
      */
     protected void scanString(final boolean add) {
         // Type of string.
@@ -1614,6 +1615,12 @@ public class Lexer extends Scanner {
         return null;
     }
 
+    /**
+     * Get the correctly localized error message for a given message id format arguments
+     * @param msgId message id
+     * @param args  format arguments
+     * @return message
+     */
     protected static String message(final String msgId, final String... args) {
         return ECMAErrors.getMessage("lexer.error." + msgId, args);
     }

test/script/basic/JDK-8016235.js

+/*
+ * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ * 
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ * 
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ * 
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ * 
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8016235 : use before definition in catch block generated erroneous bytecode
+ * as there is no guarantee anything in the try block has executed. 
+ *
+ * @test
+ * @run 
+ */
+
+function f() {
+    try {
+	var parser = {};
+    } catch (e) {
+	parser = parser.context();
+    }
+}
+
+function g() { 
+    try {
+        return "apa";
+    } catch (tmp) {
+	//for now, too conservative as var ex declaration exists on the function
+	//level, but at least the code does not break, and the analysis is driven
+	//from the catch block (the rare case), not the try block (the common case)
+        var ex = new Error("DOM Exception 5");
+        ex.code = ex.number = 5;
+        return ex;
+    }
+}