Proper notification in OnStopBinding if headers are not availble (i.e. connection failed).

Test added.
TEST=chrome_frame_net_tests
BUG=none
Review URL: http://codereview.chromium.org/593065

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

4 files changed, 252 insertions, 161 deletions

diff --git a/chrome_frame/test/chrome_frame_test_utils.h b/chrome_frame/test/chrome_frame_test_utils.h
index 1cc6e82..8c0983f 100644
--- a/chrome_frame/test/chrome_frame_test_utils.h
+++ b/chrome_frame/test/chrome_frame_test_utils.h
@@ -109,8 +109,11 @@ class LowIntegrityToken {
 // We need a UI message loop in the main thread.
 class TimedMsgLoop {
  public:
+  TimedMsgLoop() : quit_loop_invoked_(false) {}
+
   void RunFor(int seconds) {
     QuitAfter(seconds);
+    quit_loop_invoked_ = false;
     loop_.MessageLoop::Run();
   }
 
@@ -120,14 +123,21 @@ class TimedMsgLoop {
   }
 
   void Quit() {
-    loop_.PostTask(FROM_HERE, new MessageLoop::QuitTask);
+    QuitAfter(0);
   }
 
   void QuitAfter(int seconds) {
+    quit_loop_invoked_ = true;
     loop_.PostDelayedTask(FROM_HERE, new MessageLoop::QuitTask, 1000 * seconds);
   }
 
+  bool WasTimedOut() const {
+    return !quit_loop_invoked_;
+  }
+
+ private:
   MessageLoopForUI loop_;
+  bool quit_loop_invoked_;
 };
 
 // Saves typing. It's somewhat hard to create a wrapper around
diff --git a/chrome_frame/test/url_request_test.cc b/chrome_frame/test/url_request_test.cc
index e095aa5..aad95a9 100644
--- a/chrome_frame/test/url_request_test.cc
+++ b/chrome_frame/test/url_request_test.cc
@@ -1,152 +1,222 @@
-// Copyright (c) 2010 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 <atlbase.h>

-#include <atlcom.h>

-#include "app/win_util.h"

-#include "testing/gtest/include/gtest/gtest.h"

-#include "testing/gmock/include/gmock/gmock.h"

-#include "testing/gmock_mutant.h"

-

-#include "chrome_frame/urlmon_url_request.h"

-#include "chrome_frame/urlmon_url_request_private.h"

-#include "chrome_frame/test/chrome_frame_test_utils.h"

-#include "chrome_frame/test/http_server.h"

-using testing::CreateFunctor;

-

-const int kChromeFrameLongNavigationTimeoutInSeconds = 10;

-

-class MockUrlDelegate : public PluginUrlRequestDelegate {

- public:

-  MOCK_METHOD8(OnResponseStarted, void(int request_id, const char* mime_type,

-      const char* headers, int size,

-      base::Time last_modified, const std::string& peristent_cookies,

-      const std::string& redirect_url, int redirect_status));

-  MOCK_METHOD3(OnReadComplete, void(int request_id, const void* buffer,

-                                    int len));

-  MOCK_METHOD2(OnResponseEnd, void(int request_id,

-                                   const URLRequestStatus& status));

-

-  static bool ImplementsThreadSafeReferenceCounting() {

-    return false;

-  }

-  void AddRef() {}

-  void Release() {}

-

-  void PostponeReadRequest(chrome_frame_test::TimedMsgLoop* loop,

-                   UrlmonUrlRequest* request, int bytes_to_read) {

-    loop->PostDelayedTask(FROM_HERE, NewRunnableMethod(this,

-                          &MockUrlDelegate::Read, request, bytes_to_read), 0);

-  }

-

- private:

-  void Read(UrlmonUrlRequest* request, int bytes_to_read) {

-    request->Read(bytes_to_read);

-  }

-};

-

-// Simplest UrlmonUrlRequest. Retrieve a file from local web server.

-TEST(UrlmonUrlRequestTest, Simple1) {

-  MockUrlDelegate mock;

-  ChromeFrameHTTPServer server;

-  chrome_frame_test::TimedMsgLoop loop;

-  win_util::ScopedCOMInitializer init_com;

-  CComObjectStackEx<UrlmonUrlRequest> request;

-

-  server.SetUp();

-  request.AddRef();

-  request.Initialize(&mock, 1,  // request_id

-      server.Resolve(L"chrome_frame_window_open.html").spec(),

-      "get",

-      "",      // referrer

-      "",      // extra request

-      NULL,    // upload data

-      true);   // frame busting

-

-  EXPECT_CALL(mock, OnResponseStarted(1, testing::_, testing::_, testing::_,

-                                      testing::_, testing::_, testing::_,

-                                      testing::_))

-    .Times(1)

-    .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(CreateFunctor(

-        &request, &UrlmonUrlRequest::Read, 512))));

