Fixed issue 19212

Fixed a bug in json parsing.  Refactored compilation code a bit to
make it more obvious what's going on.

Review URL: http://codereview.chromium.org/165446


git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@2675 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/compiler.cc

@@ -102,7 +102,7 @@ static Handle<Code> MakeCode(FunctionLiteral* literal,
 
 
 static bool IsValidJSON(FunctionLiteral* lit) {
-  if (!lit->body()->length() == 1)
+  if (lit->body()->length() != 1)
     return false;
   Statement* stmt = lit->body()->at(0);
   if (stmt->AsExpressionStatement() == NULL)
@@ -114,7 +114,7 @@ static bool IsValidJSON(FunctionLiteral* lit) {
 
 static Handle<JSFunction> MakeFunction(bool is_global,
                                        bool is_eval,
-                                       bool is_json,
+                                       Compiler::ValidationState validate,
                                        Handle<Script> script,
                                        Handle<Context> context,
                                        v8::Extension* extension,
@@ -129,6 +129,7 @@ static Handle<JSFunction> MakeFunction(bool is_global,
   script->set_context_data((*i::Top::global_context())->data());
 
 #ifdef ENABLE_DEBUGGER_SUPPORT
+  bool is_json = (validate == Compiler::VALIDATE_JSON);
   if (is_eval || is_json) {
     script->set_compilation_type(
         is_json ? Smi::FromInt(Script::COMPILATION_TYPE_JSON) :
@@ -162,7 +163,7 @@ static Handle<JSFunction> MakeFunction(bool is_global,
   // When parsing JSON we do an ordinary parse and then afterwards
   // check the AST to ensure it was well-formed.  If not we give a
   // syntax error.
-  if (is_json && !IsValidJSON(lit)) {
+  if (validate == Compiler::VALIDATE_JSON && !IsValidJSON(lit)) {
     HandleScope scope;
     Handle<JSArray> args = Factory::NewJSArray(1);
     Handle<Object> source(script->source());
@@ -282,7 +283,7 @@ Handle<JSFunction> Compiler::Compile(Handle<String> source,
     // Compile the function and add it to the cache.
     result = MakeFunction(true,
                           false,
-                          false,
+                          DONT_VALIDATE_JSON,
                           script,
                           Handle<Context>::null(),
                           extension,
@@ -305,7 +306,11 @@ Handle<JSFunction> Compiler::Compile(Handle<String> source,
 Handle<JSFunction> Compiler::CompileEval(Handle<String> source,
                                          Handle<Context> context,
                                          bool is_global,
-                                         bool is_json) {
+                                         ValidationState validate) {
+  // Note that if validation is required then no path through this
+  // function is allowed to return a value without validating that
+  // the input is legal json.
+
   int source_length = source->length();
   Counters::total_eval_size.Increment(source_length);
   Counters::total_compile_size.Increment(source_length);
@@ -314,20 +319,26 @@ Handle<JSFunction> Compiler::CompileEval(Handle<String> source,
   VMState state(COMPILER);
 
   // Do a lookup in the compilation cache; if the entry is not there,
-  // invoke the compiler and add the result to the cache.
+  // invoke the compiler and add the result to the cache.  If we're
-  Handle<JSFunction> result =
+  // evaluating json we bypass the cache since we can't be sure a
-      CompilationCache::LookupEval(source, context, is_global);
+  // potential value in the cache has been validated.
+  Handle<JSFunction> result;
+  if (validate == DONT_VALIDATE_JSON)
+    result = CompilationCache::LookupEval(source, context, is_global);
+
   if (result.is_null()) {
     // Create a script object describing the script to be compiled.
     Handle<Script> script = Factory::NewScript(source);
     result = MakeFunction(is_global,
                           true,
-                          is_json,
+                          validate,
                           script,
                           context,
                           NULL,
                           NULL);
-    if (!result.is_null()) {
+    if (!result.is_null() && validate != VALIDATE_JSON) {
+      // For json it's unlikely that we'll ever see exactly the same
+      // string again so we don't use the compilation cache.
       CompilationCache::PutEval(source, context, is_global, result);
     }
   }

src/compiler.h

@@ -48,6 +48,8 @@ namespace internal {
 
 class Compiler : public AllStatic {
  public:
+  enum ValidationState { VALIDATE_JSON, DONT_VALIDATE_JSON };
+
   // All routines return a JSFunction.
   // If an error occurs an exception is raised and
   // the return handle contains NULL.
@@ -63,7 +65,7 @@ class Compiler : public AllStatic {
   static Handle<JSFunction> CompileEval(Handle<String> source,
                                         Handle<Context> context,
                                         bool is_global,
-                                        bool is_json);
+                                        ValidationState validation);
 
   // Compile from function info (used for lazy compilation). Returns
   // true on success and false if the compilation resulted in a stack

src/runtime.cc

@@ -4973,10 +4973,12 @@ static Object* Runtime_CompileString(Arguments args) {
 
   // Compile source string in the global context.
   Handle<Context> context(Top::context()->global_context());
+  Compiler::ValidationState validate = (is_json->IsTrue())
+    ? Compiler::VALIDATE_JSON : Compiler::DONT_VALIDATE_JSON;
   Handle<JSFunction> boilerplate = Compiler::CompileEval(source,
                                                          context,
                                                          true,
-                                                         is_json->IsTrue());
+                                                         validate);
   if (boilerplate.is_null()) return Failure::Exception();
   Handle<JSFunction> fun =
       Factory::NewFunctionFromBoilerplate(boilerplate, context);
@@ -5000,8 +5002,11 @@ static Object* CompileDirectEval(Handle<String> source) {
   bool is_global = context->IsGlobalContext();
 
   // Compile source string in the current context.
-  Handle<JSFunction> boilerplate =
+  Handle<JSFunction> boilerplate = Compiler::CompileEval(
-      Compiler::CompileEval(source, context, is_global, false);
+      source,
+      context,
+      is_global,
+      Compiler::DONT_VALIDATE_JSON);
   if (boilerplate.is_null()) return Failure::Exception();
   Handle<JSFunction> fun =
     Factory::NewFunctionFromBoilerplate(boilerplate, context);
@@ -7043,7 +7048,7 @@ static Object* Runtime_DebugEvaluate(Arguments args) {
       Compiler::CompileEval(function_source,
                             context,
                             context->IsGlobalContext(),
-                            false);
+                            Compiler::DONT_VALIDATE_JSON);
   if (boilerplate.is_null()) return Failure::Exception();
   Handle<JSFunction> compiled_function =
       Factory::NewFunctionFromBoilerplate(boilerplate, context);
@@ -7111,7 +7116,7 @@ static Object* Runtime_DebugEvaluateGlobal(Arguments args) {
       Handle<JSFunction>(Compiler::CompileEval(source,
                                                context,
                                                true,
-                                               false));
+                                               Compiler::DONT_VALIDATE_JSON));
   if (boilerplate.is_null()) return Failure::Exception();
   Handle<JSFunction> compiled_function =
       Handle<JSFunction>(Factory::NewFunctionFromBoilerplate(boilerplate,

test/mjsunit/json.js

@@ -195,3 +195,13 @@ assertEquals('{"y":6,"x":5}', JSON.stringify({x:5,y:6}, ['y', 'x']));
 
 assertEquals(undefined, JSON.stringify(undefined));
 assertEquals(undefined, JSON.stringify(function () { }));
+
+function checkIllegal(str) {
+  assertThrows(function () { JSON.parse(str); }, SyntaxError);
+}
+
+checkIllegal('1); throw "foo"; (1');
+
+var x = 0;
+eval("(1); x++; (1)");
+checkIllegal('1); x++; (1');