Combine ChannelIDService::RequestHandle and ChannelIDServiceRequest classes

net/ssl/channel_id_service.cc

 // Copyright 2014 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/channel_id_service.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
@@ skipping 90 matching lines @@
                              base::TimeTicks::Now() - start,
                              base::TimeDelta::FromMilliseconds(1),
                              base::TimeDelta::FromMinutes(5),
                              50);
   *error = OK;
   return result.Pass();
 }
 
 }  // namespace
 
-// Represents the output and result callback of a request.
-class ChannelIDServiceRequest {
- public:
-  ChannelIDServiceRequest(base::TimeTicks request_start,
-                          const CompletionCallback& callback,
-                          scoped_ptr<crypto::ECPrivateKey>* key)
-      : request_start_(request_start), callback_(callback), key_(key) {}
-
-  // Ensures that the result callback will never be made.
-  void Cancel() {
-    RecordGetChannelIDResult(ASYNC_CANCELLED);
-    callback_.Reset();
-  }
-
-  // Copies the contents of |key| to the caller's output argument and calls the
-  // callback.
-  void Post(int error, scoped_ptr<crypto::ECPrivateKey> key) {
-    switch (error) {
-      case OK: {
-        base::TimeDelta request_time = base::TimeTicks::Now() - request_start_;
-        UMA_HISTOGRAM_CUSTOM_TIMES("DomainBoundCerts.GetCertTimeAsync",
-                                   request_time,
-                                   base::TimeDelta::FromMilliseconds(1),
-                                   base::TimeDelta::FromMinutes(5),
-                                   50);
-        RecordGetChannelIDTime(request_time);
-        RecordGetChannelIDResult(ASYNC_SUCCESS);
-        break;
-      }
-      case ERR_KEY_GENERATION_FAILED:
-        RecordGetChannelIDResult(ASYNC_FAILURE_KEYGEN);
-        break;
-      case ERR_PRIVATE_KEY_EXPORT_FAILED:
-        RecordGetChannelIDResult(ASYNC_FAILURE_EXPORT_KEY);
-        break;
-      case ERR_INSUFFICIENT_RESOURCES:
-        RecordGetChannelIDResult(WORKER_FAILURE);
-        break;
-      default:
-        RecordGetChannelIDResult(ASYNC_FAILURE_UNKNOWN);
-        break;
-    }
-    if (!callback_.is_null()) {
-      if (key)
-        *key_ = key.Pass();
-      callback_.Run(error);
-    }
-    delete this;
-  }
-
-  bool canceled() const { return callback_.is_null(); }
-
- private:
-  base::TimeTicks request_start_;
-  CompletionCallback callback_;
-  scoped_ptr<crypto::ECPrivateKey>* key_;
-};
-
 // ChannelIDServiceWorker runs on a worker thread and takes care of the
 // blocking process of performing key generation. Will take care of deleting
 // itself once Start() is called.
 class ChannelIDServiceWorker {
  public:
   typedef base::Callback<void(
       const std::string&,
       int,
       scoped_ptr<ChannelIDStore::ChannelID>)> WorkerDoneCallback;
 
@@ skipping 46 matching lines @@
 
 // A ChannelIDServiceJob is a one-to-one counterpart of an
 // ChannelIDServiceWorker. It lives only on the ChannelIDService's
 // origin message loop.
 class ChannelIDServiceJob {
  public:
   ChannelIDServiceJob(bool create_if_missing)
       : create_if_missing_(create_if_missing) {
   }
 
-  ~ChannelIDServiceJob() {

[mattm, 2015/06/08 23:59:30]

Maybe keep destructor and DCHECK that requests_ is empty? (I think that a job
should only get deleted if it has finished, which clears requests_, or if all
requests have been cancelled first, which should remove them all.)

[nharper, 2015/06/09 19:19:32]

Yes, that seems reasonable. I had to re-read the code to see what is going on,
but I agree with the analysis that requests_ should be empty when the d'tor is
called: ChannelIDServiceJobs are only destroyed in
ChannelIDService::HandleResult, which first calls
ChannelIDServiceJob::HandleResult which clears requests_.

-    if (!requests_.empty())
-      DeleteAllCanceled();
-  }
-
-  void AddRequest(ChannelIDServiceRequest* request,
+  void AddRequest(ChannelIDService::Request* request,
                   bool create_if_missing = false) {
     create_if_missing_ |= create_if_missing;
     requests_.push_back(request);
   }
 
   void HandleResult(int error, scoped_ptr<crypto::ECPrivateKey> key) {
     PostAll(error, key.Pass());
   }
 
   bool CreateIfMissing() const { return create_if_missing_; }
 
+  void CancelRequest(ChannelIDService::Request* req) {
+    for (std::vector<ChannelIDService::Request*>::iterator i =

[mattm, 2015/06/08 23:59:30]

maybe use std::find?

[nharper, 2015/06/09 19:19:32]

Done.

+             requests_.begin();
+         i != requests_.end(); i++) {
+      if (*i == req) {
+        requests_.erase(i);
+        return;
+      }
+    }
+  }
+
  private:
   void PostAll(int error, scoped_ptr<crypto::ECPrivateKey> key) {
-    std::vector<ChannelIDServiceRequest*> requests;
+    std::vector<ChannelIDService::Request*> requests;
     requests_.swap(requests);
 
-    for (std::vector<ChannelIDServiceRequest*>::iterator
-         i = requests.begin(); i != requests.end(); i++) {
+    for (std::vector<ChannelIDService::Request*>::iterator i = requests.begin();
+         i != requests.end(); i++) {
       scoped_ptr<crypto::ECPrivateKey> key_copy;
       if (key)
         key_copy.reset(key->Copy());
       (*i)->Post(error, key_copy.Pass());
-      // Post() causes the ChannelIDServiceRequest to delete itself.
     }
   }
 
-  void DeleteAllCanceled() {
-    for (std::vector<ChannelIDServiceRequest*>::iterator
-         i = requests_.begin(); i != requests_.end(); i++) {
-      if ((*i)->canceled()) {
-        delete *i;
-      } else {
-        LOG(DFATAL) << "ChannelIDServiceRequest leaked!";
-      }
-    }
-  }
-
-  std::vector<ChannelIDServiceRequest*> requests_;
+  std::vector<ChannelIDService::Request*> requests_;
   bool create_if_missing_;
 };
 
 // static
 const char ChannelIDService::kEPKIPassword[] = "";
 
-ChannelIDService::RequestHandle::RequestHandle()
-    : service_(NULL),
-      request_(NULL) {}
+ChannelIDService::Request::Request() : service_(NULL) {
+}
 
-ChannelIDService::RequestHandle::~RequestHandle() {
+ChannelIDService::Request::~Request() {
   Cancel();
 }
 
-void ChannelIDService::RequestHandle::Cancel() {
-  if (request_) {
-    service_->CancelRequest(request_);
-    request_ = NULL;
+void ChannelIDService::Request::Cancel() {
+  if (service_) {
+    RecordGetChannelIDResult(ASYNC_CANCELLED);
     callback_.Reset();
+    service_->CancelRequest(server_identifier_, this);
+
+    service_ = NULL;
   }
 }
 
-void ChannelIDService::RequestHandle::RequestStarted(
+void ChannelIDService::Request::RequestStarted(
     ChannelIDService* service,
-    ChannelIDServiceRequest* request,
-    const CompletionCallback& callback) {
-  DCHECK(request_ == NULL);
+    base::TimeTicks request_start,
+    const CompletionCallback& callback,
+    scoped_ptr<crypto::ECPrivateKey>* key,
+    const std::string& server_identifier) {

[mattm, 2015/06/08 23:59:30]

Could just pass in a pointer to the Job and avoid having to do the
server_identifier search when cancelling. (Assuming I'm correct that a job never
gets deleted while it still has non-cancelled requests.)

[nharper, 2015/06/09 19:19:32]

I believe the assumption that a job never gets deleted while it still has
non-cancelled requests is correct. The Request object outlives the actual
request (and the Job object that has the request), which made me concerned that
Request::Cancel() would crash trying to access the Job when called in Request's
destructor. However, the appropriate logic is in place in Request::Post to mark
the Request as no longer active, so this isn't actually a problem.

I switched it to using a pointer to the Job so we can skip searching by
server_identifier.

+  DCHECK(service_ == NULL);
   service_ = service;
-  request_ = request;
+  request_start_ = request_start;
   callback_ = callback;
+  key_ = key;
+  server_identifier_ = server_identifier;
 }
 
-void ChannelIDService::RequestHandle::OnRequestComplete(int result) {
-  request_ = NULL;
-  // Running the callback might delete |this|, so we can't touch any of our
-  // members afterwards. Reset callback_ first.
-  base::ResetAndReturn(&callback_).Run(result);
+void ChannelIDService::Request::Post(int error,
+                                     scoped_ptr<crypto::ECPrivateKey> key) {
+  switch (error) {
+    case OK: {
+      base::TimeDelta request_time = base::TimeTicks::Now() - request_start_;
+      UMA_HISTOGRAM_CUSTOM_TIMES("DomainBoundCerts.GetCertTimeAsync",
+                                 request_time,
+                                 base::TimeDelta::FromMilliseconds(1),
+                                 base::TimeDelta::FromMinutes(5), 50);
+      RecordGetChannelIDTime(request_time);
+      RecordGetChannelIDResult(ASYNC_SUCCESS);
+      break;
+    }
+    case ERR_KEY_GENERATION_FAILED:
+      RecordGetChannelIDResult(ASYNC_FAILURE_KEYGEN);
+      break;
+    case ERR_PRIVATE_KEY_EXPORT_FAILED:
+      RecordGetChannelIDResult(ASYNC_FAILURE_EXPORT_KEY);
+      break;
+    case ERR_INSUFFICIENT_RESOURCES:
+      RecordGetChannelIDResult(WORKER_FAILURE);
+      break;
+    default:
+      RecordGetChannelIDResult(ASYNC_FAILURE_UNKNOWN);
+      break;
+  }
+  service_ = NULL;
+  if (!callback_.is_null()) {

[mattm, 2015/06/08 23:59:30]

Should we change this to a DCHECK? It shouldn't be null if Post is getting
called, right?

[nharper, 2015/06/09 19:19:32]

No, this shouldn't be changed to a DCHECK. It's valid for a null callback to be
set (e.g. only caring for synchronous responses), and there are tests that fail
if a DCHECK is added.

[mattm, 2015/06/09 19:32:24]

Hm, GetChannelID and GetOrCreateChannelID both have an "if (callback.is_null()
|| ...)" test that errors out immediately. If it does fail with that dcheck it
must be for a different reason.

[nharper, 2015/06/09 21:30:16]

The DCHECK failed because I got the condition backwards.

Since I'm adding a DCHECK, I'm also removing the if statement, as the style
guide says not to handle DCHECK failures.

+    if (key)
+      *key_ = key.Pass();
+    // Running the callback might delete |this| (e.g. the callback cleans up
+    // resources created for the request), so we can't touch any of our
+    // members afterwards. Reset callback_ first.
+    base::ResetAndReturn(&callback_).Run(error);
+  }
 }
 
 ChannelIDService::ChannelIDService(
     ChannelIDStore* channel_id_store,
     const scoped_refptr<base::TaskRunner>& task_runner)
     : channel_id_store_(channel_id_store),
       task_runner_(task_runner),
       requests_(0),
       key_store_hits_(0),
       inflight_joins_(0),
@@ skipping 12 matching lines @@
           host, registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
   if (domain.empty())
     return host;
   return domain;
 }
 
 int ChannelIDService::GetOrCreateChannelID(
     const std::string& host,
     scoped_ptr<crypto::ECPrivateKey>* key,
     const CompletionCallback& callback,
-    RequestHandle* out_req) {
+    Request* out_req) {
   DVLOG(1) << __FUNCTION__ << " " << host;
   DCHECK(CalledOnValidThread());
   base::TimeTicks request_start = base::TimeTicks::Now();
 
   if (callback.is_null() || !key || host.empty()) {
     RecordGetChannelIDResult(INVALID_ARGUMENT);
     return ERR_INVALID_ARGUMENT;
   }
 
   std::string domain = GetDomainForHost(host);
@@ skipping 23 matching lines @@
     if (!worker->Start(task_runner_)) {
       // TODO(rkn): Log to the NetLog.
       LOG(ERROR) << "ChannelIDServiceWorker couldn't be started.";
       RecordGetChannelIDResult(WORKER_FAILURE);
       return ERR_INSUFFICIENT_RESOURCES;
     }
     // We are waiting for key generation.  Create a job & request to track it.
     ChannelIDServiceJob* job = new ChannelIDServiceJob(create_if_missing);
     inflight_[domain] = job;
 
-    ChannelIDServiceRequest* request = new ChannelIDServiceRequest(
-        request_start, base::Bind(&RequestHandle::OnRequestComplete,
-                                  base::Unretained(out_req)),
-        key);
-    job->AddRequest(request);
-    out_req->RequestStarted(this, request, callback);
+    job->AddRequest(out_req);
+    out_req->RequestStarted(this, request_start, callback, key, domain);
     return ERR_IO_PENDING;
   }
 
   return err;
 }
 
 int ChannelIDService::GetChannelID(const std::string& host,
                                    scoped_ptr<crypto::ECPrivateKey>* key,
                                    const CompletionCallback& callback,
-                                   RequestHandle* out_req) {
+                                   Request* out_req) {
   DVLOG(1) << __FUNCTION__ << " " << host;
   DCHECK(CalledOnValidThread());
   base::TimeTicks request_start = base::TimeTicks::Now();
 
   if (callback.is_null() || !key || host.empty()) {
     RecordGetChannelIDResult(INVALID_ARGUMENT);
     return ERR_INVALID_ARGUMENT;
   }
 
   std::string domain = GetDomainForHost(host);
@@ skipping 24 matching lines @@
   std::map<std::string, ChannelIDServiceJob*>::iterator j;
   j = inflight_.find(server_identifier);
   if (j == inflight_.end()) {
     NOTREACHED();
     return;
   }
 
   if (err == OK) {
     // Async DB lookup found a valid channel ID.
     key_store_hits_++;
-    // ChannelIDServiceRequest::Post will do the histograms and stuff.
+    // ChannelIDService::Request::Post will do the histograms and stuff.
     HandleResult(OK, server_identifier, key.Pass());
     return;
   }
   // Async lookup failed or the channel ID was missing. Return the error
   // directly, unless the channel ID was missing and a request asked to create
   // one.
   if (err != ERR_FILE_NOT_FOUND || !j->second->CreateIfMissing()) {
     HandleResult(err, server_identifier, key.Pass());
     return;
   }
   // At least one request asked to create a channel ID => start generating a new
   // one.
   workers_created_++;
   ChannelIDServiceWorker* worker = new ChannelIDServiceWorker(
       server_identifier,
       base::Bind(&ChannelIDService::GeneratedChannelID,
                  weak_ptr_factory_.GetWeakPtr()));
   if (!worker->Start(task_runner_)) {
     // TODO(rkn): Log to the NetLog.
     LOG(ERROR) << "ChannelIDServiceWorker couldn't be started.";
     HandleResult(ERR_INSUFFICIENT_RESOURCES, server_identifier, nullptr);
   }
 }
 
 ChannelIDStore* ChannelIDService::GetChannelIDStore() {
   return channel_id_store_.get();
 }
 
-void ChannelIDService::CancelRequest(ChannelIDServiceRequest* req) {
-  DCHECK(CalledOnValidThread());
-  req->Cancel();
-}
-
 void ChannelIDService::GeneratedChannelID(
     const std::string& server_identifier,
     int error,
     scoped_ptr<ChannelIDStore::ChannelID> channel_id) {
   DCHECK(CalledOnValidThread());
 
   scoped_ptr<crypto::ECPrivateKey> key;
   if (error == OK) {
     key.reset(channel_id->key()->Copy());
     channel_id_store_->SetChannelID(channel_id.Pass());
@@ skipping 12 matching lines @@
     NOTREACHED();
     return;
   }
   ChannelIDServiceJob* job = j->second;
   inflight_.erase(j);
 
   job->HandleResult(error, key.Pass());
   delete job;
 }
 
+void ChannelIDService::CancelRequest(const std::string& server_identifier,
+                                     Request* req) {
+  DCHECK(CalledOnValidThread());
+
+  std::map<std::string, ChannelIDServiceJob*>::iterator j;
+  j = inflight_.find(server_identifier);
+  if (j == inflight_.end()) {
+    NOTREACHED();
+    return;
+  }
+  ChannelIDServiceJob* job = j->second;
+
+  job->CancelRequest(req);
+}
+
 bool ChannelIDService::JoinToInFlightRequest(
     const base::TimeTicks& request_start,
     const std::string& domain,
     scoped_ptr<crypto::ECPrivateKey>* key,
     bool create_if_missing,
     const CompletionCallback& callback,
-    RequestHandle* out_req) {
+    Request* out_req) {
   ChannelIDServiceJob* job = NULL;
   std::map<std::string, ChannelIDServiceJob*>::const_iterator j =
       inflight_.find(domain);
   if (j != inflight_.end()) {
     // A request for the same domain is in flight already. We'll attach our
     // callback, but we'll also mark it as requiring a channel ID if one's
     // mising.
     job = j->second;
     inflight_joins_++;
 
-    ChannelIDServiceRequest* request = new ChannelIDServiceRequest(
-        request_start, base::Bind(&RequestHandle::OnRequestComplete,
-                                  base::Unretained(out_req)),
-        key);
-    job->AddRequest(request, create_if_missing);
-    out_req->RequestStarted(this, request, callback);
+    job->AddRequest(out_req, create_if_missing);
+    out_req->RequestStarted(this, request_start, callback, key, domain);
     return true;
   }
   return false;
 }
 
 int ChannelIDService::LookupChannelID(const base::TimeTicks& request_start,
                                       const std::string& domain,
                                       scoped_ptr<crypto::ECPrivateKey>* key,
                                       bool create_if_missing,
                                       const CompletionCallback& callback,
-                                      RequestHandle* out_req) {
+                                      Request* out_req) {
   // Check if a channel ID key already exists for this domain.
   int err = channel_id_store_->GetChannelID(
       domain, key, base::Bind(&ChannelIDService::GotChannelID,
                               weak_ptr_factory_.GetWeakPtr()));
 
   if (err == OK) {
     // Sync lookup found a valid channel ID.
     DVLOG(1) << "Channel ID store had valid key for " << domain;
     key_store_hits_++;
     RecordGetChannelIDResult(SYNC_SUCCESS);
     base::TimeDelta request_time = base::TimeTicks::Now() - request_start;
     UMA_HISTOGRAM_TIMES("DomainBoundCerts.GetCertTimeSync", request_time);
     RecordGetChannelIDTime(request_time);
     return OK;
   }
 
   if (err == ERR_IO_PENDING) {
     // We are waiting for async DB lookup.  Create a job & request to track it.
     ChannelIDServiceJob* job = new ChannelIDServiceJob(create_if_missing);
     inflight_[domain] = job;
 
-    ChannelIDServiceRequest* request = new ChannelIDServiceRequest(
-        request_start, base::Bind(&RequestHandle::OnRequestComplete,
-                                  base::Unretained(out_req)),
-        key);
-    job->AddRequest(request);
-    out_req->RequestStarted(this, request, callback);
+    job->AddRequest(out_req);
+    out_req->RequestStarted(this, request_start, callback, key, domain);
     return ERR_IO_PENDING;
   }
 
   return err;
 }
 
 int ChannelIDService::channel_id_count() {
   return channel_id_store_->GetChannelIDCount();
 }
 
 }  // namespace net