From 9c9451eff3473e029e37fbee9919345bd45d3ada Mon Sep 17 00:00:00 2001
From: "peter.rybin@gmail.com"
 <peter.rybin@gmail.com@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Tue, 9 Mar 2010 21:45:24 +0000
Subject: [PATCH] Check that function being patched has no activations on any
 thread stack

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

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@4069 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---
 src/debug-delay.js                         |  15 ++-
 src/liveedit-delay.js                      | 200 +++++++++++++++++++----------
 src/liveedit.cc                            |   2 +-
 src/liveedit.h                             |   6 +
 src/runtime.cc                             |  38 ++++++
 src/runtime.h                              |   3 +-
 test/mjsunit/debug-liveedit-check-stack.js |  84 ++++++++++++
 test/mjsunit/fuzz-natives.js               |   3 +-
 8 files changed, 275 insertions(+), 76 deletions(-)
 create mode 100644 test/mjsunit/debug-liveedit-check-stack.js

diff --git a/src/debug-delay.js b/src/debug-delay.js
index 3bf1061..5ba5a3b 100644
--- a/src/debug-delay.js
+++ b/src/debug-delay.js
@@ -1986,9 +1986,18 @@ DebugCommandProcessor.prototype.changeLiveRequest_ = function(request, response)
   }
   
   var change_log = new Array();
-  Debug.LiveEditChangeScript(the_script, change_pos, change_len, new_string,
-                             change_log);
-      
+  try {
+    Debug.LiveEditChangeScript(the_script, change_pos, change_len, new_string,
+                               change_log);
+  } catch (e) {
+    if (e instanceof Debug.LiveEditChangeScript.Failure) {
+      // Let's treat it as a "success" so that body with change_log will be
+      // sent back. "change_log" will have "failure" field set.
+      change_log.push( { failure: true } ); 
+    } else {
+      throw e;
+    }
+  }
   response.body = {change_log: change_log};
 };
 
