Add backup URL support for SafeBrowsing data requests.

Add backup URL support for SafeBrowsing data requests.

If a request fails, an alternate URL is tried immediately. This is
only done if the configuration provides backup URLs to try. 

Three cases of request failure are supported:
  - Issues with DNS resolution, TCP connection, or SSL handshake when
  the client is believed to be connected to the internet.

  - Issues with invalid HTTP responses from the server.

  - Issues with general network connectivity from the client.

If the backup request fails as well, then the entire update is treated
as a failure and no further backups are attempted.
 
BUG=155753
TEST=unit_tests --gtest_filter=SafeBrowsingProtocolManagerTest.*


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179412

[mattm, 2013-01-12 02:24:49]

a few nits.

Change seems good but I didn't get to review the tests thoroughly.  Sending what
comments I have since I'll be out next week.

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:681: bool update_success =
success || request_type_ == CHUNK_REQUEST;
This means a failed CHUNK_REQUEST would be histogrammed as a
UPDATE_RESULT_SUCCESS?  I guess this is because it implies we successfully got
past the UPDATE_REQUEST stage, and that histogram is supposed to be only for
that part of the update and not the whole update process?  Maybe the histogram
could be named differently to make that more clear, or at least some comments
added.

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:693: request_type_ =
NO_REQUEST;
should we clear update_list_data_ here too?

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:70: void
ValidateBackupUpdateFetcherRequest(
maybe ValidateUpdateFetcherRequest should just call this function with the
correct prefix, since they look otherwise the same.

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_service.cc:62: // and reports safe
browsing hits and malware details.
Instead of duplicating the same comment for each one, maybe just say that it's
the backup url prefix used when the default url fails with error type foo

[cbentzel, 2013-01-14 11:33:35]

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:681: bool update_success =
success || request_type_ == CHUNK_REQUEST;
On 2013/01/12 02:24:49, mattm wrote:
> This means a failed CHUNK_REQUEST would be histogrammed as a
> UPDATE_RESULT_SUCCESS?  I guess this is because it implies we successfully got
> past the UPDATE_REQUEST stage, and that histogram is supposed to be only for
> that part of the update and not the whole update process?  Maybe the histogram
> could be named differently to make that more clear, or at least some comments
> added.

Yes - the histogram is only for the first update data request. We are not
consistent about terminology (for example: the actual request for list data is
called UPDATE_REQUEST and the request for chunk details is called CHUNK_REQUEST,
but the entire cycle of both is called UpdateStarted/UpdateFinished on the
delegate). I'll see if we can be consistent about terminology.

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:693: request_type_ =
NO_REQUEST;
On 2013/01/12 02:24:49, mattm wrote:
> should we clear update_list_data_ here too?

I was clearing it at the start of OnGetChunksComplete, but makes more sense to
do it here and DCHECK inside of OnGetChunksCompete. Fixed.

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:70: void
ValidateBackupUpdateFetcherRequest(
On 2013/01/12 02:24:49, mattm wrote:
> maybe ValidateUpdateFetcherRequest should just call this function with the
> correct prefix, since they look otherwise the same.

Good idea. Done.

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_service.cc:62: // and reports safe
browsing hits and malware details.
On 2013/01/12 02:24:49, mattm wrote:
> Instead of duplicating the same comment for each one, maybe just say that it's
> the backup url prefix used when the default url fails with error type foo

Done.

[cbentzel, 2013-01-14 11:39:42]

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/11784038/diff/28001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:681: bool update_success =
success || request_type_ == CHUNK_REQUEST;
On 2013/01/14 11:33:35, cbentzel wrote:
> On 2013/01/12 02:24:49, mattm wrote:
> > This means a failed CHUNK_REQUEST would be histogrammed as a
> > UPDATE_RESULT_SUCCESS?  I guess this is because it implies we successfully
got
> > past the UPDATE_REQUEST stage, and that histogram is supposed to be only for
> > that part of the update and not the whole update process?  Maybe the
histogram
> > could be named differently to make that more clear, or at least some
comments
> > added.
> 
> Yes - the histogram is only for the first update data request. We are not
> consistent about terminology (for example: the actual request for list data is
> called UPDATE_REQUEST and the request for chunk details is called
CHUNK_REQUEST,
> but the entire cycle of both is called UpdateStarted/UpdateFinished on the
> delegate). I'll see if we can be consistent about terminology.

Unfortunately, the SafeBrowsing Protocol spec does not seem to have specific
terminology for the request/response pair referred to as Update here. It lumps
both the request/response for the chunk urls and the actual retrieval of the
chunk data as "Data".

https://developers.google.com/safe-browsing/developers_guide_v2#HTTPRequestFo...

Perhaps we should call the request/response as "Chunk Specification" or
something other than update.

[noelutz, 2013-01-15 01:46:49]

Nice.  Just some nits.

thanks,
noe.

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:34: enum UpdateResult {
nit: add a comment which explains how the order of these enum values strongly
depends on the order of enum values in BackupUpdateReason?  I scratched my head
at first ;).

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:44: UPDATE_RESULT_BACKUP_START
= UPDATE_RESULT_BACKUP_CONNECT_FAIL,
Nice.  I didn't know you could use an enum value in the definition of an enum.

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:192: if (!request_.get())
I was trying to convince myself that there could never be two updates running
simultaneously.  This if statement doesn't seem sufficient since the request_
value will be set asynchronously once we've read the current state from the
database.

Maybe add the following condition? "&& request_type_ == NO_REQUEST".

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:286: UpdateFinished(false);
Did you want to retry the update request with a backup URL in that case as well?

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:631: request_.reset();
Should you issue a backup update in this case if the request_type_ is an
UPDATE_REQUEST?  That seems to indicate a network issue.

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_service.cc:73:
"https://alt3-safebrowsing.google.com/safebrowsing";
Just curious. In the CL description you mention that you want to handle DNS
resolution issues.  I was expecting one of these backup URLs to have an IP for
hostname to avoid DNS resolution altogether.  Am I misunderstanding something?

[cbentzel, 2013-01-15 12:00:08]

Thanks for the review.

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:34: enum UpdateResult {
On 2013/01/15 01:46:49, noelutz wrote:
> nit: add a comment which explains how the order of these enum values strongly
> depends on the order of enum values in BackupUpdateReason?  I scratched my
head
> at first ;).

It does right now so I'll comment. I considered a simple switch or static map to
make the transformation more straightforward.

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:192: if (!request_.get())
On 2013/01/15 01:46:49, noelutz wrote:
> I was trying to convince myself that there could never be two updates running
> simultaneously.  This if statement doesn't seem sufficient since the request_
> value will be set asynchronously once we've read the current state from the
> database.
> 
> Maybe add the following condition? "&& request_type_ == NO_REQUEST". 

That's a good safety suggestion, now that I set NO_REQUEST in UpdateFinished.
Done.

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:286: UpdateFinished(false);
On 2013/01/15 01:46:49, noelutz wrote:
> Did you want to retry the update request with a backup URL in that case as
well?

I'm not too sure about this case, but put in an http backup just in case.

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager.cc:631: request_.reset();
On 2013/01/15 01:46:49, noelutz wrote:
> Should you issue a backup update in this case if the request_type_ is an
> UPDATE_REQUEST?  That seems to indicate a network issue.

Yes, good catch.

[noelutz, 2013-01-15 18:04:07]

lgtm

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_service.cc:73:
"https://alt3-safebrowsing.google.com/safebrowsing";
On 2013/01/15 01:46:49, noelutz wrote:
> Just curious. In the CL description you mention that you want to handle DNS
> resolution issues.  I was expecting one of these backup URLs to have an IP for
> hostname to avoid DNS resolution altogether.  Am I misunderstanding something?

Still curious about that ;).

[cbentzel, 2013-01-22 20:04:30]

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/11784038/diff/40001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_service.cc:73:
"https://alt3-safebrowsing.google.com/safebrowsing";
On 2013/01/15 18:04:07, noelutz wrote:
> On 2013/01/15 01:46:49, noelutz wrote:
> > Just curious. In the CL description you mention that you want to handle DNS
> > resolution issues.  I was expecting one of these backup URLs to have an IP
for
> > hostname to avoid DNS resolution altogether.  Am I misunderstanding
something?
> 
> Still curious about that ;).

