AU: Fix potential issues with premature destruction of HTTP fetchers.

http_fetcher_unittest.cc

 // Copyright (c) 2009 The Chromium OS 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 <unistd.h>
 
 #include <string>
 #include <utility>
 #include <vector>
 
@@ skipping 178 matching lines @@
   virtual void ReceivedBytes(HttpFetcher* fetcher,
                              const char* bytes, int length) {
     char str[length + 1];
     memset(str, 0, length + 1);
     memcpy(str, bytes, length);
   }
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
     EXPECT_EQ(200, fetcher->http_response_code());
     g_main_loop_quit(loop_);
   }
+  virtual void TransferTerminated(HttpFetcher* fetcher) {
+    ADD_FAILURE();
+  }
   GMainLoop* loop_;
 };
 
 struct StartTransferArgs {
   HttpFetcher *http_fetcher;
   string url;
 };
 
 gboolean StartTransfer(gpointer data) {
   StartTransferArgs *args = reinterpret_cast<StartTransferArgs*>(data);
@@ skipping 48 matching lines @@
     char str[length + 1];
     memset(str, 0, length + 1);
     memcpy(str, bytes, length);
     CHECK(!paused_);
     paused_ = true;
     fetcher->Pause();
   }
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
     g_main_loop_quit(loop_);
   }
+  virtual void TransferTerminated(HttpFetcher* fetcher) {
+    ADD_FAILURE();
+  }
   void Unpause() {
     CHECK(paused_);
     paused_ = false;
     fetcher_->Unpause();
   }
   bool paused_;
   HttpFetcher* fetcher_;
   GMainLoop* loop_;
 };
 
@@ skipping 30 matching lines @@
   }
   g_main_loop_unref(loop);
 }
 
 namespace {
 class AbortingHttpFetcherTestDelegate : public HttpFetcherDelegate {
  public:
   virtual void ReceivedBytes(HttpFetcher* fetcher,
                              const char* bytes, int length) {}
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
-    CHECK(false);  // We should never get here
+    ADD_FAILURE(); // We should never get here

[adlr, 2010/11/17 21:33:50]

put space back before //

[petkov, 2010/11/17 21:47:54]

Done.

     g_main_loop_quit(loop_);
   }
+  virtual void TransferTerminated(HttpFetcher* fetcher) {
+    CHECK(fetcher == fetcher_.get());
+    CHECK(!once_);

[adlr, 2010/11/17 21:33:50]

use EXPECT rather than CHECK? Unless that doesn't work here. EXPECT seems to
work more places than ASSERT

[petkov, 2010/11/17 21:47:54]

Done.

+    CHECK(callback_once_);
+    callback_once_ = false;
+    // |fetcher| can be destroyed during this callback.
+    fetcher_.reset(NULL);
+ }
   void TerminateTransfer() {
     CHECK(once_);
     once_ = false;
     fetcher_->TerminateTransfer();
   }
   void EndLoop() {
     g_main_loop_quit(loop_);
   }
   bool once_;
-  HttpFetcher* fetcher_;
+  bool callback_once_;
+  scoped_ptr<HttpFetcher> fetcher_;
   GMainLoop* loop_;
 };
 
 gboolean AbortingTimeoutCallback(gpointer data) {
   AbortingHttpFetcherTestDelegate *delegate =
       reinterpret_cast<AbortingHttpFetcherTestDelegate*>(data);
   if (delegate->once_) {
     delegate->TerminateTransfer();
     return TRUE;
   } else {
     delegate->EndLoop();
     return FALSE;
   }
 }
 }  // namespace {}
 
 TYPED_TEST(HttpFetcherTest, AbortTest) {
   GMainLoop* loop = g_main_loop_new(g_main_context_default(), FALSE);
   {
     AbortingHttpFetcherTestDelegate delegate;
-    scoped_ptr<HttpFetcher> fetcher(this->NewLargeFetcher());
+    delegate.fetcher_.reset(this->NewLargeFetcher());
     delegate.once_ = true;
+    delegate.callback_once_ = true;
     delegate.loop_ = loop;
-    delegate.fetcher_ = fetcher.get();
-    fetcher->set_delegate(&delegate);
+    delegate.fetcher_->set_delegate(&delegate);
 
     typename TestFixture::HttpServer server;
     this->IgnoreServerAborting(&server);
     ASSERT_TRUE(server.started_);
     GSource* timeout_source_;
     timeout_source_ = g_timeout_source_new(0);  // ms
     g_source_set_callback(timeout_source_, AbortingTimeoutCallback, &delegate,
                           NULL);
     g_source_attach(timeout_source_, NULL);
-    fetcher->BeginTransfer(this->BigUrl());
+    delegate.fetcher_->BeginTransfer(this->BigUrl());
 
     g_main_loop_run(loop);
+    CHECK(!delegate.once_);
+    CHECK(!delegate.callback_once_);
     g_source_destroy(timeout_source_);
   }
   g_main_loop_unref(loop);
 }
 
 namespace {
 class FlakyHttpFetcherTestDelegate : public HttpFetcherDelegate {
  public:
   virtual void ReceivedBytes(HttpFetcher* fetcher,
                              const char* bytes, int length) {
     data.append(bytes, length);
   }
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
     EXPECT_TRUE(successful);
     EXPECT_EQ(206, fetcher->http_response_code());
     g_main_loop_quit(loop_);
   }
+  virtual void TransferTerminated(HttpFetcher* fetcher) {
+    ADD_FAILURE();
+  }
   string data;
   GMainLoop* loop_;
 };
 }  // namespace {}
 
 TYPED_TEST(HttpFetcherTest, FlakyTest) {
   if (this->IsMock())
     return;
   GMainLoop* loop = g_main_loop_new(g_main_context_default(), FALSE);
   {
@@ skipping 34 matching lines @@
       delete server_;
       LOG(INFO) << "server stopped";
       server_ = NULL;
     }
   }
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
     EXPECT_FALSE(successful);
     EXPECT_EQ(0, fetcher->http_response_code());
     g_main_loop_quit(loop_);
   }
