Fix SSLClientSocketOpenSSL error-handling for Channel ID.

net/ssl/server_bound_cert_service.cc

Index: net/ssl/server_bound_cert_service.cc
diff --git a/net/ssl/server_bound_cert_service.cc b/net/ssl/server_bound_cert_service.cc
index 61d610b5bb625378973371b49fca4e261f60720c..6c4573bd55371acf0fabffa1e6c8575379063a61 100644
--- a/net/ssl/server_bound_cert_service.cc
+++ b/net/ssl/server_bound_cert_service.cc
@@ -516,9 +516,9 @@ void ServerBoundCertService::GotServerBoundCert(
     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;
   }