DtlsIdentityStore[Interface/Impl] updated, DtlsIdentityService to be removed

talk/app/webrtc/dtlsidentitystore.cc

Index: talk/app/webrtc/dtlsidentitystore.cc
diff --git a/talk/app/webrtc/dtlsidentitystore.cc b/talk/app/webrtc/dtlsidentitystore.cc
index abd35e1fab522c47334f432bc2ccb7bba6888e17..ef18b731f4e15c944b4f04723e2ba1f7ee3627cb 100644
--- a/talk/app/webrtc/dtlsidentitystore.cc
+++ b/talk/app/webrtc/dtlsidentitystore.cc
@@ -30,18 +30,19 @@
 #include "talk/app/webrtc/webrtcsessiondescriptionfactory.h"
 #include "webrtc/base/logging.h"
 
-using webrtc::DTLSIdentityRequestObserver;
+using webrtc::DtlsIdentityRequestObserver;
 using webrtc::WebRtcSessionDescriptionFactory;
 
 namespace webrtc {
 
+const char* DtlsIdentityStoreImpl::common_name_ = "WebRTC";
+
 namespace {
 
 enum {
   MSG_DESTROY,
   MSG_GENERATE_IDENTITY,
-  MSG_GENERATE_IDENTITY_RESULT,
-  MSG_RETURN_FREE_IDENTITY
+  MSG_GENERATE_IDENTITY_RESULT
 };
 
 typedef rtc::ScopedMessageData<rtc::SSLIdentity> IdentityResultMessageData;
@@ -51,11 +52,17 @@ typedef rtc::ScopedMessageData<rtc::SSLIdentity> IdentityResultMessageData;
 // 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 {
+class DtlsIdentityStoreImpl::WorkerTask : public sigslot::has_slots<>,
+                                          public rtc::MessageHandler {
  public:
-  explicit WorkerTask(DtlsIdentityStore* store)
-      : signaling_thread_(rtc::Thread::Current()), store_(store) {
+  explicit WorkerTask(
+      DtlsIdentityStoreImpl* store, rtc::KeyType key_type,
+      bool is_preemptive)
+      : signaling_thread_(rtc::Thread::Current()),
+        store_(store),
+        key_type_(key_type),
+        is_preemptive_(is_preemptive) {
+    DCHECK(!is_preemptive_ || key_type_ == rtc::KT_RSA);
     store_->SignalDestroyed.connect(this, &WorkerTask::OnStoreDestroyed);
   }
 
@@ -63,8 +70,28 @@ class DtlsIdentityStore::WorkerTask : public sigslot::has_slots<>,
 
  private:
   void GenerateIdentity_w() {
+    if (is_preemptive_) {
+      DCHECK(key_type_ == rtc::KT_RSA);
+      // If |store_| has a |free_rsa_identity_| we can take it and abort trying
+      // to generate one.

[hbos, 2015/06/15 10:10:34]

In the old code, |free_identity_| was checked when RequestIdentity was called
and if we had none then we generated a new one. This has the flaw that if we are
currently generating an identity for |free_identity_|, but we have not finished
yet, the old code would  signal to "unnecessarily" generate another identity.

This seemed like a problem to me and I added store_->ReleaseFreeRSAIdentity here
to abort generating another one, but at the cost of having to use a lock around
|free_(rsa_)identity_|.

As I'm writing this though I realized that generating an extra identity
"unnecessarily" would not have been a practical problem, since we'd want to
queue a preemptive RSA generation in the background as soon as |free_identity_|
was released anyway. The end result being the same as long as |worker_thread_|
!= |signaling_thread_|.

I'm sure you'll object to me adding this lock so we can discuss me changing this
up a bit.

[hbos, 2015/06/18 08:43:12]

In PS4 I changed the implementation quite a bit again...

+      rtc::CritScope cs(&cs_);
+      if (store_) {
+        rtc::scoped_ptr<rtc::SSLIdentity> identity(
+            store_->ReleaseFreeRSAIdentity());
+        if (identity.get()) {
+          store_->PostGenerateIdentityResult_w(identity.Pass());
+          return;
+        }
+      } else {
+        return;  // Store has already been destroyed, no need to generate.
+      }
+    }
+
+    // TODO(hbos): Use key_type_ when torbjorng's CL has landed.
+    LOG(LS_INFO) << "Generating identity. Key type (TODO: should use): "
+                 << key_type_;
     rtc::scoped_ptr<rtc::SSLIdentity> identity(
-        rtc::SSLIdentity::Generate(DtlsIdentityStore::kIdentityName));
+        rtc::SSLIdentity::Generate(common_name_));
 
     {
       rtc::CritScope cs(&cs_);
@@ -100,49 +127,43 @@ class DtlsIdentityStore::WorkerTask : public sigslot::has_slots<>,
   }
 
   rtc::Thread* const signaling_thread_;
+  // Locking this prevents |store_| from being destroyed if it has not already
+  // been so.
   rtc::CriticalSection cs_;
-  DtlsIdentityStore* store_;
+  DtlsIdentityStoreImpl* store_;
+  rtc::KeyType key_type_;
+  bool is_preemptive_;
 };
 
-// Arbitrary constant used as common name for the identity.
-// Chosen to make the certificates more readable.
-const char DtlsIdentityStore::kIdentityName[] = "WebRTC";
-
-DtlsIdentityStore::DtlsIdentityStore(rtc::Thread* signaling_thread,
-                                     rtc::Thread* worker_thread)
+DtlsIdentityStoreImpl::DtlsIdentityStoreImpl(rtc::Thread* signaling_thread,
+                                             rtc::Thread* worker_thread)
     : signaling_thread_(signaling_thread),
       worker_thread_(worker_thread),
-      pending_jobs_(0) {}
+      pending_requests_(),
+      free_rsa_identity_(nullptr),
+      free_rsa_identity_cs_() {}
 
-DtlsIdentityStore::~DtlsIdentityStore() {
+DtlsIdentityStoreImpl::~DtlsIdentityStoreImpl() {
   SignalDestroyed();
 }
 
-void DtlsIdentityStore::Initialize() {
+void DtlsIdentityStoreImpl::Initialize() {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
-  // Do not aggressively generate the free identity if the worker thread and the
-  // signaling thread are the same.
-  if (worker_thread_ != signaling_thread_) {
-    GenerateIdentity();
-  }
+  // Preemptively generate an RSA identity (|free_rsa_identity_|) unless the
+  // worker thread and signaling thread are the same (expensive).
+  if (worker_thread_ != signaling_thread_)
+    GenerateIdentity(rtc::KT_RSA, nullptr);
 }
 
-void DtlsIdentityStore::RequestIdentity(DTLSIdentityRequestObserver* observer) {
+void DtlsIdentityStoreImpl::RequestIdentity(
+    rtc::KeyType key_type, DtlsIdentityRequestObserver* observer) {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
   DCHECK(observer);
 
-  // Must return the free identity async.
-  if (free_identity_.get()) {
-    IdentityResultMessageData* msg =
-        new IdentityResultMessageData(free_identity_.release());
-    signaling_thread_->Post(this, MSG_RETURN_FREE_IDENTITY, msg);
-  }
-
-  pending_observers_.push(observer);
-  GenerateIdentity();
+  GenerateIdentity(key_type, observer);
 }
 
-void DtlsIdentityStore::OnMessage(rtc::Message* msg) {
+void DtlsIdentityStoreImpl::OnMessage(rtc::Message* msg) {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
   switch (msg->message_id) {
     case MSG_GENERATE_IDENTITY_RESULT: {
@@ -151,80 +172,88 @@ void DtlsIdentityStore::OnMessage(rtc::Message* msg) {
       OnIdentityGenerated(pdata->data().Pass());
       break;
     }
-    case MSG_RETURN_FREE_IDENTITY: {
-      rtc::scoped_ptr<IdentityResultMessageData> pdata(
-          static_cast<IdentityResultMessageData*>(msg->pdata));
-      ReturnIdentity(pdata->data().Pass());
-      break;
-    }
   }
 }
 
-bool DtlsIdentityStore::HasFreeIdentityForTesting() const {
+bool DtlsIdentityStoreImpl::HasFreeIdentityForTesting() const {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
-  return free_identity_.get() != nullptr;
+  rtc::CritScope cs(&free_rsa_identity_cs_);
+  return free_rsa_identity_.get() != nullptr;
 }
 
-void DtlsIdentityStore::GenerateIdentity() {
+void DtlsIdentityStoreImpl::GenerateIdentity(
+    rtc::KeyType key_type, DtlsIdentityRequestObserver* observer) {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
-  pending_jobs_++;
-  LOG(LS_VERBOSE) << "New DTLS identity generation is posted, "
-                  << "pending_identities=" << pending_jobs_;
+#if !defined(NDEBUG)
+  {
+    // Only grab lock for DCHECK if debugging.
+    rtc::CritScope cs(&free_rsa_identity_cs_);
+    // Preemptive generation requests (!|observer|) are only performed with RSA
+    // and must only be performed when we don't already have one generated
+    // (|free_rsa_identity_|) and don't already have such request pending.
+    DCHECK(observer || (key_type == rtc::KT_RSA && !free_rsa_identity_.get() &&
+                        !HasPendingRSARequest()));
+  }
+#endif
 
-  WorkerTask* task = new WorkerTask(this);
+  // Enqueue the request.

[hbos, 2015/06/15 10:10:34]

I made it simple by always queuing a request and handling them the same way.
However, ECDA being cheap we could generate those on the signaling thread and
avoid a couple of context switches. (Would still have to do Post though so that
the result is returned async.)

We could also avoid a couple of context switches by checking if we have a
|free_rsa_identity_| already here like the old code.

[hbos, 2015/06/18 08:43:12]

Addressed in PS4.

+  pending_requests_.push_back(IdentityRequest(key_type, observer));
+  LOG(LS_VERBOSE) << "New DTLS identity generation is posted, "
+                  << "pending requests: " << pending_requests_.size();
+  // Post one WorkerTask per request.
+  WorkerTask* task = new WorkerTask(this, key_type, !observer);
   // 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(
-    rtc::scoped_ptr<rtc::SSLIdentity> identity) {
-  DCHECK(rtc::Thread::Current() == signaling_thread_);
-
-  pending_jobs_--;
-  LOG(LS_VERBOSE) << "A DTLS identity generation job returned, "
-                  << "pending_identities=" << pending_jobs_;
-
-  if (pending_observers_.empty()) {
-    if (!free_identity_.get()) {
-      free_identity_.reset(identity.release());
-      LOG(LS_VERBOSE) << "A free DTLS identity is saved";
-    }
-    return;
-  }
-  ReturnIdentity(identity.Pass());
-}
-
-void DtlsIdentityStore::ReturnIdentity(
+void DtlsIdentityStoreImpl::OnIdentityGenerated(
     rtc::scoped_ptr<rtc::SSLIdentity> identity) {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
-  DCHECK(!free_identity_.get());
-  DCHECK(!pending_observers_.empty());
 
-  rtc::scoped_refptr<DTLSIdentityRequestObserver> observer =
-      pending_observers_.front();
-  pending_observers_.pop();
+  DCHECK(!pending_requests_.empty());
+  IdentityRequest request = pending_requests_.front();
+  pending_requests_.pop_front();
+  // TODO(hbos): would be nice to be able to do:
+  //DCHECK(request.key_type_ == same as of the one generated);
 
-  if (identity.get()) {
-    observer->OnSuccessWithIdentityObj(identity.Pass());
+  LOG(LS_VERBOSE) << "A DTLS identity generation job returned, "
+                  << "pending requests: " << pending_requests_.size();
+
+
+  // Has on observer?
+  if (request.observer_.get() == nullptr) {
+    // Not having an observer: result should be stored in |free_rsa_identity_|.
+    rtc::CritScope cs(&free_rsa_identity_cs_);
+    DCHECK(!free_rsa_identity_.get());
+    free_rsa_identity_.reset(identity.release());
+    if (free_rsa_identity_.get())
+      LOG(LS_VERBOSE) << "A free SSL identity (RSA) was saved";
+    else
+      LOG(LS_WARNING) << "Failed to generate SSL identity (preemptively)";
   } else {
-    // Pass an arbitrary error code.
-    observer->OnFailure(0);
-    LOG(LS_WARNING) << "Failed to generate SSL identity";
-  }
+    // Return the result to the observer.
+    if (identity.get()) {
+      request.observer_->OnSuccessWithIdentityObj(identity.Pass());
+    } else {
+      LOG(LS_WARNING) << "Failed to generate SSL identity";
+      // Pass an arbitrary error code.
+      request.observer_->OnFailure(0);
+    }
 
-  // Do not aggressively generate the free identity if the worker thread and the
-  // signaling thread are the same.
-  if (worker_thread_ != signaling_thread_ &&
-      pending_observers_.empty() &&
-      pending_jobs_ == 0) {
-    // Generate a free identity in the background.
-    GenerateIdentity();
+    // Preemptively generate (another) RSA identity?
+    rtc::CritScope cs(&free_rsa_identity_cs_);
+    if (!free_rsa_identity_.get() && !HasPendingRSARequest()) {
+      // Preemptively generate an RSA identity (|free_rsa_identity_|) unless the
+      // worker thread and signaling thread are the same (expensive).
+      if (worker_thread_ != signaling_thread_)
+        GenerateIdentity(rtc::KT_RSA, nullptr);
+    }
   }
 }
 
-void DtlsIdentityStore::PostGenerateIdentityResult_w(
+void DtlsIdentityStoreImpl::PostGenerateIdentityResult_w(
     rtc::scoped_ptr<rtc::SSLIdentity> identity) {
   DCHECK(rtc::Thread::Current() == worker_thread_);
 
@@ -232,4 +261,18 @@ void DtlsIdentityStore::PostGenerateIdentityResult_w(
       new IdentityResultMessageData(identity.release());
   signaling_thread_->Post(this, MSG_GENERATE_IDENTITY_RESULT, msg);
 }
+
+bool DtlsIdentityStoreImpl::HasPendingRSARequest() {
+  for (const IdentityRequest& request : pending_requests_) {
+    if (request.key_type_ == rtc::KT_RSA)
+      return true;
+  }
+  return false;
+}
+
+rtc::SSLIdentity* DtlsIdentityStoreImpl::ReleaseFreeRSAIdentity() {
+  rtc::CritScope cs(&free_rsa_identity_cs_);
+  return free_rsa_identity_.release();
+}
+
 }  // namespace webrtc