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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by blundell
Modified:
3 years, 1 month 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?
4 years, 1 month ago (2012-11-05 13:14:00 UTC) #1
Ryan Sleevi (out til 12-8)
Regrettably, I've since popped the stack from our IRC conversation. I recall that one of ...
4 years, 1 month 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 ...
4 years, 1 month ago (2012-11-05 15:19:02 UTC) #3
Ryan Sleevi (out til 12-8)
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 ...
4 years, 1 month ago (2012-11-06 15:43:46 UTC) #4
blundell
Ryan, PTAL!
4 years ago (2012-11-08 14:13:11 UTC) #5
Ryan Sleevi (out til 12-8)
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 ...
4 years 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, ...
4 years 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
4 years 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
4 years 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
4 years ago (2012-11-09 06:45:17 UTC) #10
commit-bot: I haz the power
Change committed as 166869
4 years ago (2012-11-09 07:31:51 UTC) #11
Nico
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 ...
3 years, 1 month 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 ...
3 years, 1 month ago (2013-11-05 16:01:31 UTC) #13
Nico
3 years, 1 month 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 f8e48bd