From 9142c42df29f5cd7966ac0101ff0e5ec04fac0ee Mon Sep 17 00:00:00 2001 From: "christian.plesner.hansen@gmail.com" Date: Fri, 27 Mar 2009 00:24:49 +0000 Subject: [PATCH] Fixed a bunch of memory leaks in tests, including: - String traversal test data (now in a zone) - Debug message thread (now joined on exit) - Threading test threads (now joined on exit) - Changed message tests framework to cope with valgrind Also, fixed a bug where we'd try to delete stack-allocated objects when tearing down v8. Good times. git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1622 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- include/v8.h | 15 +++++++++++++-- samples/shell.cc | 10 ++++++++-- src/api.cc | 6 ++++++ src/debug.cc | 24 ++++++++++++++++++++---- src/debug.h | 5 ++++- src/flag-definitions.h | 2 +- src/flags.cc | 31 ++++++++++++++++++------------- src/log.cc | 5 ++--- src/log.h | 5 ++++- src/platform-linux.cc | 1 + src/v8.cc | 2 ++ src/zone-inl.h | 6 ++++++ src/zone.h | 3 +++ test/cctest/cctest.cc | 2 ++ test/cctest/test-api.cc | 6 +++++- test/cctest/test-strings.cc | 9 +++++++-- test/message/testcfg.py | 8 +++++++- 17 files changed, 109 insertions(+), 31 deletions(-) diff --git a/include/v8.h b/include/v8.h index a6851c9..2be7670 100644 --- a/include/v8.h +++ b/include/v8.h @@ -1080,13 +1080,13 @@ class V8EXPORT Object : public Value { * access check info, the object cannot be accessed by anyone. */ void TurnOnAccessCheck(); - + /** * Returns the identity hash for this object. The current implemenation uses * a hidden property on the object to store the identity hash. */ int GetIdentityHash(); - + /** * Access hidden properties on JavaScript objects. These properties are * hidden from the executing JavaScript and only accessible through the V8 @@ -2033,6 +2033,17 @@ class V8EXPORT V8 { */ static void ResumeProfiler(); + /** + * Releases any resources used by v8 and stops any utility threads + * that may be running. Note that disposing v8 is permanent, it + * cannot be reinitialized. + * + * It should generally not be necessary to dispose v8 before exiting + * a process, this should happen automatically. It is only necessary + * to use if the process needs the resources taken up by v8. + */ + static bool Dispose(); + private: V8(); diff --git a/samples/shell.cc b/samples/shell.cc index d85e6ea..5c46c10 100644 --- a/samples/shell.cc +++ b/samples/shell.cc @@ -45,7 +45,7 @@ v8::Handle ReadFile(const char* name); void ReportException(v8::TryCatch* handler); -int main(int argc, char* argv[]) { +int RunMain(int argc, char* argv[]) { v8::V8::SetFlagsFromCommandLine(&argc, argv, true); v8::HandleScope handle_scope; // Create a template for the global object. @@ -95,11 +95,17 @@ int main(int argc, char* argv[]) { return 1; } } - if (run_shell) RunShell(context); return 0; } +int main(int argc, char* argv[]) { + int result = RunMain(argc, argv); + v8::V8::Dispose(); + return result; +} + + // Extracts a C string from a V8 Utf8Value. const char* ToCString(const v8::String::Utf8Value& value) { return *value ? *value : ""; diff --git a/src/api.cc b/src/api.cc index 0b05b83..21808c5 100644 --- a/src/api.cc +++ b/src/api.cc @@ -2272,6 +2272,12 @@ bool v8::V8::Initialize() { } +bool v8::V8::Dispose() { + i::V8::TearDown(); + return true; +} + + const char* v8::V8::GetVersion() { return "1.1.4 (candidate)"; } diff --git a/src/debug.cc b/src/debug.cc index 7e0b298..db75d0b 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -1737,6 +1737,15 @@ void Debugger::SetEventListener(Handle callback, } +void Debugger::TearDown() { + if (message_thread_ != NULL) { + message_thread_->Stop(); + delete message_thread_; + message_thread_ = NULL; + } +} + + void Debugger::SetMessageHandler(v8::DebugMessageHandler handler, void* data) { message_handler_ = handler; message_handler_data_ = data; @@ -1852,16 +1861,23 @@ void Debugger::StopAgent() { DebugMessageThread::DebugMessageThread() : host_running_(true), command_queue_(kQueueInitialSize), - message_queue_(kQueueInitialSize) { + message_queue_(kQueueInitialSize), + keep_running_(true) { command_received_ = OS::CreateSemaphore(0); message_received_ = OS::CreateSemaphore(0); } -// Does not free resources held by DebugMessageThread -// because this cannot be done thread-safely. +// Should only be done after the thread is done running. DebugMessageThread::~DebugMessageThread() { + delete command_received_; + delete message_received_; } +void DebugMessageThread::Stop() { + keep_running_ = false; + SendMessage(Vector(NULL, 0)); + Join(); +} // Puts an event coming from V8 on the queue. Creates // a copy of the JSON formatted event string managed by the V8. @@ -1909,7 +1925,7 @@ bool DebugMessageThread::SetEventJSONFromEvent(Handle event_data) { void DebugMessageThread::Run() { // Sends debug events to an installed debugger message callback. - while (true) { + while (keep_running_) { // Wait and Get are paired so that semaphore count equals queue length. message_received_->Wait(); Logger::DebugTag("Get message from event message_queue."); diff --git a/src/debug.h b/src/debug.h index c29b49b..d33c701 100644 --- a/src/debug.h +++ b/src/debug.h @@ -429,6 +429,7 @@ class Debugger { bool auto_continue); static void SetEventListener(Handle callback, Handle data); static void SetMessageHandler(v8::DebugMessageHandler handler, void* data); + static void TearDown(); static void SetHostDispatchHandler(v8::DebugHostDispatchHandler handler, void* data); static void SendMessage(Vector message); @@ -527,7 +528,7 @@ class LockingMessageQueue BASE_EMBEDDED { class DebugMessageThread: public Thread { public: DebugMessageThread(); // Called from API thread. - virtual ~DebugMessageThread(); // Never called. + virtual ~DebugMessageThread(); // Called by V8 thread. Reports events from V8 VM. // Also handles command processing in stopped state of V8, // when host_running_ is false. @@ -554,6 +555,7 @@ class DebugMessageThread: public Thread { // Check whether there are commands in the queue. bool HasCommands() { return !command_queue_.IsEmpty(); } + void Stop(); bool host_running_; // Is the debugging host running or stopped? Semaphore* command_received_; // Non-zero when command queue is non-empty. @@ -564,6 +566,7 @@ class DebugMessageThread: public Thread { static const int kQueueInitialSize = 4; LockingMessageQueue command_queue_; LockingMessageQueue message_queue_; + bool keep_running_; DISALLOW_COPY_AND_ASSIGN(DebugMessageThread); }; diff --git a/src/flag-definitions.h b/src/flag-definitions.h index c3580c5..db49453 100644 --- a/src/flag-definitions.h +++ b/src/flag-definitions.h @@ -62,7 +62,7 @@ // printing / etc in the flag parser code. We only do this for writable flags. #elif defined(FLAG_MODE_META) #define FLAG_FULL(ftype, ctype, nam, def, cmt) \ - { Flag::TYPE_##ftype, #nam, &FLAG_##nam, &FLAGDEFAULT_##nam, cmt }, + { Flag::TYPE_##ftype, #nam, &FLAG_##nam, &FLAGDEFAULT_##nam, cmt, false }, #define FLAG_READONLY(ftype, ctype, nam, def, cmt) #else diff --git a/src/flags.cc b/src/flags.cc index 31e35f0..733d31a 100644 --- a/src/flags.cc +++ b/src/flags.cc @@ -58,6 +58,7 @@ struct Flag { void* valptr_; // Pointer to the global flag variable. const void* defptr_; // Pointer to the default value. const char* cmt_; // A comment about the flags purpose. + bool owns_ptr_; // Does the flag own its string value? FlagType type() const { return type_; } @@ -80,9 +81,17 @@ struct Flag { return reinterpret_cast(valptr_); } - const char** string_variable() const { + const char* string_value() const { ASSERT(type_ == TYPE_STRING); - return reinterpret_cast(valptr_); + return *reinterpret_cast(valptr_); + } + + void set_string_value(const char *value, bool owns_ptr) { + ASSERT(type_ == TYPE_STRING); + const char **ptr = reinterpret_cast(valptr_); + if (owns_ptr_ && *ptr != NULL) DeleteArray(*ptr); + *ptr = value; + owns_ptr_ = owns_ptr; } JSArguments* args_variable() const { @@ -125,7 +134,7 @@ struct Flag { case TYPE_FLOAT: return *float_variable() == float_default(); case TYPE_STRING: { - const char* str1 = *string_variable(); + const char* str1 = string_value(); const char* str2 = string_default(); if (str2 == NULL) return str1 == NULL; if (str1 == NULL) return str2 == NULL; @@ -151,7 +160,7 @@ struct Flag { *float_variable() = float_default(); break; case TYPE_STRING: - *string_variable() = string_default(); + set_string_value(string_default(), false); break; case TYPE_ARGS: *args_variable() = args_default(); @@ -197,7 +206,7 @@ static SmartPointer ToString(Flag* flag) { buffer.Add("%f", FmtElm(*flag->float_variable())); break; case Flag::TYPE_STRING: { - const char* str = *flag->string_variable(); + const char* str = flag->string_value(); buffer.Add("%s", str ? str : "NULL"); break; } @@ -389,17 +398,17 @@ int FlagList::SetFlagsFromCommandLine(int* argc, *flag->float_variable() = strtod(value, &endp); break; case Flag::TYPE_STRING: - *flag->string_variable() = value; + flag->set_string_value(value ? StrDup(value) : NULL, true); break; case Flag::TYPE_ARGS: { int start_pos = (value == NULL) ? i : i - 1; int js_argc = *argc - start_pos; const char** js_argv = NewArray(js_argc); if (value != NULL) { - js_argv[0] = value; + js_argv[0] = StrDup(value); } for (int k = i; k < *argc; k++) { - js_argv[k - start_pos] = argv[k]; + js_argv[k - start_pos] = StrDup(argv[k]); } *flag->args_variable() = JSArguments(js_argc, js_argv); i = *argc; // Consume all arguments @@ -491,11 +500,7 @@ int FlagList::SetFlagsFromString(const char* str, int len) { // cleanup DeleteArray(argv); - // don't delete copy0 since the substrings - // may be pointed to by FLAG variables! - // (this is a memory leak, but it's minor since this - // code is only used for debugging, or perhaps once - // during initialization). + DeleteArray(copy0); return result; } diff --git a/src/log.cc b/src/log.cc index f3443fa..c4998c3 100644 --- a/src/log.cc +++ b/src/log.cc @@ -407,6 +407,7 @@ FILE* Logger::logfile_ = NULL; Profiler* Logger::profiler_ = NULL; Mutex* Logger::mutex_ = NULL; VMState* Logger::current_state_ = NULL; +VMState Logger::bottom_state_(OTHER); SlidingStateWindow* Logger::sliding_state_window_ = NULL; #endif // ENABLE_LOGGING_AND_PROFILING @@ -1017,7 +1018,7 @@ bool Logger::Setup() { mutex_ = OS::CreateMutex(); } - current_state_ = new VMState(OTHER); + current_state_ = &bottom_state_; // as log is initialized early with V8, we can assume that JS execution // frames can never reach this point on stack @@ -1052,8 +1053,6 @@ void Logger::TearDown() { profiler_ = NULL; } - // Deleting the current_state_ has the side effect of assigning to it(!). - while (current_state_) delete current_state_; delete sliding_state_window_; delete ticker_; diff --git a/src/log.h b/src/log.h index d238f9b..8573d71 100644 --- a/src/log.h +++ b/src/log.h @@ -83,7 +83,7 @@ class LogMessageBuilder; #endif -class VMState { +class VMState BASE_EMBEDDED { #ifdef ENABLE_LOGGING_AND_PROFILING public: explicit VMState(StateTag state); @@ -252,6 +252,9 @@ class Logger { // A stack of VM states. static VMState* current_state_; + // Singleton bottom or default vm state. + static VMState bottom_state_; + // SlidingStateWindow instance keeping a sliding window of the most // recent VM states. static SlidingStateWindow* sliding_state_window_; diff --git a/src/platform-linux.cc b/src/platform-linux.cc index a834667..f4672b4 100644 --- a/src/platform-linux.cc +++ b/src/platform-linux.cc @@ -417,6 +417,7 @@ class ThreadHandle::PlatformData : public Malloced { case ThreadHandle::INVALID: thread_ = kNoThread; break; } } + pthread_t thread_; // Thread handle for pthread. }; diff --git a/src/v8.cc b/src/v8.cc index 7eb39bc..9158b52 100644 --- a/src/v8.cc +++ b/src/v8.cc @@ -111,6 +111,8 @@ void V8::TearDown() { Heap::TearDown(); Logger::TearDown(); + Debugger::TearDown(); + has_been_setup_ = false; has_been_disposed_ = true; } diff --git a/src/zone-inl.h b/src/zone-inl.h index fd66b64..69b9a0a 100644 --- a/src/zone-inl.h +++ b/src/zone-inl.h @@ -50,6 +50,12 @@ inline void* Zone::New(int size) { } +template +T* Zone::NewArray(int length) { + return static_cast(Zone::New(length * sizeof(T))); +} + + bool Zone::excess_allocation() { return segment_bytes_allocated_ > zone_excess_limit_; } diff --git a/src/zone.h b/src/zone.h index 45c52a2..df69155 100644 --- a/src/zone.h +++ b/src/zone.h @@ -58,6 +58,9 @@ class Zone { // allocating new segments of memory on demand using malloc(). static inline void* New(int size); + template + static inline T* NewArray(int length); + // Delete all objects and free all memory allocated in the Zone. static void DeleteAll(); diff --git a/test/cctest/cctest.cc b/test/cctest/cctest.cc index d908890..2807c8b 100644 --- a/test/cctest/cctest.cc +++ b/test/cctest/cctest.cc @@ -30,6 +30,7 @@ #include #include #include "cctest.h" +#include "debug.h" CcTest* CcTest::last_ = NULL; @@ -120,5 +121,6 @@ int main(int argc, char* argv[]) { } if (print_run_count && tests_run != 1) printf("Ran %i tests.\n", tests_run); + v8::V8::Dispose(); return 0; } diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index c9f1f42..87cc98c 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -154,7 +154,7 @@ class ApiTestFuzzer: public v8::internal::Thread { class RegisterThreadedTest { public: explicit RegisterThreadedTest(CcTest::TestFunction* callback) - : callback_(callback) { + : fuzzer_(NULL), callback_(callback) { prev_ = first_; first_ = this; count_++; @@ -5098,6 +5098,10 @@ void ApiTestFuzzer::ContextSwitch() { void ApiTestFuzzer::TearDown() { fuzzing_ = false; + for (int i = 0; i < RegisterThreadedTest::count(); i++) { + ApiTestFuzzer *fuzzer = RegisterThreadedTest::nth(i)->fuzzer_; + if (fuzzer != NULL) fuzzer->Join(); + } } diff --git a/test/cctest/test-strings.cc b/test/cctest/test-strings.cc index f8c1a46..b38d645 100644 --- a/test/cctest/test-strings.cc +++ b/test/cctest/test-strings.cc @@ -11,6 +11,7 @@ #include "factory.h" #include "cctest.h" +#include "zone-inl.h" unsigned int seed = 123; @@ -48,6 +49,8 @@ static const int SUPER_DEEP_DEPTH = 80 * 1024; static void InitializeBuildingBlocks( Handle building_blocks[NUMBER_OF_BUILDING_BLOCKS]) { + // A list of pointers that we don't have any interest in cleaning up. + // If they are reachable from a root then leak detection won't complain. for (int i = 0; i < NUMBER_OF_BUILDING_BLOCKS; i++) { int len = gen() % 16; if (len > 14) { @@ -80,7 +83,7 @@ static void InitializeBuildingBlocks( } case 2: { class Resource: public v8::String::ExternalStringResource, - public Malloced { + public ZoneObject { public: explicit Resource(Vector string): data_(string.start()) { length_ = string.length(); @@ -92,7 +95,7 @@ static void InitializeBuildingBlocks( const uc16* data_; size_t length_; }; - uc16* buf = NewArray(len); + uc16* buf = Zone::NewArray(len); for (int j = 0; j < len; j++) { buf[j] = gen() % 65536; } @@ -212,6 +215,7 @@ TEST(Traverse) { InitializeVM(); v8::HandleScope scope; Handle building_blocks[NUMBER_OF_BUILDING_BLOCKS]; + ZoneScope zone(DELETE_ON_EXIT); InitializeBuildingBlocks(building_blocks); Handle flat = ConstructBalanced(building_blocks); FlattenString(flat); @@ -303,6 +307,7 @@ TEST(Slice) { InitializeVM(); v8::HandleScope scope; Handle building_blocks[NUMBER_OF_BUILDING_BLOCKS]; + ZoneScope zone(DELETE_ON_EXIT); InitializeBuildingBlocks(building_blocks); seed = 42; diff --git a/test/message/testcfg.py b/test/message/testcfg.py index f259d37..6004282 100644 --- a/test/message/testcfg.py +++ b/test/message/testcfg.py @@ -41,6 +41,11 @@ class MessageTestCase(test.TestCase): self.config = config self.mode = mode + def IgnoreLine(self, str): + """Ignore empty lines and valgrind output.""" + if not str: return True + else: return str.startswith('==') or str.startswith('**') + def IsFailureOutput(self, output): f = file(self.expected) # Skip initial '#' comment and spaces @@ -58,7 +63,8 @@ class MessageTestCase(test.TestCase): pattern = '^%s$' % pattern patterns.append(pattern) # Compare actual output with the expected - outlines = [ s for s in output.stdout.split('\n') if s ] + raw_lines = output.stdout.split('\n') + outlines = [ s for s in raw_lines if not self.IgnoreLine(s) ] if len(outlines) != len(patterns): return True for i in xrange(len(patterns)): -- 2.7.4