Parser & internalization fix: ensure no heap allocs during GetString(Handle<String>).

src/ast-value-factory.cc

 // Copyright 2014 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 211 matching lines @@
 
 const AstRawString* AstValueFactory::GetTwoByteString(
     Vector<const uint16_t> literal) {
   uint32_t hash = StringHasher::HashSequentialString<uint16_t>(
       literal.start(), literal.length(), hash_seed_);
   return GetString(hash, false, Vector<const byte>::cast(literal));
 }
 
 
 const AstRawString* AstValueFactory::GetString(Handle<String> literal) {
+  // 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()) {
-    return GetOneByteString(content.ToOneByteVector());

[rossberg, 2014/11/05 12:39:51]

I'm confused, what's the reason to inline these functions? Unless I'm missing
something, the code is identical.

[marja, 2014/11/05 12:41:41]

constness.

Another option would be to make these API functions return AstRawString* instead
of const AstRawString* but that's also somewhat meh.

(The users are anyway supposed to e.g., internalize the AstRawStrings they get,
so returning const AstRawString* is more correct..)

[rossberg, 2014/11/05 12:46:21]

Why not make the functions non-const but private, and have the public ones be
thin const wrappers?

+    Vector<const uint8_t> literal = content.ToOneByteVector();
+    uint32_t hash = StringHasher::HashSequentialString<uint8_t>(
+        literal.start(), literal.length(), hash_seed_);
+    result = GetString(hash, true, literal);
+  } else {
+    DCHECK(content.IsTwoByte());
+    Vector<const uint16_t> literal = content.ToUC16Vector();
+    uint32_t hash = StringHasher::HashSequentialString<uint16_t>(
+        literal.start(), literal.length(), hash_seed_);
+    result = GetString(hash, false, Vector<const byte>::cast(literal));
   }
-  DCHECK(content.IsTwoByte());
-  return GetTwoByteString(content.ToUC16Vector());
+  isolate_ = saved_isolate;
+  result->string_ = literal;
+  return result;
 }
 
 
 const AstConsString* AstValueFactory::NewConsString(
     const AstString* left, const AstString* right) {
   // This Vector will be valid as long as the Collector is alive (meaning that
   // the AstRawString will not be moved).
   AstConsString* new_string = new (zone_) AstConsString(left, right);
   strings_.Add(new_string);
   if (isolate_) {
@@ skipping 92 matching lines @@
 }
 
 
 const AstValue* AstValueFactory::NewTheHole() {
   GENERATE_VALUE_GETTER(the_hole_value_, AstValue::THE_HOLE);
 }
 
 
 #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
   // return this AstRawString.
   AstRawString key(is_one_byte, literal_bytes, hash);
   HashMap::Entry* entry = string_table_.Lookup(&key, hash, true);
   if (entry->value == NULL) {
     // Copy literal contents for later comparison.
     int length = literal_bytes.length();
     byte* new_literal_bytes = zone_->NewArray<byte>(length);
     memcpy(new_literal_bytes, literal_bytes.start(), length);
     AstRawString* new_string = new (zone_) AstRawString(
         is_one_byte, Vector<const byte>(new_literal_bytes, length), hash);
     entry->key = new_string;
     strings_.Add(new_string);
     if (isolate_) {
       new_string->Internalize(isolate_);
     }
     entry->value = reinterpret_cast<void*>(1);
   }
   return reinterpret_cast<AstRawString*>(entry->key);
 }
 
 
 } }  // namespace v8::internal