Revert "Fix an issue in DtlsIdentityStore when the store is destroyed before the worker thread task returns."

This reverts commit 45bc01a7172402aa4bb8d457474300533c273413.
TBR=pthatcher@webrtc.org
BUG=

Review URL: https://webrtc-codereview.appspot.com/47559004

Cr-Commit-Position: refs/heads/master@{#8711}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8711 4adac7df-926f-26a2-2b94-8c16560cd09d

talk/app/webrtc/dtlsidentitystore.cc

@@ -44,48 +44,8 @@ enum {
 };
 
 typedef rtc::ScopedMessageData<rtc::SSLIdentity> IdentityResultMessageData;
-
 }  // namespace
 
-// This class runs on the worker thread to generate the identity. It's necessary
-// to separate this class from DtlsIdentityStore so that it can live on the
-// worker thread after DtlsIdentityStore is destroyed.
-class DtlsIdentityStore::WorkerTask : public sigslot::has_slots<>,
-                                      public rtc::MessageHandler {
- public:
-  explicit WorkerTask(DtlsIdentityStore* store) : store_(store) {
-    store_->SignalDestroyed.connect(this, &WorkerTask::OnStoreDestroyed);
-  };
-
-  void GenerateIdentity() {
-    rtc::scoped_ptr<rtc::SSLIdentity> identity(
-      rtc::SSLIdentity::Generate(DtlsIdentityStore::kIdentityName));
-
-    {
-      rtc::CritScope cs(&cs_);
-      if (store_) {
-        store_->PostGenerateIdentityResult_w(identity.Pass());
-      }
-    }
-  }
-
-  void OnMessage(rtc::Message* msg) override {
-    DCHECK(msg->message_id == MSG_GENERATE_IDENTITY);
-    GenerateIdentity();
-    // Deleting msg->pdata will destroy the WorkerTask.
-    delete msg->pdata;
-  }
-
- private:
-  void OnStoreDestroyed() {
-    rtc::CritScope cs(&cs_);
-    store_ = NULL;
-  }
-
-  rtc::CriticalSection cs_;
-  DtlsIdentityStore* store_;
-};
-
 // Arbitrary constant used as common name for the identity.
 // Chosen to make the certificates more readable.
 const char DtlsIdentityStore::kIdentityName[] = "WebRTC";
