Adding logic to restart the browser if there is an exception

content/test/gpu/gpu_tests/gpu_integration_test.py

Index: content/test/gpu/gpu_tests/gpu_integration_test.py
diff --git a/content/test/gpu/gpu_tests/gpu_integration_test.py b/content/test/gpu/gpu_tests/gpu_integration_test.py
index 21a283be021fd1b200f1974ed6ae2319de4bd986..f44c41a28cf2ab1dc9a54a4800d0090804d49dd1 100644
--- a/content/test/gpu/gpu_tests/gpu_integration_test.py
+++ b/content/test/gpu/gpu_tests/gpu_integration_test.py
@@ -126,4 +126,10 @@ class GpuIntegrationTest(
     raise NotImplementedError
 
   def setUp(self):
-    self.tab = self.browser.tabs[0]
+    try:
+      self.tab = self.browser.tabs[0]
+    except Exception:
+      # restart the browser to make sure a failure in a test doesn't
+      # propagate to the next test iteration.
+      logging.exception("Failure during browser startup")
+      self._RestartBrowser('failure in setup')

[nednguyen, 2016/07/21 18:01:55]

I still think we should reraise after this, otherwise this will not surface the
failure.

[eyaich, 2016/07/21 18:35:36]

The failure is on the test prior, so the exception that we care about has
already been raised for that test.  This is simply catching the state of the
browser that failed from the last tests and restarting it so this test has a
chance to pass.  Raising the error again here would fail this test and defeat
the whole purpose of this CL in the first place.

[nednguyen, 2016/07/21 18:40:30]

I think that we may still have case in which:

test N not fail
test N + 1 's setUp fail.

I would also expect most of the test failure from test N should also trigger a
browser restart in test N already:
https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/gpu_integrati...

[eyaich, 2016/07/21 18:48:02]

How would test N+1's setUp fail unless tab[0] wasn't available, ie the browser
wasn't up from either 1) a previous failure or 2) the browser isn't able to be
started at all in which case all tests in that process would fail.  

If we raise an exception again here what is the point of this try catch block
other than to restart the browser again?  I guess it would restart it for the
N+2 test, but then N+1 might fail unnecessarily. 

Also, if if test N was expected to fail the browser wouldn't restart regardless
of the failure reason.  Side note: why don't we restart the browser after the
expected failure case, won't the browser be down after the exception?

[nednguyen, 2016/07/21 18:57:51]

Note that all the communication between telemetry & chrome happens
asynchronously, so it's possible that:

Test N : Browser is aive
Test N : Telemetry tells Browser to do X
Test N : Browser does X
Test N : Telemetry poke browser to get the state of the page, browser return
"foo"
Some where between test N & test N + 1: Browser crashes
Test N + 1.setUp:Telemetry contacts the browser again, but it's already crashed
& throw CrashException.
Yes, in case there is the exception, I would prefer N+1 fails here rather than
nothing fails.  When there is failure, adding an extra failure is bad, but not
as bad as comparing to no failure at all if there is a crash here that wasn't
shown in previous test run.

[eyaich, 2016/07/21 19:04:09]

Ok I agree that not reporting an error at all is worse than reporting an extra
one.  Added the raise.