Fix a crash in Logger::LogCompiledFunctions due to a presence of scripts with dispose...

When starting JS profiling under Chromium, a map from function addresses to function names is created. During it, for sourceful scripts, an attempt to access script source is made. This can cause a crash, if a source is an external string, which already has been disposed. We had a similar problem in the past with DebugGetLoadedScripts.

BUG=http://crbug.com/23768
TEST=test-log/Issue23768

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

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@3027 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/debug.cc b/src/debug.cc
index ec658d68f06844f7ca1a93697b763b7311be58ff..4f89f92e97c9c23edd775089a61f7d6fff7d7230 100644 (file)
--- a/src/debug.cc
+++ b/src/debug.cc
@@ -1677,22 +1677,6 @@ void Debug::ClearMirrorCache() {
 }
 
 
-// If an object given is an external string, check that the underlying
-// resource is accessible. For other kinds of objects, always return true.
-static bool IsExternalStringValid(Object* str) {
-  if (!str->IsString() || !StringShape(String::cast(str)).IsExternal()) {
-    return true;
-  }
-  if (String::cast(str)->IsAsciiRepresentation()) {
-    return ExternalAsciiString::cast(str)->resource() != NULL;
-  } else if (String::cast(str)->IsTwoByteRepresentation()) {
-    return ExternalTwoByteString::cast(str)->resource() != NULL;
-  } else {
-    return true;
-  }
-}
-
-
 void Debug::CreateScriptCache() {
   HandleScope scope;
 
@@ -1711,7 +1695,7 @@ void Debug::CreateScriptCache() {
   while (iterator.has_next()) {
     HeapObject* obj = iterator.next();
     ASSERT(obj != NULL);
-    if (obj->IsScript() && IsExternalStringValid(Script::cast(obj)->source())) {
+    if (obj->IsScript() && Script::cast(obj)->HasValidSource()) {
       script_cache_->Add(Handle<Script>(Script::cast(obj)));
       count++;
     }

diff --git a/src/log.cc b/src/log.cc
index d225c3b49515a4df9d3574fe6246d92bcc95d62d..1c82f8525c95f954e87091a7d3b964e7c6fe24aa 100644 (file)
--- a/src/log.cc
+++ b/src/log.cc
@@ -1070,37 +1070,33 @@ int Logger::GetLogLines(int from_pos, char* dest_buf, int max_size) {
 }
 
 
-void Logger::LogCompiledFunctions() {
-  HandleScope scope;
-  Handle<SharedFunctionInfo>* sfis = NULL;
+static int EnumerateCompiledFunctions(Handle<SharedFunctionInfo>* sfis) {
+  AssertNoAllocation no_alloc;
   int compiled_funcs_count = 0;
-
-  {
-    AssertNoAllocation no_alloc;
-
-    HeapIterator iterator;
-    while (iterator.has_next()) {
-      HeapObject* obj = iterator.next();
-      ASSERT(obj != NULL);
-      if (obj->IsSharedFunctionInfo()
-          && SharedFunctionInfo::cast(obj)->is_compiled()) {
-        ++compiled_funcs_count;
-      }
+  HeapIterator iterator;
+  while (iterator.has_next()) {
+    HeapObject* obj = iterator.next();
+    ASSERT(obj != NULL);
+    if (!obj->IsSharedFunctionInfo()) continue;
+    SharedFunctionInfo* sfi = SharedFunctionInfo::cast(obj);
+    if (sfi->is_compiled()
+        && (!sfi->script()->IsScript()
+            || Script::cast(sfi->script())->HasValidSource())) {
+      if (sfis != NULL)
+        sfis[compiled_funcs_count] = Handle<SharedFunctionInfo>(sfi);
+      ++compiled_funcs_count;
     }
+  }
+  return compiled_funcs_count;
+}
 
-    sfis = NewArray< Handle<SharedFunctionInfo> >(compiled_funcs_count);
-    iterator.reset();
 
-    int i = 0;
-    while (iterator.has_next()) {
-      HeapObject* obj = iterator.next();
-      ASSERT(obj != NULL);
-      if (obj->IsSharedFunctionInfo()
-          && SharedFunctionInfo::cast(obj)->is_compiled()) {
-        sfis[i++] = Handle<SharedFunctionInfo>(SharedFunctionInfo::cast(obj));
-      }
-    }
-  }
+void Logger::LogCompiledFunctions() {
+  HandleScope scope;
+  const int compiled_funcs_count = EnumerateCompiledFunctions(NULL);
+  Handle<SharedFunctionInfo>* sfis =
+      NewArray< Handle<SharedFunctionInfo> >(compiled_funcs_count);
+  EnumerateCompiledFunctions(sfis);
 
   // During iteration, there can be heap allocation due to
   // GetScriptLineNumber call.

diff --git a/src/objects-inl.h b/src/objects-inl.h
index 29b886d87bd5c08ea5d2a95f30db4ee622b7bf4e..22e12ac7d0f4741f910ba120a20473d43fa1c39c 100644 (file)
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -2361,6 +2361,20 @@ INT_ACCESSORS(SharedFunctionInfo, this_property_assignments_count,
               kThisPropertyAssignmentsCountOffset)
 
 
+bool Script::HasValidSource() {
+  Object* src = this->source();
+  if (!src->IsString()) return true;
+  String* src_str = String::cast(src);
+  if (!StringShape(src_str).IsExternal()) return true;
+  if (src_str->IsAsciiRepresentation()) {
+    return ExternalAsciiString::cast(src)->resource() != NULL;
+  } else if (src_str->IsTwoByteRepresentation()) {
+    return ExternalTwoByteString::cast(src)->resource() != NULL;
+  }
+  return true;
+}
+
+
 void SharedFunctionInfo::DontAdaptArguments() {
   ASSERT(code()->kind() == Code::BUILTIN);
   set_formal_parameter_count(kDontAdaptArgumentsSentinel);

diff --git a/src/objects.h b/src/objects.h
index e9430f5b34f5ed25ea0511ef2a964e1e0da3d900..c78c73b3e5b302e15114b78be659bd1275c1a927 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -2998,6 +2998,10 @@ class Script: public Struct {
 
   static inline Script* cast(Object* obj);
 
+  // If script source is an external string, check that the underlying
+  // resource is accessible. Otherwise, always return true.
+  inline bool HasValidSource();
+
 #ifdef DEBUG
   void ScriptPrint();
   void ScriptVerify();

diff --git a/test/cctest/test-log.cc b/test/cctest/test-log.cc
index 65ab50a04f2b0d040b01bc617313d73e123d1c64..3983215a29685596df5b8ac9b7e4aca676cbf887 100644 (file)
--- a/test/cctest/test-log.cc
+++ b/test/cctest/test-log.cc
@@ -430,6 +430,50 @@ TEST(ProfMultipleThreads) {
 #endif  // __linux__
 
 
+// Test for issue http://crbug.com/23768 in Chromium.
+// Heap can contain scripts with already disposed external sources.
+// We need to verify that LogCompiledFunctions doesn't crash on them.
+namespace {
+
+class SimpleExternalString : public v8::String::ExternalStringResource {
+ public:
+  explicit SimpleExternalString(const char* source)
+      : utf_source_(strlen(source)) {
+    for (int i = 0; i < utf_source_.length(); ++i)
+      utf_source_[i] = source[i];
+  }
+  virtual ~SimpleExternalString() {}
+  virtual size_t length() const { return utf_source_.length(); }
+  virtual const uint16_t* data() const { return utf_source_.start(); }
+ private:
+  i::ScopedVector<uint16_t> utf_source_;
+};
+
+}  // namespace
+
+TEST(Issue23768) {
+  v8::HandleScope scope;
+  v8::Handle<v8::Context> env = v8::Context::New();
+  env->Enter();
+
+  SimpleExternalString source_ext_str("(function ext() {})();");
+  v8::Local<v8::String> source = v8::String::NewExternal(&source_ext_str);
+  // Script needs to have a name in order to trigger InitLineEnds execution.
+  v8::Handle<v8::String> origin = v8::String::New("issue-23768-test");
+  v8::Handle<v8::Script> evil_script = v8::Script::Compile(source, origin);
+  CHECK(!evil_script.IsEmpty());
+  CHECK(!evil_script->Run().IsEmpty());
+  i::Handle<i::ExternalTwoByteString> i_source(
+      i::ExternalTwoByteString::cast(*v8::Utils::OpenHandle(*source)));
+  // This situation can happen if source was an external string disposed
+  // by its owner.
+  i_source->set_resource(NULL);
+
+  // Must not crash.
+  i::Logger::LogCompiledFunctions();
+}
+
+
 static inline bool IsStringEqualTo(const char* r, const char* s) {
   return strncmp(r, s, strlen(r)) == 0;
 }