Adding DCHECKs and constness to DtlsIdentityStore.

talk/app/webrtc/dtlsidentitystore.cc

 /*
  * libjingle
  * Copyright 2015 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 57 matching lines @@
     {
       rtc::CritScope cs(&cs_);
       if (store_) {
         store_->PostGenerateIdentityResult_w(identity.Pass());
       }
     }
   }
 
   void OnMessage(rtc::Message* msg) override {
     switch (msg->message_id) {
       case MSG_GENERATE_IDENTITY:

[hbos, 2015/06/09 12:59:12]

You could add a DCHECK here that we are on the worker thread if you add a worker
thread param to the WorkerTask constructor.

[tommi, 2015/06/09 13:17:17]

I thought about that but decided to not add another member variable for now.

         GenerateIdentity();
 
         // Must delete |this|, owned by msg->pdata, on the signaling thread to
         // avoid races on disconnecting the signal.
         signaling_thread_->Post(this, MSG_DESTROY, msg->pdata);
         break;
       case MSG_DESTROY:

[hbos, 2015/06/09 12:59:11]

And here that we are on the signaling thread.

[tommi, 2015/06/09 13:17:18]

Done.  There also is one in the dtor.

         delete msg->pdata;

[hbos, 2015/06/09 12:59:11]

Add a comment that this line will delete |this| to clarify that one must not
touch member variables after this line.

[tommi, 2015/06/09 13:17:18]

Done

         break;
       default:
         CHECK(false) << "Unexpected message type";
     }
   }
 
  private:
   void OnStoreDestroyed() {
     rtc::CritScope cs(&cs_);
     store_ = NULL;
@@ skipping 12 matching lines @@
                                      rtc::Thread* worker_thread)
     : signaling_thread_(signaling_thread),
       worker_thread_(worker_thread),
       pending_jobs_(0) {}
 
 DtlsIdentityStore::~DtlsIdentityStore() {
   SignalDestroyed();
 }
 
 void DtlsIdentityStore::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();
   }
 }
 
 void DtlsIdentityStore::RequestIdentity(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();
 }
 
 void DtlsIdentityStore::OnMessage(rtc::Message* msg) {

[hbos, 2015/06/09 12:59:11]

And here that we are on the signaling thread for both MSG cases.

[tommi, 2015/06/09 13:17:18]

Done.

   switch (msg->message_id) {
     case MSG_GENERATE_IDENTITY_RESULT: {
       rtc::scoped_ptr<IdentityResultMessageData> pdata(
           static_cast<IdentityResultMessageData*>(msg->pdata));
       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 {
-  return free_identity_.get();
+  DCHECK(rtc::Thread::Current() == signaling_thread_);
+  return free_identity_.get() != nullptr;
 }
 
 void DtlsIdentityStore::GenerateIdentity() {

[hbos, 2015/06/09 12:59:11]

It's confusing that we have two GenerateIdentity in the same file, and figuring
out what is going on requires staring at the code for a bit. Can we rename one
of them for clarity?

Perhaps this can be called StartGenerateIdentityOnWorkerThread() if that's not
too long of a name. Or with a less descriptive name perhaps add a comment.

[tommi, 2015/06/09 13:17:17]

I added the _w() post fix to the one that runs on the worker thread.  That seems
to be the convention in this file.  Also made most methods of WorkerTask
private.

+  DCHECK(rtc::Thread::Current() == signaling_thread_);
   pending_jobs_++;
   LOG(LS_VERBOSE) << "New DTLS identity generation is posted, "
                   << "pending_identities=" << pending_jobs_;
 
   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);
 }
@@ skipping 11 matching lines @@
       free_identity_.reset(identity.release());
       LOG(LS_VERBOSE) << "A free DTLS identity is saved";
     }
     return;
   }
   ReturnIdentity(identity.Pass());
 }
 
 void DtlsIdentityStore::ReturnIdentity(
     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();
 
   if (identity.get()) {
     observer->OnSuccessWithIdentityObj(identity.Pass());
   } else {
     // Pass an arbitrary error code.
     observer->OnFailure(0);
     LOG(LS_WARNING) << "Failed to generate SSL identity";
   }
 
   // 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();
+    // Generate a free identity in the background.
+    GenerateIdentity();

[hbos, 2015/06/09 12:59:12]

(This generates in case we want to request another identity in the future,
right?)

[tommi, 2015/06/09 13:17:17]

yes.  I guess that's what the "free identity" reference in the comment refers
to.

   }
 }
 
 void DtlsIdentityStore::PostGenerateIdentityResult_w(
     rtc::scoped_ptr<rtc::SSLIdentity> identity) {
   DCHECK(rtc::Thread::Current() == worker_thread_);
 
   IdentityResultMessageData* msg =
       new IdentityResultMessageData(identity.release());
   signaling_thread_->Post(this, MSG_GENERATE_IDENTITY_RESULT, msg);
 }
 }  // namespace webrtc