Fix SSLClientSocketOpenSSL error-handling for Channel ID.

net/ssl/server_bound_cert_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ssl/server_bound_cert_service.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
@@ skipping 498 matching lines @@
   }
 
   if (err == OK) {
     // Async DB lookup found a valid cert.
     DVLOG(1) << "Cert store had valid cert for " << server_identifier;
     cert_store_hits_++;
     // ServerBoundCertServiceRequest::Post will do the histograms and stuff.
     HandleResult(OK, server_identifier, key, cert);
     return;
   }
-  // Async lookup did not find a valid cert. If no request asked to create one,
-  // return the error directly.
-  if (!j->second->CreateIfMissing()) {
+  // Async lookup failed. If lookup gave an error or failed to find a valid cert

[wtc, 2014/06/19 22:00:11]

What does "lookup gave an error" mean? It seems to be referring to the test "err
!= ERR_FILE_NOT_FOUND", but I don't understand why "err != ERR_FILE_NOT_FOUND"
means lookup gave an error. Is ERR_FILE_NOT_FOUND not considered an error? Or
does ERR_FILE_NOT_FOUND actually mean "cert not found"?

[davidben, 2014/06/19 22:36:38]

Oh yeah, I meant to explain this here. Yeah, ERR_FILE_NOT_FOUND actually means
"cert not found" here. This is to mirror the synchronous codepath:

https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/server_bou...

which handles ERR_FILE_NOT_FOUND as a signal to create a new key, but otherwise
returns the error.

DefaultServerBoundCertStore returns ERR_FILE_NOT_FOUND if the cert wasn't found:
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/default_se...

In fact, it doesn't look like DefaultServerBoundCertStore ever returns anything
other than OK and ERR_FILE_NOT_FOUND. The
DefaultServerBoundCertStore::PersistentStore interface has no way to signal a
load error. If we get a load error in the sqlite implementation, we just drop
the database and return nothing. So this change doesn't do anything outside of
test code.

Within test code, we can only interpose on ServerBoundCertStore and not
ServerBoundCertService. Without this change, it sees the fake error and just
generates a new one. This makes the (totally unused in real code)
ServerBoundCertStore error-handling match between the synchronous and
asynchronous codepaths so we can asynchronously fail at the
ServerBoundCertService level.

Note that although asynchronously failing in ServerBoundCertStore is not
possible in our real code, it IS possible to asynchronously fail in
ServerBoundCertService if GenerateCert fails for some reason. But there isn't a
convenient way to interpose there, so I just went with this one.

---

Anyway, rephrased that comment to be clearer.

+  // and no request asked to create one, return the error directly.
+  if (err != ERR_FILE_NOT_FOUND || !j->second->CreateIfMissing()) {
     HandleResult(err, server_identifier, key, cert);
     return;
   }
   // At least one request asked to create a cert => start generating a new one.
   workers_created_++;
   ServerBoundCertServiceWorker* worker = new ServerBoundCertServiceWorker(
       server_identifier,
       base::Bind(&ServerBoundCertService::GeneratedServerBoundCert,
                  weak_ptr_factory_.GetWeakPtr()));
   if (!worker->Start(task_runner_)) {
@@ skipping 135 matching lines @@
   }
 
   return err;
 }
 
 int ServerBoundCertService::cert_count() {
   return server_bound_cert_store_->GetCertCount();
 }
 
 }  // namespace net