Chromium Code Reviews
Help | Chromium Project | Sign in
(34)

Issue 11347039: Ensure that OCSPIOLoop is associated with right thread if restarted. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 9 months ago by blundell (OOO until August 16)
Modified:
2 years, 8 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Ensure that OCSPIOLoop is associated with right thread if restarted. In tests, g_ocsp_io_loop can be shut down and restarted. Ensure that if it is restarted it is associated with the current IO thread to avoid DCHECK's potentially going off when it is subsequently (re-)shut down. BUG= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166869

Patch Set 1 #

Patch Set 2 : Edits #

Total comments: 8

Patch Set 3 : Response to review #

Total comments: 4

Patch Set 4 : Response to review #

Total comments: 3

Messages

Total messages: 14 (0 generated)
blundell (OOO until August 16)
Ryan, This patch is following up our discussion on IRC last week. WDYT?
3 years, 8 months ago (2012-11-05 13:14:00 UTC) #1
Ryan Sleevi (slow)
Regrettably, I've since popped the stack from our IRC conversation. I recall that one of ...
3 years, 8 months ago (2012-11-05 14:43:30 UTC) #2
blundell (OOO until August 16)
Thanks for the review. Comments answered...no code changes as of yet, pending discussion on which ...
3 years, 8 months ago (2012-11-05 15:19:02 UTC) #3
Ryan Sleevi (slow)
http://codereview.chromium.org/11347039/diff/2001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): http://codereview.chromium.org/11347039/diff/2001/net/ocsp/nss_ocsp.cc#newcode69 net/ocsp/nss_ocsp.cc:69: } On 2012/11/05 15:19:02, blundell wrote: > I thought ...
3 years, 8 months ago (2012-11-06 15:43:46 UTC) #4
blundell (OOO until August 16)
Ryan, PTAL!
3 years, 8 months ago (2012-11-08 14:13:11 UTC) #5
Ryan Sleevi (slow)
lgtm https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h File net/ocsp/nss_ocsp.h (right): https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h#newcode30 net/ocsp/nss_ocsp.h:30: // and associate it with the current IO ...
3 years, 8 months ago (2012-11-08 14:20:11 UTC) #6
blundell (OOO until August 16)
https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h File net/ocsp/nss_ocsp.h (right): https://chromiumcodereview.appspot.com/11347039/diff/7001/net/ocsp/nss_ocsp.h#newcode30 net/ocsp/nss_ocsp.h:30: // and associate it with the current IO thread, ...
3 years, 8 months ago (2012-11-08 15:59:08 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/11347039/11001
3 years, 8 months ago (2012-11-08 15:59:24 UTC) #8
commit-bot: I haz the power
Retried try job too often for step(s) content_browsertests
3 years, 8 months ago (2012-11-08 19:02:36 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/11347039/11001
3 years, 8 months ago (2012-11-09 06:45:17 UTC) #10
commit-bot: I haz the power
Change committed as 166869
3 years, 8 months ago (2012-11-09 07:31:51 UTC) #11
Nico (ooo until Aug 1)
https://chromiumcodereview.appspot.com/11347039/diff/11001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): https://chromiumcodereview.appspot.com/11347039/diff/11001/net/ocsp/nss_ocsp.cc#newcode88 net/ocsp/nss_ocsp.cc:88: thread_checker_.CalledOnValidThread(); As is, this is a no-op. You probably ...
2 years, 8 months ago (2013-11-05 03:01:29 UTC) #12
blundell (OOO until August 16)
https://codereview.chromium.org/11347039/diff/11001/net/ocsp/nss_ocsp.cc File net/ocsp/nss_ocsp.cc (right): https://codereview.chromium.org/11347039/diff/11001/net/ocsp/nss_ocsp.cc#newcode88 net/ocsp/nss_ocsp.cc:88: thread_checker_.CalledOnValidThread(); This call is not a no-op: it associates ...
2 years, 8 months ago (2013-11-05 16:01:31 UTC) #13
Nico (ooo until Aug 1)
2 years, 8 months ago (2013-11-05 16:29:54 UTC) #14
Message was sent while issue was closed.
https://codereview.chromium.org/11347039/diff/11001/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://codereview.chromium.org/11347039/diff/11001/net/ocsp/nss_ocsp.cc#newc...
net/ocsp/nss_ocsp.cc:88: thread_checker_.CalledOnValidThread();
On 2013/11/05 16:01:33, blundell wrote:
> This call is not a no-op: it associates |thread_checker_| with the current
> thread after just having detached it. I could put it inside a DCHECK, but as
> long as |CalledOnValidThread()| is the only public way to associate a
> thread_checker_ with a new thread after having called |DetachFromThread()|,
I'm
> not sure why we would enforce that CalledOnValidThread() always has to be
called
> in the context of something that's using the returned value.

Aha! This seems to be only place in the codebase where CalledOnValidThread() is
used like this. Maybe there could be a comment explaining this, and we could
wrap the call with ignore_result()?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 0973731