@@ -96,16 +56,10 @@ DtlsIdentityStore::DtlsIdentityStore(rtc::Thread* signaling_thread,
       worker_thread_(worker_thread),
       pending_jobs_(0) {}
 
-DtlsIdentityStore::~DtlsIdentityStore() {
+DtlsIdentityStore::~DtlsIdentityStore() {}
-  SignalDestroyed();
-}
 
 void DtlsIdentityStore::Initialize() {
-  // Do not aggressively generate the free identity if the worker thread and the
+  GenerateIdentity();
-  // signaling thread are the same.
-  if (worker_thread_ != signaling_thread_) {
-    GenerateIdentity();
-  }
 }
 
 void DtlsIdentityStore::RequestIdentity(DTLSIdentityRequestObserver* observer) {
@@ -125,6 +79,9 @@ void DtlsIdentityStore::RequestIdentity(DTLSIdentityRequestObserver* observer) {
 
 void DtlsIdentityStore::OnMessage(rtc::Message* msg) {
   switch (msg->message_id) {
+    case MSG_GENERATE_IDENTITY:
+      GenerateIdentity_w();
+      break;
     case MSG_GENERATE_IDENTITY_RESULT: {
       rtc::scoped_ptr<IdentityResultMessageData> pdata(
           static_cast<IdentityResultMessageData*>(msg->pdata));
@@ -148,12 +105,7 @@ void DtlsIdentityStore::GenerateIdentity() {
   pending_jobs_++;
   LOG(LS_VERBOSE) << "New DTLS identity generation is posted, "
                   << "pending_identities=" << pending_jobs_;
-
+  worker_thread_->Post(this, MSG_GENERATE_IDENTITY, NULL);
-  WorkerTask* task = new WorkerTask(this);
-  // The WorkerTask is owned by the message data to make sure it will not be
-  // leaked even if the task does not get run.
-  IdentityTaskMessageData* msg = new IdentityTaskMessageData(task);
-  worker_thread_->Post(task, MSG_GENERATE_IDENTITY, msg);
 }
 
 void DtlsIdentityStore::OnIdentityGenerated(
@@ -191,22 +143,19 @@ void DtlsIdentityStore::ReturnIdentity(
     LOG(LS_WARNING) << "Failed to generate SSL identity";
   }
 
-  // Do not aggressively generate the free identity if the worker thread and the
+  if (pending_observers_.empty() && pending_jobs_ == 0) {
-  // signaling thread are the same.
+    // Generate a free identity in the background.
-  if (worker_thread_ != signaling_thread_ &&
+    GenerateIdentity();
-      pending_observers_.empty() &&
-      pending_jobs_ == 0) {
-      // Generate a free identity in the background.
-      GenerateIdentity();
   }
 }
 
-void DtlsIdentityStore::PostGenerateIdentityResult_w(
+void DtlsIdentityStore::GenerateIdentity_w() {
-    rtc::scoped_ptr<rtc::SSLIdentity> identity) {
   DCHECK(rtc::Thread::Current() == worker_thread_);
 
-  IdentityResultMessageData* msg =
+  rtc::SSLIdentity* identity = rtc::SSLIdentity::Generate(kIdentityName);
-      new IdentityResultMessageData(identity.release());
+
+  IdentityResultMessageData* msg = new IdentityResultMessageData(identity);
   signaling_thread_->Post(this, MSG_GENERATE_IDENTITY_RESULT, msg);
 }
+
 }  // namespace webrtc

talk/app/webrtc/dtlsidentitystore.h

@@ -33,7 +33,6 @@
 
 #include "talk/app/webrtc/peerconnectioninterface.h"
 #include "webrtc/base/messagehandler.h"
-#include "webrtc/base/messagequeue.h"
 #include "webrtc/base/scoped_ptr.h"
 #include "webrtc/base/scoped_ref_ptr.h"
 
@@ -52,7 +51,7 @@ class DtlsIdentityStore : public rtc::MessageHandler {
 
   DtlsIdentityStore(rtc::Thread* signaling_thread,
                     rtc::Thread* worker_thread);
-  virtual ~DtlsIdentityStore();
+  ~DtlsIdentityStore();
 
   // Initialize will start generating the free identity in the background.
   void Initialize();
@@ -68,16 +67,11 @@ class DtlsIdentityStore : public rtc::MessageHandler {
   bool HasFreeIdentityForTesting() const;
 
  private:
-  sigslot::signal0<sigslot::multi_threaded_local> SignalDestroyed;
-  class WorkerTask;
-  typedef rtc::ScopedMessageData<DtlsIdentityStore::WorkerTask>
-      IdentityTaskMessageData;
-
   void GenerateIdentity();
   void OnIdentityGenerated(rtc::scoped_ptr<rtc::SSLIdentity> identity);
   void ReturnIdentity(rtc::scoped_ptr<rtc::SSLIdentity> identity);
 
-  void PostGenerateIdentityResult_w(rtc::scoped_ptr<rtc::SSLIdentity> identity);
+  void GenerateIdentity_w();
 
   rtc::Thread* signaling_thread_;
   rtc::Thread* worker_thread_;

talk/app/webrtc/dtlsidentitystore_unittest.cc

@@ -80,12 +80,10 @@ class MockDtlsIdentityRequestObserver :
 class DtlsIdentityStoreTest : public testing::Test {
  protected:
   DtlsIdentityStoreTest()
-      : worker_thread_(new rtc::Thread()),
+      : store_(new DtlsIdentityStore(rtc::Thread::Current(),
-        store_(new DtlsIdentityStore(rtc::Thread::Current(),
+                                     rtc::Thread::Current())),
-                                     worker_thread_.get())),
         observer_(
             new rtc::RefCountedObject<MockDtlsIdentityRequestObserver>()) {
-    CHECK(worker_thread_->Start());
     store_->Initialize();
   }
   ~DtlsIdentityStoreTest() {}
@@ -97,7 +95,6 @@ class DtlsIdentityStoreTest : public testing::Test {
     rtc::CleanupSSL();
   }
 
-  rtc::scoped_ptr<rtc::Thread> worker_thread_;
   rtc::scoped_ptr<DtlsIdentityStore> store_;
   rtc::scoped_refptr<MockDtlsIdentityRequestObserver> observer_;
 };
@@ -109,21 +106,4 @@ TEST_F(DtlsIdentityStoreTest, RequestIdentitySuccess) {
   EXPECT_TRUE_WAIT(observer_->LastRequestSucceeded(), kTimeoutMs);
 
   EXPECT_TRUE_WAIT(store_->HasFreeIdentityForTesting(), kTimeoutMs);
-
-  observer_->Reset();
-
-  // Verifies that the callback is async when a free identity is ready.
-  store_->RequestIdentity(observer_.get());
-  EXPECT_FALSE(observer_->call_back_called());
-  EXPECT_TRUE_WAIT(observer_->LastRequestSucceeded(), kTimeoutMs);
-}
-
-TEST_F(DtlsIdentityStoreTest, DeleteStoreEarlyNoCrash) {
-  EXPECT_FALSE(store_->HasFreeIdentityForTesting());
-
-  store_->RequestIdentity(observer_.get());
-  store_.reset();
-
-  worker_thread_->Stop();
-  EXPECT_FALSE(observer_->call_back_called());
 }

talk/app/webrtc/peerconnectionfactory.cc

@@ -132,11 +132,6 @@ PeerConnectionFactory::~PeerConnectionFactory() {
   DCHECK(signaling_thread_->IsCurrent());
   channel_manager_.reset(NULL);
   default_allocator_factory_ = NULL;
-
-  // Make sure |worker_thread_| and |signaling_thread_| outlive
-  // |dtls_identity_store_|.
-  dtls_identity_store_.reset(NULL);
-
   if (owns_ptrs_) {
     if (wraps_current_thread_)
       rtc::ThreadManager::Instance()->UnwrapCurrentThread();