Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2595723002:
Allow CertNetFetcher to be shutdown from the network thread (Closed)
DescriptionAllow CertNetFetcher to be shut down from the network thread
In order to be suitable for use from within CertVerifyProc, this CL makes
CertNetFetcher shareable between the network and worker threads. The
worker thread will start and wait for fetch requests, but the network
thread needs to be able to create and shutdown the fetcher. Shutdown is
needed to be able to destroy the URLRequestContext cleanly: since the
CertNetFetcher will have a reference to the URLRequestContext, the network
thread needs to be able to shut down the fetcher and cancel outstanding
requests before destroying the URLRequestContext.
Thus this CL makes CertNetFetcher refcounted (replacing its refcounted
bridge class CertNetFetcherCore) and gives it a Shutdown method to cancel
its outstanding requests and prevent new ones from starting.
Note that there is still a race where a worker thread could issue a fetch
request while the network thread is shutting down. In this case,
the fetch task would never run on the network thread and WaitForResult would
hang indefinitely. I'm not sure what to do about this except give
WaitForResult() a timeout like nss_ocsp does.
BUG=657549
Review-Url: https://codereview.chromium.org/2595723002
Cr-Commit-Position: refs/heads/master@{#443490}
Committed: https://chromium.googlesource.com/chromium/src/+/6f57ec35020860e39d1b6f520169bbeb9f3790a8
Patch Set 1 #Patch Set 2 : add shutdown flag, unit tests #Patch Set 3 : update comments, and adapt for cert_verify_tool #Patch Set 4 : tweak comments #
Total comments: 23
Patch Set 5 : eroman comments #Patch Set 6 : fix AIA tests #
Total comments: 37
Patch Set 7 : eroman comments #
Total comments: 2
Patch Set 8 : fix verify tool #if #
Total comments: 17
Patch Set 9 : last round of eroman comments #
Total comments: 5
Patch Set 10 : update comments #Messages
Total messages: 52 (33 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== temp, not ready for review Make CertNetFetcher refcounted so that it can be shutdown on IO-thread teardown BUG= ========== to ========== Allow CertNetFetcher to be shut down from the network thread In order to be suitable for use from within CertVerifyProc, this CL makes CertNetFetcher shareable between the network and worker threads. The worker thread will start and wait for fetch requests, but the network thread needs to be able to create and shutdown the fetcher. Shutdown is needed to be able to destroy the URLRequestContext cleanly: since the CertNetFetcher will have a reference to the URLRequestContext, the network thread needs to be able to shut down the fetcher and cancel outstanding requests before destroying the URLRequestContext. Thus this CL makes CertNetFetcher refcounted (replacing its refcounted bridge class CertNetFetcherCore) and gives it a Shutdown method to cancel its outstanding requests and prevent new ones from starting. Note that there is still a race where a worker thread could issue a fetch request while the network thread is shutting down. In this case, the fetch task would never run on the network thread and WaitForResult would hang indefinitely. I'm not sure what to do about this except give WaitForResult() a timeout like nss_ocsp does. BUG=657549 ==========
estark@chromium.org changed reviewers: + eroman@chromium.org
eroman, PTAL? https://codereview.chromium.org/2595723002/diff/60001/net/test/url_request/ur... File net/test/url_request/url_request_hanging_read_job.cc (right): https://codereview.chromium.org/2595723002/diff/60001/net/test/url_request/ur... net/test/url_request/url_request_hanging_read_job.cc:95: if (is_done()) This is a little weird. I added it because CertNetFetcherImplTest.ShutdownCancelsRequests has the following sequence of events: - DoFetch queues a DoFetchOnNetworkThread task - The test queues a Shutdown task on the network thread - DoFetchOnNetworkThread task runs, queuing a StartAsync task - Shutdown task runs, cancelling the URLRequest - StartAsync task runs. NotifyHeadersComplete() is not supposed to be called if the request has been cancelled, which is why I needed to add this is_done() check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friendly ping
https://codereview.chromium.org/2595723002/diff/60001/net/cert/cert_net_fetch... File net/cert/cert_net_fetcher.h (right): https://codereview.chromium.org/2595723002/diff/60001/net/cert/cert_net_fetch... net/cert/cert_net_fetcher.h:49: // Shuts down the CertNetFetcher and cancels outstanding requests. Please document what the expectation is for outstanding requests: - Are they completed with an abortion error - Are they left hanging - Something in between https://codereview.chromium.org/2595723002/diff/60001/net/cert/cert_net_fetch... net/cert/cert_net_fetcher.h:64: // These methods may return nullptr if a request couldn't be started, I am not sure this is the best interface. Because shutdown happens asynchronously, it is unpredictable whether a worker thread is going to see shutdown manifest as a nullptr return here. In my opinion it is simpler and more consistent for clients to treat these the same, and not need to special case nullptr. When a request fails due to shutdown (either because it has already shut down, or because it is shut down after returning the Result pointer), we should mark the request as completed by signalling the waitable event. https://codereview.chromium.org/2595723002/diff/60001/net/cert/internal/cert_... File net/cert/internal/cert_issuer_source_aia.cc (right): https://codereview.chromium.org/2595723002/diff/60001/net/cert/internal/cert_... net/cert/internal/cert_issuer_source_aia.cc:62: return; See comment regarding interface https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:123: // for issuing requests. |context| must remain valid for the entire This comment is no longer true. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:127: // Deletion implicitly cancels any outstanding requests. This comment isn't quite true anymore either. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:395: DCHECK(requests_.empty()); It looks like Job::Cancel() doesn't clear requests however https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:660: context_getter->GetNetworkTaskRunner()->PostTask( There is a possible race with: (1) Create (on network thread) (2) call Shutdown() on network thread (before the init task has run). Per comment in header file, I think we should only support creation from the network thread (initialization can be more direct then). https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... File net/cert_net/cert_net_fetcher_impl.h (right): https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.h:24: // URLRequestContext (which must outlived the CertNetFetcher) on the typo outlived. Also as far as word-smithing, it needn't necessarily "outlive" the CertNetFetcher, just need to call CertNetFetcher::Shutdown() to dissociate the URLRequestContext* and cancel requests before it potentially does die. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.h:30: scoped_refptr<base::SingleThreadTaskRunner> task_runner, I don't think we should have this variant (that takes a task_runner + URLRequestContext*) Because URLRequestContext's lifetime is tied to the network thread, passing raw pointers to it on anything other than the network thread is suspicious. I think the only version we should support and expose in this header file is: CreateCertNetFetcher(URLRequestContext*) And this must be called from the network thread. The URLRequestContextGetter variant was to support creation from other threads, which per the comment below I think we should remove. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.h:37: NET_EXPORT scoped_refptr<CertNetFetcher> CreateCertNetFetcherOnCallerThread( I don't think we want to expose this function. The only place that uses it is verify_using_path_builder.cc, and that file can be changed to invert the initialization order. https://codereview.chromium.org/2595723002/diff/60001/net/tools/cert_verify_t... File net/tools/cert_verify_tool/verify_using_path_builder.cc (right): https://codereview.chromium.org/2595723002/diff/60001/net/tools/cert_verify_t... net/tools/cert_verify_tool/verify_using_path_builder.cc:293: net::CertIssuerSourceAia aia_cert_issuer_source(cert_net_fetcher.get()); net::CertIssuerSourceAia should be updated to hold a reference to the CertNetFetcher.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
https://codereview.chromium.org/2595723002/diff/60001/net/cert/cert_net_fetch... File net/cert/cert_net_fetcher.h (right): https://codereview.chromium.org/2595723002/diff/60001/net/cert/cert_net_fetch... net/cert/cert_net_fetcher.h:49: // Shuts down the CertNetFetcher and cancels outstanding requests. On 2017/01/03 20:42:37, eroman (slow) wrote: > Please document what the expectation is for outstanding requests: > - Are they completed with an abortion error > - Are they left hanging > - Something in between Done. We can't guarantee the completion of requests that have not yet been created on the network thread (as I mentioned in the CL description), so I documented that requests can be left hanging. I think it might be better to make the Wait() a TimedWait(), however, so that requests aren't left hanging forever. WDYT? https://codereview.chromium.org/2595723002/diff/60001/net/cert/cert_net_fetch... net/cert/cert_net_fetcher.h:64: // These methods may return nullptr if a request couldn't be started, On 2017/01/03 20:42:37, eroman (slow) wrote: > I am not sure this is the best interface. > > Because shutdown happens asynchronously, it is unpredictable whether a worker > thread is going to see shutdown manifest as a nullptr return here. > > In my opinion it is simpler and more consistent for clients to treat these the > same, and not need to special case nullptr. > > When a request fails due to shutdown (either because it has already shut down, > or because it is shut down after returning the Result pointer), we should mark > the request as completed by signalling the waitable event. Done. Now if the fetcher has already been shutdown or the PostTask fails, DoFetch immediately signals the request with ERR_ABORTED and returns it. https://codereview.chromium.org/2595723002/diff/60001/net/cert/internal/cert_... File net/cert/internal/cert_issuer_source_aia.cc (right): https://codereview.chromium.org/2595723002/diff/60001/net/cert/internal/cert_... net/cert/internal/cert_issuer_source_aia.cc:62: return; On 2017/01/03 20:42:37, eroman (slow) wrote: > See comment regarding interface Done. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:123: // for issuing requests. |context| must remain valid for the entire On 2017/01/03 20:42:37, eroman (slow) wrote: > This comment is no longer true. Done. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:127: // Deletion implicitly cancels any outstanding requests. On 2017/01/03 20:42:37, eroman (slow) wrote: > This comment isn't quite true anymore either. Done. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:395: DCHECK(requests_.empty()); On 2017/01/03 20:42:37, eroman (slow) wrote: > It looks like Job::Cancel() doesn't clear requests however Done. Job::Cancel now calls FailRequest which clears the requests after signalling them. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:660: context_getter->GetNetworkTaskRunner()->PostTask( On 2017/01/03 20:42:37, eroman (slow) wrote: > There is a possible race with: > > (1) Create (on network thread) > (2) call Shutdown() on network thread (before the init task has run). > > Per comment in header file, I think we should only support creation from the > network thread (initialization can be more direct then). Done. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... File net/cert_net/cert_net_fetcher_impl.h (right): https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.h:24: // URLRequestContext (which must outlived the CertNetFetcher) on the On 2017/01/03 20:42:37, eroman (slow) wrote: > typo outlived. > > Also as far as word-smithing, it needn't necessarily "outlive" the > CertNetFetcher, just need to call CertNetFetcher::Shutdown() to dissociate the > URLRequestContext* and cancel requests before it potentially does die. Done. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.h:30: scoped_refptr<base::SingleThreadTaskRunner> task_runner, On 2017/01/03 20:42:38, eroman (slow) wrote: > I don't think we should have this variant (that takes a task_runner + > URLRequestContext*) > > Because URLRequestContext's lifetime is tied to the network thread, passing raw > pointers to it on anything other than the network thread is suspicious. > > I think the only version we should support and expose in this header file is: > > CreateCertNetFetcher(URLRequestContext*) > > And this must be called from the network thread. > > The URLRequestContextGetter variant was to support creation from other threads, > which per the comment below I think we should remove. Done. https://codereview.chromium.org/2595723002/diff/60001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.h:37: NET_EXPORT scoped_refptr<CertNetFetcher> CreateCertNetFetcherOnCallerThread( On 2017/01/03 20:42:37, eroman (slow) wrote: > I don't think we want to expose this function. > > The only place that uses it is verify_using_path_builder.cc, and that file can > be changed to invert the initialization order. Done; however, I'm not sure if I did this properly in verify_using_path_builder.cc. It seems a little convoluted the way that I did it, not sure if there is a simpler way. https://codereview.chromium.org/2595723002/diff/60001/net/tools/cert_verify_t... File net/tools/cert_verify_tool/verify_using_path_builder.cc (right): https://codereview.chromium.org/2595723002/diff/60001/net/tools/cert_verify_t... net/tools/cert_verify_tool/verify_using_path_builder.cc:293: net::CertIssuerSourceAia aia_cert_issuer_source(cert_net_fetcher.get()); On 2017/01/03 20:42:38, eroman (slow) wrote: > net::CertIssuerSourceAia should be updated to hold a reference to the > CertNetFetcher. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping (sorry to rush you... I'm reeeeeally hoping to get android AIA fetching into m57 if possible)
will review the new patchset asap today
https://codereview.chromium.org/2595723002/diff/100001/net/cert/cert_net_fetc... File net/cert/cert_net_fetcher.h (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert/cert_net_fetc... net/cert/cert_net_fetcher.h:51: // Request::WaitForResult() calls will be completed. Could you add some comments that answer the questions: * What thread(s) can this be called on * Is it safe to call more than once? * Does it need to be called before destruction of CertNetFetcher, or does destruction implicitly Shutdown? Some of this is duplication with the class level comments, but having it here is useful. https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... File net/cert/internal/cert_issuer_source_aia.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... net/cert/internal/cert_issuer_source_aia.cc:98: : cert_fetcher_(cert_fetcher) {} std::move() https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... File net/cert/internal/cert_issuer_source_aia_unittest.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... net/cert/internal/cert_issuer_source_aia_unittest.cc:124: scoped_refptr<StrictMock<MockCertNetFetcher>> mock_fetcher( nit: make_scoped_refptr() ? https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... net/cert/internal/cert_issuer_source_aia_unittest.cc:158: EXPECT_CALL(*mock_fetcher.get(), nit: I don't believe the .get() is needed here -- scoped_refptr<> has an overloaded operator*(). https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:131: // request->OnJobCompleted() will be invoked. Thanks for fixing https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:134: // words it is guaranteed that request->OnJobCompleted() will only be Can you remove the comment about guaranteeing OnJobCompleted() is not called synchronously? That behavior was a carry-over from the callback-based approach and isn't needed anymore. By removing that guartantee we should be able to simplify this change. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:305: // Create a request and attaches it to the job. When the job completes it will side-comment: Could you update Create --> Creates ? https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:319: // cancellation. If StartURLRequest() is invoked subsequently, it will See earlier comment. We should be able to call r->OnJobCompleted() immediately without needing to use a deferred task. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:412: requests_.push_back(request); std::move() https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:430: if (cancelled_) { I don't think we need this boolean. AsyncCertNetFetcherImpl::Shutdown() should call Cancel() on each job (which informs each RequestCore of completion), and then delete the Job. So we shouldn't end up calling StartURLRequest after shutdown/cancellation right? https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:438: // TODO(eroman): Don't post a task for this case. [optional] While you are here, you could also remove this post task (shouldn't need to post a task for this; previously the concern was not invoking the completion callback synchronously, but that is no longer the design we have). https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:560: for (auto request : requests_) { nit: remove the spurious reference counting during this loop https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:608: for (JobSet::iterator job = jobs_.begin(); job != jobs_.end();) { Does post-increment work? for (auto it = jobs_.begin(); it != jobs_.end(); ) { // Canceling the job also deletes it. (it++)->Cancel(); } That said this is pretty fragile and I don't think we should structure it that way. Instead, how about making Cancel() just cancel the job but not delete it? (then shutdown can bulk clear the jobs after the Cancel() loop). How about instead changing Cancel() so that it does not delete. Maybe something Could you change Cancel() to not delete itself? https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:729: ~CertNetFetcherImpl() override {} From what I can tell this relies on Shutdown() having already been called (meaning the contract is Shutdown() is ALWAYS called. Please add an assertion to verify this. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:736: request->Cancel(); I don't think this is right. RequestCore::Cancel() cancels any outstanding URLRequest (of which there is none yet), and DOES NOT notify the client waiting on the request. I believe this needs something akin to SignalImmediateError() instead. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:751: base::AutoLock auto_lock(shutdown_lock_); Are you locking here to better handle the case where the thread identified by task_runner_ (the network thread) is stopped? Because otherwise posting a task and then checking cancellation while on the Network thread (without a lock) is simpler. (I wonder if that case is encompassed by PostTask() returning false?) Either way, please add comments. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl_unittest.cc:599: URLRequestHangingReadJob::AddUrlHandler(); Is there any other code that uses this? Won't this affect the global state of subsequent test cases? https://codereview.chromium.org/2595723002/diff/100001/net/tools/cert_verify_... File net/tools/cert_verify_tool/verify_using_path_builder.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/tools/cert_verify_... net/tools/cert_verify_tool/verify_using_path_builder.cc:371: base::RunLoop run_loop; I don't think we should have a message loop on the main thread. Instead: ### Setup: (1) Start up a base::Thread for the network thread (2) Post a task to the network thread that creates the URLRequestContext, and the CertNetFetcherImpl. Use a WaitableEvent to block until this initialization is complete (or alternately subclass a Thread). (3) Use the CertNetFetcherImpl to initialize the CertIssuerSourceAia (4) call PathBuilder::Run() ### Cleanup (5) Post a task to the network thread that destroys the CertNetFetcherImpl and URLRequestContext (no need for a WaitableEvent, since joining the thread will block until it runs). If you have trouble refactoring this feel free to just "#if 0" out the code, and I can switch it over after you land your AIA change.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, PTAL. https://codereview.chromium.org/2595723002/diff/100001/net/cert/cert_net_fetc... File net/cert/cert_net_fetcher.h (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert/cert_net_fetc... net/cert/cert_net_fetcher.h:51: // Request::WaitForResult() calls will be completed. On 2017/01/11 01:56:59, eroman (slow) wrote: > Could you add some comments that answer the questions: > > * What thread(s) can this be called on > * Is it safe to call more than once? > * Does it need to be called before destruction of CertNetFetcher, or does > destruction implicitly Shutdown? > > Some of this is duplication with the class level comments, but having it here is > useful. Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... File net/cert/internal/cert_issuer_source_aia.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... net/cert/internal/cert_issuer_source_aia.cc:98: : cert_fetcher_(cert_fetcher) {} On 2017/01/11 01:56:59, eroman (slow) wrote: > std::move() Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... File net/cert/internal/cert_issuer_source_aia_unittest.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... net/cert/internal/cert_issuer_source_aia_unittest.cc:124: scoped_refptr<StrictMock<MockCertNetFetcher>> mock_fetcher( On 2017/01/11 01:56:59, eroman (slow) wrote: > nit: make_scoped_refptr() ? Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert/internal/cert... net/cert/internal/cert_issuer_source_aia_unittest.cc:158: EXPECT_CALL(*mock_fetcher.get(), On 2017/01/11 01:56:59, eroman (slow) wrote: > nit: I don't believe the .get() is needed here -- scoped_refptr<> has an > overloaded operator*(). Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:134: // words it is guaranteed that request->OnJobCompleted() will only be On 2017/01/11 01:56:59, eroman (slow) wrote: > Can you remove the comment about guaranteeing OnJobCompleted() is not called > synchronously? > > That behavior was a carry-over from the callback-based approach and isn't needed > anymore. > > By removing that guartantee we should be able to simplify this change. Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:305: // Create a request and attaches it to the job. When the job completes it will On 2017/01/11 01:56:59, eroman (slow) wrote: > side-comment: Could you update Create --> Creates ? Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:319: // cancellation. If StartURLRequest() is invoked subsequently, it will On 2017/01/11 01:57:00, eroman (slow) wrote: > See earlier comment. > > We should be able to call r->OnJobCompleted() immediately without needing to use > a deferred task. Addressed the earlier comment, but I don't think I see what needs to be changed here? https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:412: requests_.push_back(request); On 2017/01/11 01:56:59, eroman (slow) wrote: > std::move() Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:430: if (cancelled_) { On 2017/01/11 01:56:59, eroman (slow) wrote: > I don't think we need this boolean. > > AsyncCertNetFetcherImpl::Shutdown() should call Cancel() on each job (which > informs each RequestCore of completion), and then delete the Job. > > So we shouldn't end up calling StartURLRequest after shutdown/cancellation > right? Ah, yes, that's right, thanks. Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:438: // TODO(eroman): Don't post a task for this case. On 2017/01/11 01:57:00, eroman (slow) wrote: > [optional] While you are here, you could also remove this post task (shouldn't > need to post a task for this; previously the concern was not invoking the > completion callback synchronously, but that is no longer the design we have). Done. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:560: for (auto request : requests_) { On 2017/01/11 01:56:59, eroman (slow) wrote: > nit: remove the spurious reference counting during this loop Done. (I think -- this is done via const auto&, correct?) https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:608: for (JobSet::iterator job = jobs_.begin(); job != jobs_.end();) { On 2017/01/11 01:56:59, eroman (slow) wrote: > Does post-increment work? > > for (auto it = jobs_.begin(); it != jobs_.end(); ) { > // Canceling the job also deletes it. > (it++)->Cancel(); > } > > That said this is pretty fragile and I don't think we should structure it that > way. > > Instead, how about making Cancel() just cancel the job but not delete it? > > (then shutdown can bulk clear the jobs after the Cancel() loop). Done. > > How about instead changing Cancel() so that it does not delete. > > Maybe something > Could you change Cancel() to not delete itself? https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:729: ~CertNetFetcherImpl() override {} On 2017/01/11 01:56:59, eroman (slow) wrote: > From what I can tell this relies on Shutdown() having already been called > (meaning the contract is Shutdown() is ALWAYS called. > > Please add an assertion to verify this. Done. (This invalidated one of the tests I had, which was stopping the network thread before shutting down the fetcher. That would now violate the contract that Shutdown must be called before the fetcher is destroyed, so I removed that test.) https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:736: request->Cancel(); On 2017/01/11 01:56:59, eroman (slow) wrote: > I don't think this is right. > > RequestCore::Cancel() cancels any outstanding URLRequest (of which there is > none yet), and DOES NOT notify the client waiting on the request. > > I believe this needs something akin to SignalImmediateError() instead. Ah, oops, that was indeed supposed to be SignalImmediateError. Fixed, and I attempted to write a test for this but I'm not sure if I did something reasonable; see comment in the unit tests. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:751: base::AutoLock auto_lock(shutdown_lock_); On 2017/01/11 01:56:59, eroman (slow) wrote: > Are you locking here to better handle the case where the thread identified by > task_runner_ (the network thread) is stopped? > > Because otherwise posting a task and then checking cancellation while on the > Network thread (without a lock) is simpler. > > (I wonder if that case is encompassed by PostTask() returning false?) > > Either way, please add comments. Yes, I was thinking of that case, because I can't convince myself that we can ever rely on PostTask returning false. But, since we aren't in general guaranteeing that requests ever complete, I guess it's simpler to not try to handle this case specially, so I removed the |shutdown_| flag and lock and added a comment. https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl_unittest.cc:599: URLRequestHangingReadJob::AddUrlHandler(); On 2017/01/11 01:57:00, eroman (slow) wrote: > Is there any other code that uses this? > > Won't this affect the global state of subsequent test cases? Changed to remove handlers in teardown. https://codereview.chromium.org/2595723002/diff/100001/net/tools/cert_verify_... File net/tools/cert_verify_tool/verify_using_path_builder.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/tools/cert_verify_... net/tools/cert_verify_tool/verify_using_path_builder.cc:371: base::RunLoop run_loop; On 2017/01/11 01:57:00, eroman (slow) wrote: > I don't think we should have a message loop on the main thread. > > Instead: > > ### Setup: > (1) Start up a base::Thread for the network thread > (2) Post a task to the network thread that creates the URLRequestContext, and > the CertNetFetcherImpl. Use a WaitableEvent to block until this initialization > is complete (or alternately subclass a Thread). > > (3) Use the CertNetFetcherImpl to initialize the CertIssuerSourceAia > (4) call PathBuilder::Run() > > ### Cleanup > (5) Post a task to the network thread that destroys the CertNetFetcherImpl and > URLRequestContext (no need for a WaitableEvent, since joining the thread will > block until it runs). > > > If you have trouble refactoring this feel free to just "#if 0" out the code, and > I can switch it over after you land your AIA change. Thanks. I #if'd it out for now. I'm happy to come back and update this but would like to unblock the AIA change. https://codereview.chromium.org/2595723002/diff/140001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/140001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:424: requests_.push_back(std::move(request)); Switched the order of these lines so I could use std::move as suggested https://codereview.chromium.org/2595723002/diff/140001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:614: job->StartURLRequest(context_); This little reshuffling was to accommodate the fact that StartURLRequest can now call OnJobCompleted synchronously.
https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:736: request->Cancel(); On 2017/01/12 01:06:31, estark wrote: > On 2017/01/11 01:56:59, eroman (slow) wrote: > > I don't think this is right. > > > > RequestCore::Cancel() cancels any outstanding URLRequest (of which there is > > none yet), and DOES NOT notify the client waiting on the request. > > > > I believe this needs something akin to SignalImmediateError() instead. > > Ah, oops, that was indeed supposed to be SignalImmediateError. Fixed, and I > attempted to write a test for this but I'm not sure if I did something > reasonable; see comment in the unit tests. Sorry, this comment about the test is out-of-date. Since I removed the |shutdown_| flag, this case is now tested by the test that shuts down the fetcher before starting the request, so I didn't need an additional test for it.
Thanks, looks good! I will do a deeper review pass later tonight (I know you are eager to get this checked in) It does take me a little while to review each patchset as the edge cases are kind of subtle; wish I could respond more quickly :) https://codereview.chromium.org/2595723002/diff/100001/net/tools/cert_verify_... File net/tools/cert_verify_tool/verify_using_path_builder.cc (right): https://codereview.chromium.org/2595723002/diff/100001/net/tools/cert_verify_... net/tools/cert_verify_tool/verify_using_path_builder.cc:371: base::RunLoop run_loop; On 2017/01/12 01:06:31, estark wrote: > On 2017/01/11 01:57:00, eroman (slow) wrote: > > I don't think we should have a message loop on the main thread. > > > > Instead: > > > > ### Setup: > > (1) Start up a base::Thread for the network thread > > (2) Post a task to the network thread that creates the URLRequestContext, and > > the CertNetFetcherImpl. Use a WaitableEvent to block until this initialization > > is complete (or alternately subclass a Thread). > > > > (3) Use the CertNetFetcherImpl to initialize the CertIssuerSourceAia > > (4) call PathBuilder::Run() > > > > ### Cleanup > > (5) Post a task to the network thread that destroys the CertNetFetcherImpl and > > URLRequestContext (no need for a WaitableEvent, since joining the thread will > > block until it runs). > > > > > > If you have trouble refactoring this feel free to just "#if 0" out the code, > and > > I can switch it over after you land your AIA change. > > Thanks. I #if'd it out for now. I'm happy to come back and update this but would > like to unblock the AIA change. Sure, sounds good. (The tool is just used by mattm and me -- temporarily commenting out support for AIA is fine).
On 2017/01/12 02:11:37, eroman (slow) wrote: > Thanks, looks good! > > I will do a deeper review pass later tonight (I know you are eager to get this > checked in) > > It does take me a little while to review each patchset as the edge cases are > kind of subtle; wish I could respond more quickly :) I understand, all the edge cases are hurting my brain! Didn't want to rush you and definitely wouldn't want to check in something subpar just for the sake of meeting a milestone; just wanted you to know what I was shooting for. Thanks! > > https://codereview.chromium.org/2595723002/diff/100001/net/tools/cert_verify_... > File net/tools/cert_verify_tool/verify_using_path_builder.cc (right): > > https://codereview.chromium.org/2595723002/diff/100001/net/tools/cert_verify_... > net/tools/cert_verify_tool/verify_using_path_builder.cc:371: base::RunLoop > run_loop; > On 2017/01/12 01:06:31, estark wrote: > > On 2017/01/11 01:57:00, eroman (slow) wrote: > > > I don't think we should have a message loop on the main thread. > > > > > > Instead: > > > > > > ### Setup: > > > (1) Start up a base::Thread for the network thread > > > (2) Post a task to the network thread that creates the URLRequestContext, > and > > > the CertNetFetcherImpl. Use a WaitableEvent to block until this > initialization > > > is complete (or alternately subclass a Thread). > > > > > > (3) Use the CertNetFetcherImpl to initialize the CertIssuerSourceAia > > > (4) call PathBuilder::Run() > > > > > > ### Cleanup > > > (5) Post a task to the network thread that destroys the CertNetFetcherImpl > and > > > URLRequestContext (no need for a WaitableEvent, since joining the thread > will > > > block until it runs). > > > > > > > > > If you have trouble refactoring this feel free to just "#if 0" out the code, > > and > > > I can switch it over after you land your AIA change. > > > > Thanks. I #if'd it out for now. I'm happy to come back and update this but > would > > like to unblock the AIA change. > > Sure, sounds good. > (The tool is just used by mattm and me -- temporarily commenting out support for > AIA is fine).
LGTM! https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (left): https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:411: // TODO(eroman): Don't post a task for this case. Thanks! https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:384: void RequestCore::Cancel() { Optional: May consider renaming this "CancelJob" to set it apart a bit more from SignalImmediateError. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:396: bytes_.clear(); optional: Could replace these last two lines with SignalImmediateError. That would mean the completion event is additionally signalled, but that seems fine (shouldn't be waited on) https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:409: error_ = ERR_ABORTED; [optional] Could also assert that job_ is null. Technically |job_| should only be accessed from the network thread, but should be safe for reasons given above. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:410: completion_event_.Signal(); optional: May want to clear bytes_ just in case. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:604: job->AttachRequest(request); std::move() https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:613: job->AttachRequest(request); std::move https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:619: for (const auto& job : jobs_) { optional: Could also maybe make deletion == cancellation. May not be as clear though, so use whatever you think works best. ~Job() { Cancel(); } And then here jobs_.clear() is sufficient (or maybe even just ~AsyncCertNetFetcherImpl). https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:684: impl_->Shutdown(); optional: Per comment above, could combine shutdown and destructor. (in which case impl_.reset() is sufficient).
Thanks! Will update the Android CL tomorrow. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:384: void RequestCore::Cancel() { On 2017/01/13 01:13:52, eroman (slow) wrote: > Optional: May consider renaming this "CancelJob" to set it apart a bit more from > SignalImmediateError. Done. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:396: bytes_.clear(); On 2017/01/13 01:13:52, eroman (slow) wrote: > optional: Could replace these last two lines with SignalImmediateError. That > would mean the completion event is additionally signalled, but that seems fine > (shouldn't be waited on) Done. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:409: error_ = ERR_ABORTED; On 2017/01/13 01:13:52, eroman (slow) wrote: > [optional] Could also assert that job_ is null. Technically |job_| should only > be accessed from the network thread, but should be safe for reasons given above. Done. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:410: completion_event_.Signal(); On 2017/01/13 01:13:53, eroman (slow) wrote: > optional: May want to clear bytes_ just in case. Done. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:604: job->AttachRequest(request); On 2017/01/13 01:13:53, eroman (slow) wrote: > std::move() Done. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:613: job->AttachRequest(request); On 2017/01/13 01:13:52, eroman (slow) wrote: > std::move Done. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:619: for (const auto& job : jobs_) { On 2017/01/13 01:13:52, eroman (slow) wrote: > optional: Could also maybe make deletion == cancellation. May not be as clear > though, so use whatever you think works best. > > ~Job() { > Cancel(); > } > > And then here jobs_.clear() is sufficient (or maybe even just > ~AsyncCertNetFetcherImpl). Acknowledged; I find the explicit cancellation a little more readable. https://codereview.chromium.org/2595723002/diff/160001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:684: impl_->Shutdown(); On 2017/01/13 01:13:52, eroman (slow) wrote: > optional: Per comment above, could combine shutdown and destructor. (in which > case impl_.reset() is sufficient). Acknowledged.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2595723002/#ps180001 (title: "last round of eroman comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2595723002/diff/180001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/180001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:229: // does not signal completion. Can be called from any thread. Change the comment about signalling completion (with the latest patchset it does now). https://codereview.chromium.org/2595723002/diff/180001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:401: // Writing to |error_| or |job_| is safe here from either thread, because did you mean |bytes_| instead of |job_| ?
The CQ bit was unchecked by estark@chromium.org
https://codereview.chromium.org/2595723002/diff/180001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/180001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:229: // does not signal completion. Can be called from any thread. On 2017/01/13 02:51:17, eroman (slow) wrote: > Change the comment about signalling completion (with the latest patchset it does > now). Done. https://codereview.chromium.org/2595723002/diff/180001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:401: // Writing to |error_| or |job_| is safe here from either thread, because On 2017/01/13 02:51:17, eroman (slow) wrote: > did you mean |bytes_| instead of |job_| ? I did indeed mean |job_|, but it really should have been both, for the same reasoning. Changed to "data members that are normally only written on the network thread."
lgtm https://codereview.chromium.org/2595723002/diff/180001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2595723002/diff/180001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:401: // Writing to |error_| or |job_| is safe here from either thread, because On 2017/01/13 02:57:33, estark wrote: > On 2017/01/13 02:51:17, eroman (slow) wrote: > > did you mean |bytes_| instead of |job_| ? > > I did indeed mean |job_|, but it really should have been both, for the same > reasoning. Changed to "data members that are normally only written on the > network thread." New comment is good. What I think was weird in the earlier incarnation is it made it sound like we were *writing* to job_ which was not the case.
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1484276693382300,
"parent_rev": "089eff58490359a9eeebe61a104baa629ce5a1df", "commit_rev":
"6f57ec35020860e39d1b6f520169bbeb9f3790a8"}
Message was sent while issue was closed.
Description was changed from ========== Allow CertNetFetcher to be shut down from the network thread In order to be suitable for use from within CertVerifyProc, this CL makes CertNetFetcher shareable between the network and worker threads. The worker thread will start and wait for fetch requests, but the network thread needs to be able to create and shutdown the fetcher. Shutdown is needed to be able to destroy the URLRequestContext cleanly: since the CertNetFetcher will have a reference to the URLRequestContext, the network thread needs to be able to shut down the fetcher and cancel outstanding requests before destroying the URLRequestContext. Thus this CL makes CertNetFetcher refcounted (replacing its refcounted bridge class CertNetFetcherCore) and gives it a Shutdown method to cancel its outstanding requests and prevent new ones from starting. Note that there is still a race where a worker thread could issue a fetch request while the network thread is shutting down. In this case, the fetch task would never run on the network thread and WaitForResult would hang indefinitely. I'm not sure what to do about this except give WaitForResult() a timeout like nss_ocsp does. BUG=657549 ========== to ========== Allow CertNetFetcher to be shut down from the network thread In order to be suitable for use from within CertVerifyProc, this CL makes CertNetFetcher shareable between the network and worker threads. The worker thread will start and wait for fetch requests, but the network thread needs to be able to create and shutdown the fetcher. Shutdown is needed to be able to destroy the URLRequestContext cleanly: since the CertNetFetcher will have a reference to the URLRequestContext, the network thread needs to be able to shut down the fetcher and cancel outstanding requests before destroying the URLRequestContext. Thus this CL makes CertNetFetcher refcounted (replacing its refcounted bridge class CertNetFetcherCore) and gives it a Shutdown method to cancel its outstanding requests and prevent new ones from starting. Note that there is still a race where a worker thread could issue a fetch request while the network thread is shutting down. In this case, the fetch task would never run on the network thread and WaitForResult would hang indefinitely. I'm not sure what to do about this except give WaitForResult() a timeout like nss_ocsp does. BUG=657549 Review-Url: https://codereview.chromium.org/2595723002 Cr-Commit-Position: refs/heads/master@{#443490} Committed: https://chromium.googlesource.com/chromium/src/+/6f57ec35020860e39d1b6f520169... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/chromium/src/+/6f57ec35020860e39d1b6f520169... |
