Fix shutdown crash in CertVerifier by using a MessageLoopProxy.

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..d5acd99faa47db5e260b7953e8d84f9097fa34ae 100644
--- a/net/base/cert_verifier.cc
+++ b/net/base/cert_verifier.cc
@@ -9,6 +9,7 @@
 #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"
@@ -31,7 +32,7 @@ class CertVerifier::Request :
         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) {
   }
 
@@ -49,20 +50,16 @@ class CertVerifier::Request :
     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() {
@@ -107,7 +104,9 @@ class CertVerifier::Request :
 
   // 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_;