Fix shutdown crash in CertVerifier by using a MessageLoopProxy.

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"

[wtc, 2010/11/23 19:17:13]

Nit: you should be able to remove #include "base/message_loop.h"
now.

[willchan no longer on Chromium, 2010/11/23 20:32:42]

Done.

+#include "base/message_loop_proxy.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:
   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()),
+        origin_loop_(base::MessageLoopProxy::CreateForCurrentThread()),

[wtc, 2010/11/23 19:17:13]

It would be nice to update the comment in message_loop_proxy.h
to highlight the importance of MessageLoopProxy::CreateForCurrentThread().
Right now it only says:
    You can obtain a MessageLoopProxy via
    Thread::message_loop_proxy().
But when we don't have access to the Thread object, we
have to call MessageLoopProxy::CreateForCurrentThread().

[willchan no longer on Chromium, 2010/11/23 20:32:42]

Done.

         error_(OK) {
   }
 
   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);
+    scoped_ptr<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_) {
-        origin_loop_->PostTask(FROM_HERE, reply);
-        reply = NULL;
-      }
+    AutoLock locked(origin_loop_lock_);
+    if (origin_loop_) {
+      bool posted = origin_loop_->PostTask(FROM_HERE, reply.release());
+      // Try to catch leaked CertVerifiers on shutdown with this DCHECK.
+      DCHECK(posted);

[wtc, 2010/11/23 19:17:13]

I suggest using an LOG(ERROR) or LOG(WARNING) message instead
of DCHECK.  This DCHECK failure is difficult to track down,
and we already know it will fail.  We should not train people
to ignore DCHECK failures.

[willchan no longer on Chromium, 2010/11/23 20:32:42]

Done.

     }
-
-    // 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_;
@@ skipping 24 matching lines @@
   // 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_;
+  // Use a MessageLoopProxy in case the owner of the CertVerifier is leaked, so

[wtc, 2010/11/23 19:17:13]

This comment should elaborate on how a leaked owner of
CertVerifier leads to a crash.  I think it's because if
the owner is leaked, it won't delete the CertVerifier before
the origin loop is gone, so the PostTask call on the origin
loop can crash.

[willchan no longer on Chromium, 2010/11/23 20:32:42]

Done.

+  // this code won't crash: http://crbug.com/42275.
+  scoped_refptr<base::MessageLoopProxy> origin_loop_;

[wtc, 2010/11/23 19:17:13]

Should we rename this member origin_loop_proxy_?

[willchan no longer on Chromium, 2010/11/23 20:32:42]

Done.

 
   // Assigned on the worker thread, read on the origin thread.
   int error_;
   CertVerifyResult result_;
 };
 
 //-----------------------------------------------------------------------------
 
 CertVerifier::CertVerifier() {
 }
@@ skipping 25 matching lines @@
           NewRunnableMethod(request_.get(), &Request::DoVerify), true)) {
     NOTREACHED();
     request_ = NULL;
     return ERR_FAILED;
   }
 
   return ERR_IO_PENDING;
 }
 
 }  // namespace net