-

-

-  EXPECT_CALL(mock, OnReadComplete(1, testing::_, testing::Gt(0)))

-    .Times(testing::AtLeast(1))

-    .WillRepeatedly(testing::InvokeWithoutArgs(CreateFunctor(&mock,

-        &MockUrlDelegate::PostponeReadRequest, &loop, &request, 64)));

-

-  EXPECT_CALL(mock, OnResponseEnd(1, testing::_))

-    .Times(1)

-    .WillOnce(QUIT_LOOP_SOON(loop, 2));

-

-  request.Start();

-  loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);

-  request.Release();

-  server.TearDown();

-}

-

-// Simplest test - retrieve file from local web server.

-TEST(UrlmonUrlRequestManagerTest, Simple1) {

-  MockUrlDelegate mock;

-  ChromeFrameHTTPServer server;

-  chrome_frame_test::TimedMsgLoop loop;

-  server.SetUp();

-  scoped_ptr<UrlmonUrlRequestManager> mgr(new UrlmonUrlRequestManager());

-  mgr->set_delegate(&mock);

-  IPC::AutomationURLRequest r1 = {

-      server.Resolve(L"chrome_frame_window_open.html").spec(), "get" };

-

-  EXPECT_CALL(mock, OnResponseStarted(1, testing::_, testing::_, testing::_,

-                             testing::_, testing::_, testing::_, testing::_))

-      .Times(1)

-      .WillOnce(testing::InvokeWithoutArgs(CreateFunctor(mgr.get(),

-          &PluginUrlRequestManager::ReadUrlRequest, 0, 1, 512)));

-

-  EXPECT_CALL(mock, OnReadComplete(1, testing::_, testing::Gt(0)))

-    .Times(testing::AtLeast(1))

-    .WillRepeatedly(testing::InvokeWithoutArgs(CreateFunctor(mgr.get(),

-        &PluginUrlRequestManager::ReadUrlRequest, 0, 1, 2)));

-

-  EXPECT_CALL(mock, OnResponseEnd(1, testing::_))

-    .Times(1)

-    .WillOnce(QUIT_LOOP_SOON(loop, 2));

-

-  mgr->StartUrlRequest(0, 1, r1);

-  loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);

-  mgr.reset();

-  server.TearDown();

-

-}

-

-TEST(UrlmonUrlRequestManagerTest, Abort1) {

-  MockUrlDelegate mock;

-  ChromeFrameHTTPServer server;

-  chrome_frame_test::TimedMsgLoop loop;

-  server.SetUp();

-  scoped_ptr<UrlmonUrlRequestManager> mgr(new UrlmonUrlRequestManager());

-  mgr->set_delegate(&mock);

-  IPC::AutomationURLRequest r1 = {

-      server.Resolve(L"chrome_frame_window_open.html").spec(), "get" };

-

-  EXPECT_CALL(mock, OnResponseStarted(1, testing::_, testing::_, testing::_,

-                               testing::_, testing::_, testing::_, testing::_))

-    .Times(1)

-    .WillOnce(testing::DoAll(

-        testing::InvokeWithoutArgs(CreateFunctor(mgr.get(),

-            &PluginUrlRequestManager::EndUrlRequest, 0, 1, URLRequestStatus())),

-        testing::InvokeWithoutArgs(CreateFunctor(mgr.get(),

-            &PluginUrlRequestManager::ReadUrlRequest, 0, 1, 2))));

-

-  EXPECT_CALL(mock, OnReadComplete(1, testing::_, testing::Gt(0)))

-    .Times(0);

-

-  EXPECT_CALL(mock, OnResponseEnd(1, testing::_))

-    .Times(1)

-    .WillOnce(QUIT_LOOP_SOON(loop, 2));

-

-  mgr->StartUrlRequest(0, 1, r1);

-  loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);

