Handle timeout for update requests.

components/safe_browsing_db/v4_update_protocol_manager.cc

Index: components/safe_browsing_db/v4_update_protocol_manager.cc
diff --git a/components/safe_browsing_db/v4_update_protocol_manager.cc b/components/safe_browsing_db/v4_update_protocol_manager.cc
index ae43b2e3b404fcbc3fd97af82a2cb15a0b1f9dc6..1098f7f09c0a0873be2acb8754dc36f7c5ed8d85 100644
--- a/components/safe_browsing_db/v4_update_protocol_manager.cc
+++ b/components/safe_browsing_db/v4_update_protocol_manager.cc
@@ -70,6 +70,9 @@ 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 {
@@ -250,7 +253,6 @@ bool V4UpdateProtocolManager::ParseUpdateResponse(
         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()) {
@@ -295,7 +297,16 @@ void V4UpdateProtocolManager::IssueUpdateRequest() {
   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 ----------------------------------
@@ -305,6 +316,8 @@ void V4UpdateProtocolManager::OnURLFetchComplete(
     const net::URLFetcher* source) {
   DCHECK(CalledOnValidThread());
 
+  timeout_timer_.Stop();
+
   int response_code = source->GetResponseCode();
   net::URLRequestStatus status = source->GetStatus();
   V4ProtocolManagerUtil::RecordHttpResponseOrErrorCode(