Don't crash when confronted with many set-cookie headers.

Process cookies in a loop instead of via recursion when SetCookieWithOptionsAsync completes synchronously.

BUG=109388
TEST=URLRequestTestHTTP.GetTest_ManyCookies

Review URL: https://chromiumcodereview.appspot.com/10447092

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@142253 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 133 insertions, 25 deletions

diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index d393677..4300d11 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -497,42 +497,80 @@ void URLRequestHttpJob::SaveCookiesAndNotifyHeadersComplete(int result) {
   SaveNextCookie();
 }
 
+// If the save occurs synchronously, SaveNextCookie will loop and save the next
+// cookie. If the save is deferred, the callback is responsible for continuing
+// to iterate through the cookies.
+// TODO(erikwright): Modify the CookieStore API to indicate via return value
+// whether it completed synchronously or asynchronously.
+// See http://crbug.com/131066.
 void URLRequestHttpJob::SaveNextCookie() {
-  if (response_cookies_save_index_ == response_cookies_.size()) {
-    response_cookies_.clear();
-    response_cookies_save_index_ = 0;
-    SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
-    NotifyHeadersComplete();
-    return;
-  }
-
   // No matter what, we want to report our status as IO pending since we will
   // be notifying our consumer asynchronously via OnStartCompleted.
   SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
 
-  CookieOptions options;
+  // Used to communicate with the callback. See the implementation of
+  // OnCookieSaved.
+  scoped_refptr<SharedBoolean> callback_pending = new SharedBoolean(false);
+  scoped_refptr<SharedBoolean> save_next_cookie_running =
+      new SharedBoolean(true);
+
   if (!(request_info_.load_flags & LOAD_DO_NOT_SAVE_COOKIES) &&
-      request_->context()->cookie_store()) {
+      request_->context()->cookie_store() &&
+      response_cookies_.size() > 0) {
     CookieOptions options;
     options.set_include_httponly();
-    if (CanSetCookie(
-        response_cookies_[response_cookies_save_index_], &options)) {
-      request_->context()->cookie_store()->SetCookieWithOptionsAsync(
-          request_->url(), response_cookies_[response_cookies_save_index_],
-          options, base::Bind(&URLRequestHttpJob::OnCookieSaved,
-                              weak_factory_.GetWeakPtr()));
-      return;
+
+    net::CookieStore::SetCookiesCallback callback(
+        base::Bind(&URLRequestHttpJob::OnCookieSaved,
+                   weak_factory_.GetWeakPtr(),
+                   save_next_cookie_running,
+                   callback_pending));
+
+    // Loop through the cookies as long as SetCookieWithOptionsAsync completes
+    // synchronously.
+    while (!callback_pending->data &&
+           response_cookies_save_index_ < response_cookies_.size()) {
+      if (CanSetCookie(
+          response_cookies_[response_cookies_save_index_], &options)) {
+        callback_pending->data = true;
+        request_->context()->cookie_store()->SetCookieWithOptionsAsync(
+            request_->url(), response_cookies_[response_cookies_save_index_],
+            options, callback);
+      }
+      ++response_cookies_save_index_;
     }
   }
-  CookieHandled();
-}
 
-void URLRequestHttpJob::OnCookieSaved(bool cookie_status) {
-  CookieHandled();
+  save_next_cookie_running->data = false;
+
+  if (!callback_pending->data) {
+    response_cookies_.clear();
+    response_cookies_save_index_ = 0;
+    SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
+    NotifyHeadersComplete();
+    return;
+  }
 }
 
-void URLRequestHttpJob::CookieHandled() {
-  response_cookies_save_index_++;
+// |save_next_cookie_running| is true when the callback is bound and set to
+// false when SaveNextCookie exits, allowing the callback to determine if the
+// save occurred synchronously or asynchronously.
+// |callback_pending| is false when the callback is invoked and will be set to
+// true by the callback, allowing SaveNextCookie to detect whether the save
+// occurred synchronously.
+// See SaveNextCookie() for more information.
+void URLRequestHttpJob::OnCookieSaved(
+    scoped_refptr<SharedBoolean> save_next_cookie_running,
+    scoped_refptr<SharedBoolean> callback_pending,
+    bool cookie_status) {
+  callback_pending->data = false;
+
+  // If we were called synchronously, return.
+  if (save_next_cookie_running->data) {
+    return;
+  }
+
+  // We were called asynchronously, so trigger the next save.
   // We may have been canceled within OnSetCookie.
   if (GetStatus().is_success()) {
     SaveNextCookie();
diff --git a/net/url_request/url_request_http_job.h b/net/url_request/url_request_http_job.h
index 3ff218f..51b900c 100644
--- a/net/url_request/url_request_http_job.h
+++ b/net/url_request/url_request_http_job.h
@@ -139,6 +139,8 @@ class URLRequestHttpJob : public URLRequestJob {
     FINISHED
   };
 
+  typedef base::RefCountedData<bool> SharedBoolean;
+
   class HttpFilterContext;
 
   virtual ~URLRequestHttpJob();
@@ -167,8 +169,12 @@ class URLRequestHttpJob : public URLRequestJob {
       const std::string& cookie_line,
       const std::vector<CookieStore::CookieInfo>& cookie_infos);
   void DoStartTransaction();
-  void OnCookieSaved(bool cookie_status);
-  void CookieHandled();
+
+  // See the implementation for a description of save_next_cookie_running and
+  // callback_pending.
+  void OnCookieSaved(scoped_refptr<SharedBoolean> save_next_cookie_running,
+                     scoped_refptr<SharedBoolean> callback_pending,
+                     bool cookie_status);
 
   // Some servers send the body compressed, but specify the content length as
   // the uncompressed size. If this is the case, we return true in order
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index e6b7658..5f1d7c1 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -551,6 +551,34 @@ class URLRequestTestHTTP : public URLRequestTest {
                         strlen(expected_data)));
   }
 
+  bool DoManyCookiesRequest(int num_cookies){
+    TestDelegate d;
+    URLRequest r(test_server_.GetURL("set-many-cookies?" +
+                                     base::IntToString(num_cookies)),
+                                     &d);
+    r.set_context(&default_context_);
+
+    r.Start();
+    EXPECT_TRUE(r.is_pending());
+
+    MessageLoop::current()->Run();
+
+    bool is_success = r.status().is_success();
+
+    if (!is_success){
+      // Requests handled by ChromeFrame send a less precise error message,
+      // ERR_CONNECTION_ABORTED.
+      EXPECT_TRUE(r.status().error() == ERR_RESPONSE_HEADERS_TOO_BIG ||
+                  r.status().error() == ERR_CONNECTION_ABORTED);
+      // The test server appears to be unable to handle subsequent requests
+      // after this error is triggered. Force it to restart.
+      EXPECT_TRUE(test_server_.Stop());
+      EXPECT_TRUE(test_server_.Start());
+    }
+
+    return is_success;
+  }
+
   LocalHttpTestServer test_server_;
 };
 
@@ -1175,6 +1203,42 @@ TEST_F(URLRequestTestHTTP, GetTest_NoCache) {
   }
 }
 
