Reland: Fix JSON parser Handle leak (previous CL 1041483004)

R=mstarzinger@chromium.org
BUG=v8:3976
BUG=472504
LOG=y

Review URL: https://codereview.chromium.org/1051833002

Cr-Commit-Position: refs/heads/master@{#27571}

src/flag-definitions.h

@@ -843,6 +843,8 @@ DEFINE_BOOL(gc_verbose, false, "print stuff during garbage collection")
 DEFINE_BOOL(heap_stats, false, "report heap statistics before and after GC")
 DEFINE_BOOL(code_stats, false, "report code statistics after GC")
 DEFINE_BOOL(print_handles, false, "report handles after GC")
+DEFINE_BOOL(check_handle_count, false,
+            "Check that there are not too many handles at GC")
 DEFINE_BOOL(print_global_handles, false, "report global handles after GC")
 
 // TurboFan debug-only flags.

src/heap/heap.cc

@@ -618,6 +618,7 @@ void Heap::GarbageCollectionEpilogue() {
   if (FLAG_gc_verbose) Print();
   if (FLAG_code_stats) ReportCodeStatistics("After GC");
 #endif
+  if (FLAG_check_handle_count) CheckHandleCount();
   if (FLAG_deopt_every_n_garbage_collections > 0) {
     // TODO(jkummerow/ulan/jarin): This is not safe! We can't assume that
     // the topmost optimized frame can be deoptimized safely, because it
@@ -5654,6 +5655,24 @@ void Heap::PrintHandles() {
 
 #endif
 
+class CheckHandleCountVisitor : public ObjectVisitor {
+ public:
+  CheckHandleCountVisitor() : handle_count_(0) {}
+  ~CheckHandleCountVisitor() { CHECK(handle_count_ < 2000); }
+  void VisitPointers(Object** start, Object** end) {
+    handle_count_ += end - start;
+  }
+
+ private:
+  ptrdiff_t handle_count_;
+};
+
+
+void Heap::CheckHandleCount() {
+  CheckHandleCountVisitor v;
+  isolate_->handle_scope_implementer()->Iterate(&v);
+}
+
 
 Space* AllSpaces::next() {
   switch (counter_++) {

src/heap/heap.h

@@ -983,6 +983,7 @@ class Heap {
   }
 
   static bool RootIsImmortalImmovable(int root_index);
+  void CheckHandleCount();
 
 #ifdef VERIFY_HEAP
   // Verify the heap is in its normal state before or after a GC.

src/json-parser.h

@@ -16,6 +16,9 @@
 namespace v8 {
 namespace internal {
 
+enum ParseElementResult { kElementFound, kElementNotFound, kNullHandle };
+
+
 // A simple json parser.
 template <bool seq_one_byte>
 class JsonParser BASE_EMBEDDED {
@@ -155,6 +158,10 @@ class JsonParser BASE_EMBEDDED {
   // JavaScript array.
   Handle<Object> ParseJsonObject();
 
+  // Helper for ParseJsonObject. Parses the form "123": obj, which is recorded
+  // as an element, not a property.
+  ParseElementResult ParseElement(Handle<JSObject> json_object);
+
   // Parses a JSON array literal (grammar production JSONArray). An array
   // literal is a square-bracketed and comma separated sequence (possibly empty)
   // of JSON values.
@@ -299,6 +306,41 @@ Handle<Object> JsonParser<seq_one_byte>::ParseJsonValue() {
 }
 
 
+template <bool seq_one_byte>
+ParseElementResult JsonParser<seq_one_byte>::ParseElement(
+    Handle<JSObject> json_object) {
+  uint32_t index = 0;
+  // Maybe an array index, try to parse it.
+  if (c0_ == '0') {
+    // With a leading zero, the string has to be "0" only to be an index.
+    Advance();
+  } else {
+    do {
+      int d = c0_ - '0';
+      if (index > 429496729U - ((d > 5) ? 1 : 0)) break;
+      index = (index * 10) + d;
+      Advance();
+    } while (IsDecimalDigit(c0_));
+  }
+
+  if (c0_ == '"') {
+    // Successfully parsed index, parse and store element.
+    AdvanceSkipWhitespace();
+
+    if (c0_ == ':') {
+      AdvanceSkipWhitespace();
+      Handle<Object> value = ParseJsonValue();
+      if (!value.is_null()) {
+        JSObject::SetOwnElement(json_object, index, value, SLOPPY).Assert();
+        return kElementFound;
+      } else {
+        return kNullHandle;
+      }
+    }
+  }
+  return kElementNotFound;
+}
+
 // Parse a JSON object. Position must be right at '{'.
 template <bool seq_one_byte>
 Handle<Object> JsonParser<seq_one_byte>::ParseJsonObject() {
@@ -320,35 +362,12 @@ Handle<Object> JsonParser<seq_one_byte>::ParseJsonObject() {
       int start_position = position_;
       Advance();
 
-      uint32_t index = 0;
       if (IsDecimalDigit(c0_)) {
-        // Maybe an array index, try to parse it.
-        if (c0_ == '0') {
-          // With a leading zero, the string has to be "0" only to be an index.
-          Advance();
-        } else {
-          do {
-            int d = c0_ - '0';
-            if (index > 429496729U - ((d > 5) ? 1 : 0)) break;
-            index = (index * 10) + d;
-            Advance();
-          } while (IsDecimalDigit(c0_));
-        }
-
-        if (c0_ == '"') {
-          // Successfully parsed index, parse and store element.
-          AdvanceSkipWhitespace();
-
-          if (c0_ != ':') return ReportUnexpectedCharacter();
-          AdvanceSkipWhitespace();
-          Handle<Object> value = ParseJsonValue();
-          if (value.is_null()) return ReportUnexpectedCharacter();
-
-          JSObject::SetOwnElement(json_object, index, value, SLOPPY).Assert();
-          continue;
-        }
-        // Not an index, fallback to the slow path.
+        ParseElementResult element_result = ParseElement(json_object);
+        if (element_result == kNullHandle) return Handle<Object>::null();
+        if (element_result == kElementFound) continue;
       }
+      // Not an index, fallback to the slow path.
 
       position_ = start_position;
 #ifdef DEBUG
@@ -360,81 +379,109 @@ Handle<Object> JsonParser<seq_one_byte>::ParseJsonObject() {
 
       // Try to follow existing transitions as long as possible. Once we stop
       // transitioning, no transition can be found anymore.
+      DCHECK(transitioning);
+      // First check whether there is a single expected transition. If so, try
+      // to parse it first.
+      bool follow_expected = false;
+      Handle<Map> target;
+      if (seq_one_byte) {
+        key = TransitionArray::ExpectedTransitionKey(map);
+        follow_expected = !key.is_null() && ParseJsonString(key);
+      }
+      // If the expected transition hits, follow it.
+      if (follow_expected) {
+        target = TransitionArray::ExpectedTransitionTarget(map);
+      } else {
+        // If the expected transition failed, parse an internalized string and
+        // try to find a matching transition.
+        key = ParseJsonInternalizedString();
+        if (key.is_null()) return ReportUnexpectedCharacter();
+
+        target = TransitionArray::FindTransitionToField(map, key);
+        // If a transition was found, follow it and continue.
+        transitioning = !target.is_null();
+      }
+      if (c0_ != ':') return ReportUnexpectedCharacter();
+
+      AdvanceSkipWhitespace();
+      value = ParseJsonValue();
+      if (value.is_null()) return ReportUnexpectedCharacter();
+
       if (transitioning) {
-        // First check whether there is a single expected transition. If so, try
-        // to parse it first.
-        bool follow_expected = false;
-        Handle<Map> target;
-        if (seq_one_byte) {
-          key = TransitionArray::ExpectedTransitionKey(map);
-          follow_expected = !key.is_null() && ParseJsonString(key);
-        }
-        // If the expected transition hits, follow it.
-        if (follow_expected) {
-          target = TransitionArray::ExpectedTransitionTarget(map);
+        PropertyDetails details =
+            target->instance_descriptors()->GetDetails(descriptor);
+        Representation expected_representation = details.representation();
+
+        if (value->FitsRepresentation(expected_representation)) {
+          if (expected_representation.IsHeapObject() &&
+              !target->instance_descriptors()
+                   ->GetFieldType(descriptor)
+                   ->NowContains(value)) {
+            Handle<HeapType> value_type(
+                value->OptimalType(isolate(), expected_representation));
+            Map::GeneralizeFieldType(target, descriptor,
+                                     expected_representation, value_type);
+          }
+          DCHECK(target->instance_descriptors()
+                     ->GetFieldType(descriptor)
+                     ->NowContains(value));
+          properties.Add(value, zone());
+          map = target;
+          descriptor++;
+          continue;
         } else {
-          // If the expected transition failed, parse an internalized string and
-          // try to find a matching transition.
-          key = ParseJsonInternalizedString();
-          if (key.is_null()) return ReportUnexpectedCharacter();
-
-          target = TransitionArray::FindTransitionToField(map, key);
-          // If a transition was found, follow it and continue.
-          transitioning = !target.is_null();
+          transitioning = false;
         }
-        if (c0_ != ':') return ReportUnexpectedCharacter();
+      }
 
-        AdvanceSkipWhitespace();
-        value = ParseJsonValue();
-        if (value.is_null()) return ReportUnexpectedCharacter();
+      DCHECK(!transitioning);
 
-        if (transitioning) {
-          PropertyDetails details =
-              target->instance_descriptors()->GetDetails(descriptor);
-          Representation expected_representation = details.representation();
+      // Commit the intermediate state to the object and stop transitioning.
+      CommitStateToJsonObject(json_object, map, &properties);
 
-          if (value->FitsRepresentation(expected_representation)) {
-            if (expected_representation.IsHeapObject() &&
-                !target->instance_descriptors()
-                     ->GetFieldType(descriptor)
-                     ->NowContains(value)) {
-              Handle<HeapType> value_type(value->OptimalType(
-                      isolate(), expected_representation));
-              Map::GeneralizeFieldType(target, descriptor,
-                                       expected_representation, value_type);
-            }
-            DCHECK(target->instance_descriptors()->GetFieldType(
-                    descriptor)->NowContains(value));
-            properties.Add(value, zone());
-            map = target;
-            descriptor++;
-            continue;
-          } else {
-            transitioning = false;
-          }
+      Runtime::DefineObjectProperty(json_object, key, value, NONE).Check();
+    } while (transitioning && MatchSkipWhiteSpace(','));
+
+    // If we transitioned until the very end, transition the map now.
+    if (transitioning) {
+      CommitStateToJsonObject(json_object, map, &properties);
+    } else {
+      while (MatchSkipWhiteSpace(',')) {
+        HandleScope local_scope(isolate());
+        if (c0_ != '"') return ReportUnexpectedCharacter();
+
+        int start_position = position_;
+        Advance();
+
+        if (IsDecimalDigit(c0_)) {
+          ParseElementResult element_result = ParseElement(json_object);
+          if (element_result == kNullHandle) return Handle<Object>::null();
+          if (element_result == kElementFound) continue;
         }
+        // Not an index, fallback to the slow path.
+
+        position_ = start_position;
+#ifdef DEBUG
+        c0_ = '"';
+#endif
+
+        Handle<String> key;
+        Handle<Object> value;
 
-        // Commit the intermediate state to the object and stop transitioning.
-        CommitStateToJsonObject(json_object, map, &properties);
-      } else {
         key = ParseJsonInternalizedString();
         if (key.is_null() || c0_ != ':') return ReportUnexpectedCharacter();
 
         AdvanceSkipWhitespace();
         value = ParseJsonValue();
         if (value.is_null()) return ReportUnexpectedCharacter();
+
+        Runtime::DefineObjectProperty(json_object, key, value, NONE).Check();
       }
+    }
 
-      Runtime::DefineObjectProperty(json_object, key, value, NONE).Check();
-    } while (MatchSkipWhiteSpace(','));
     if (c0_ != '}') {
       return ReportUnexpectedCharacter();
     }
-
-    // If we transitioned until the very end, transition the map now.
-    if (transitioning) {
-      CommitStateToJsonObject(json_object, map, &properties);
-    }
   }
   AdvanceSkipWhitespace();
   return scope.CloseAndEscape(json_object);

test/mjsunit/regress/regress-3976.js

@@ -25,7 +25,7 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-// Flags: --max-old-space-size=60
+// Flags: --max-old-space-size=60 --check-handle-count
 
 table = [];
 

test/mjsunit/regress/regress-472504.js

+// Copyright 2015 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Should not crash in ASAN.
+function shouldThrow() {
+    shouldThrow(JSON.parse('{"0":1}'));
+}
+assertThrows("shouldThrow()", RangeError);