A browser test created an echo server, waited for it to start listening,

ran its tests, sent it a message asking it to quit, and then sometimes
IMMEDIATELY KILLED IT without giving it a chance to process the quit
message. This triggered an assertion failure in the server destructor that
was largely harmless but good for the purpose of pointing out this
programming error.

I fixed the problem by adding a WaitableEvent in the echo server that gets
signaled when cleanup is complete. Then the browser test is able to wait
on that event (with waiting on the UI thread and signaling on the IO thread)
before letting the server go out of scope.

BUG=109337
TEST=already covered.


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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@117692 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 24 insertions, 0 deletions

diff --git a/chrome/browser/extensions/api/socket/socket_apitest.cc b/chrome/browser/extensions/api/socket/socket_apitest.cc
index aa18ae9..a2f81a8 100644
--- a/chrome/browser/extensions/api/socket/socket_apitest.cc
+++ b/chrome/browser/extensions/api/socket/socket_apitest.cc
@@ -61,6 +61,9 @@ IN_PROC_BROWSER_TEST_F(SocketApiTest, SocketCreateBad) {
 }
 
 // http://code.google.com/p/chromium/issues/detail?id=109337
+//
+// Currently believed to be fixed, but we're leaving it marked flaky
+// for a few days to let it percolate through the trybots.
 IN_PROC_BROWSER_TEST_F(SocketApiTest, FLAKY_SocketExtension) {
   scoped_refptr<extensions::TestEchoServerUDP> server =
       new extensions::TestEchoServerUDP();
@@ -79,4 +82,6 @@ IN_PROC_BROWSER_TEST_F(SocketApiTest, FLAKY_SocketExtension) {
   listener.Reply(port);
 
   EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
+
+  EXPECT_TRUE(server->WaitUntilFinished());
 }
diff --git a/chrome/browser/extensions/api/socket/test_echo_server_udp.cc b/chrome/browser/extensions/api/socket/test_echo_server_udp.cc
index 4188030..bfabfc0 100644
--- a/chrome/browser/extensions/api/socket/test_echo_server_udp.cc
+++ b/chrome/browser/extensions/api/socket/test_echo_server_udp.cc
@@ -28,6 +28,7 @@ const std::string TestEchoServerUDP::kQuitPattern = "*QUIT*";
 
 TestEchoServerUDP::TestEchoServerUDP()
     : listening_event_(true, false),
+      cleanup_completed_event_(true, false),
       port_(0),
       server_log_(new CapturingNetLog(CapturingNetLog::kUnbounded)),
       socket_(NULL),
@@ -42,6 +43,7 @@ TestEchoServerUDP::~TestEchoServerUDP() {
 }
 
 int TestEchoServerUDP::Start() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   base::TimeDelta max_time = base::TimeDelta::FromSeconds(5);
 
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
@@ -53,7 +55,14 @@ int TestEchoServerUDP::Start() {
     return -1;
 }
 
+bool TestEchoServerUDP::WaitUntilFinished() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  base::TimeDelta max_time = base::TimeDelta::FromSeconds(5);
+  return cleanup_completed_event_.TimedWait(max_time);
+}
+
 void TestEchoServerUDP::RunOnIOThread() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   CreateListeningSocket();
   listening_event_.Signal();
   Echo();
@@ -138,6 +147,7 @@ void TestEchoServerUDP::CleanUpOnIOThread() {
   // we'll delete it right now.
   delete socket_;
   socket_ = NULL;
+  cleanup_completed_event_.Signal();
 }
 
 void TestEchoServerUDP::CreateUDPAddress(std::string ip_str, int port,
diff --git a/chrome/browser/extensions/api/socket/test_echo_server_udp.h b/chrome/browser/extensions/api/socket/test_echo_server_udp.h
index 35cde60..2823490 100644
--- a/chrome/browser/extensions/api/socket/test_echo_server_udp.h
+++ b/chrome/browser/extensions/api/socket/test_echo_server_udp.h
@@ -35,6 +35,14 @@ class TestEchoServerUDP
   // that thread. Returns the listening port.
   int Start();
 
+  // Waits until pending cleanup is finished. This method doesn't exactly match
+  // the Start() method because quitting is initiated by a command sent by UDP
+  // from the client to the server, rather than being initiated by a method.
+  //
+  // Returns true if successful. Fails if we had to wait an unreasonably long
+  // time, which likely means that we weren't asked to start cleaning up.
+  bool WaitUntilFinished();
+
  private:
   static const int kMaxRead = 1024;
   static const std::string kEOLPattern;
@@ -61,6 +69,7 @@ class TestEchoServerUDP
                    const net::IPEndPoint& address);
 
   base::WaitableEvent listening_event_;
+  base::WaitableEvent cleanup_completed_event_;
   int port_;
   scoped_ptr<net::CapturingNetLog> server_log_;
   net::UDPServerSocket* socket_;  // See CleanUpOnIOThread re raw pointer.