Domain Reliability: More security review.

components/domain_reliability/context.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 "components/domain_reliability/context.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/values.h"
 #include "components/domain_reliability/dispatcher.h"
 #include "net/base/net_errors.h"
 #include "net/url_request/url_request_context_getter.h"
 
 using base::DictionaryValue;
 using base::ListValue;
 using base::Value;
 
 namespace domain_reliability {
 
 namespace {
+
 const char* kReporter = "chrome";

[Ryan Sleevi, 2014/04/25 01:00:02]

layering violation: This should not be hardcoded in //components.

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

 typedef std::deque<DomainReliabilityBeacon> BeaconDeque;
 typedef BeaconDeque::iterator BeaconIterator;
 typedef BeaconDeque::const_iterator BeaconConstIterator;
+
 }  // namespace
 
 const int DomainReliabilityContext::kMaxQueuedBeacons = 150;
 
 DomainReliabilityContext::DomainReliabilityContext(
     MockableTime* time,
     const DomainReliabilityScheduler::Params& scheduler_params,
     DomainReliabilityDispatcher* dispatcher,
     DomainReliabilityUploader* uploader,
     scoped_ptr<const DomainReliabilityConfig> config)
     : config_(config.Pass()),
       time_(time),
       scheduler_(time, config_->collectors.size(), scheduler_params,
                  base::Bind(&DomainReliabilityContext::ScheduleUpload,
                             base::Unretained(this))),
       dispatcher_(dispatcher),
       uploader_(uploader),
       beacon_count_(0),
       weak_factory_(this) {
   InitializeResourceStates();
 }
 
 DomainReliabilityContext::~DomainReliabilityContext() {}
 
 void DomainReliabilityContext::AddBeacon(
     const DomainReliabilityBeacon& beacon,
     const GURL& url) {
   int index = config_->GetResourceIndexForUrl(url);
   if (index < 0)
     return;

[Ryan Sleevi, 2014/04/25 01:00:02]

size_t & npos (aka ((size_t)~0)) for a safer approach here

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

   DCHECK_GT(states_.size(), static_cast<size_t>(index));
 
   ResourceState* state = states_[index];
   bool success = beacon.http_response_code >= 200 &&
                  beacon.http_response_code < 400;
   if (success)
     ++state->successful_requests;
   else
     ++state->failed_requests;
 
   VLOG(1) << "Received Beacon: "
           << state->config->name << " "
           << beacon.status << " "
           << beacon.chrome_error << " "
           << beacon.http_response_code << " "
           << beacon.server_ip << " "
           << beacon.elapsed.InMilliseconds() << "ms";

[Ryan Sleevi, 2014/04/25 01:00:02]

every VLOG costs space in release. Make sure you're making the right trade-offs
here, and perhaps take a bug to clean it up after ship & stablization.

With our frequent canary releases, it's often easier to add logging in on a
case-by-case basis than leave it there "for when you need it"

 
   bool reported = false;
   bool evicted = false;
   if (state->config->DecideIfShouldReportRequest(success)) {
     state->beacons.push_back(beacon);
     ++beacon_count_;
     if (beacon_count_ > kMaxQueuedBeacons) {
       RemoveOldestBeacon();
       evicted = true;
     }
     scheduler_.OnBeaconAdded();
     reported = true;
     UMA_HISTOGRAM_SPARSE_SLOWLY("DomainReliability.ReportedBeaconError",
                                 -beacon.chrome_error);
     // TODO(ttuttle): Histogram HTTP response code?
   }
 
   UMA_HISTOGRAM_BOOLEAN("DomainReliability.BeaconReported", reported);
   UMA_HISTOGRAM_BOOLEAN("DomainReliability.AddBeaconDidEvict", evicted);
 }
 
 void DomainReliabilityContext::GetQueuedDataForTesting(
     int resource_index,
     std::vector<DomainReliabilityBeacon>* beacons_out,
     int* successful_requests_out,
     int* failed_requests_out) const {
   DCHECK_LE(0, resource_index);
   DCHECK_GT(static_cast<int>(states_.size()), resource_index);

[Ryan Sleevi, 2014/04/25 01:00:02]

-_-

static_cast<int> of a size_t? Aww hell naw...

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

   const ResourceState& state = *states_[resource_index];
   if (beacons_out) {
     beacons_out->resize(state.beacons.size());
     std::copy(state.beacons.begin(), state.beacons.end(), beacons_out->begin());

[Ryan Sleevi, 2014/04/25 01:00:02]

beacons_out->assign(state.becons.begin(), state.becons.end()) also works.
Different tradeoffs, but probably fine for testing.

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

   }
   if (successful_requests_out)
     *successful_requests_out = state.successful_requests;
   if (failed_requests_out)
     *failed_requests_out = state.failed_requests;
 }
 
 DomainReliabilityContext::ResourceState::ResourceState(
     DomainReliabilityContext* context,
     const DomainReliabilityConfig::Resource* config)
@@ skipping 153 matching lines @@
 
   min_resource->RemoveOldestBeacon();
   --beacon_count_;
   // If that just removed a beacon counted in uploading_beacon_count_, decrement
   // that.
   if (uploading_beacon_count_ > 0)
     --uploading_beacon_count_;
 }
 
 }  // namespace domain_reliability