Fix an issue in DtlsIdentityStore when the store is destroyed before the worker thread task returns.
BUG=crbug/464995
R=pthatcher@webrtc.org
Committed: https://code.google.com/p/webrtc/source/detail?r=8689
Review URL: https://webrtc-codereview.appspot.com/42659004
Cr-Commit-Position: refs/heads/master@{#8701}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8701 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/talk/app/webrtc/dtlsidentitystore.cc b/talk/app/webrtc/dtlsidentitystore.cc
index 7beb68d..4cc348d 100644
--- a/talk/app/webrtc/dtlsidentitystore.cc
+++ b/talk/app/webrtc/dtlsidentitystore.cc
@@ -44,8 +44,48 @@
};
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";
@@ -56,10 +96,16 @@
worker_thread_(worker_thread),
pending_jobs_(0) {}
-DtlsIdentityStore::~DtlsIdentityStore() {}
+DtlsIdentityStore::~DtlsIdentityStore() {
+ SignalDestroyed();
+}
void DtlsIdentityStore::Initialize() {
- GenerateIdentity();
+ // 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) {
@@ -79,9 +125,6 @@
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));
@@ -105,7 +148,12 @@
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(
@@ -149,13 +197,12 @@
}
}
-void DtlsIdentityStore::GenerateIdentity_w() {
+void DtlsIdentityStore::PostGenerateIdentityResult_w(
+ rtc::scoped_ptr<rtc::SSLIdentity> identity) {
DCHECK(rtc::Thread::Current() == worker_thread_);
- rtc::SSLIdentity* identity = rtc::SSLIdentity::Generate(kIdentityName);
-
- IdentityResultMessageData* msg = new IdentityResultMessageData(identity);
+ IdentityResultMessageData* msg =
+ new IdentityResultMessageData(identity.release());
signaling_thread_->Post(this, MSG_GENERATE_IDENTITY_RESULT, msg);
}
-
} // namespace webrtc
diff --git a/talk/app/webrtc/dtlsidentitystore.h b/talk/app/webrtc/dtlsidentitystore.h
index 2d52249..ffe8067 100644
--- a/talk/app/webrtc/dtlsidentitystore.h
+++ b/talk/app/webrtc/dtlsidentitystore.h
@@ -33,6 +33,7 @@
#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"
@@ -51,7 +52,7 @@
DtlsIdentityStore(rtc::Thread* signaling_thread,
rtc::Thread* worker_thread);
- ~DtlsIdentityStore();
+ virtual ~DtlsIdentityStore();
// Initialize will start generating the free identity in the background.
void Initialize();
@@ -67,11 +68,16 @@
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 GenerateIdentity_w();
+ void PostGenerateIdentityResult_w(rtc::scoped_ptr<rtc::SSLIdentity> identity);
rtc::Thread* signaling_thread_;
rtc::Thread* worker_thread_;
diff --git a/talk/app/webrtc/dtlsidentitystore_unittest.cc b/talk/app/webrtc/dtlsidentitystore_unittest.cc
index 67b7cb2..262e773 100644
--- a/talk/app/webrtc/dtlsidentitystore_unittest.cc
+++ b/talk/app/webrtc/dtlsidentitystore_unittest.cc
@@ -80,10 +80,12 @@
class DtlsIdentityStoreTest : public testing::Test {
protected:
DtlsIdentityStoreTest()
- : store_(new DtlsIdentityStore(rtc::Thread::Current(),
- rtc::Thread::Current())),
+ : worker_thread_(new rtc::Thread()),
+ store_(new DtlsIdentityStore(rtc::Thread::Current(),
+ worker_thread_.get())),
observer_(
new rtc::RefCountedObject<MockDtlsIdentityRequestObserver>()) {
+ CHECK(worker_thread_->Start());
store_->Initialize();
}
~DtlsIdentityStoreTest() {}
@@ -95,6 +97,7 @@
rtc::CleanupSSL();
}
+ rtc::scoped_ptr<rtc::Thread> worker_thread_;
rtc::scoped_ptr<DtlsIdentityStore> store_;
rtc::scoped_refptr<MockDtlsIdentityRequestObserver> observer_;
};
@@ -106,4 +109,21 @@
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());
}
diff --git a/talk/app/webrtc/peerconnectionfactory.cc b/talk/app/webrtc/peerconnectionfactory.cc
index 9f1e858..1c93376 100644
--- a/talk/app/webrtc/peerconnectionfactory.cc
+++ b/talk/app/webrtc/peerconnectionfactory.cc
@@ -132,6 +132,11 @@
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();
