Don't call RemoveDestructionObserver on non-IO thread.

Don't call RemoveDestructionObserver on non-IO thread.

RemoveDestructionObserver is expected to be called on the IO thread.
Instead, just checking io_loop_ is NULL in destructor.
IO thread should be deleted and call WillDestroyCurrentMessageLoop() before deleting singletons in AtExitManager.

BUG=28526, 28769
TEST=none

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

[wtc1, 2009-12-10 00:28:41]

On Fri, Dec 4, 2009 at 2:20 AM,  <ukai@chromium.org> wrote:
> Reviewers: wtc,
>
> Description:
> Delete OCSPInitSingleton on IO thread.
>
> BUG=28526,28769
> TEST=none
>
> Please review this at http://codereview.chromium.org/460067

I will review this CL tomorrow.  Sorry about the delay.

Wan-Teh

[wtc, 2009-12-11 02:12:30]

This CL looks correct, but it adds a static Lock object.
That violates our policy of avoiding static objects.

I wonder if we can assume that the IO thread is destroyed
before the OCSPInitSingleton singleton object is destroyed.
If we can assume that, then we know
OCSPInitSingleton::WillDestroyCurrentMessageLoop will
be called before the OCSPInitSingleton destructor is
called, and so the destructor doesn't need to call
  io_loop_->RemoveDestructionObserver(this);
We can just DCHECK(!io_loop_) in the destructor.

What do you think?

http://codereview.chromium.org/460067/diff/4001/4
File net/ocsp/nss_ocsp.cc (right):

http://codereview.chromium.org/460067/diff/4001/4#newcode91
net/ocsp/nss_ocsp.cc:91: static Lock lock_;
I don't understand why lock_, ocsp_init_, and
io_loop_ all become static members.  Is it because
we can't use the Singleton template?

The lock_ static member is especially problematic
because our policy is to avoid static objects.

http://codereview.chromium.org/460067/diff/4001/4#newcode93
net/ocsp/nss_ocsp.cc:93: static OCSPInitSingleton* ocsp_init_;
Please name this member ocsp_init_singleton_.

http://codereview.chromium.org/460067/diff/4001/4#newcode325
net/ocsp/nss_ocsp.cc:325: io_loop_ = NULL;
I think we also need to call
  io_loop_->RemoveDestructionObserver(this);
in this method.

Note that WillDestroyCurrentMessageLoop also calls
  io_loop_->RemoveDestructionObserver(this);
from WillDestroyCurrentMessageLoop?
this method. Is it harmless to call

[wtc, 2009-12-11 02:17:54]

Another issue is whether we shut down the worker threads.

If we don't shut down the worker threads, then we need to
find out whether the worker threads are still running when
the process is destroying the various Singletons.

maruel: do we destroy Singletons using atexit handlers?
When the process is running atexit handlers, are the other
threads still running?

[M-A Ruel, 2009-12-11 02:32:40]

I'm against this change, it adds a global variable with a constructor (the
lock).

You should use a LeakySingleton and delete it manually if you want/need to.
Probably don't need at all.

[ukai, 2009-12-11 03:30:44]

On 2009/12/11 02:32:40, Marc-Antoine Ruel wrote:
> I'm against this change, it adds a global variable with a constructor (the
> lock).
> 
> You should use a LeakySingleton and delete it manually if you want/need to.
> Probably don't need at all.

Initially, I used a LeakySingleton and did as wtc said, but found it fails in
net_unittests, because IO thread was not deleted before the singleton is
deleted.  I'll check it again.

[ukai, 2009-12-14 04:42:38]

Seems to be ok just to use Singleton and checks io_loop_ is NULL (by
WillDestroyCurrnetMessageLoop()).

Could you take a look again, please?
Thanks.

[M-A Ruel, 2009-12-14 15:14:38]

http://codereview.chromium.org/460067/diff/10001/9002
File net/ocsp/nss_ocsp.cc (right):

http://codereview.chromium.org/460067/diff/10001/9002#newcode32
net/ocsp/nss_ocsp.cc:32: static const int kRecvBufferSize = 4096;
Style issue: shouldn't use both static and anonymous namespace. I know you
didn't write this code but please fix by removing static keyword.

http://codereview.chromium.org/460067/diff/10001/9002#newcode42
net/ocsp/nss_ocsp.cc:42: DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
Move the dcheck inside the lock.

Do you want a SWMR lock instead?

http://codereview.chromium.org/460067/diff/10001/9002#newcode72
net/ocsp/nss_ocsp.cc:72: DCHECK(!io_loop_);
You should grab the lock here too.

http://codereview.chromium.org/460067/diff/10001/9002#newcode252
net/ocsp/nss_ocsp.cc:252: DCHECK(!io_loop_);
Grab the lock here too

http://codereview.chromium.org/460067/diff/10001/9002#newcode273
net/ocsp/nss_ocsp.cc:273: io_loop_ = MessageLoopForIO::current();
Do you hold the lock while modifying it?