-  mgr.reset();

-  server.TearDown();

-}

+// Copyright (c) 2010 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 <atlbase.h>
+#include <atlcom.h>
+#include "app/win_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gmock_mutant.h"
+
+#include "chrome_frame/urlmon_url_request.h"
+#include "chrome_frame/urlmon_url_request_private.h"
+#include "chrome_frame/test/chrome_frame_test_utils.h"
+#include "chrome_frame/test/http_server.h"
+using testing::CreateFunctor;
+
+const int kChromeFrameLongNavigationTimeoutInSeconds = 10;
+
+class MockUrlDelegate : public PluginUrlRequestDelegate {
+ public:
+  MOCK_METHOD8(OnResponseStarted, void(int request_id, const char* mime_type,
+      const char* headers, int size,
+      base::Time last_modified, const std::string& peristent_cookies,
+      const std::string& redirect_url, int redirect_status));
+  MOCK_METHOD3(OnReadComplete, void(int request_id, const void* buffer,
+                                    int len));
+  MOCK_METHOD2(OnResponseEnd, void(int request_id,
+                                   const URLRequestStatus& status));
+
+  static bool ImplementsThreadSafeReferenceCounting() {
+    return false;
+  }
+  void AddRef() {}
+  void Release() {}
+
+  void PostponeReadRequest(chrome_frame_test::TimedMsgLoop* loop,
+                   UrlmonUrlRequest* request, int bytes_to_read) {
+    loop->PostDelayedTask(FROM_HERE, NewRunnableMethod(this,
+                          &MockUrlDelegate::Read, request, bytes_to_read), 0);
+  }
+
+ private:
+  void Read(UrlmonUrlRequest* request, int bytes_to_read) {
+    request->Read(bytes_to_read);
+  }
+};
+
+// Simplest UrlmonUrlRequest. Retrieve a file from local web server.
+TEST(UrlmonUrlRequestTest, Simple1) {
+  MockUrlDelegate mock;
+  ChromeFrameHTTPServer server;
+  chrome_frame_test::TimedMsgLoop loop;
+  win_util::ScopedCOMInitializer init_com;
+  CComObjectStackEx<UrlmonUrlRequest> request;
+
+  server.SetUp();
+  request.AddRef();
+  request.Initialize(&mock, 1,  // request_id
+      server.Resolve(L"files/chrome_frame_window_open.html").spec(),
+      "get",
+      "",      // referrer
+      "",      // extra request
+      NULL,    // upload data
+      true);   // frame busting
+
+  testing::InSequence s;
+  EXPECT_CALL(mock, OnResponseStarted(1, testing::_, testing::_, testing::_,
+                                      testing::_, testing::_, testing::_,
+                                      testing::_))
+    .Times(1)
+    .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(CreateFunctor(
+        &request, &UrlmonUrlRequest::Read, 512))));
+
+
+  EXPECT_CALL(mock, OnReadComplete(1, testing::_, testing::Gt(0)))
+    .Times(testing::AtLeast(1))
+    .WillRepeatedly(testing::InvokeWithoutArgs(CreateFunctor(&mock,
+        &MockUrlDelegate::PostponeReadRequest, &loop, &request, 64)));
+
+  EXPECT_CALL(mock, OnResponseEnd(1, testing::_))
+    .Times(1)
+    .WillOnce(QUIT_LOOP_SOON(loop, 2));
+
+  request.Start();
+  loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);
+  request.Release();
+  server.TearDown();
+}
+
+TEST(UrlmonUrlRequestTest, UnreachableUrl) {
+  MockUrlDelegate mock;
+  chrome_frame_test::TimedMsgLoop loop;
+  win_util::ScopedCOMInitializer init_com;
+  CComObjectStackEx<UrlmonUrlRequest> request;
+  ChromeFrameHTTPServer server;
+  server.SetUp();
+  GURL unreachable = server.Resolve(L"files/non_existing.html");
+  server.TearDown();
+
+  request.AddRef();
+  request.Initialize(&mock, 1,  // request_id
+      unreachable.spec(), "get",
+      "",      // referrer
+      "",      // extra request
+      NULL,    // upload data
+      true);   // frame busting
+
+  EXPECT_CALL(mock, OnResponseEnd(1, testing::Property(
+              &URLRequestStatus::os_error, net::ERR_TUNNEL_CONNECTION_FAILED)))
+    .Times(1)
+    .WillOnce(QUIT_LOOP_SOON(loop, 2));
+
+  request.Start();
+  loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);
+  request.Release();
+}
+
+TEST(UrlmonUrlRequestTest, ZeroLengthResponse) {
+  MockUrlDelegate mock;
+  ChromeFrameHTTPServer server;
+  chrome_frame_test::TimedMsgLoop loop;
+  win_util::ScopedCOMInitializer init_com;
+  CComObjectStackEx<UrlmonUrlRequest> request;
+
+  server.SetUp();
+  request.AddRef();
+  request.Initialize(&mock, 1,  // request_id
+      server.Resolve(L"files/empty.html").spec(), "get",
+      "",      // referrer
+      "",      // extra request
+      NULL,    // upload data
+      true);   // frame busting
+
+  // Expect headers
+  EXPECT_CALL(mock, OnResponseStarted(1, testing::_, testing::_, testing::_,
+                                      testing::_, testing::_, testing::_,
+                                      testing::_))
+    .Times(1)
+    .WillOnce(QUIT_LOOP(loop));
+
+  request.Start();
+  loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);
+  EXPECT_FALSE(loop.WasTimedOut());
+
+  // Should stay quiet, since we do not ask for anything for awhile.
+  EXPECT_CALL(mock, OnResponseEnd(1, testing::_)).Times(0);
+  loop.RunFor(3);
+
+  // Invoke read. Only now the response end ("server closed the connection")
+  // is supposed to be delivered.
+  EXPECT_CALL(mock, OnResponseEnd(1, testing::Property(
+                                     &URLRequestStatus::is_success, true)))
+      .Times(1);
+  request.Read(512);
+  request.Release();
+  server.TearDown();
+}
+
+// Simplest test - retrieve file from local web server.
+TEST(UrlmonUrlRequestManagerTest, Simple1) {
+  MockUrlDelegate mock;
+  ChromeFrameHTTPServer server;
+  chrome_frame_test::TimedMsgLoop loop;
+  server.SetUp();
+  scoped_ptr<UrlmonUrlRequestManager> mgr(new UrlmonUrlRequestManager());
+  mgr->set_delegate(&mock);
+  IPC::AutomationURLRequest r1 = {
+      server.Resolve(L"files/chrome_frame_window_open.html").spec(), "get" };
+
+  EXPECT_CALL(mock, OnResponseStarted(1, testing::_, testing::_, testing::_,
+                             testing::_, testing::_, testing::_, testing::_))
+      .Times(1)
+      .WillOnce(testing::InvokeWithoutArgs(CreateFunctor(mgr.get(),
+          &PluginUrlRequestManager::ReadUrlRequest, 0, 1, 512)));
+
+  EXPECT_CALL(mock, OnReadComplete(1, testing::_, testing::Gt(0)))
+    .Times(testing::AtLeast(1))
+    .WillRepeatedly(testing::InvokeWithoutArgs(CreateFunctor(mgr.get(),
+        &PluginUrlRequestManager::ReadUrlRequest, 0, 1, 2)));
+
+  EXPECT_CALL(mock, OnResponseEnd(1, testing::_))
+    .Times(1)
+    .WillOnce(QUIT_LOOP_SOON(loop, 2));
+
+  mgr->StartUrlRequest(0, 1, r1);
+  loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);
+  mgr.reset();
+  server.TearDown();
+}
+
+TEST(UrlmonUrlRequestManagerTest, Abort1) {
+  MockUrlDelegate mock;
+  ChromeFrameHTTPServer server;
+  chrome_frame_test::TimedMsgLoop loop;
+  server.SetUp();
+  scoped_ptr<UrlmonUrlRequestManager> mgr(new UrlmonUrlRequestManager());
+  mgr->set_delegate(&mock);
+  IPC::AutomationURLRequest r1 = {
+      server.Resolve(L"files/chrome_frame_window_open.html").spec(), "get" };
+
+  EXPECT_CALL(mock, OnResponseStarted(1, testing::_, testing::_, testing::_,
+                               testing::_, testing::_, testing::_, testing::_))
+    .Times(1)
+    .WillOnce(testing::DoAll(
+        testing::InvokeWithoutArgs(CreateFunctor(mgr.get(),
+            &PluginUrlRequestManager::EndUrlRequest, 0, 1, URLRequestStatus())),
+        testing::InvokeWithoutArgs(CreateFunctor(mgr.get(),
+            &PluginUrlRequestManager::ReadUrlRequest, 0, 1, 2))));
+
+  EXPECT_CALL(mock, OnReadComplete(1, testing::_, testing::Gt(0)))
+    .Times(0);
+
+  EXPECT_CALL(mock, OnResponseEnd(1, testing::_))
+    .Times(1)
+    .WillOnce(QUIT_LOOP_SOON(loop, 2));
+
+  mgr->StartUrlRequest(0, 1, r1);
+  loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);
+  mgr.reset();
+  server.TearDown();
+}
+
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index c03b3f0..ec61ca9 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -37,6 +37,7 @@ STDMETHODIMP UrlmonUrlRequest::SendStream::Write(const void * buffer,
 
 UrlmonUrlRequest::UrlmonUrlRequest()
     : pending_read_size_(0),
+      headers_received_(false),
       thread_(NULL),
       parent_window_(NULL) {
   DLOG(INFO) << StringPrintf("Created request. Obj: %X", this);
@@ -216,16 +217,29 @@ STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
     }
 
     // The code below seems easy but it is not. :)
+    // The network policy in Chrome network is that error code/end_of_stream
+    // should be returned only as a result of read (or start) request.
+    // Here is the possible cases:
+    // cached_data|pending_read
+    //     FALSE  |FALSE    => EndRequest if no headers, otherwise wait for Read
+    //     FALSE  |TRUE     => EndRequest.
+    //     TRUE   |FALSE    => Wait for Read.
+    //     TRUE   |TRUE     => Something went wrong!!
+
     // we cannot have pending read and data_avail at the same time.
     DCHECK(!(pending_read_size_ > 0 && cached_data_.is_valid()));
 
-    // We have some data, but Chrome has not yet read it. Wait until Chrome
-    // read the remaining of the data and then send the error/success code.
     if (cached_data_.is_valid()) {
       ReleaseBindings();
       return S_OK;
     }
 
+    if (headers_received_ && pending_read_size_ == 0) {
+      ReleaseBindings();
+      return S_OK;
+    }
+
+    // No headers or there is a pending read from Chrome.
     NotifyDelegateAndDie();
     return S_OK;
   }
