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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by blundell (OOO until April 28)
Modified:
5 months, 2 weeks ago
CC:
chromium-reviews_chromium.org, 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) Lint Patch
M net/ocsp/nss_ocsp.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments 0 errors Download
M net/ocsp/nss_ocsp.cc View 1 2 2 chunks +18 lines, -0 lines 3 comments ? errors Download
Commit:

Messages

Total messages: 14
blundell (OOO until April 28)
Ryan, This patch is following up our discussion on IRC last week. WDYT?
1 year, 5 months ago #1
Ryan Sleevi
Regrettably, I've since popped the stack from our IRC conversation. I recall that one of ...
1 year, 5 months ago #2
blundell (OOO until April 28)
Thanks for the review. Comments answered...no code changes as of yet, pending discussion on which ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #4
blundell (OOO until April 28)
Ryan, PTAL!
1 year, 5 months ago #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 ...
1 year, 5 months ago #6
blundell (OOO until April 28)
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, ...
1 year, 5 months ago #7
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/11347039/11001
1 year, 5 months ago #8
I haz the power (commit-bot)
Retried try job too often for step(s) content_browsertests
1 year, 5 months ago #9
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/11347039/11001
1 year, 5 months ago #10
I haz the power (commit-bot)
Change committed as 166869
1 year, 5 months ago #11
Nico (ooo Apr 18 - Apr 20)
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 ...
5 months, 2 weeks ago #12
blundell (OOO until April 28)
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 ...
5 months, 2 weeks ago #13
Nico (ooo Apr 18 - Apr 20)
5 months, 2 weeks ago #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 1280:2d3e6564b7b6