Add tests for redirect responses from SafeBrowsingProtocolManager.

Add tests for redirect responses from SafeBrowsingProtocolManager.

This also fixes two corner-case bugs discovered along the way:

 a) If the final redirect response is empty, UpdateFinished() is never called on SafeBrowsingService.

 b) If the last redirect response takes longer to persist the chunks to the database than the interval to the next update, the next update will start before the previous one completed.

BUG=158246, 161550, 161551
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174269

[mattm, 2012-11-16 21:48:46]

http://codereview.chromium.org/11419041/diff/6001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/11419041/diff/6001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:289: if
(chunk_request_urls_.empty() && !chunk_pending_to_write_ &&
so if we hit this case, does something else schedule the next update once the
chunk is written?  Is there a test of this case?

[cbentzel, 2012-11-16 21:52:35]

http://codereview.chromium.org/11419041/diff/6001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/11419041/diff/6001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:289: if
(chunk_request_urls_.empty() && !chunk_pending_to_write_ &&
On 2012/11/16 21:48:46, mattm wrote:
> so if we hit this case, does something else schedule the next update once the
> chunk is written?  Is there a test of this case?

This is handled in OnAddChunksComplete.

The test that I added - SafeBrowsingProtocolManagerTest.ChunksRedirectResponse 
and related ones - test this case.

[cbentzel, 2012-11-16 21:53:53]

Ah - you are right, and fantastic catch!

This is not checked in the unit tests, and is not handled correctly.

I will add checks for this.


On Fri, Nov 16, 2012 at 4:52 PM, <cbentzel@chromium.org> wrote:

>
> http://codereview.chromium.**org/11419041/diff/6001/chrome/**
>
browser/safe_browsing/**protocol_manager.cc<http://codereview.chromium.org/11419041/diff/6001/chrome/browser/safe_browsing/protocol_manager.cc>
> File chrome/browser/safe_browsing/**protocol_manager.cc (right):
>
> http://codereview.chromium.**org/11419041/diff/6001/chrome/**
>
browser/safe_browsing/**protocol_manager.cc#newcode289<http://codereview.chromium.org/11419041/diff/6001/chrome/browser/safe_browsing/protocol_manager.cc#newcode289>
> chrome/browser/safe_browsing/**protocol_manager.cc:289: if
> (chunk_request_urls_.empty() && !chunk_pending_to_write_ &&
> On 2012/11/16 21:48:46, mattm wrote:
>
>> so if we hit this case, does something else schedule the next update
>>
> once the
>
>> chunk is written?  Is there a test of this case?
>>
>
> This is handled in OnAddChunksComplete.
>
> The test that I added -
> SafeBrowsingProtocolManagerTes**t.ChunksRedirectResponse  and related ones
> - test this case.
>
>
http://codereview.chromium.**org/11419041/<http://codereview.chromium.org/114...
>

[cbentzel, 2012-11-30 11:24:23]

OK, finally got back to this.

I fixed the restart logic and added checks for this in the unit test.

One thing I did do was centralize the logic for restarting another update inside
of UpdateFinished() itself, to prevent future issues. If you prefer, I could
also localize the change in this CL to just the specific bug you pointed out.

[mattm, 2012-12-05 23:58:10]

Still reviewing, but the diff for the unittest gets mangled pretty badly due to
removing that one test.  Maybe split that into a preceding CL.

https://codereview.chromium.org/11419041/diff/3009/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/protocol_manager.h (right):

https://codereview.chromium.org/11419041/diff/3009/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/protocol_manager.h:293: // update_timeout_timer_ is
used to prevent update requests from taking too
Comment doesn't match variable name.

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:447: 
extra line

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:448:
TEST_F(SafeBrowsingProtocolManagerTest, UpdateResponseBadyBody) {
s/Bady/Bad/

[mattm, 2012-12-06 00:51:40]

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:355:
EXPECT_TRUE(pm->IsUpdateScheduled());
Could inserting some EXPECT_FALSE(pm->IsUpdateScheduled()); in strategic places
also be useful?

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:647: // The update
response is successful, and has a reset command.
s/reset/redirect/
(in following tests too)

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:658:
ASSERT_TRUE(chunk_url_fetcher != NULL);
Should test the fetched URL value as well, at least in one of these tests.

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:807:
net::TestURLFetcher* first_chunk_url_fetcher =
should probably check the URLs on these requests too..

[cbentzel, 2012-12-17 11:24:10]

Sorry for the slow response. Merged the CL against the previous one which
removed the test so the diff in protocol_manager_unittest.cc is resolved. Also
cleaned up some additional comments along the way.

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:355:
EXPECT_TRUE(pm->IsUpdateScheduled());
On 2012/12/06 00:51:40, mattm wrote:
> Could inserting some EXPECT_FALSE(pm->IsUpdateScheduled()); in strategic
places
> also be useful?

I put it in a few locations (such as the multiple redirect responses). Not sure
if it helps that much.

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:447: 
On 2012/12/05 23:58:10, mattm wrote:
> extra line

Done.

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:448:
TEST_F(SafeBrowsingProtocolManagerTest, UpdateResponseBadyBody) {
On 2012/12/05 23:58:10, mattm wrote:
> s/Bady/Bad/

Done.

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:647: // The update
response is successful, and has a reset command.
On 2012/12/06 00:51:40, mattm wrote:
> s/reset/redirect/
> (in following tests too)

Done.

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:658:
ASSERT_TRUE(chunk_url_fetcher != NULL);
On 2012/12/06 00:51:40, mattm wrote:
> Should test the fetched URL value as well, at least in one of these tests.

Done.

https://codereview.chromium.org/11419041/diff/9/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:807:
net::TestURLFetcher* first_chunk_url_fetcher =
On 2012/12/06 00:51:40, mattm wrote:
> should probably check the URLs on these requests too..

Done.

[cbentzel, 2012-12-17 22:13:14]

On 2012/12/17 11:24:10, cbentzel wrote:
> Sorry for the slow response. Merged the CL against the previous one which
> removed the test so the diff in protocol_manager_unittest.cc is resolved. Also
> cleaned up some additional comments along the way.

Ignore this. I merged locally - but not reflective of upstream. Waiting for the
other CL which removes UpdateEmptyBody to land.

[cbentzel, 2012-12-18 14:05:10]

Fixed the merge. Please re-review. Thanks.

[mattm, 2012-12-19 03:58:48]

lgtm

https://codereview.chromium.org/11419041/diff/32001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/11419041/diff/32001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:608: if (enabled_)
Just for weakptr safety, these callback.Run() calls should both also test
!callback.is_null() before running the callback.

[cbentzel, 2012-12-19 13:18:33]

https://codereview.chromium.org/11419041/diff/32001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/11419041/diff/32001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:608: if (enabled_)
On 2012/12/19 03:58:48, mattm wrote:
> Just for weakptr safety, these callback.Run() calls should both also test
> !callback.is_null() before running the callback.

I am confused about which weakptr you mean.

a) The callback itself is just a value-like object.
b) The shared data for the callback is held with a refptr.
c) The ProtocolManager is Unretained rather than a weakptr, with the constraint
that it must stay alive when enabled_ is true.

It's possible that c) is no longer a valid assumption, particularly with the
split of DatabaseManager from ProtocolService. I'll take a look.

[mattm, 2012-12-19 18:57:40]

On 2012/12/19 13:18:33, cbentzel wrote:
>
https://codereview.chromium.org/11419041/diff/32001/chrome/browser/safe_brows...
> File chrome/browser/safe_browsing/database_manager.cc (right):
> 
>
https://codereview.chromium.org/11419041/diff/32001/chrome/browser/safe_brows...
> chrome/browser/safe_browsing/database_manager.cc:608: if (enabled_)
> On 2012/12/19 03:58:48, mattm wrote:
> > Just for weakptr safety, these callback.Run() calls should both also test
> > !callback.is_null() before running the callback.
> 
> I am confused about which weakptr you mean.
> 
> a) The callback itself is just a value-like object.
> b) The shared data for the callback is held with a refptr.
> c) The ProtocolManager is Unretained rather than a weakptr, with the
constraint
> that it must stay alive when enabled_ is true.
> 
> It's possible that c) is no longer a valid assumption, particularly with the
> split of DatabaseManager from ProtocolService. I'll take a look.

Sorry, didn't mean there are any weakptrs in play at the moment.  Just that to
make the interface "weakptr safe" it's a good idea to check the callbacks
is_null before calling Run.  It's not necessary for the current code, but
generally when I see a function that takes a callback argument, I would assume I
can pass a callback that uses weakptr and have it do the right thing. So I think
it's just a good idea for future-proofing.

[commit-bot: I haz the power, 2012-12-20 11:58:46]

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

[commit-bot: I haz the power, 2012-12-20 11:58:48]

Failed to apply patch for
chrome/browser/safe_browsing/protocol_manager_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/safe_browsing/protocol_manager_unittest.cc
  Hunk #1 succeeded at 60 (offset -1 lines).
  Hunk #2 succeeded at 260 (offset -1 lines).
  Hunk #3 succeeded at 275 (offset -46 lines).
  Hunk #4 FAILED at 359.
  Hunk #5 succeeded at 341 (offset -46 lines).
  Hunk #6 FAILED at 407.
  Hunk #7 succeeded at 426 (offset -46 lines).
  Hunk #8 succeeded at 461 (offset -46 lines).
  Hunk #9 succeeded at 495 (offset -46 lines).
  Hunk #10 FAILED at 574.
  Hunk #11 succeeded at 553 with fuzz 2 (offset -46 lines).
  Hunk #12 succeeded at 561 (offset -46 lines).
  3 out of 12 hunks FAILED -- saving rejects to file
chrome/browser/safe_browsing/protocol_manager_unittest.cc.rej

Patch:       chrome/browser/safe_browsing/protocol_manager_unittest.cc
Index: chrome/browser/safe_browsing/protocol_manager_unittest.cc
diff --git a/chrome/browser/safe_browsing/protocol_manager_unittest.cc
b/chrome/browser/safe_browsing/protocol_manager_unittest.cc
index
aad096da455b925aff9af0869fc486217baf5a31..37482f98526f4d5db1a1b0c9a81ffe44f2a4cdb3
100644
--- a/chrome/browser/safe_browsing/protocol_manager_unittest.cc
+++ b/chrome/browser/safe_browsing/protocol_manager_unittest.cc
@@ -61,6 +61,14 @@ class SafeBrowsingProtocolManagerTest : public testing::Test
{
                    "&pver=2.2" + key_param_),
               url_fetcher->GetOriginalURL());
   }
+
+  void ValidateRedirectFetcherRequest(const net::TestURLFetcher* url_fetcher,
+                                      const std::string& expected_url) {
+    ASSERT_TRUE(url_fetcher);
+    EXPECT_EQ(net::LOAD_DISABLE_CACHE, url_fetcher->GetLoadFlags());
+    EXPECT_EQ("", url_fetcher->upload_data());
+    EXPECT_EQ(GURL(expected_url), url_fetcher->GetOriginalURL());
+  }
 };
 
 // Ensure that we respect section 5 of the SafeBrowsing protocol specification.
@@ -253,7 +261,8 @@ class MockProtocolDelegate : public
SafeBrowsingProtocolManagerDelegate {
   MOCK_METHOD1(UpdateFinished, void(bool));
   MOCK_METHOD0(ResetDatabase, void());
   MOCK_METHOD1(GetChunks, void(GetChunksCallback));
-  MOCK_METHOD2(AddChunks, void(const std::string&, SBChunkList*));
+  MOCK_METHOD3(AddChunks, void(const std::string&, SBChunkList*,
+                               AddChunksCallback));
   MOCK_METHOD1(DeleteChunks, void(std::vector<SBChunkDelete>*));
 };
 
@@ -312,6 +321,22 @@ void InvokeGetChunksCallback(
   callback.Run(ranges, database_error);
 }
 
+// |HandleAddChunks| deletes the chunks and asynchronously invokes
+// |callback| since SafeBrowsingProtocolManager is not re-entrant at the time
+// this is called. This guarantee is part of the
+// SafeBrowsingProtocolManagerDelegate contract.
+void HandleAddChunks(
+    const std::string& unused_list,
+    SBChunkList* chunks,
+    SafeBrowsingProtocolManagerDelegate::AddChunksCallback callback) {
+  delete chunks;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner(
+      base::ThreadTaskRunnerHandle::Get());
+  if (!task_runner)
+    return;
+  task_runner->PostTask(FROM_HERE, callback);
+}
+
 }  // namespace
 
 // Tests that the Update protocol will be skipped if there are problems
@@ -334,6 +359,8 @@ TEST_F(SafeBrowsingProtocolManagerTest,
ProblemAccessingDatabase) {
 
   pm->ForceScheduleNextUpdate(base::TimeDelta());
   runner->RunTasks();
+
+  EXPECT_TRUE(pm->IsUpdateScheduled());
 }
 
 // Tests the contents of the POST body when there are contents in the
@@ -362,6 +389,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, ExistingDatabase) {
       Invoke(testing::CreateFunctor(InvokeGetChunksCallback,
                                     ranges,
                                     false)));
+  EXPECT_CALL(test_delegate, UpdateFinished(true)).Times(1);
 
   scoped_ptr<SafeBrowsingProtocolManager> pm(
       CreateProtocolManager(&test_delegate));
@@ -381,39 +409,12 @@ TEST_F(SafeBrowsingProtocolManagerTest, ExistingDatabase)
{
   EXPECT_EQ(GURL("https://prefix.com/foo/downloads?client=unittest&appver=1.0"
                  "&pver=2.2" + key_param_),
             url_fetcher->GetOriginalURL());
-}
-
-// Tests what happens when there is a response with no chunks.
-TEST_F(SafeBrowsingProtocolManagerTest, UpdateResponseEmptyBody) {
-  scoped_refptr<ImmediateSingleThreadTaskRunner> runner(
-      new ImmediateSingleThreadTaskRunner());
-  base::ThreadTaskRunnerHandle runner_handler(runner);
-  net::TestURLFetcherFactory url_fetcher_factory;
-
-  testing::StrictMock<MockProtocolDelegate> test_delegate;
-  EXPECT_CALL(test_delegate, UpdateStarted()).Times(1);
-  EXPECT_CALL(test_delegate, GetChunks(_)).WillOnce(
-      Invoke(testing::CreateFunctor(InvokeGetChunksCallback,
-                                    std::vector<SBListChunkRanges>(),
-                                    false)));
-  EXPECT_CALL(test_delegate, UpdateFinished(true)).Times(1);
-
-  scoped_ptr<SafeBrowsingProtocolManager> pm(
-      CreateProtocolManager(&test_delegate));
-
-  // Kick off initialization. This returns chunks from the DB synchronously.
-  pm->ForceScheduleNextUpdate(base::TimeDelta());
-  runner->RunTasks();
-
-  // We should have an URLFetcher at this point in time.
-  net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
-  ValidateUpdateFetcherRequest(url_fetcher);
-
-  // The update response is successful, but an empty body.
   url_fetcher->set_status(net::URLRequestStatus());
   url_fetcher->set_response_code(200);
   url_fetcher->SetResponseString("");
   url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+
+  EXPECT_TRUE(pm->IsUpdateScheduled());
 }
 
 TEST_F(SafeBrowsingProtocolManagerTest, UpdateResponseBadBody) {
@@ -446,6 +447,8 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseBadBody) {
   url_fetcher->set_response_code(200);
   url_fetcher->SetResponseString("THIS_IS_A_BAD_RESPONSE");
   url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+
+  EXPECT_TRUE(pm->IsUpdateScheduled());
 }
 
 // Tests what happens when there is an error in the update response.
@@ -479,6 +482,8 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpError) {
   url_fetcher->set_response_code(404);
   url_fetcher->SetResponseString("");
   url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+
+  EXPECT_TRUE(pm->IsUpdateScheduled());
 }
 
 // Tests what happens when there is an error with the connection.
@@ -511,6 +516,8 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseConnectionError) {
   url_fetcher->set_status(net::URLRequestStatus(net::URLRequestStatus::FAILED,
                                                 net::ERR_CONNECTION_RESET));
   url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+
+  EXPECT_TRUE(pm->IsUpdateScheduled());
 }
 
 // Tests what happens when there is a timeout before an update response.
@@ -542,6 +549,8 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseTimeout) {
   // The first time RunTasks is called above, the update timeout timer is not
   // handled. This call of RunTasks will handle the update.
   runner->RunTasks();
+
+  EXPECT_TRUE(pm->IsUpdateScheduled());
 }
 
 // Tests what happens when there is a reset command in the response.
@@ -567,7 +576,6 @@ TEST_F(SafeBrowsingProtocolManagerTest, UpdateResponseReset)
{
   pm->ForceScheduleNextUpdate(base::TimeDelta());
   runner->RunTasks();
 
-  // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
   ValidateUpdateFetcherRequest(url_fetcher);
 
@@ -576,4 +584,228 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseReset) {
   url_fetcher->set_response_code(200);
   url_fetcher->SetResponseString("r:pleasereset\n");
   url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+
+  EXPECT_TRUE(pm->IsUpdateScheduled());
+}
+
+// Tests a single valid update response, followed by a single redirect response
+// that has an valid, but empty body.
+TEST_F(SafeBrowsingProtocolManagerTest, EmptyRedirectResponse) {
+  scoped_refptr<ImmediateSingleThreadTaskRunner> runner(
+      new ImmediateSingleThreadTaskRunner());
+  base::ThreadTaskRunnerHandle runner_handler(runner);
+  net::TestURLFetcherFactory url_fetcher_factory;
+
+  testing::StrictMock<MockProtocolDelegate> test_delegate;
+  EXPECT_CALL(test_delegate, UpdateStarted()).Times(1);
+  EXPECT_CALL(test_delegate, GetChunks(_)).WillOnce(
+      Invoke(testing::CreateFunctor(InvokeGetChunksCallback,
+                                    std::vector<SBListChunkRanges>(),
+                                    false)));
+  EXPECT_CALL(test_delegate, UpdateFinished(true)).Times(1);
+
+  scoped_ptr<SafeBrowsingProtocolManager> pm(
+      CreateProtocolManager(&test_delegate));
+
+  // Kick off initialization. This returns chunks from the DB synchronously.
+  pm->ForceScheduleNextUpdate(base::TimeDelta());
+  runner->RunTasks();
+
+  // The update response contains a single redirect command.
+  net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
+  ValidateUpdateFetcherRequest(url_fetcher);
+  url_fetcher->set_status(net::URLRequestStatus());
+  url_fetcher->set_response_code(200);
+  url_fetcher->SetResponseString(
+      "i:goog-phish-shavar\n"
+      "u:redirect-server.example.com/path\n");
+  url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+
+  // The redirect response contains an empty body.
+  net::TestURLFetcher* chunk_url_fetcher =
+      url_fetcher_factory.GetFetcherByID(1);
+  ValidateRedirectFetcherRequest(
+      chunk_url_fetcher, "https://redirect-server.example.com/path");
+  chunk_url_fetcher->set_status(net::URLRequestStatus());
+  chunk_url_fetcher->set_response_code(200);
+  chunk_url_fetcher->SetResponseString("");
+  chunk_url_fetcher->delegate()->OnURLFetchComplete(chunk_url_fetcher);
+
+  EXPECT_TRUE(pm->IsUpdateScheduled());
+}
+
+// Tests a single valid up…
(message too large)

[commit-bot: I haz the power, 2012-12-20 19:39:19]

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

[commit-bot: I haz the power, 2012-12-20 23:16:52]

Change committed as 174269