http://codereview.chromium.org/460067/diff/10001/9002#newcode303
net/ocsp/nss_ocsp.cc:303: if (io_loop_)
lock?

[ukai, 2009-12-15 04:26:09]

http://codereview.chromium.org/460067/diff/10001/9002
File net/ocsp/nss_ocsp.cc (right):

http://codereview.chromium.org/460067/diff/10001/9002#newcode32
net/ocsp/nss_ocsp.cc:32: static const int kRecvBufferSize = 4096;
On 2009/12/14 15:14:39, Marc-Antoine Ruel wrote:
> Style issue: shouldn't use both static and anonymous namespace. I know you
> didn't write this code but please fix by removing static keyword.

Done.

http://codereview.chromium.org/460067/diff/10001/9002#newcode42
net/ocsp/nss_ocsp.cc:42: DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
On 2009/12/14 15:14:39, Marc-Antoine Ruel wrote:
> Move the dcheck inside the lock.
> 
> Do you want a SWMR lock instead?

Done.

http://codereview.chromium.org/460067/diff/10001/9002#newcode72
net/ocsp/nss_ocsp.cc:72: DCHECK(!io_loop_);
On 2009/12/14 15:14:39, Marc-Antoine Ruel wrote:
> You should grab the lock here too.

Done.

http://codereview.chromium.org/460067/diff/10001/9002#newcode252
net/ocsp/nss_ocsp.cc:252: DCHECK(!io_loop_);
On 2009/12/14 15:14:39, Marc-Antoine Ruel wrote:
> Grab the lock here too

Done.

http://codereview.chromium.org/460067/diff/10001/9002#newcode273
net/ocsp/nss_ocsp.cc:273: io_loop_ = MessageLoopForIO::current();
On 2009/12/14 15:14:39, Marc-Antoine Ruel wrote:
> Do you hold the lock while modifying it?

Done.

http://codereview.chromium.org/460067/diff/10001/9002#newcode303
net/ocsp/nss_ocsp.cc:303: if (io_loop_)
On 2009/12/14 15:14:39, Marc-Antoine Ruel wrote:
> lock?

Done.

[M-A Ruel, 2009-12-15 18:43:22]

lgtm with comments

http://codereview.chromium.org/460067/diff/12001/13001
File net/ocsp/nss_ocsp.cc (right):

http://codereview.chromium.org/460067/diff/12001/13001#newcode129
net/ocsp/nss_ocsp.cc:129: DCHECK(!io_loop_);
This function is fine to be called without a lock? If so, comment why.

http://codereview.chromium.org/460067/diff/12001/13001#newcode280
net/ocsp/nss_ocsp.cc:280: io_loop_->AddDestructionObserver(this);
err, put that inside the lock too.

http://codereview.chromium.org/460067/diff/12001/13001#newcode309
net/ocsp/nss_ocsp.cc:309: AutoLock autolock(lock_);
The sad side-effect is that the lock will be taken unneededly in release. Maybe
worth a #ifndef NDEBUG ?

http://codereview.chromium.org/460067/diff/12001/13001#newcode563
net/ocsp/nss_ocsp.cc:563: OCSPInitSingleton::OCSPInitSingleton()
I wonder why the constructor has been moved this far at the bottom of the file.
Please don't move this function in this change though.

[wtc, 2009-12-16 01:49:18]

LGTM.  Reviewing this code always makes my head hurt :-)

http://codereview.chromium.org/460067/diff/12001/13001
File net/ocsp/nss_ocsp.cc (right):

http://codereview.chromium.org/460067/diff/12001/13001#newcode45
net/ocsp/nss_ocsp.cc:45: request_context_ = NULL;
Do we need to set request_context_ to NULL? Or is this
just defensive programming?

http://codereview.chromium.org/460067/diff/12001/13001#newcode129
net/ocsp/nss_ocsp.cc:129: DCHECK(!io_loop_);
On 2009/12/15 18:43:22, Marc-Antoine Ruel wrote:
> This function is fine to be called without a lock? If so, comment why.

This subtle point is worth a comment.  Some sort of locking
is indeed required, but it's done by the PostTaskToIOLoop
method.

http://codereview.chromium.org/460067/diff/12001/13001#newcode252
net/ocsp/nss_ocsp.cc:252: AutoLock autolock(lock_);
If we don't have reference leaks, when this destructor
is called, there should be only one thread that has
a reference to this object, and so that thread doesn't
need to lock lock_.

http://codereview.chromium.org/460067/diff/12001/13001#newcode563
net/ocsp/nss_ocsp.cc:563: OCSPInitSingleton::OCSPInitSingleton()
On 2009/12/15 18:43:22, Marc-Antoine Ruel wrote:
> I wonder why the constructor has been moved this far at the bottom of the
file.
> Please don't move this function in this change though.

Yeah, I also hate this.  We should move this function
in a follow-up CL.