Properly handle screen recorder shutdown in ChromotingHost.

BUG=91620
TEST=Host doesn't crash when shutting down.

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

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

5 files changed, 71 insertions, 47 deletions

diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index 1aeaae7..fbfc0ea 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -55,6 +55,7 @@ ChromotingHost::ChromotingHost(ChromotingHostContext* context,
       access_verifier_(access_verifier),
       allow_nat_traversal_(allow_nat_traversal),
       state_(kInitial),
+      stopping_recorders_(0),
       protocol_config_(protocol::CandidateSessionConfig::CreateDefault()),
       is_curtained_(false),
       is_it2me_(false) {
@@ -376,13 +377,11 @@ void ChromotingHost::OnClientDisconnected(ConnectionToClient* connection) {
     recorder_->RemoveConnection(connection);
     // The recorder only exists to serve the unique authenticated client.
     // If that client has disconnected, then we can kill the recorder.
-    if (client->get()->authenticated()) {
-      recorder_->Stop(NULL);
-      recorder_ = NULL;
-    }
+    if (client->get()->authenticated())
+      StopScreenRecorder();
   }
 
-  // Close the connection to connection just to be safe.
+  // Close the connection to client just to be safe.
   connection->Disconnect();
 
   // Also remove reference to ConnectionToClient from this object.
@@ -538,6 +537,35 @@ void ChromotingHost::ProcessPreAuthentication(
   client->get()->OnAuthorizationComplete(true);
 }
 
+void ChromotingHost::StopScreenRecorder() {
+  DCHECK(MessageLoop::current() == context_->main_message_loop());
+  DCHECK(recorder_.get());
+
+  ++stopping_recorders_;
+  recorder_->Stop(base::Bind(&ChromotingHost::OnScreenRecorderStopped, this));
+  recorder_ = NULL;
+}
+
+void ChromotingHost::OnScreenRecorderStopped() {
+  if (MessageLoop::current() != context_->main_message_loop()) {
+    context_->main_message_loop()->PostTask(
+        FROM_HERE, base::Bind(&ChromotingHost::OnScreenRecorderStopped, this));
+    return;
+  }
+
+  --stopping_recorders_;
+  DCHECK_GE(stopping_recorders_, 0);
+
+  bool stopping;
+  {
+    base::AutoLock auto_lock(lock_);
+    stopping = state_ == kStopping;
+  }
+
+  if (!stopping_recorders_ && stopping)
+    ShutdownFinish();
+}
+
 void ChromotingHost::ShutdownNetwork() {
   if (MessageLoop::current() != context_->network_message_loop()) {
     context_->network_message_loop()->PostTask(
@@ -573,8 +601,8 @@ void ChromotingHost::ShutdownRecorder() {
   }
 
   if (recorder_.get()) {
-    recorder_->Stop(NewRunnableMethod(this, &ChromotingHost::ShutdownFinish));
-  } else {
+    StopScreenRecorder();
+  } else if (!stopping_recorders_) {
     ShutdownFinish();
   }
 }
diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h
index fb26aec..bb9e80e 100644
--- a/remoting/host/chromoting_host.h
+++ b/remoting/host/chromoting_host.h
@@ -182,6 +182,9 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   void ProcessPreAuthentication(
       const scoped_refptr<protocol::ConnectionToClient>& connection);
 
+  void StopScreenRecorder();
+  void OnScreenRecorderStopped();
+
   // The following methods are called during shutdown.
   void ShutdownNetwork();
   void ShutdownRecorder();
@@ -212,6 +215,12 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   // and read by jingle thread.
   State state_;
 
+  // Number of screen recorders that are currently being
+  // stopped. Normally set to 0 or 1, but in some cases it may be
+  // greater than 1, particularly if when second client can connect
+  // immidiately after previous one disconnected.
+  int stopping_recorders_;
+
   // Lock is to lock the access to |state_|.
   base::Lock lock_;
 
diff --git a/remoting/host/screen_recorder.cc b/remoting/host/screen_recorder.cc
index ce982e6..5d3de30 100644
--- a/remoting/host/screen_recorder.cc
+++ b/remoting/host/screen_recorder.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
@@ -67,9 +68,20 @@ void ScreenRecorder::Start() {
       FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoStart));
 }
 
-void ScreenRecorder::Stop(Task* done_task) {
-  capture_loop_->PostTask(
-      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoStop, done_task));
+void ScreenRecorder::Stop(const base::Closure& done_task) {
+  if (MessageLoop::current() != capture_loop_) {
+    capture_loop_->PostTask(FROM_HERE, base::Bind(
+        &ScreenRecorder::Stop, this, done_task));
+    return;
+  }
+
+  DCHECK(!done_task.is_null());
+
+  capture_timer_.Stop();
+  is_recording_ = false;
+
+  network_loop_->PostTask(FROM_HERE, base::Bind(
+      &ScreenRecorder::DoStopOnNetworkThread, this, done_task));
 }
 
 void ScreenRecorder::SetMaxRate(double rate) {
@@ -148,30 +160,6 @@ void ScreenRecorder::DoStart() {
   DoCapture();
 }
 
-void ScreenRecorder::DoStop(Task* done_task) {
-  DCHECK_EQ(capture_loop_, MessageLoop::current());
-
-  base::ScopedTaskRunner done_runner(done_task);
-
-  // We might have not started when we receive a stop command, simply run the
-  // task and then return.
-  if (!is_recording_)
-    return;
-
-  capture_timer_.Stop();
-  is_recording_ = false;
-
-  DCHECK_GE(recordings_, 0);
-  if (recordings_) {
-    network_loop_->PostTask(
-        FROM_HERE,
-        NewTracedMethod(this,
-                        &ScreenRecorder::DoStopOnNetworkThread,
-                        done_runner.Release()));
-    return;
-  }
-}
-
 void ScreenRecorder::DoSetMaxRate(double max_rate) {
   DCHECK_EQ(capture_loop_, MessageLoop::current());
 
@@ -341,7 +329,7 @@ void ScreenRecorder::DoRemoveAllClients() {
   connections_.clear();
 }
 
-void ScreenRecorder::DoStopOnNetworkThread(Task* done_task) {
+void ScreenRecorder::DoStopOnNetworkThread(const base::Closure& done_task) {
   DCHECK_EQ(network_loop_, MessageLoop::current());
 
   // There could be tasks on the network thread when this method is being
@@ -385,7 +373,7 @@ void ScreenRecorder::DoEncode(
   TraceContext::tracer()->PrintString("Encode Done");
 }
 
-void ScreenRecorder::DoStopOnEncodeThread(Task* done_task) {
+void ScreenRecorder::DoStopOnEncodeThread(const base::Closure& done_task) {
   DCHECK_EQ(encode_loop_, MessageLoop::current());
 
   encoder_stopped_ = true;
@@ -393,8 +381,7 @@ void ScreenRecorder::DoStopOnEncodeThread(Task* done_task) {
   // When this method is being executed there are no more tasks on encode thread
   // for this object. We can then post a task to capture thread to finish the
   // stop sequence.
-  if (done_task)
-    capture_loop_->PostTask(FROM_HERE, done_task);
+  capture_loop_->PostTask(FROM_HERE, done_task);
 }
 
 void ScreenRecorder::EncodedDataAvailableCallback(VideoPacket* packet) {
diff --git a/remoting/host/screen_recorder.h b/remoting/host/screen_recorder.h
index 3007aa5..64eb1ad 100644
--- a/remoting/host/screen_recorder.h
+++ b/remoting/host/screen_recorder.h
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
@@ -85,7 +86,7 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
 
   // Stop the recording session. |done_task| is executed when recording is fully
   // stopped. This object cannot be used again after |task| is executed.
-  void Stop(Task* done_task);
+  void Stop(const base::Closure& done_task);
 
   // Set the maximum capture rate. This is denoted by number of updates
   // in one second. The actual system may run in a slower rate than the maximum
@@ -114,8 +115,6 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
   // Capturer thread ----------------------------------------------------------
 
   void DoStart();
-  void DoStop(Task* done_task);
-
   void DoSetMaxRate(double max_rate);
 
   // Hepler method to schedule next capture using the current rate.
@@ -140,7 +139,7 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
   void DoRemoveAllClients();
 
   // Signal network thread to cease activities.
-  void DoStopOnNetworkThread(Task* done_task);
+  void DoStopOnNetworkThread(const base::Closure& done_task);
 
   // Callback for the last packet in one update. Deletes |packet| and
   // schedules next screen capture.
@@ -151,7 +150,7 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
   void DoEncode(scoped_refptr<CaptureData> capture_data);
 
   // Perform stop operations on encode thread.
-  void DoStopOnEncodeThread(Task* done_task);
+  void DoStopOnEncodeThread(const base::Closure& done_task);
 
   // EncodedDataAvailableCallback takes ownership of |packet|.
   void EncodedDataAvailableCallback(VideoPacket* packet);
diff --git a/remoting/host/screen_recorder_unittest.cc b/remoting/host/screen_recorder_unittest.cc
index a1c4f13..55a7e45 100644
--- a/remoting/host/screen_recorder_unittest.cc
+++ b/remoting/host/screen_recorder_unittest.cc
@@ -2,11 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "remoting/host/screen_recorder.h"
+
+#include "base/bind.h"
 #include "base/message_loop.h"
 #include "base/task.h"
 #include "remoting/base/base_mock_objects.h"
 #include "remoting/host/host_mock_objects.h"
-#include "remoting/host/screen_recorder.h"
 #include "remoting/proto/video.pb.h"
 #include "remoting/protocol/protocol_mock_objects.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -184,8 +186,7 @@ TEST_F(ScreenRecorderTest, StartAndStop) {
       .WillOnce(DoAll(
           FinishSend(),
           StopScreenRecorder(record_,
-                             NewRunnableFunction(&QuitMessageLoop,
-                                                 &message_loop_))))
+                             base::Bind(&QuitMessageLoop, &message_loop_))))
       .RetiresOnSaturation();
 
   // Add the mock client connection to the session.
@@ -197,7 +198,7 @@ TEST_F(ScreenRecorderTest, StartAndStop) {
 }
 
 TEST_F(ScreenRecorderTest, StopWithoutStart) {
-  record_->Stop(NewRunnableFunction(&QuitMessageLoop, &message_loop_));
+  record_->Stop(base::Bind(&QuitMessageLoop, &message_loop_));
   message_loop_.Run();
 }