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

net/base/cert_verifier.cc

Index: net/base/cert_verifier.cc
diff --git a/net/base/cert_verifier.cc b/net/base/cert_verifier.cc
index 4e941335653adea573ae13ae3dadcb18ebed3c01..70317c9bc022f4903185af2fa0c2e260b57e0b65 100644
--- a/net/base/cert_verifier.cc
+++ b/net/base/cert_verifier.cc
@@ -17,7 +17,9 @@
 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,
@@ -33,6 +35,8 @@ class CertVerifier::Request :
         callback_(callback),
         origin_loop_(MessageLoop::current()),
         error_(OK) {
+    if (origin_loop_)
+      origin_loop_->AddDestructionObserver(this);
   }
 
   void DoVerify() {
@@ -55,7 +59,7 @@ class CertVerifier::Request :
     // 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;
       }
@@ -84,15 +88,34 @@ class CertVerifier::Request :
 
   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() {

[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);
+    }
+  }
 
-  ~Request() {}
+  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_;
@@ -114,6 +137,12 @@ class CertVerifier::Request :
   CertVerifyResult result_;
 };
 
+struct CertVerifier::RequestTraits {
+  static void Destruct(const CertVerifier::Request* request) {
+    request->OnDestruct();
+  }
+};
+
 //-----------------------------------------------------------------------------
 
 CertVerifier::CertVerifier() {