+  virtual void TransferTerminated(HttpFetcher* fetcher) {
+    ADD_FAILURE();
+  }
   GMainLoop* loop_;
   PythonHttpServer* server_;
 };
 }  // namespace {}
 
 
 TYPED_TEST(HttpFetcherTest, FailureTest) {
   if (this->IsMock())
     return;
   GMainLoop* loop = g_main_loop_new(g_main_context_default(), FALSE);
@@ skipping 54 matching lines @@
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
     EXPECT_EQ(expected_successful_, successful);
     if (expected_successful_)
       EXPECT_EQ(200, fetcher->http_response_code());
     else {
       EXPECT_GE(fetcher->http_response_code(), 301);
       EXPECT_LE(fetcher->http_response_code(), 307);
     }
     g_main_loop_quit(loop_);
   }
+  virtual void TransferTerminated(HttpFetcher* fetcher) {
+    ADD_FAILURE();
+  }
   bool expected_successful_;
   string data;
   GMainLoop* loop_;
 };
 
 // RedirectTest takes ownership of |http_fetcher|.
 void RedirectTest(bool expected_successful,
                   const string& url,
                   HttpFetcher* http_fetcher) {
   GMainLoop* loop = g_main_loop_new(g_main_context_default(), FALSE);
@@ skipping 59 matching lines @@
   RedirectTest(false, url, this->NewLargeFetcher());
 }
 
 namespace {
 class MultiHttpFetcherTestDelegate : public HttpFetcherDelegate {
  public:
   MultiHttpFetcherTestDelegate(int expected_response_code)
       : expected_response_code_(expected_response_code) {}
   virtual void ReceivedBytes(HttpFetcher* fetcher,
                              const char* bytes, int length) {
+    EXPECT_EQ(fetcher, fetcher_.get());
     data.append(bytes, length);
   }
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
+    EXPECT_EQ(fetcher, fetcher_.get());
     EXPECT_EQ(expected_response_code_ != 0, successful);
     if (expected_response_code_ != 0)
       EXPECT_EQ(expected_response_code_, fetcher->http_response_code());
+    // Destroy the fetcher (because we're allowed to).
+    fetcher_.reset(NULL);
     g_main_loop_quit(loop_);
   }
+  virtual void TransferTerminated(HttpFetcher* fetcher) {
+    ADD_FAILURE();
+  }
+  scoped_ptr<HttpFetcher> fetcher_;
   int expected_response_code_;
   string data;
   GMainLoop* loop_;
 };
 
 void MultiTest(HttpFetcher* fetcher_in,
                const string& url,
                const MultiHttpFetcher<LibcurlHttpFetcher>::RangesVect& ranges,
                const string& expected_prefix,
                off_t expected_size,
                int expected_response_code) {
   GMainLoop* loop = g_main_loop_new(g_main_context_default(), FALSE);
   {
     MultiHttpFetcherTestDelegate delegate(expected_response_code);
     delegate.loop_ = loop;
-    scoped_ptr<HttpFetcher> fetcher(fetcher_in);
+    delegate.fetcher_.reset(fetcher_in);
     MultiHttpFetcher<LibcurlHttpFetcher>* multi_fetcher =
-        dynamic_cast<MultiHttpFetcher<LibcurlHttpFetcher>*>(fetcher.get());
+        dynamic_cast<MultiHttpFetcher<LibcurlHttpFetcher>*>(fetcher_in);
     ASSERT_TRUE(multi_fetcher);
     multi_fetcher->set_ranges(ranges);
     multi_fetcher->SetConnectionAsExpensive(false);
     multi_fetcher->SetBuildType(false);
-    fetcher->set_delegate(&delegate);
+    multi_fetcher->set_delegate(&delegate);
 
-    StartTransferArgs start_xfer_args = {fetcher.get(), url};
+    StartTransferArgs start_xfer_args = {multi_fetcher, url};
 
     g_timeout_add(0, StartTransfer, &start_xfer_args);
     g_main_loop_run(loop);
 
     EXPECT_EQ(expected_size, delegate.data.size());
     EXPECT_EQ(expected_prefix,
               string(delegate.data.data(), expected_prefix.size()));
   }
   g_main_loop_unref(loop);
 }
 }  // namespace {}
 
