Update CertVerifier to watch for the origin loop's destruction, so that

net/base/cert_verifier.cc

 // Copyright (c) 2008 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/cert_verifier.h"
 
 #if defined(USE_NSS)
 #include <private/pprthred.h>  // PR_DetatchThread
 #endif
 
 #include "base/message_loop.h"
 #include "base/worker_pool.h"
 #include "net/base/cert_verify_result.h"
 #include "net/base/net_errors.h"
 #include "net/base/x509_certificate.h"
 
 namespace net {
 
 class CertVerifier::Request :
-    public base::RefCountedThreadSafe<CertVerifier::Request> {
+    public base::RefCountedThreadSafe<CertVerifier::Request,
+                                      CertVerifier::RequestTraits>,
+    public MessageLoop::DestructionObserver {
  public:
   Request(CertVerifier* verifier,
           X509Certificate* cert,
           const std::string& hostname,
           int flags,
           CertVerifyResult* verify_result,
           CompletionCallback* callback)
       : cert_(cert),
         hostname_(hostname),
         flags_(flags),
         verifier_(verifier),
         verify_result_(verify_result),
         callback_(callback),
         origin_loop_(MessageLoop::current()),
         error_(OK) {
+    if (origin_loop_)
+      origin_loop_->AddDestructionObserver(this);
   }
 
   void DoVerify() {
     // Running on the worker thread
     error_ = cert_->Verify(hostname_, flags_, &result_);
 #if defined(USE_NSS)
     // Detach the thread from NSPR.
     // Calling NSS functions attaches the thread to NSPR, which stores
     // the NSPR thread ID in thread-specific data.
     // The threads in our thread pool terminate after we have called
@@ skipping 31 matching lines @@
     // Drop the verifier's reference to us.  Do this before running the
     // callback since the callback might result in the verifier being
     // destroyed.
     verifier_->request_ = NULL;
 
     callback_->Run(error_);
   }
 
   void Cancel() {
     verifier_ = NULL;
+    AutoLock locked(origin_loop_lock_);
+    if (origin_loop_) {
+      origin_loop_->RemoveDestructionObserver(this);
+      origin_loop_ = NULL;
+    }
+  }
 
+  // MessageLoop::DestructionObserver override.
+  virtual void WillDestroyCurrentMessageLoop() {
+    LOG(ERROR) << "CertVerifier wasn't deleted before the thread was deleted.";
     AutoLock locked(origin_loop_lock_);
     origin_loop_ = NULL;
   }
 
  private:
   friend class base::RefCountedThreadSafe<CertVerifier::Request>;
+  friend class DeleteTask<CertVerifier::Request>;
+  friend struct CertVerifier::RequestTraits;
 
-  ~Request() {}
+  ~Request() {
+    Cancel();
+  }
+
+  // If we have an origin_loop_, the observer needs to be removed on that
+  // thread.  Using OnDestruct is called by RequestTraits to ensure this

[wtc, 2010/11/11 22:24:11]

Nit: remove "Using".

[Zachary Kuznia, 2010/11/15 10:25:09]

Done.

+  // happens.
+  void OnDestruct() const {
+    AutoLock locked(origin_loop_lock_);

[willchan no longer on Chromium, 2010/11/12 00:32:29]

Won't this crash?  On line 122, you delete this.  When we exit the scope,
|origin_loop_lock_| will already be deleted.  So AutoLock will try to unlock a
deleted lock.  That's broken.

[wtc, 2010/11/12 00:44:00]

willchan: you're right.  Good catch.  By the way, can a
'const' method delete 'this'?

zork: please fix this before you check this in. You can
try:
 void OnDestruct() const {
   {
     AutoLock locked(origin_loop_lock_);
     if (origin_loop_ && origin_loop_ != MessageLoop::current()) {
       origin_loop_->DeleteSoon(FROM_HERE, this);
       return;
     }
   }
   delete this;
 }

Or you can call Lock() and Unlock() on origin_loop_lock_
manually.

[Zachary Kuznia, 2010/11/15 10:25:09]

Fixed.

You can actually delete 'this' in a const method, but you can't access the lock.
 I seem to have uploaded a stale file last time, I actually needed to add a
const_cast to the Destruct() method below.

+    if (origin_loop_ && origin_loop_ != MessageLoop::current()) {
+      origin_loop_->DeleteSoon(FROM_HERE, this);
+    } else {
+      delete this;
+    }
+  }
 
   // Set on the origin thread, read on the worker thread.
   scoped_refptr<X509Certificate> cert_;
   std::string hostname_;
   // bitwise OR'd of X509Certificate::VerifyFlags.
   int flags_;
 
   // Only used on the origin thread (where Verify was called).
   CertVerifier* verifier_;
   CertVerifyResult* verify_result_;
   CompletionCallback* callback_;
 
   // Used to post ourselves onto the origin thread.
   Lock origin_loop_lock_;
   MessageLoop* origin_loop_;
 
   // Assigned on the worker thread, read on the origin thread.
   int error_;
   CertVerifyResult result_;
 };
 
+struct CertVerifier::RequestTraits {
+  static void Destruct(const CertVerifier::Request* request) {
+    request->OnDestruct();
+  }
+};
+
 //-----------------------------------------------------------------------------
 
 CertVerifier::CertVerifier() {
 }
 
 CertVerifier::~CertVerifier() {
   if (request_)
     request_->Cancel();
 }
 
@@ skipping 19 matching lines @@
           NewRunnableMethod(request_.get(), &Request::DoVerify), true)) {
     NOTREACHED();
     request_ = NULL;
     return ERR_FAILED;
   }
 
   return ERR_IO_PENDING;
 }
 
 }  // namespace net