diff --git a/src/liveedit-delay.js b/src/liveedit-delay.js
index 78a3432..ac96956 100644
--- a/src/liveedit-delay.js
+++ b/src/liveedit-delay.js
@@ -47,39 +47,9 @@
 Debug.LiveEditChangeScript = function(script, change_pos, change_len, new_str,
     change_log) {
 
-  function Assert(condition, message) {
-    if (!condition) {
-      if (message) {
-        throw "Assert " + message;
-      } else {
-        throw "Assert";
-      }
-    }
-  }
-    
-  // An object describing function compilation details. Its index fields
-  // apply to indexes inside array that stores these objects.
-  function FunctionCompileInfo(raw_array) {
-    this.function_name = raw_array[0];
-    this.start_position = raw_array[1];
-    this.end_position = raw_array[2];
-    this.param_num = raw_array[3];
-    this.code = raw_array[4];
-    this.scope_info = raw_array[5];
-    this.outer_index = raw_array[6];
-    this.next_sibling_index = null;
-    this.raw_array = raw_array;
-  }
-  
-  // A structure describing SharedFunctionInfo.
-  function SharedInfoWrapper(raw_array) {
-    this.function_name = raw_array[0];
-    this.start_position = raw_array[1];
-    this.end_position = raw_array[2];
-    this.info = raw_array[3];
-    this.raw_array = raw_array;
-  }
-  
+  // So far the function works as namespace.
+  var liveedit = Debug.LiveEditChangeScript;
+  var Assert = liveedit.Assert;
 
   // Fully compiles source string as a script. Returns Array of
   // FunctionCompileInfo -- a descriptions of all functions of the script.
@@ -98,7 +68,7 @@ Debug.LiveEditChangeScript = function(script, change_pos, change_len, new_str,
     var compile_info = new Array();
     var old_index_map = new Array();
     for (var i = 0; i < raw_compile_info.length; i++) {
-        compile_info.push(new FunctionCompileInfo(raw_compile_info[i]));
+        compile_info.push(new liveedit.FunctionCompileInfo(raw_compile_info[i]));
         old_index_map.push(i);
     }
     
@@ -169,30 +139,6 @@ Debug.LiveEditChangeScript = function(script, change_pos, change_len, new_str,
     return index;
   }
 
-  // Compares a function interface old and new version, whether it
-  // changed or not.
-  function CompareFunctionExpectations(function_info1, function_info2) {
-    // Check that function has the same number of parameters (there may exist
-    // an adapter, that won't survive function parameter number change).
-    if (function_info1.param_num != function_info2.param_num) {
-      return false;
-    }
-    var scope_info1 = function_info1.scope_info;
-    var scope_info2 = function_info2.scope_info;
-    
-    if (!scope_info1) {
-      return !scope_info2;
-    }
-    
-    if (scope_info1.length != scope_info2.length) {
-      return false;
-    }
-
-    // Check that outer scope structure is not changed. Otherwise the function
-    // will not properly work with existing scopes.
-    return scope_info1.toString() == scope_info2.toString(); 
-  }
-  
   // Variable forward declarations. Preprocessor "Minifier" needs them.
   var old_compile_info;
   var shared_infos;
@@ -265,6 +211,12 @@ Debug.LiveEditChangeScript = function(script, change_pos, change_len, new_str,
 
   // Find all SharedFunctionInfo's that are compiled from this script.
   var shared_raw_list = %LiveEditFindSharedFunctionInfosForScript(script);
+
+  var shared_infos = new Array();
+
+  for (var i = 0; i < shared_raw_list.length; i++) {
+    shared_infos.push(new liveedit.SharedInfoWrapper(shared_raw_list[i]));
+  }
   
   // Gather compile information about old version of script.
   var old_compile_info = DebugGatherCompileInfo(old_source);
@@ -274,12 +226,13 @@ Debug.LiveEditChangeScript = function(script, change_pos, change_len, new_str,
   try {
     new_compile_info = DebugGatherCompileInfo(new_source);
   } catch (e) {
-    throw "Failed to compile new version of script: " + e;
+    throw new liveedit.Failure("Failed to compile new version of script: " + e);
   }
 
   // An index of a single function, that is going to have its code replaced.
   var function_being_patched =
-      FindChangedFunction(old_compile_info, change_pos, change_len_old); 
+      FindChangedFunction(old_compile_info, change_pos, change_len_old);
+
   // In old and new script versions function with a change should have the
   // same indexes.
   var function_being_patched2 =
@@ -290,7 +243,8 @@ Debug.LiveEditChangeScript = function(script, change_pos, change_len, new_str,
   // Check that function being patched has the same expectations in a new
   // version. Otherwise we cannot safely patch its behavior and should
   // choose the outer function instead.
-  while (!CompareFunctionExpectations(old_compile_info[function_being_patched],
+  while (!liveedit.CompareFunctionExpectations(
+      old_compile_info[function_being_patched],
       new_compile_info[function_being_patched])) {
 
     Assert(old_compile_info[function_being_patched].outer_index == 
@@ -300,22 +254,18 @@ Debug.LiveEditChangeScript = function(script, change_pos, change_len, new_str,
     Assert(function_being_patched != -1);
   }
   
-  // TODO: Need to check here that there are no activations of the function
-  // being patched on stack.  
+  // Check that function being patched is not currently on stack.
+  liveedit.CheckStackActivations(
+      [ FindFunctionInfo(function_being_patched) ], change_log );
+  
 
   // Committing all changes.
-  var old_script_name = script.name + " (old)";
+  var old_script_name = liveedit.CreateNameForOldScript(script); 
 
   // Update the script text and create a new script representing an old
   // version of the script.
   var old_script = %LiveEditReplaceScript(script, new_source, old_script_name);
 
-  var shared_infos = new Array();
-
-  for (var i = 0; i < shared_raw_list.length; i++) {
-    shared_infos.push(new SharedInfoWrapper(shared_raw_list[i]));
-  }
-
   PatchCode(new_compile_info[function_being_patched],
       FindFunctionInfo(function_being_patched));
 
@@ -364,3 +314,113 @@ Debug.LiveEditChangeScript = function(script, change_pos, change_len, new_str,
   }
 }
 
+Debug.LiveEditChangeScript.Assert = function(condition, message) {
+  if (!condition) {
+    if (message) {
+      throw "Assert " + message;
+    } else {
+      throw "Assert";
+    }
+  }
+}
+    
+// An object describing function compilation details. Its index fields
+// apply to indexes inside array that stores these objects.
+Debug.LiveEditChangeScript.FunctionCompileInfo = function(raw_array) {
+  this.function_name = raw_array[0];
+  this.start_position = raw_array[1];
+  this.end_position = raw_array[2];
+  this.param_num = raw_array[3];
+  this.code = raw_array[4];
+  this.scope_info = raw_array[5];
+  this.outer_index = raw_array[6];
+  this.next_sibling_index = null;
+  this.raw_array = raw_array;
+}
+  
+// A structure describing SharedFunctionInfo.
+Debug.LiveEditChangeScript.SharedInfoWrapper = function(raw_array) {
+  this.function_name = raw_array[0];
+  this.start_position = raw_array[1];
+  this.end_position = raw_array[2];
+  this.info = raw_array[3];
+  this.raw_array = raw_array;
+}
+
+// Adds a suffix to script name to mark that it is old version.
+Debug.LiveEditChangeScript.CreateNameForOldScript = function(script) {
+  // TODO(635): try better than this; support several changes.
+  return script.name + " (old)";
+}
+
+// Compares a function interface old and new version, whether it
+// changed or not.
+Debug.LiveEditChangeScript.CompareFunctionExpectations =
+    function(function_info1, function_info2) {
+  // Check that function has the same number of parameters (there may exist
+  // an adapter, that won't survive function parameter number change).
+  if (function_info1.param_num != function_info2.param_num) {
+    return false;
+  }
+  var scope_info1 = function_info1.scope_info;
+  var scope_info2 = function_info2.scope_info;
+  
+  if (!scope_info1) {
+    return !scope_info2;
+  }
+  
+  if (scope_info1.length != scope_info2.length) {
+    return false;
+  }
+
+  // Check that outer scope structure is not changed. Otherwise the function
+  // will not properly work with existing scopes.
+  return scope_info1.toString() == scope_info2.toString(); 
+}
+
+// For array of wrapped shared function infos checks that none of them
+// have activations on stack (of any thread). Throws a Failure exception
+// if this proves to be false.
+Debug.LiveEditChangeScript.CheckStackActivations = function(shared_wrapper_list,
+                                                            change_log) {
+  var liveedit = Debug.LiveEditChangeScript;
+	  
+  var shared_list = new Array();
+  for (var i = 0; i < shared_wrapper_list.length; i++) {
+    shared_list[i] = shared_wrapper_list[i].info;
+  }
+  var result = %LiveEditCheckStackActivations(shared_list);
+  var problems = new Array();
+  for (var i = 0; i < shared_list.length; i++) {
+    if (result[i] == liveedit.FunctionPatchabilityStatus.FUNCTION_BLOCKED_ON_STACK) {
+      var shared = shared_list[i];
+      var description = {
+          name: shared.function_name,
+          start_pos: shared.start_position, 
+          end_pos: shared.end_position
+      };
+      problems.push(description);
+    }
+  }
+  if (problems.length > 0) {
+    change_log.push( { functions_on_stack: problems } );
+    throw new liveedit.Failure("Blocked by functions on stack");
+  }
+}
+
+// A copy of the FunctionPatchabilityStatus enum from liveedit.h
+Debug.LiveEditChangeScript.FunctionPatchabilityStatus = {
+    FUNCTION_AVAILABLE_FOR_PATCH: 0,
+    FUNCTION_BLOCKED_ON_STACK: 1
+}
+
+
+// A logical failure in liveedit process. This means that change_log
+// is valid and consistent description of what happened.
+Debug.LiveEditChangeScript.Failure = function(message) {
+  this.message = message;
+}
+
+Debug.LiveEditChangeScript.Failure.prototype.toString = function() {
+  return "LiveEdit Failure: " + this.message;     
+}
diff --git a/src/liveedit.cc b/src/liveedit.cc
index 2c27e9f..513cc02 100644
--- a/src/liveedit.cc
+++ b/src/liveedit.cc
@@ -320,9 +320,9 @@ JSArray* LiveEdit::GatherCompileInfo(Handle<Script> script,
   CompilationZoneScope zone_scope(DELETE_ON_EXIT);
 
   FunctionInfoListener listener;
+  Handle<Object> original_source = Handle<Object>(script->source());
   script->set_source(*source);
   active_function_info_listener = &listener;
-  Handle<Object> original_source = Handle<Object>(script->source());
   CompileScriptForTracker(script);
   active_function_info_listener = NULL;
   script->set_source(*original_source);
diff --git a/src/liveedit.h b/src/liveedit.h
index 19caf95..efbcd74 100644
--- a/src/liveedit.h
+++ b/src/liveedit.h
@@ -90,6 +90,12 @@ class LiveEdit : AllStatic {
 
   static void PatchFunctionPositions(Handle<JSArray> shared_info_array,
                                      Handle<JSArray> position_change_array);
+
+  // A copy of this is in liveedit-delay.js.
+  enum FunctionPatchabilityStatus {
+    FUNCTION_AVAILABLE_FOR_PATCH = 0,
+    FUNCTION_BLOCKED_ON_STACK = 1
+  };
 };
 
 #endif  // ENABLE_DEBUGGER_SUPPORT
diff --git a/src/runtime.cc b/src/runtime.cc
index e8abdef..6d3a158 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -8443,6 +8443,44 @@ static Object* Runtime_LiveEditPatchFunctionPositions(Arguments args) {
 }
 
 
+static LiveEdit::FunctionPatchabilityStatus FindFunctionCodeOnStacks(
+    Handle<SharedFunctionInfo> shared) {
+  // TODO(635): check all threads, not only the current one.
+  for (StackFrameIterator it; !it.done(); it.Advance()) {
+    StackFrame* frame = it.frame();
+    if (frame->code() == shared->code()) {
+      return LiveEdit::FUNCTION_BLOCKED_ON_STACK;
+    }
+  }
+  return LiveEdit::FUNCTION_AVAILABLE_FOR_PATCH;
+}
+
+// For array of SharedFunctionInfo's (each wrapped in JSValue)
+// checks that none of them have activations on stacks (of any thread).
+// Returns array of the same length with corresponding results of
+// LiveEdit::FunctionPatchabilityStatus type.
+static Object* Runtime_LiveEditCheckStackActivations(Arguments args) {
+  ASSERT(args.length() == 1);
+  HandleScope scope;
+  CONVERT_ARG_CHECKED(JSArray, shared_array, 0);
+
+
+  int len = Smi::cast(shared_array->length())->value();
+  Handle<JSArray> result = Factory::NewJSArray(len);
+
+  for (int i = 0; i < len; i++) {
+    JSValue* wrapper = JSValue::cast(shared_array->GetElement(i));
+    Handle<SharedFunctionInfo> shared(
+        SharedFunctionInfo::cast(wrapper->value()));
+    LiveEdit::FunctionPatchabilityStatus check_res =
+        FindFunctionCodeOnStacks(shared);
+    SetElement(result, i, Handle<Smi>(Smi::FromInt(check_res)));
+  }
+
+  return *result;
+}
+
+
 #endif  // ENABLE_DEBUGGER_SUPPORT
 
 #ifdef ENABLE_LOGGING_AND_PROFILING
diff --git a/src/runtime.h b/src/runtime.h
index f9dc599..8c2f86d 100644
--- a/src/runtime.h
+++ b/src/runtime.h
@@ -331,7 +331,8 @@ namespace internal {
   F(LiveEditReplaceScript, 3, 1) \
   F(LiveEditReplaceFunctionCode, 2, 1) \
   F(LiveEditRelinkFunctionToScript, 2, 1) \
-  F(LiveEditPatchFunctionPositions, 2, 1)
+  F(LiveEditPatchFunctionPositions, 2, 1) \
+  F(LiveEditCheckStackActivations, 1, 1)
 #else
 #define RUNTIME_FUNCTION_LIST_DEBUGGER_SUPPORT(F)
 #endif
diff --git a/test/mjsunit/debug-liveedit-check-stack.js b/test/mjsunit/debug-liveedit-check-stack.js
new file mode 100644
index 0000000..1d788be
--- /dev/null
+++ b/test/mjsunit/debug-liveedit-check-stack.js
@@ -0,0 +1,84 @@
+// Copyright 2010 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
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (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: --expose-debug-as debug
+// Get the Debug object exposed from the debug context global object.
+
+Debug = debug.Debug
+
+eval(
+    "function ChooseAnimal(callback) {\n " +
+    "  callback();\n" +
+    "  return 'Cat';\n" +
+    "}\n"
+);
+
+function Noop() {}
+var res = ChooseAnimal(Noop);
+
+assertEquals("Cat", res);
+
+var script = Debug.findScript(ChooseAnimal);
+
+var orig_animal = "'Cat'";
+var patch_pos = script.source.indexOf(orig_animal);
+var new_animal_patch = "'Capybara'";
+
+var got_exception = false;
+var successfully_changed = false;
+
+function Changer() {
+  // Never try the same patch again.
+  assertEquals(false, successfully_changed);
+  var change_log = new Array();
+  try {
+    Debug.LiveEditChangeScript(script, patch_pos, orig_animal.length, new_animal_patch, change_log);
+    successfully_changed = true;
+  } catch (e) {
+    if (e instanceof Debug.LiveEditChangeScript.Failure) {
+      got_exception = true;
+      print(e);
+    } else {
+      throw e;
+    }
+  }
+  print("Change log: " + JSON.stringify(change_log) + "\n");
+}
+
+var new_res = ChooseAnimal(Changer);
+// Function must be not pached.
+assertEquals("Cat", new_res);
+
+assertEquals(true, got_exception);
+
+// This time it should succeed.
+Changer();
+
+new_res = ChooseAnimal(Noop);
+// Function must be not pached.
+assertEquals("Capybara", new_res);
+
diff --git a/test/mjsunit/fuzz-natives.js b/test/mjsunit/fuzz-natives.js
index 730f7b4..3750388 100644
--- a/test/mjsunit/fuzz-natives.js
+++ b/test/mjsunit/fuzz-natives.js
@@ -158,7 +158,8 @@ var knownProblems = {
   "LiveEditReplaceScript": true,
   "LiveEditReplaceFunctionCode": true,
   "LiveEditRelinkFunctionToScript": true,
-  "LiveEditPatchFunctionPositions": true
+  "LiveEditPatchFunctionPositions": true,
+  "LiveEditCheckStackActivations": true
 };
 
 var currentlyUncallable = {
-- 
2.7.4