+// This test has the server send a large number of cookies to the client.
+// To ensure that no number of cookies causes a crash, a galloping binary
+// search is used to estimate that maximum number of cookies that are accepted
+// by the browser. Beyond the maximum number, the request will fail with
+// ERR_RESPONSE_HEADERS_TOO_BIG.
+TEST_F(URLRequestTestHTTP, GetTest_ManyCookies) {
+  ASSERT_TRUE(test_server_.Start());
+
+  int lower_bound = 0;
+  int upper_bound = 1;
+
+  // Double the number of cookies until the response header limits are
+  // exceeded.
+  while (DoManyCookiesRequest(upper_bound)) {
+    lower_bound = upper_bound;
+    upper_bound *= 2;
+    ASSERT_LT(upper_bound, 1000000);
+  }
+
+  int tolerance = upper_bound * 0.005;
+  if (tolerance < 2)
+    tolerance = 2;
+
+  // Perform a binary search to find the highest possible number of cookies,
+  // within the desired tolerance.
+  while (upper_bound - lower_bound >= tolerance) {
+    int num_cookies = (lower_bound + upper_bound) / 2;
+
+    if (DoManyCookiesRequest(num_cookies))
+      lower_bound = num_cookies;
+    else
+      upper_bound = num_cookies;
+  }
+  // Success: the test did not crash.
+}
+
 TEST_F(URLRequestTestHTTP, GetTest) {
   ASSERT_TRUE(test_server_.Start());