Sorry - it may be reasonable to do a raw IP only. One concern is possible load
on a single IP (not necessarily single IP), especially since we probably we may
not be able to use anycast here.

[mattm, 2013-01-22 23:09:05]

https://codereview.chromium.org/11784038/diff/48003/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11784038/diff/48003/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:569: // Respond to the
backup unsuccessfully.
Should respond successfully to ensure that it's actually the timeout that is
causing the backup request to fail?  Or does it even make sense to respond at
all, shouldn't the timeout handler be destroying the request?

[cbentzel, 2013-01-23 13:18:28]

https://codereview.chromium.org/11784038/diff/48003/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11784038/diff/48003/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:569: // Respond to the
backup unsuccessfully.
On 2013/01/22 23:09:05, mattm wrote:
> Should respond successfully to ensure that it's actually the timeout that is
> causing the backup request to fail?  Or does it even make sense to respond at
> all, shouldn't the timeout handler be destroying the request?

Great catch. The backup timeout logic in protocol_manager.cc was correct, but
the test was not actually exercising this. Fixed.

[cbentzel, 2013-01-23 14:22:46]

https://codereview.chromium.org/11784038/diff/63001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11784038/diff/63001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:566: // Two calls of
RunPendingTask are required here. The first run invokes the
I'm a bit worried that this is too subtle or may lead to a flaky test, and will
look for alternative approaches.

[cbentzel, 2013-01-23 20:13:23]

https://codereview.chromium.org/11784038/diff/63001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11784038/diff/63001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:566: // Two calls of
RunPendingTask are required here. The first run invokes the
On 2013/01/23 14:22:46, cbentzel wrote:
> I'm a bit worried that this is too subtle or may lead to a flaky test, and
will
> look for alternative approaches.

And it looks like it is flaky as evidenced by the tryjobs.

[cbentzel, 2013-01-28 17:10:09]

Latest patch fixes the backup timeout test. A little gnarly, but I could not
think of a better alternative.

[mattm, 2013-01-28 23:55:16]

lgtm.  w/ optional suggestion.

https://codereview.chromium.org/11784038/diff/72001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11784038/diff/72001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:575:
runner->RunPendingTasks();
Maybe clearer to write this as 
if (!pm->IsUpdateScheduled())
  runner->RunPendingTasks(); ?

[commit-bot: I haz the power, 2013-01-29 10:31:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/11784038/78002

[commit-bot: I haz the power, 2013-01-29 11:41:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-29 12:26:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/11784038/78002

[commit-bot: I haz the power, 2013-01-29 13:04:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-29 19:44:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/11784038/78002

[commit-bot: I haz the power, 2013-01-29 21:23:56]

Change committed as 179412