Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(100)

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

Created:
8 years, 1 month ago by blundell
Modified:
7 years, 1 month ago
Reviewers:
Nico, Ryan Sleevi
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
Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -0 lines) Patch
M net/ocsp/nss_ocsp.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M net/ocsp/nss_ocsp.cc View 1 2 2 chunks +18 lines, -0 lines 3 comments Download

Messages

Total messages: 14 (0 generated)
blundell
Ryan, This patch is following up our discussion on IRC last week. WDYT?
8 years, 1 month 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 ...
8 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 ...
8 years, 1 month 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 ...
8 years, 1 month ago (2012-11-06 15:43:46 UTC) #4
blundell
Ryan, PTAL!
8 years, 1 month 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 ...
8 years, 1 month 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, ...
8 years, 1 month 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
8 years, 1 month 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
8 years, 1 month 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
8 years, 1 month ago (2012-11-09 06:45:17 UTC) #10
commit-bot: I haz the power
Change committed as 166869
8 years, 1 month 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 ...
7 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 ...
7 years, 1 month ago (2013-11-05 16:01:31 UTC) #13
Nico
7 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()?

Powered by Google App Engine
This is Rietveld 408576698