update_engine: ReceivedBytes returns boolean on transfer completion/termination
Currently, there are situations that a HttpFetcherDelegate.ReceivedBytes can
cause an eventual transfer completion or termination. This can eventually cause
the object (holding an instance) of HttpFetcherDelegate to be deallocated. So
there should not be any access to any member variable if the object is
deallocated. In this CL we add a return value to ReceivedBytes to indicate
explicitly that this situation happened so we can be careful about acceessing
member variables after a call to this.
BUG=chromium:868520
TEST=unittests
Change-Id: I18db33910f6171c4e426d9bbe604fa1ae07a56dc
Reviewed-on: https://chromium-review.googlesource.com/1158124
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
diff --git a/common/file_fetcher.cc b/common/file_fetcher.cc
index d0a109b..3836e54 100644
--- a/common/file_fetcher.cc
+++ b/common/file_fetcher.cc
@@ -138,8 +138,9 @@
delegate_->TransferComplete(this, true);
} else {
bytes_copied_ += bytes_read;
- if (delegate_)
- delegate_->ReceivedBytes(this, buffer_.data(), bytes_read);
+ if (delegate_ &&
+ !delegate_->ReceivedBytes(this, buffer_.data(), bytes_read))
+ return;
ScheduleRead();
}
}
diff --git a/common/http_fetcher.h b/common/http_fetcher.h
index 3f7b2e8..b2fba1c 100644
--- a/common/http_fetcher.h
+++ b/common/http_fetcher.h
@@ -18,6 +18,7 @@
#define UPDATE_ENGINE_COMMON_HTTP_FETCHER_H_
#include <deque>
+#include <memory>
#include <string>
#include <vector>
@@ -186,8 +187,9 @@
public:
virtual ~HttpFetcherDelegate() = default;
- // Called every time bytes are received.
- virtual void ReceivedBytes(HttpFetcher* fetcher,
+ // Called every time bytes are received. Returns false if this call causes the
+ // transfer be terminated or completed otherwise it returns true.
+ virtual bool ReceivedBytes(HttpFetcher* fetcher,
const void* bytes,
size_t length) = 0;
diff --git a/common/http_fetcher_unittest.cc b/common/http_fetcher_unittest.cc
index 73110e9..a0b0522 100644
--- a/common/http_fetcher_unittest.cc
+++ b/common/http_fetcher_unittest.cc
@@ -412,12 +412,13 @@
public:
HttpFetcherTestDelegate() = default;
- void ReceivedBytes(HttpFetcher* /* fetcher */,
+ bool ReceivedBytes(HttpFetcher* /* fetcher */,
const void* bytes,
size_t length) override {
data.append(reinterpret_cast<const char*>(bytes), length);
// Update counters
times_received_bytes_called_++;
+ return true;
}
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
@@ -560,11 +561,13 @@
namespace {
class PausingHttpFetcherTestDelegate : public HttpFetcherDelegate {
public:
- void ReceivedBytes(HttpFetcher* fetcher,
- const void* /* bytes */, size_t /* length */) override {
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* /* bytes */,
+ size_t /* length */) override {
CHECK(!paused_);
paused_ = true;
fetcher->Pause();
+ return true;
}
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
MessageLoop::current()->BreakLoop();
@@ -641,8 +644,11 @@
namespace {
class AbortingHttpFetcherTestDelegate : public HttpFetcherDelegate {
public:
- void ReceivedBytes(HttpFetcher* fetcher,
- const void* bytes, size_t length) override {}
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* bytes,
+ size_t length) override {
+ return true;
+ }
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
ADD_FAILURE(); // We should never get here
MessageLoop::current()->BreakLoop();
@@ -736,9 +742,11 @@
namespace {
class FlakyHttpFetcherTestDelegate : public HttpFetcherDelegate {
public:
- void ReceivedBytes(HttpFetcher* fetcher,
- const void* bytes, size_t length) override {
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* bytes,
+ size_t length) override {
data.append(reinterpret_cast<const char*>(bytes), length);
+ return true;
}
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
EXPECT_TRUE(successful);
@@ -800,13 +808,15 @@
}
}
- void ReceivedBytes(HttpFetcher* fetcher,
- const void* bytes, size_t length) override {
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* bytes,
+ size_t length) override {
if (server_) {
LOG(INFO) << "Stopping server in ReceivedBytes";
server_.reset();
LOG(INFO) << "server stopped";
}
+ return true;
}
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
EXPECT_FALSE(successful);
@@ -974,9 +984,11 @@
public:
explicit RedirectHttpFetcherTestDelegate(bool expected_successful)
: expected_successful_(expected_successful) {}
- void ReceivedBytes(HttpFetcher* fetcher,
- const void* bytes, size_t length) override {
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* bytes,
+ size_t length) override {
data.append(reinterpret_cast<const char*>(bytes), length);
+ return true;
}
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
EXPECT_EQ(expected_successful_, successful);
@@ -1073,10 +1085,12 @@
explicit MultiHttpFetcherTestDelegate(int expected_response_code)
: expected_response_code_(expected_response_code) {}
- void ReceivedBytes(HttpFetcher* fetcher,
- const void* bytes, size_t length) override {
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* bytes,
+ size_t length) override {
EXPECT_EQ(fetcher, fetcher_.get());
data.append(reinterpret_cast<const char*>(bytes), length);
+ return true;
}
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
@@ -1272,7 +1286,7 @@
explicit MultiHttpFetcherTerminateTestDelegate(size_t terminate_trigger_bytes)
: terminate_trigger_bytes_(terminate_trigger_bytes) {}
- void ReceivedBytes(HttpFetcher* fetcher,
+ bool ReceivedBytes(HttpFetcher* fetcher,
const void* bytes,
size_t length) override {
LOG(INFO) << "ReceivedBytes, " << length << " bytes.";
@@ -1285,6 +1299,7 @@
base::Unretained(fetcher_.get())));
}
bytes_downloaded_ += length;
+ return true;
}
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
@@ -1338,9 +1353,11 @@
namespace {
class BlockedTransferTestDelegate : public HttpFetcherDelegate {
public:
- void ReceivedBytes(HttpFetcher* fetcher,
- const void* bytes, size_t length) override {
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* bytes,
+ size_t length) override {
ADD_FAILURE();
+ return true;
}
void TransferComplete(HttpFetcher* fetcher, bool successful) override {
EXPECT_FALSE(successful);
diff --git a/common/multi_range_http_fetcher.cc b/common/multi_range_http_fetcher.cc
index dc4b7c1..0a19c6a 100644
--- a/common/multi_range_http_fetcher.cc
+++ b/common/multi_range_http_fetcher.cc
@@ -86,7 +86,7 @@
}
// State change: Downloading -> Downloading or Pending transfer ended
-void MultiRangeHttpFetcher::ReceivedBytes(HttpFetcher* fetcher,
+bool MultiRangeHttpFetcher::ReceivedBytes(HttpFetcher* fetcher,
const void* bytes,
size_t length) {
CHECK_LT(current_index_, ranges_.size());
@@ -99,15 +99,9 @@
range.length() - bytes_received_this_range_);
}
LOG_IF(WARNING, next_size <= 0) << "Asked to write length <= 0";
- if (delegate_) {
- delegate_->ReceivedBytes(this, bytes, next_size);
- // If the transfer was already terminated by |delegate_|, return immediately
- // to avoid calling TerminateTransfer() again.
- if (!base_fetcher_active_) {
- LOG(INFO) << "Delegate has terminated the transfer.";
- return;
- }
- }
+ if (delegate_ && !delegate_->ReceivedBytes(this, bytes, next_size))
+ return false;
+
bytes_received_this_range_ += length;
if (range.HasLength() && bytes_received_this_range_ >= range.length()) {
// Terminates the current fetcher. Waits for its TransferTerminated
@@ -115,9 +109,10 @@
// signalling the delegate that the whole multi-transfer is complete
// before all fetchers are really done and cleaned up.
pending_transfer_ended_ = true;
- LOG(INFO) << "terminating transfer";
+ LOG(INFO) << "Terminating transfer.";
fetcher->TerminateTransfer();
}
+ return true;
}
// State change: Downloading or Pending transfer ended -> Stopped
diff --git a/common/multi_range_http_fetcher.h b/common/multi_range_http_fetcher.h
index 54ddfbc..763c287 100644
--- a/common/multi_range_http_fetcher.h
+++ b/common/multi_range_http_fetcher.h
@@ -146,7 +146,7 @@
// HttpFetcherDelegate overrides.
// State change: Downloading -> Downloading or Pending transfer ended
- void ReceivedBytes(HttpFetcher* fetcher,
+ bool ReceivedBytes(HttpFetcher* fetcher,
const void* bytes,
size_t length) override;
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index 87f30ad..50ddeb0 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -545,8 +545,8 @@
}
bytes_downloaded_ += payload_size;
in_write_callback_ = true;
- if (delegate_)
- delegate_->ReceivedBytes(this, ptr, payload_size);
+ if (delegate_ && !delegate_->ReceivedBytes(this, ptr, payload_size))
+ return payload_size;
in_write_callback_ = false;
return payload_size;
}
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 3502048..e08a868 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -801,11 +801,12 @@
// We just store the response in the buffer. Once we've received all bytes,
// we'll look in the buffer and decide what to do.
-void OmahaRequestAction::ReceivedBytes(HttpFetcher *fetcher,
+bool OmahaRequestAction::ReceivedBytes(HttpFetcher* fetcher,
const void* bytes,
size_t length) {
const uint8_t* byte_ptr = reinterpret_cast<const uint8_t*>(bytes);
response_buffer_.insert(response_buffer_.end(), byte_ptr, byte_ptr + length);
+ return true;
}
namespace {
diff --git a/omaha_request_action.h b/omaha_request_action.h
index c21bb37..c3c8233 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -167,8 +167,9 @@
std::string Type() const override { return StaticType(); }
// Delegate methods (see http_fetcher.h)
- void ReceivedBytes(HttpFetcher *fetcher,
- const void* bytes, size_t length) override;
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* bytes,
+ size_t length) override;
void TransferComplete(HttpFetcher *fetcher, bool successful) override;
diff --git a/payload_consumer/download_action.cc b/payload_consumer/download_action.cc
index 1654c2a..ab9f2e8 100644
--- a/payload_consumer/download_action.cc
+++ b/payload_consumer/download_action.cc
@@ -318,7 +318,7 @@
bytes_received_ = offset;
}
-void DownloadAction::ReceivedBytes(HttpFetcher* fetcher,
+bool DownloadAction::ReceivedBytes(HttpFetcher* fetcher,
const void* bytes,
size_t length) {
// Note that bytes_received_ is the current offset.
@@ -345,7 +345,7 @@
// the TransferTerminated callback. Otherwise, this and the HTTP fetcher
// objects may get destroyed before all callbacks are complete.
TerminateProcessing();
- return;
+ return false;
}
// Call p2p_manager_->FileMakeVisible() when we've successfully
@@ -356,6 +356,7 @@
system_state_->p2p_manager()->FileMakeVisible(p2p_file_id_);
p2p_visible_ = true;
}
+ return true;
}
void DownloadAction::TransferComplete(HttpFetcher* fetcher, bool successful) {
diff --git a/payload_consumer/download_action.h b/payload_consumer/download_action.h
index 6e6f057..028a99a 100644
--- a/payload_consumer/download_action.h
+++ b/payload_consumer/download_action.h
@@ -97,8 +97,9 @@
int GetHTTPResponseCode() { return http_fetcher_->http_response_code(); }
// HttpFetcherDelegate methods (see http_fetcher.h)
- void ReceivedBytes(HttpFetcher* fetcher,
- const void* bytes, size_t length) override;
+ bool ReceivedBytes(HttpFetcher* fetcher,
+ const void* bytes,
+ size_t length) override;
void SeekToOffset(off_t offset) override;
void TransferComplete(HttpFetcher* fetcher, bool successful) override;
void TransferTerminated(HttpFetcher* fetcher) override;