Issue 2368: LiveEdit crashes when new object/array literal is added

src/liveedit.cc

Index: src/liveedit.cc
diff --git a/src/liveedit.cc b/src/liveedit.cc
index 2a3aafc1f1daa355d817f3d0031315844a4d748f..cc5c28dd667f1a952510d9a20ed969c013c16190 100644
--- a/src/liveedit.cc
+++ b/src/liveedit.cc
@@ -703,12 +703,14 @@ class FunctionInfoWrapper : public JSArrayBasedStruct<FunctionInfoWrapper> {
       : JSArrayBasedStruct<FunctionInfoWrapper>(array) {
   }
   void SetInitialProperties(Handle<String> name, int start_position,
-                            int end_position, int param_num, int parent_index) {
+                            int end_position, int param_num,
+                            int literal_count, int parent_index) {
     HandleScope scope;
     this->SetField(kFunctionNameOffset_, name);
     this->SetSmiValueField(kStartPositionOffset_, start_position);
     this->SetSmiValueField(kEndPositionOffset_, end_position);
     this->SetSmiValueField(kParamNumOffset_, param_num);
+    this->SetSmiValueField(kLiteralNumOffset_, literal_count);
     this->SetSmiValueField(kParentIndexOffset_, parent_index);
   }
   void SetFunctionCode(Handle<Code> function_code,
@@ -726,6 +728,9 @@ class FunctionInfoWrapper : public JSArrayBasedStruct<FunctionInfoWrapper> {
     Handle<JSValue> info_holder = WrapInJSValue(info);
     this->SetField(kSharedFunctionInfoOffset_, info_holder);
   }
+  int GetLiteralCount() {
+    return this->GetSmiValueField(kLiteralNumOffset_);
+  }
   int GetParentIndex() {
     return this->GetSmiValueField(kParentIndexOffset_);
   }
@@ -759,7 +764,8 @@ class FunctionInfoWrapper : public JSArrayBasedStruct<FunctionInfoWrapper> {
   static const int kOuterScopeInfoOffset_ = 6;
   static const int kParentIndexOffset_ = 7;
   static const int kSharedFunctionInfoOffset_ = 8;
-  static const int kSize_ = 9;
+  static const int kLiteralNumOffset_ = 9;
+  static const int kSize_ = 10;
 
   friend class JSArrayBasedStruct<FunctionInfoWrapper>;
 };
@@ -819,6 +825,7 @@ class FunctionInfoListener {
     FunctionInfoWrapper info = FunctionInfoWrapper::Create();
     info.SetInitialProperties(fun->name(), fun->start_position(),
                               fun->end_position(), fun->parameter_count(),
+                              fun->materialized_literal_count(),
                               current_parent_index_);
     current_parent_index_ = len_;
     SetElementNonStrict(result_, len_, info.GetJSArray());
@@ -1014,6 +1021,44 @@ static void ReplaceCodeObject(Handle<Code> original,
 }
 
 
+// Finds all instances of JSFunction that refers to the provided shared_info.
+// Return their number and optionally writes them into output array.
+static int FindJSFunctions(Handle<SharedFunctionInfo> shared_info,
+                           Handle<FixedArray> output) {
+  int pos = 0;
+
+  AssertNoAllocation no_allocations_please;
+
+  SharedFunctionInfo* shared_info_raw = *shared_info;
+  HeapIterator iterator;
+  for (HeapObject* obj = iterator.next(); obj != NULL; obj = iterator.next()) {
+    if (obj->IsJSFunction()) {
+      JSFunction* function = JSFunction::cast(obj);
+      if (function->shared() == shared_info_raw) {
+        if (!output.is_null()) {
+          output->set(pos, function);
+        }
+        pos++;
+      }
+    }
+  }
+  return pos;
+}
+
+
+// Finds all instances of JSFunction that refers to the provided shared_info
+// and returns array with them.
+static Handle<FixedArray> CollectJSFunctions(
+    Handle<SharedFunctionInfo> shared_info, Isolate* isolate) {
+  int size = FindJSFunctions(shared_info, Handle<FixedArray>::null());
+  Handle<FixedArray> result = isolate->factory()->NewFixedArray(size);
+  if (size > 0) {
+    FindJSFunctions(shared_info, result);

[Yang, 2012/10/23 16:10:31]

Running twice over the heap seems really inefficient.

[Peter Rybin, 2012/11/11 03:28:47]

In fact I only copy how Runtime_DebugReferencedBy works. We cannot allocate
buffer when iterating and we cannot put found pointers in Zone, because they
must be wrapped as handles.

However I totally refactored the code to separately support a most probably case
when literal count didn't change -- we can perform this by a single heap
iteration.

+  }
+  return result;
+}
+
+
 // Check whether the code is natural function code (not a lazy-compile stub
 // code).
 static bool IsJSFunctionCode(Code* code) {
@@ -1080,9 +1125,10 @@ MaybeObject* LiveEdit::ReplaceFunctionCode(
     Handle<JSArray> new_compile_info_array,
     Handle<JSArray> shared_info_array) {
   HandleScope scope;
+  Isolate* isolate = Isolate::Current();
 
   if (!SharedInfoWrapper::IsInstance(shared_info_array)) {
-    return Isolate::Current()->ThrowIllegalOperation();
+    return isolate->ThrowIllegalOperation();
   }
 
   FunctionInfoWrapper compile_info_wrapper(new_compile_info_array);
@@ -1113,6 +1159,48 @@ MaybeObject* LiveEdit::ReplaceFunctionCode(
   shared_info->set_start_position(start_position);
   shared_info->set_end_position(end_position);
 
+  // Patch function literals.
+  // TODO(prybin): is the following legit?
+  // Name 'literals' is a misnomer. Rather it's a cache for complex object
+  // boilerplates and for a native context. We must clean cached values or
+  // adjust array size.

[Yang, 2012/10/23 16:10:31]

Wouldn't adjusting array size imply cleaning cached values as well? Please add
that to the comment.

[Peter Rybin, 2012/11/11 03:28:47]

Done.

+  {
+    int new_literal_count = compile_info_wrapper.GetLiteralCount();
+    if (new_literal_count > 0) {
+      new_literal_count += JSFunction::kLiteralsPrefixSize;
+    }
+    shared_info->set_num_literals(new_literal_count);
+
+    Handle<FixedArray> function_instances =
+        CollectJSFunctions(shared_info, isolate);
+    for (int i = 0; i < function_instances->length(); i++) {

[Yang, 2012/10/23 16:10:31]

Instead of looping over function instances that you have to tediously gather,
couldn't you iterate over the heap here and skip every object that is not a
function instance of shared_info? That way you wouldn't need to allocate a new
FixedArray and wouldn't need to iterate the heap twice to find out that
FixedArray's size.

[Peter Rybin, 2012/11/11 03:28:47]

I have to update literal array, and possibly create a new one -- an operation
impossible when iterating heap (as I understand).

+      Handle<JSFunction> fun(JSFunction::cast(function_instances->get(i)));
+      Handle<FixedArray> old_literals(fun->literals());
+      if (old_literals->length() == new_literal_count) {
+        for (int j = JSFunction::kLiteralsPrefixSize;
+            j < new_literal_count; j++) {
+          old_literals->set(j, isolate->heap()->undefined_value());

[Yang, 2012/10/23 16:10:31]

I guess you could use FixedArray->set_undefined here.

[Peter Rybin, 2012/11/11 03:28:47]

Done.

+        }
+      } else {
+        Handle<FixedArray> new_literals =
+            isolate->factory()->NewFixedArray(new_literal_count);
+        if (new_literal_count > 0) {
+          Handle<Context> native_context;
+          if (old_literals->length() >
+              JSFunction::kLiteralNativeContextIndex) {
+            native_context = Handle<Context>(
+                JSFunction::NativeContextFromLiterals(fun->literals()));
+          } else {
+            native_context = Handle<Context>(fun->context()->native_context());
+          }
+          new_literals->set(JSFunction::kLiteralNativeContextIndex,
+              *native_context);
+        }
+        fun->set_literals(*new_literals);
+      }

[Yang, 2012/10/23 16:10:31]

I assume that we are not dealing with optimized code here, right? In that case
only, clearing the literals array is safe (according to mstarzinger@).

[Peter Rybin, 2012/11/11 03:28:47]

May be it more safe to always create an accurate literal array. I don't do a lot
of asserts about optimized/non-optimized.

+    }
+  }
+
   shared_info->set_construct_stub(
       Isolate::Current()->builtins()->builtin(
           Builtins::kJSConstructStubGeneric));