From abb5fd1ee187148a478c08e4722ed8c34572a423 Mon Sep 17 00:00:00 2001
From: "marja@chromium.org" <marja@chromium.org>
Date: Thu, 6 Nov 2014 08:28:37 +0000
Subject: [PATCH] Retry: Parser & internalization fix: ensure no heap allocs
 during GetString(Handle<String>).

The bug has always been there: when the parser is operating in the "immediately
internalize" mode and calls GetString, we get FlatContent of a string and then
do heap allocation.

The bug was uncovered by https://codereview.chromium.org/693803004/ (which put
the parser to the "immediately internalize" mode more often), but looking at the
code, it's possible that it can happen in other cases too.

This CL makes AstValueFactory handle this situation gracefully: it won't try to
internalize inside GetString(Handle<String>); it's unnecessary anyway since we
have the Handle<String> already.

BUG=
R=rossberg@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#25179}
git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@25179 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---
 src/ast-value-factory.cc | 31 +++++++++++++++++++++----------
 src/ast-value-factory.h  | 14 ++++++++++----
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/src/ast-value-factory.cc b/src/ast-value-factory.cc
index 895ce39..518be23 100644
--- a/src/ast-value-factory.cc
+++ b/src/ast-value-factory.cc
@@ -212,7 +212,7 @@ void AstValue::Internalize(Isolate* isolate) {
 }
 
 
-const AstRawString* AstValueFactory::GetOneByteString(
+AstRawString* AstValueFactory::GetOneByteStringInternal(
     Vector<const uint8_t> literal) {
   uint32_t hash = StringHasher::HashSequentialString<uint8_t>(
       literal.start(), literal.length(), hash_seed_);
@@ -220,7 +220,7 @@ const AstRawString* AstValueFactory::GetOneByteString(
 }
 
 
-const AstRawString* AstValueFactory::GetTwoByteString(
+AstRawString* AstValueFactory::GetTwoByteStringInternal(
     Vector<const uint16_t> literal) {
   uint32_t hash = StringHasher::HashSequentialString<uint16_t>(
       literal.start(), literal.length(), hash_seed_);
@@ -229,13 +229,24 @@ const AstRawString* AstValueFactory::GetTwoByteString(
 
 
 const AstRawString* AstValueFactory::GetString(Handle<String> literal) {
-  DisallowHeapAllocation no_gc;
-  String::FlatContent content = literal->GetFlatContent();
-  if (content.IsOneByte()) {
-    return GetOneByteString(content.ToOneByteVector());
+  // For the FlatContent to stay valid, we shouldn't do any heap
+  // allocation. Make sure we won't try to internalize the string in GetString.
+  AstRawString* result = NULL;
+  Isolate* saved_isolate = isolate_;
+  isolate_ = NULL;
+  {
+    DisallowHeapAllocation no_gc;
+    String::FlatContent content = literal->GetFlatContent();
+    if (content.IsOneByte()) {
+      result = GetOneByteStringInternal(content.ToOneByteVector());
+    } else {
+      DCHECK(content.IsTwoByte());
+      result = GetTwoByteStringInternal(content.ToUC16Vector());
+    }
   }
-  DCHECK(content.IsTwoByte());
-  return GetTwoByteString(content.ToUC16Vector());
+  isolate_ = saved_isolate;
+  if (isolate_) result->Internalize(isolate_);
+  return result;
 }
 
 
@@ -348,8 +359,8 @@ const AstValue* AstValueFactory::NewTheHole() {
 
 #undef GENERATE_VALUE_GETTER
 
-const AstRawString* AstValueFactory::GetString(
-    uint32_t hash, bool is_one_byte, Vector<const byte> literal_bytes) {
+AstRawString* AstValueFactory::GetString(uint32_t hash, bool is_one_byte,
+                                         Vector<const byte> literal_bytes) {
   // literal_bytes here points to whatever the user passed, and this is OK
   // because we use vector_compare (which checks the contents) to compare
   // against the AstRawStrings which are in the string_table_. We should not
diff --git a/src/ast-value-factory.h b/src/ast-value-factory.h
index 071ca9c..09a4140 100644
--- a/src/ast-value-factory.h
+++ b/src/ast-value-factory.h
@@ -287,12 +287,16 @@ class AstValueFactory {
 
   Zone* zone() const { return zone_; }
 
-  const AstRawString* GetOneByteString(Vector<const uint8_t> literal);
+  const AstRawString* GetOneByteString(Vector<const uint8_t> literal) {
+    return GetOneByteStringInternal(literal);
+  }
   const AstRawString* GetOneByteString(const char* string) {
     return GetOneByteString(Vector<const uint8_t>(
         reinterpret_cast<const uint8_t*>(string), StrLength(string)));
   }
-  const AstRawString* GetTwoByteString(Vector<const uint16_t> literal);
+  const AstRawString* GetTwoByteString(Vector<const uint16_t> literal) {
+    return GetTwoByteStringInternal(literal);
+  }
   const AstRawString* GetString(Handle<String> literal);
   const AstConsString* NewConsString(const AstString* left,
                                      const AstString* right);
@@ -327,8 +331,10 @@ class AstValueFactory {
   const AstValue* NewTheHole();
 
  private:
-  const AstRawString* GetString(uint32_t hash, bool is_one_byte,
-                                Vector<const byte> literal_bytes);
+  AstRawString* GetOneByteStringInternal(Vector<const uint8_t> literal);
+  AstRawString* GetTwoByteStringInternal(Vector<const uint16_t> literal);
+  AstRawString* GetString(uint32_t hash, bool is_one_byte,
+                          Vector<const byte> literal_bytes);
 
   // All strings are copied here, one after another (no NULLs inbetween).
   HashMap string_table_;
-- 
2.7.4