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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 9 months ago by blundell
Modified:
1 year, 9 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
Ryan, This patch is following up our discussion on IRC last week. WDYT?
2 years, 9 months ago (2012-11-05 13:14:00 UTC) #1
Ryan Sleevi
Regrettably, I've since popped the stack from our IRC conversation. I recall that one of ...
2 years, 9 months ago (2012-11-05 14:43:30 UTC) #2
blundell
Thanks for the review. Comments answered...no code changes as of yet, pending discussion on which ...
2 years, 9 months ago (2012-11-05 15:19:02 UTC) #3
Ryan Sleevi
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 ...
2 years, 9 months ago (2012-11-06 15:43:46 UTC) #4
blundell
Ryan, PTAL!
2 years, 8 months ago (2012-11-08 14:13:11 UTC) #5
Ryan Sleevi
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 ...
2 years, 8 months ago (2012-11-08 14:20:11 UTC) #6
blundell
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, ...
2 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
2 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
2 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
2 years, 8 months ago (2012-11-09 06:45:17 UTC) #10
commit-bot: I haz the power
Change committed as 166869
2 years, 8 months ago (2012-11-09 07:31:51 UTC) #11
Nico (hiding)
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 ...
1 year, 9 months ago (2013-11-05 03:01:29 UTC) #12
blundell
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 ...
1 year, 9 months ago (2013-11-05 16:01:31 UTC) #13
Nico (hiding)
1 year, 9 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 3ea459f