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
     // PR_Cleanup.  Unless we detach them from NSPR, net_unittests gets
     // segfaults on shutdown when the threads' thread-specific data
     // destructors run.
     PR_DetachThread();
 #endif
 
     Task* reply = NewRunnableMethod(this, &Request::DoCallback);
 
     // The origin loop could go away while we are trying to post to it, so we
     // need to call its PostTask method inside a lock.  See ~CertVerifier.
     {
       AutoLock locked(origin_loop_lock_);
-      if (origin_loop_) {
+      if (origin_loop_ && verifier_) {

[wtc, 2010/11/10 22:18:20]

BUG: Is this change necessary?

DoCallback will return immediately if verifier_ is NULL
(see lines 76-77 below).  verifier_ can only be accessed on
the origin thread (see lines 126-127 below).

I guess you're trying to avoid the PostTask call after
the Request has been Canceled (because Cancel no longer
sets origin_loop_ to NULL)?

If so, we should fix this problem by changing Cancel()
back to what it was in an earlier version of this CL:

  void Cancel() {
    verifier_ = NULL;
 
    AutoLock locked(origin_loop_lock_);
    if (origin_loop_) {
      origin_loop_->RemoveDestructionObserver(this);
      origin_loop_ = NULL;
    }
  }

[Zachary Kuznia, 2010/11/11 10:35:31]

I was trying to avoid a DCHECK if Cancel was called on a different thread, but
if we don't have to worry about that, the original version is fine.  Fixed.

         origin_loop_->PostTask(FROM_HERE, reply);
         reply = NULL;
       }
     }
 
     // Does nothing if it got posted.
     delete reply;
   }
 
   void DoCallback() {
     // Running on the origin thread.
 
     // We may have been cancelled!
     if (!verifier_)
       return;
 
     *verify_result_ = result_;
 
     // 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;
+  }
 
+  // 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() {

[wtc, 2010/11/10 22:18:20]

Please document that if origin_loop_ is not NULL, Request
must be destroyed on the origin thread, and
RequestTraits/OnDestruct ensures that.  Did I understand this
point correctly?

[Zachary Kuznia, 2010/11/11 10:35:31]

That's correct.  Added a comment to OnDestruct.

+    AutoLock locked(origin_loop_lock_);
+    if (origin_loop_) {

[wtc, 2010/11/10 22:18:20]

Nit: omit the curly braces.

[Zachary Kuznia, 2010/11/11 10:35:31]

Moved back to Cancel().

+      origin_loop_->RemoveDestructionObserver(this);
+    }
+  }
+
+  void OnDestruct() const {

[willchan no longer on Chromium, 2010/11/10 22:33:17]

I am concerned about this.  DeleteSoon() will post a DeleteTask.  The problem
is, if this is happening in shutdown (which is the original bug, a shutdown
crash), then |origin_loop_| may still be alive, but is stopping.  So the task
may end up in the loop, but it may not be run.  In this case, the DeleteTask
will get deleted, but it won't call delete on the Request, so it will get leaked
and we will get valgrind errors.  That's better than crashing, but it'll cause
memory bots to go red :(

Actually, there's a more serious bug here.  |origin_loop_| is not locked.  The
DeleteSoon() call may crash as you may be calling this on a deleted object.  You
need to lock this call.

Also, this OnDestruct() as written will post to |origin_loop_| even if we're
already on the thread with the |origin_loop_|.  That seems unnecessary and seems
more likely to cause the race that leads to the pending DeleteTask being deleted
that I explained above.

[Zachary Kuznia, 2010/11/11 10:35:31]

Added the lock, and changed the if() so that it's deleted immediately if called
on the origin thread.

+    if (origin_loop_) {
+      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