diff options
| -rw-r--r-- | base/base.gyp | 4 | ||||
| -rw-r--r-- | base/leak_tracker.h | 106 | ||||
| -rw-r--r-- | base/leak_tracker_unittest.cc | 108 | ||||
| -rw-r--r-- | base/linked_list.h | 135 | ||||
| -rw-r--r-- | base/linked_list_unittest.cc | 245 | ||||
| -rw-r--r-- | chrome/browser/browser_process_impl.cc | 50 | ||||
| -rw-r--r-- | chrome/browser/browser_process_impl.h | 4 | ||||
| -rw-r--r-- | chrome/browser/net/url_fetcher.h | 3 | ||||
| -rw-r--r-- | net/url_request/url_request.h | 3 |
9 files changed, 647 insertions, 11 deletions
diff --git a/base/base.gyp b/base/base.gyp index 9bb37cb..d0e3c4f 100644 --- a/base/base.gyp +++ b/base/base.gyp @@ -161,6 +161,8 @@ 'keyboard_codes_win.h', 'lazy_instance.cc', 'lazy_instance.h', + 'leak_tracker.h', + 'linked_list.h', 'linked_ptr.h', 'linux_util.cc', 'linux_util.h', @@ -581,6 +583,8 @@ 'json_reader_unittest.cc', 'json_writer_unittest.cc', 'lazy_instance_unittest.cc', + 'leak_tracker_unittest.cc', + 'linked_list_unittest.cc', 'linked_ptr_unittest.cc', 'mac_util_unittest.cc', 'message_loop_unittest.cc', diff --git a/base/leak_tracker.h b/base/leak_tracker.h new file mode 100644 index 0000000..d289691e --- /dev/null +++ b/base/leak_tracker.h @@ -0,0 +1,106 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef BASE_LEAK_TRACKER_H_ +#define BASE_LEAK_TRACKER_H_ + +#ifndef NDEBUG +#include "base/debug_util.h" +#include "base/linked_list.h" +#include "base/logging.h" +#endif + +// LeakTracker is a debug helper to verify that all instances of a class +// have been destroyed. +// +// It is particularly useful for classes that are bound to a single thread -- +// before destroying that thread, one can check that there are no remaining +// instances of that class. +// +// For example, to enable leak tracking for class URLRequest, start by +// adding a member variable of type LeakTracker<URLRequest>. +// +// class URLRequest { +// ... +// private: +// base::LeakTracker<URLRequest> leak_tracker_; +// }; +// +// +// Next, when we believe all instances of URLRequest have been deleted: +// +// LeakTracker<URLRequest>::CheckForLeaks(); +// +// Should the check fail (because there are live instances of URLRequest), +// then the allocation callstack for each leaked instances is dumped to +// the error log. +// +// In RELEASE mode the check has no effect. + +namespace base { + +#ifdef NDEBUG + +// In release mode we do nothing. +template<typename T> +class LeakTracker { + public: + static void CheckForLeaks() {} + static int NumLiveInstances() { return -1; } +}; + +#else + +// In debug mode we track where the object was allocated from. + +template<typename T> +class LeakTracker : public LinkNode<LeakTracker<T> > { + public: + LeakTracker() { + instances()->Append(this); + } + + ~LeakTracker() { + this->RemoveFromList(); + } + + static void CheckForLeaks() { + // Walk the allocation list and print each entry it contains. + int count = 0; + for (LinkNode<LeakTracker<T> >* node = instances()->head(); + node != instances()->end(); + node = node->next()) { + ++count; + LOG(ERROR) << "Leaked " << node << " which was allocated by:"; + node->value()->allocation_stack_.PrintBacktrace(); + } + DCHECK_EQ(0, count); + } + + static int NumLiveInstances() { + // Walk the allocation list and count how many entries it has. + int count = 0; + for (LinkNode<LeakTracker<T> >* node = instances()->head(); + node != instances()->end(); + node = node->next()) { + ++count; + } + return count; + } + + private: + // Each specialization of LeakTracker gets its own static storage. + static LinkedList<LeakTracker<T> >* instances() { + static LinkedList<LeakTracker<T> > list; + return &list; + } + + StackTrace allocation_stack_; +}; + +#endif // NDEBUG + +} // namespace base + +#endif // BASE_LEAK_TRACKER_H_ diff --git a/base/leak_tracker_unittest.cc b/base/leak_tracker_unittest.cc new file mode 100644 index 0000000..8f0e2f2 --- /dev/null +++ b/base/leak_tracker_unittest.cc @@ -0,0 +1,108 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/leak_tracker.h" +#include "base/scoped_ptr.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace { + +class ClassA { + private: + base::LeakTracker<ClassA> leak_tracker_; +}; + +class ClassB { + private: + base::LeakTracker<ClassB> leak_tracker_; +}; + +#ifdef NDEBUG + +// In RELEASE mode, leak tracking is disabled. +TEST(LeakTrackerTest, ReleaseMode) { + EXPECT_EQ(-1, base::LeakTracker<ClassA>::NumLiveInstances()); + EXPECT_EQ(-1, base::LeakTracker<ClassB>::NumLiveInstances()); + + // Use scoped_ptr so compiler doesn't complain about unused variables. + scoped_ptr<ClassA> a1(new ClassA); + scoped_ptr<ClassB> b1(new ClassB); + scoped_ptr<ClassB> b2(new ClassB); + + EXPECT_EQ(-1, base::LeakTracker<ClassA>::NumLiveInstances()); + EXPECT_EQ(-1, base::LeakTracker<ClassB>::NumLiveInstances()); +} + +#else + +// In DEBUG mode, leak tracking should work. +TEST(LeakTrackerTest, DebugMode) { + { + ClassA a1; + + EXPECT_EQ(1, base::LeakTracker<ClassA>::NumLiveInstances()); + EXPECT_EQ(0, base::LeakTracker<ClassB>::NumLiveInstances()); + + ClassB b1; + ClassB b2; + + EXPECT_EQ(1, base::LeakTracker<ClassA>::NumLiveInstances()); + EXPECT_EQ(2, base::LeakTracker<ClassB>::NumLiveInstances()); + + scoped_ptr<ClassA> a2(new ClassA); + + EXPECT_EQ(2, base::LeakTracker<ClassA>::NumLiveInstances()); + EXPECT_EQ(2, base::LeakTracker<ClassB>::NumLiveInstances()); + + a2.reset(); + + EXPECT_EQ(1, base::LeakTracker<ClassA>::NumLiveInstances()); + EXPECT_EQ(2, base::LeakTracker<ClassB>::NumLiveInstances()); + } + + EXPECT_EQ(0, base::LeakTracker<ClassA>::NumLiveInstances()); + EXPECT_EQ(0, base::LeakTracker<ClassB>::NumLiveInstances()); +} + +// Try some orderings of create/remove to hit different cases in the linked-list +// assembly. +TEST(LeakTrackerTest, DebugMode_LinkedList) { + EXPECT_EQ(0, base::LeakTracker<ClassB>::NumLiveInstances()); + + scoped_ptr<ClassA> a1(new ClassA); + scoped_ptr<ClassA> a2(new ClassA); + scoped_ptr<ClassA> a3(new ClassA); + scoped_ptr<ClassA> a4(new ClassA); + + EXPECT_EQ(4, base::LeakTracker<ClassA>::NumLiveInstances()); + + // Remove the head of the list (a1). + a1.reset(); + EXPECT_EQ(3, base::LeakTracker<ClassA>::NumLiveInstances()); + + // Remove the tail of the list (a4). + a4.reset(); + EXPECT_EQ(2, base::LeakTracker<ClassA>::NumLiveInstances()); + + // Append to the new tail of the list (a3). + scoped_ptr<ClassA> a5(new ClassA); + EXPECT_EQ(3, base::LeakTracker<ClassA>::NumLiveInstances()); + + a2.reset(); + a3.reset(); + + EXPECT_EQ(1, base::LeakTracker<ClassA>::NumLiveInstances()); + + a5.reset(); + EXPECT_EQ(0, base::LeakTracker<ClassA>::NumLiveInstances()); +} + +TEST(LeakTrackerTest, DebugMode_NoOpCheckForLeaks) { + // There are no live instances of ClassA, so this should do nothing. + base::LeakTracker<ClassA>::CheckForLeaks(); +} + +#endif // NDEBUG + +} // namespace diff --git a/base/linked_list.h b/base/linked_list.h new file mode 100644 index 0000000..c197008 --- /dev/null +++ b/base/linked_list.h @@ -0,0 +1,135 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef BASE_LINKED_LIST_H_ +#define BASE_LINKED_LIST_H_ + +// Simple LinkedList type. +// +// To use, start by declaring the class which will be contained in the linked +// list, as extending LinkNode (this gives it next/previous pointers). +// +// class MyNodeType : public LinkNode<MyNodeType> { +// ... +// }; +// +// Next, to keep track of the list's head/tail, use a LinkedList instance: +// +// LinkedList<MyNodeType> list; +// +// To add elements to the list, use any of LinkedList::Append, +// LinkNode::InsertBefore, or LinkNode::InsertAfter: +// +// LinkNode<MyNodeType>* n1 = ...; +// LinkNode<MyNodeType>* n2 = ...; +// LinkNode<MyNodeType>* n3 = ...; +// +// list.Append(n1); +// list.Append(n3); +// n3->InsertBefore(n3); +// +// Lastly, to iterate through the linked list forwards: +// +// for (LinkNode<MyNodeType>* node = list.head(); +// node != list.end(); +// node = node->next()) { +// MyNodeType* value = node->value(); +// ... +// } +// +// Or to iterate the linked list backwards: +// +// for (LinkNode<MyNodeType>* node = list.tail(); +// node != list.end(); +// node = node->previous()) { +// MyNodeType* value = node->value(); +// ... +// } +// + +namespace base { + +template <typename T> +class LinkNode { + public: + LinkNode() : previous_(0), next_(0) {} + LinkNode(LinkNode<T>* previous, LinkNode<T>* next) + : previous_(previous), next_(next) {} + + // Insert |this| into the linked list, before |e|. + void InsertBefore(LinkNode<T>* e) { + this->next_ = e; + this->previous_ = e->previous_; + e->previous_->next_ = this; + e->previous_ = this; + } + + // Insert |this| into the linked list, after |e|. + void InsertAfter(LinkNode<T>* e) { + this->next_ = e->next_; + this->previous_ = e; + e->next_->previous_ = this; + e->next_ = this; + } + + // Remove |this| from the linked list. + void RemoveFromList() { + this->previous_->next_ = this->next_; + this->next_->previous_ = this->previous_; + } + + LinkNode<T>* previous() const { + return previous_; + } + + LinkNode<T>* next() const { + return next_; + } + + // Cast from the node-type to the value type. + const T* value() const { + return reinterpret_cast<const T*>(this); + } + + T* value() { + return reinterpret_cast<T*>(this); + } + + private: + LinkNode<T>* previous_; + LinkNode<T>* next_; +}; + +template <typename T> +class LinkedList { + public: + // The "root" node is self-referential, and forms the basis of a circular + // list (root_.next() will point back to the start of the list, + // and root_->previous() wraps around to the end of the list). + LinkedList() : root_(&root_, &root_) {} + + // Appends |e| to the end of the linked list. + void Append(LinkNode<T>* e) { + e->InsertBefore(&root_); + } + + LinkNode<T>* head() const { + return root_.next(); + } + + LinkNode<T>* tail() const { + return root_.previous(); + } + + const LinkNode<T>* end() const { + return &root_; + } + + private: + LinkNode<T> root_; +}; + +} // namespace base + +#endif // BASE_LINKED_LIST_H_ diff --git a/base/linked_list_unittest.cc b/base/linked_list_unittest.cc new file mode 100644 index 0000000..835d927 --- /dev/null +++ b/base/linked_list_unittest.cc @@ -0,0 +1,245 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/linked_list.h" +#include "base/basictypes.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace base { +namespace { + +class Node : public LinkNode<Node> { + public: + explicit Node(int id) : id_(id) {} + + int id() const { return id_; } + + private: + int id_; +}; + +// Checks that when iterating |list| (either from head to tail, or from +// tail to head, as determined by |forward|), we get back |node_ids|, +// which is an array of size |num_nodes|. +void ExpectListContentsForDirection(const LinkedList<Node>& list, + int num_nodes, const int* node_ids, bool forward) { + int i = 0; + for (const LinkNode<Node>* node = (forward ? list.head() : list.tail()); + node != list.end(); + node = (forward ? node->next() : node->previous())) { + ASSERT_LT(i, num_nodes); + int index_of_id = forward ? i : num_nodes - i - 1; + EXPECT_EQ(node_ids[index_of_id], node->value()->id()); + ++i; + } + EXPECT_EQ(num_nodes, i); +} + +void ExpectListContents(const LinkedList<Node>& list, + int num_nodes, + const int* node_ids) { + { + SCOPED_TRACE("Iterating forward (from head to tail)"); + ExpectListContentsForDirection(list, num_nodes, node_ids, true); + } + { + SCOPED_TRACE("Iterating backward (from tail to head)"); + ExpectListContentsForDirection(list, num_nodes, node_ids, false); + } +} + +TEST(LinkedList, Empty) { + LinkedList<Node> list; + EXPECT_EQ(list.end(), list.head()); + EXPECT_EQ(list.end(), list.tail()); + ExpectListContents(list, 0, NULL); +} + + +TEST(LinkedList, Append) { + LinkedList<Node> list; + ExpectListContents(list, 0, NULL); + + Node n1(1); + list.Append(&n1); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n1, list.tail()); + { + const int expected[] = {1}; + ExpectListContents(list, arraysize(expected), expected); + } + + Node n2(2); + list.Append(&n2); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n2, list.tail()); + { + const int expected[] = {1, 2}; + ExpectListContents(list, arraysize(expected), expected); + } + + Node n3(3); + list.Append(&n3); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n3, list.tail()); + { + const int expected[] = {1, 2, 3}; + ExpectListContents(list, arraysize(expected), expected); + } +} + +TEST(LinkedList, RemoveFromList) { + LinkedList<Node> list; + + Node n1(1); + Node n2(2); + Node n3(3); + Node n4(4); + Node n5(5); + + list.Append(&n1); + list.Append(&n2); + list.Append(&n3); + list.Append(&n4); + list.Append(&n5); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n5, list.tail()); + { + const int expected[] = {1, 2, 3, 4, 5}; + ExpectListContents(list, arraysize(expected), expected); + } + + // Remove from the middle. + n3.RemoveFromList(); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n5, list.tail()); + { + const int expected[] = {1, 2, 4, 5}; + ExpectListContents(list, arraysize(expected), expected); + } + + // Remove from the tail. + n5.RemoveFromList(); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n4, list.tail()); + { + const int expected[] = {1, 2, 4}; + ExpectListContents(list, arraysize(expected), expected); + } + + // Remove from the head. + n1.RemoveFromList(); + + EXPECT_EQ(&n2, list.head()); + EXPECT_EQ(&n4, list.tail()); + { + const int expected[] = {2, 4}; + ExpectListContents(list, arraysize(expected), expected); + } + + // Empty the list. + n2.RemoveFromList(); + n4.RemoveFromList(); + + ExpectListContents(list, 0, NULL); + EXPECT_EQ(list.end(), list.head()); + EXPECT_EQ(list.end(), list.tail()); + + // Fill the list once again. + list.Append(&n1); + list.Append(&n2); + list.Append(&n3); + list.Append(&n4); + list.Append(&n5); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n5, list.tail()); + { + const int expected[] = {1, 2, 3, 4, 5}; + ExpectListContents(list, arraysize(expected), expected); + } +} + +TEST(LinkedList, InsertBefore) { + LinkedList<Node> list; + + Node n1(1); + Node n2(2); + Node n3(3); + Node n4(4); + + list.Append(&n1); + list.Append(&n2); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n2, list.tail()); + { + const int expected[] = {1, 2}; + ExpectListContents(list, arraysize(expected), expected); + } + + n3.InsertBefore(&n2); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n2, list.tail()); + { + const int expected[] = {1, 3, 2}; + ExpectListContents(list, arraysize(expected), expected); + } + + n4.InsertBefore(&n1); + + EXPECT_EQ(&n4, list.head()); + EXPECT_EQ(&n2, list.tail()); + { + const int expected[] = {4, 1, 3, 2}; + ExpectListContents(list, arraysize(expected), expected); + } +} + +TEST(LinkedList, InsertAfter) { + LinkedList<Node> list; + + Node n1(1); + Node n2(2); + Node n3(3); + Node n4(4); + + list.Append(&n1); + list.Append(&n2); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n2, list.tail()); + { + const int expected[] = {1, 2}; + ExpectListContents(list, arraysize(expected), expected); + } + + n3.InsertAfter(&n2); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n3, list.tail()); + { + const int expected[] = {1, 2, 3}; + ExpectListContents(list, arraysize(expected), expected); + } + + n4.InsertAfter(&n1); + + EXPECT_EQ(&n1, list.head()); + EXPECT_EQ(&n3, list.tail()); + { + const int expected[] = {1, 4, 2, 3}; + ExpectListContents(list, arraysize(expected), expected); + } +} + +} // namespace +} // namespace base diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc index 552e171..4007cf6 100644 --- a/chrome/browser/browser_process_impl.cc +++ b/chrome/browser/browser_process_impl.cc @@ -58,7 +58,7 @@ class BrowserProcessSubThread : public ChromeThread { : ChromeThread(identifier) { } - ~BrowserProcessSubThread() { + virtual ~BrowserProcessSubThread() { // We cannot rely on our base class to stop the thread since we want our // CleanUp function to run. Stop(); @@ -92,6 +92,26 @@ class BrowserProcessSubThread : public ChromeThread { NotificationService* notification_service_; }; +class IOThread : public BrowserProcessSubThread { + public: + IOThread() : BrowserProcessSubThread(ChromeThread::IO) {} + + virtual ~IOThread() { + // We cannot rely on our base class to stop the thread since we want our + // CleanUp function to run. + Stop(); + } + + protected: + virtual void CleanUp() { + // URLFetcher and URLRequest instances must NOT outlive the IO thread. + base::LeakTracker<URLRequest>::CheckForLeaks(); + base::LeakTracker<URLFetcher>::CheckForLeaks(); + + BrowserProcessSubThread::CleanUp(); + } +}; + } // namespace BrowserProcessImpl::BrowserProcessImpl(const CommandLine& command_line) @@ -172,13 +192,6 @@ BrowserProcessImpl::~BrowserProcessImpl() { resource_dispatcher_host()->Shutdown(); } - // Shutdown DNS prefetching now to ensure that network stack objects - // living on the IO thread get destroyed before the IO thread goes away. - if (io_thread_.get()) { - io_thread_->message_loop()->PostTask(FROM_HERE, - NewRunnableFunction(chrome_browser_net::EnsureDnsPrefetchShutdown)); - } - #if defined(OS_LINUX) // The IO thread must outlive the BACKGROUND_X11 thread. background_x11_thread_.reset(); @@ -187,7 +200,7 @@ BrowserProcessImpl::~BrowserProcessImpl() { // Need to stop io_thread_ before resource_dispatcher_host_, since // io_thread_ may still deref ResourceDispatcherHost and handle resource // request before going away. - io_thread_.reset(); + ResetIOThread(); // Clean up state that lives on the file_thread_ before it goes away. if (resource_dispatcher_host_.get()) { @@ -307,8 +320,7 @@ void BrowserProcessImpl::CreateIOThread() { background_x11_thread_.swap(background_x11_thread); #endif - scoped_ptr<base::Thread> thread( - new BrowserProcessSubThread(ChromeThread::IO)); + scoped_ptr<base::Thread> thread(new IOThread); base::Thread::Options options; options.message_loop_type = MessageLoop::TYPE_IO; if (!thread->StartWithOptions(options)) @@ -316,6 +328,22 @@ void BrowserProcessImpl::CreateIOThread() { io_thread_.swap(thread); } +void BrowserProcessImpl::ResetIOThread() { + if (io_thread_.get()) { + io_thread_->message_loop()->PostTask(FROM_HERE, + NewRunnableFunction(CleanupOnIOThread)); + } + io_thread_.reset(); +} + +// static +void BrowserProcessImpl::CleanupOnIOThread() { + // Shutdown DNS prefetching now to ensure that network stack objects + // living on the IO thread get destroyed before the IO thread goes away. + chrome_browser_net::EnsureDnsPrefetchShutdown(); + // TODO(eroman): can this be merged into IOThread::CleanUp() ? +} + void BrowserProcessImpl::CreateFileThread() { DCHECK(!created_file_thread_ && file_thread_.get() == NULL); created_file_thread_ = true; diff --git a/chrome/browser/browser_process_impl.h b/chrome/browser/browser_process_impl.h index 421ba03..ac0edde 100644 --- a/chrome/browser/browser_process_impl.h +++ b/chrome/browser/browser_process_impl.h @@ -193,7 +193,11 @@ class BrowserProcessImpl : public BrowserProcess, public NonThreadSafe { void CreateResourceDispatcherHost(); void CreatePrefService(); void CreateMetricsService(); + void CreateIOThread(); + void ResetIOThread(); + static void CleanupOnIOThread(); + void CreateFileThread(); void CreateDBThread(); void CreateTemplateURLModel(); diff --git a/chrome/browser/net/url_fetcher.h b/chrome/browser/net/url_fetcher.h index 0ed4715..6b1c87c 100644 --- a/chrome/browser/net/url_fetcher.h +++ b/chrome/browser/net/url_fetcher.h @@ -10,6 +10,7 @@ #ifndef CHROME_BROWSER_URL_FETCHER_H_ #define CHROME_BROWSER_URL_FETCHER_H_ +#include "base/leak_tracker.h" #include "base/message_loop.h" #include "base/ref_counted.h" #include "chrome/browser/net/url_fetcher_protect.h" @@ -163,6 +164,8 @@ class URLFetcher { static Factory* factory_; + base::LeakTracker<URLFetcher> leak_tracker_; + DISALLOW_EVIL_CONSTRUCTORS(URLFetcher); }; diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h index e9539cb..5a97fc6 100644 --- a/net/url_request/url_request.h +++ b/net/url_request/url_request.h @@ -9,6 +9,7 @@ #include <string> #include <vector> +#include "base/leak_tracker.h" #include "base/linked_ptr.h" #include "base/logging.h" #include "base/ref_counted.h" @@ -592,6 +593,8 @@ class URLRequest { // this to determine which URLRequest to allocate sockets to first. int priority_; + base::LeakTracker<URLRequest> leak_tracker_; + DISALLOW_COPY_AND_ASSIGN(URLRequest); }; |