-TYPED_TEST(HttpFetcherTest, MultiHttpFetcherSimplTest) {
+TYPED_TEST(HttpFetcherTest, MultiHttpFetcherSimpleTest) {
   if (!this->IsMulti())
     return;
   typename TestFixture::HttpServer server;
   ASSERT_TRUE(server.started_);
 
   MultiHttpFetcher<LibcurlHttpFetcher>::RangesVect ranges;
   ranges.push_back(make_pair(0, 25));
   ranges.push_back(make_pair(99, -1));
   MultiTest(this->NewLargeFetcher(),
             this->BigUrl(),
@@ skipping 59 matching lines @@
 class BlockedTransferTestDelegate : public HttpFetcherDelegate {
  public:
   virtual void ReceivedBytes(HttpFetcher* fetcher,
                              const char* bytes, int length) {
     ADD_FAILURE();
   }
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
     EXPECT_FALSE(successful);
     g_main_loop_quit(loop_);
   }
+  virtual void TransferTerminated(HttpFetcher* fetcher) {
+    ADD_FAILURE();
+  }
   GMainLoop* loop_;
 };
 
 }  // namespace
 
 TYPED_TEST(HttpFetcherTest, BlockedTransferTest) {
   if (this->IsMock() || this->IsMulti())
     return;
 
   for (int i = 0; i < 2; i++) {
@@ skipping 19 matching lines @@
     StartTransferArgs start_xfer_args =
         { fetcher.get(), LocalServerUrlForPath(this->SmallUrl()) };
 
     g_timeout_add(0, StartTransfer, &start_xfer_args);
     g_main_loop_run(loop);
     g_main_loop_unref(loop);
   }
 }
 
 }  // namespace chromeos_update_engine