@@ -424,11 +438,7 @@ STDMETHODIMP UrlmonUrlRequest::OnResponse(DWORD dwResponseCode,
   DLOG(INFO) << __FUNCTION__ << " " << url() << std::endl << " headers: " <<
       std::endl << response_headers;
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
-  if (!binding_) {
-    DLOG(WARNING) << __FUNCTION__
-                  << ": Ignoring as the binding was aborted due to a redirect";
-    return S_OK;
-  }
+  DCHECK(binding_ != NULL);
 
   std::string raw_headers = WideToUTF8(response_headers);
 
@@ -458,7 +468,6 @@ STDMETHODIMP UrlmonUrlRequest::OnResponse(DWORD dwResponseCode,
     }
   }
 
-
   std::string url_for_persistent_cookies;
   std::string persistent_cookies;
 
@@ -488,6 +497,7 @@ STDMETHODIMP UrlmonUrlRequest::OnResponse(DWORD dwResponseCode,
   }
 
   // Inform the delegate.
+  headers_received_ = true;
   delegate_->OnResponseStarted(id(),
                     "",                   // mime_type
                     raw_headers.c_str(),  // headers
diff --git a/chrome_frame/urlmon_url_request_private.h b/chrome_frame/urlmon_url_request_private.h
index 7d95486..13a40d4 100644
--- a/chrome_frame/urlmon_url_request_private.h
+++ b/chrome_frame/urlmon_url_request_private.h
@@ -300,6 +300,7 @@ class UrlmonUrlRequest
   size_t pending_read_size_;
   PlatformThreadId thread_;
   HWND parent_window_;
+  bool headers_received_;
 
   DISALLOW_COPY_AND_ASSIGN(UrlmonUrlRequest);
 };