Factor the execution of the test method into a separate function to ensure that any exceptions that are thrown go out of scope and no longer hold references to SB objects that need to be freed before teardown.

amccarth-google · amccarth-google · commit 6ecdbc87e6e4 · 2015-10-15T22:39:55.000Z
Differential Revision: http://reviews.llvm.org/D13788 llvm-svn: 250467
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
@@ -1276,7 +1276,7 @@ Target::ModuleAdded (const ModuleList& module_list, const ModuleSP &module_sp)
 void
 Target::ModuleRemoved (const ModuleList& module_list, const ModuleSP &module_sp)
 {
-    // A module is being added to this target for the first time
+    // A module is being removed from this target.
     if (m_valid)
     {
         ModuleList my_module_list;
diff --git a/lldb/test/lldbtest.py b/lldb/test/lldbtest.py
@@ -32,6 +32,7 @@
 """
 
 import abc
+import gc
 import glob
 import os, sys, traceback
 import os.path
@@ -2544,6 +2545,11 @@ def tearDown(self):
         #import traceback
         #traceback.print_stack()
 
+        # Ensure all the references to SB objects have gone away so that we can
+        # be sure that all test-specific resources have been freed before we
+        # attempt to delete the targets.
+        gc.collect()
+
         # Delete the target(s) from the debugger as a general cleanup step.
         # This includes terminating the process for each target, if any.
         # We'd like to reuse the debugger for our next test without incurring
diff --git a/lldb/test/unittest2/case.py b/lldb/test/unittest2/case.py
@@ -353,32 +353,7 @@ def run(self, result=None):
             except Exception:
                 result.addError(self, sys.exc_info())
             else:
-                try:
-                    testMethod()
-                except self.failureException:
-                    result.addFailure(self, sys.exc_info())
-                except _ExpectedFailure, e:
-                    addExpectedFailure = getattr(result, 'addExpectedFailure', None)
-                    if addExpectedFailure is not None:
-                        addExpectedFailure(self, e.exc_info, e.bugnumber)
-                    else: 
-                        warnings.warn("Use of a TestResult without an addExpectedFailure method is deprecated", 
-                                      DeprecationWarning)
-                        result.addSuccess(self)
-                except _UnexpectedSuccess, x:
-                    addUnexpectedSuccess = getattr(result, 'addUnexpectedSuccess', None)
-                    if addUnexpectedSuccess is not None:
-                        addUnexpectedSuccess(self, x.bugnumber)
-                    else:
-                        warnings.warn("Use of a TestResult without an addUnexpectedSuccess method is deprecated", 
-                                      DeprecationWarning)
-                        result.addFailure(self, sys.exc_info())
-                except SkipTest, e:
-                    self._addSkip(result, str(e))
-                except Exception:
-                    result.addError(self, sys.exc_info())
-                else:
-                    success = True
+                success = self.runMethod(testMethod, result)
 
                 try:
                     self.tearDown()
@@ -399,6 +374,42 @@ def run(self, result=None):
                 if stopTestRun is not None:
                     stopTestRun()
 
+    def runMethod(self, testMethod, result):
+        """Runs the test method and catches any exception that might be thrown.
+
+        This is factored out of TestCase.run() to ensure that any exception
+        thrown during the test goes out of scope before tearDown.  Otherwise, an
+        exception could hold references to Python objects that are bound to
+        SB objects and prevent them from being deleted in time.
+        """
+        try:
+            testMethod()
+        except self.failureException:
+            result.addFailure(self, sys.exc_info())
+        except _ExpectedFailure, e:
+            addExpectedFailure = getattr(result, 'addExpectedFailure', None)
+            if addExpectedFailure is not None:
+                addExpectedFailure(self, e.exc_info, e.bugnumber)
+            else:
+                warnings.warn("Use of a TestResult without an addExpectedFailure method is deprecated",
+                              DeprecationWarning)
+                result.addSuccess(self)
+        except _UnexpectedSuccess, x:
+            addUnexpectedSuccess = getattr(result, 'addUnexpectedSuccess', None)
+            if addUnexpectedSuccess is not None:
+                addUnexpectedSuccess(self, x.bugnumber)
+            else:
+                warnings.warn("Use of a TestResult without an addUnexpectedSuccess method is deprecated",
+                              DeprecationWarning)
+                result.addFailure(self, sys.exc_info())
+        except SkipTest, e:
+            self._addSkip(result, str(e))
+        except Exception:
+            result.addError(self, sys.exc_info())
+        else:
+            return True
+        return False
+
     def doCleanups(self):
         """Execute all cleanup functions. Normally called for you after
         tearDown."""

Original file line number	Diff line number	Diff line change
`@@ -1276,7 +1276,7 @@ Target::ModuleAdded (const ModuleList& module_list, const ModuleSP &module_sp)`
`1276`	`1276`	`void`
`1277`	`1277`	`Target::ModuleRemoved (const ModuleList& module_list, const ModuleSP &module_sp)`
`1278`	`1278`	`{`
`1279`		`- // A module is being added to this target for the first time`
	`1279`	`+ // A module is being removed from this target.`
`1280`	`1280`	`if (m_valid)`
`1281`	`1281`	`{`
`1282`	`1282`	`ModuleList my_module_list;`