Handle timeout for update requests.

components/safe_browsing_db/v4_update_protocol_manager.cc

 // Copyright 2016 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/safe_browsing_db/v4_update_protocol_manager.h"
 
 #include <utility>
 
 #include "base/base64url.h"
 #include "base/macros.h"
@@ skipping 52 matching lines @@
 }  // namespace
 
 namespace safe_browsing {
 
 // Minimum time, in seconds, from start up before we must issue an update query.
 static const int kV4TimerStartIntervalSecMin = 60;
 
 // Maximum time, in seconds, from start up before we must issue an update query.
 static const int kV4TimerStartIntervalSecMax = 300;
 
+// Maximum time, in seconds, to wait for a response to an update request.
+static const int kV4TimerUpdateWaitSecMax = 30;
+
 // The default V4UpdateProtocolManagerFactory.
 class V4UpdateProtocolManagerFactoryImpl
     : public V4UpdateProtocolManagerFactory {
  public:
   V4UpdateProtocolManagerFactoryImpl() {}
   ~V4UpdateProtocolManagerFactoryImpl() override {}
   std::unique_ptr<V4UpdateProtocolManager> CreateProtocolManager(
       net::URLRequestContextGetter* request_context_getter,
       const V4ProtocolConfig& config,
       V4UpdateCallback callback) override {
@@ skipping 160 matching lines @@
         response.minimum_wait_duration().seconds();
 
     // Do not let the next_update_interval_ to be too low.
     if (minimum_wait_duration_seconds < kV4TimerStartIntervalSecMin) {
       minimum_wait_duration_seconds = kV4TimerStartIntervalSecMin;
     }
     next_update_interval_ =
         base::TimeDelta::FromSeconds(minimum_wait_duration_seconds);
   }
 
-  // TODO(vakh): Do something useful with this response.
   for (ListUpdateResponse& list_update_response :
        *response.mutable_list_update_responses()) {
     if (!list_update_response.has_platform_type()) {
       RecordParseUpdateResult(NO_PLATFORM_TYPE_ERROR);
     } else if (!list_update_response.has_threat_entry_type()) {
       RecordParseUpdateResult(NO_THREAT_ENTRY_TYPE_ERROR);
     } else if (!list_update_response.has_threat_type()) {
       RecordParseUpdateResult(NO_THREAT_TYPE_ERROR);
     } else if (!list_update_response.has_new_client_state()) {
       RecordParseUpdateResult(NO_STATE_ERROR);
@@ skipping 24 matching lines @@
       url_fetcher_id_++, update_url, net::URLFetcher::GET, this);
   fetcher->SetExtraRequestHeaders(headers.ToString());
   data_use_measurement::DataUseUserData::AttachToFetcher(
       fetcher.get(), data_use_measurement::DataUseUserData::SAFE_BROWSING);
 
   request_.reset(fetcher.release());
 
   request_->SetLoadFlags(net::LOAD_DISABLE_CACHE);
   request_->SetRequestContext(request_context_getter_.get());
   request_->Start();
-  // TODO(vakh): Handle request timeout.
+
+  // Begin the update request timeout.
+  timeout_timer_.Start(FROM_HERE,
+                       TimeDelta::FromSeconds(kV4TimerUpdateWaitSecMax), this,
+                       &V4UpdateProtocolManager::HandleTimeout);
+}
+
+void V4UpdateProtocolManager::HandleTimeout() {
+  request_.reset();

[Nathan Parker, 2016/11/02 21:56:57]

This should log some UMA value to indicate the request failed.

[vakh (use Gerrit instead), 2016/11/02 22:37:22]

I thought about it but I couldn't find a good way to log the occurence of an
event. Perhaps a UMA_HISTOGRAM_ENUMERATION that logs SUCCESS/FAILURE? Is there a
better way to log this?

[Nathan Parker, 2016/11/05 00:32:21]

Yes, you can use a UMA_HISTOGRAM just as a counter.  You could compare that to
the one that logs the time of responses, or success/error statuses, to see what
fraction of requests are timing out.

+  IssueUpdateRequest();

[Nathan Parker, 2016/11/02 21:56:57]

This will keep trying every 30 seconds. Is that what we want?  Maybe it should
have a fixed number of total requests (3?) and then give up and wait  for a full
update cycle (30 min).  t 

[Scott Hess - ex-Googler, 2016/11/02 22:05:08]

Does the spec address this?  I see language about how to handle errors, but not
about timeouts.

If the requests are reaching the Internet, then we definitely shouldn't be
stacking them up like this because an unresponsive server is definitely bad!  If
the requests are _not_ reaching the Internet, then we shouldn't be stacking them
up because we're going to repeated power up the CPU to process our requests and
the radio to attempt to send them.

[vakh (use Gerrit instead), 2016/11/02 22:37:22]

Sure, that's possible. But what happens after that? We can't pause update
requests.
One way to do this is to perform backoff (different from the backoff specified
by the protocol) and cap the backoff: 30 sec, 1 min, 2 min, 4 min, 4 min, 4
min...
Not sure what you mean here. This error is happening when we try to fetch the
next update (typically) 30 mins after the last one.
Are you saying that we should just skip an update entirely? That's worth
considering but it means that one failure can lead to somewhat stale DB.

[vakh (use Gerrit instead), 2016/11/02 22:37:22]

No, I don't see it: https://developers.google.com/safe-browsing/v4/update-api
So, just drop the request on timeout and schedule the next update at the last
known frequency?
This would be similar to Nathan's proposal. I am neutral to that idea.

Note that the request can also get dropped because of an intermittent connection
failure and I am sure there are a variety of other reasons.

I'll also check with the SafeBrowsing team if they have any opinion about it.

[vakh (use Gerrit instead), 2016/11/08 17:51:40]

Changed the code to schedule a normal update after timeout.
Please note that the first request is scheduled between 1 and 5 minutes after
startup so if the first update request after startup fails, it'll still be
retried at the same random interval (between 1 and 5 minutes). I think that's
reasonable because the DB may be really out of date at startup.

 }
 
 // net::URLFetcherDelegate implementation ----------------------------------
 
 // SafeBrowsing request responses are handled here.
 void V4UpdateProtocolManager::OnURLFetchComplete(
     const net::URLFetcher* source) {
   DCHECK(CalledOnValidThread());
 
+  timeout_timer_.Stop();
+
   int response_code = source->GetResponseCode();
   net::URLRequestStatus status = source->GetStatus();
   V4ProtocolManagerUtil::RecordHttpResponseOrErrorCode(
       "SafeBrowsing.V4UpdateHttpResponseOrErrorCode", status, response_code);
 
   last_response_time_ = Time::Now();
 
   std::unique_ptr<ParsedServerResponse> parsed_server_response(
       new ParsedServerResponse);
   if (status.is_success() && response_code == net::HTTP_OK) {
@@ skipping 34 matching lines @@
 
 void V4UpdateProtocolManager::GetUpdateUrlAndHeaders(
     const std::string& req_base64,
     GURL* gurl,
     net::HttpRequestHeaders* headers) const {
   V4ProtocolManagerUtil::GetRequestUrlAndHeaders(
       req_base64, "threatListUpdates:fetch", config_, gurl, headers);
 }
 
 }  // namespace safe_browsing