Fix shutdown crash in CertVerifier by using a MessageLoopProxy.

Fix shutdown crash in CertVerifier by using a MessageLoopProxy.

The CertVerifier is not getting Cancel()'d because something which owns it is getting leaked, most likely a URLRequestJob.  Therefore, we can end up accessing a deleted MessageLoop on shutdown.  MessageLoopProxy prevents accessing a deleted MessageLoop on shutdown, instead it just deletes the task, which isn't great, but it's better than crashing.  We should fix the root cause eventually, which is a leak of the URLRequestJob.

BUG=42275, chromium-os:8179
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67172

[wtc, 2010-11-23 18:26:05]

willchan: is this CL solving the same problem as zork's
CL (http://codereview.chromium.org/4299001/show)?

[willchan no longer on Chromium, 2010-11-23 18:33:21]

Oh yeah.  I forgot about that.  Thought the bug looked familiar :P  I'll
assign it to him and mark it fixed.

On Tue, Nov 23, 2010 at 10:26 AM, <wtc@chromium.org> wrote:

> willchan: is this CL solving the same problem as zork's
> CL (http://codereview.chromium.org/4299001/show)?
>
>
> http://codereview.chromium.org/5347001/
>

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

LGTM.  Please check this in for M9.  I suggest some changes
below.  You only need to make two changes (DCHECK=>LOG(ERROR)
and renaming origin_loop_), the other changes are optional.

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:11: #include "base/message_loop.h"
Nit: you should be able to remove #include "base/message_loop.h"
now.

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:35:
origin_loop_(base::MessageLoopProxy::CreateForCurrentThread()),
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().

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:61: DCHECK(posted);
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.

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:107: // Use a MessageLoopProxy in case the owner of
the CertVerifier is leaked, so
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.

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:109: scoped_refptr<base::MessageLoopProxy>
origin_loop_;
Should we rename this member origin_loop_proxy_?

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

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:11: #include "base/message_loop.h"
On 2010/11/23 19:17:13, wtc wrote:
> Nit: you should be able to remove #include "base/message_loop.h"
> now.

Done.

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:35:
origin_loop_(base::MessageLoopProxy::CreateForCurrentThread()),
On 2010/11/23 19:17:13, wtc wrote:
> 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().

Done.

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:61: DCHECK(posted);
On 2010/11/23 19:17:13, wtc wrote:
> 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.

Done.

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:107: // Use a MessageLoopProxy in case the owner of
the CertVerifier is leaked, so
On 2010/11/23 19:17:13, wtc wrote:
> 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.

Done.

http://codereview.chromium.org/5347001/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:109: scoped_refptr<base::MessageLoopProxy>
origin_loop_;
On 2010/11/23 19:17:13, wtc wrote:
> Should we rename this member origin_loop_proxy_?

Done.

http://codereview.chromium.org/5347001/diff/11001/base/message_loop_proxy_imp...
File base/message_loop_proxy_impl.cc (right):

http://codereview.chromium.org/5347001/diff/11001/base/message_loop_proxy_imp...
base/message_loop_proxy_impl.cc:81: // We shouldn't use MessageLoop::current()
since it uses LazyInstance which
This is safe in the CertVerifier case because the MessageLoop being destroyed is
IO thread's message loop, which always gets destroyed before ~AtExitManager.