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;