Skip to content

Commit

Permalink
FileWriterDelegate: Don't flush while async write in progress
Browse files Browse the repository at this point in the history 
If the URLRequestJob signals a read error while an async write is
in progress, don't attempt to flush the FileStreamWriter as that
violates its API. Read/write errors are now routed to separate
handlers, and no flush is attempted after a write error.

Bug: 795881, 732321
Change-Id: I3585b1543df893ac98e9e392e068cf54bd3f0222
Reviewed-on: https://chromium-review.googlesource.com/914933
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536818}
  • Loading branch information
@inexorabletash
inexorabletash authored and Commit Bot committed Feb 14, 2018
1 parent 103ed27 commit ae92808
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 37 deletions.
46 changes: 36 additions & 10 deletions storage/browser/fileapi/file_writer_delegate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,27 +69,27 @@ void FileWriterDelegate::OnReceivedRedirect(
const net::RedirectInfo& redirect_info,
bool* defer_redirect) {
NOTREACHED();
OnError(base::File::FILE_ERROR_SECURITY);
OnReadError(base::File::FILE_ERROR_SECURITY);
}

void FileWriterDelegate::OnAuthRequired(net::URLRequest* request,
net::AuthChallengeInfo* auth_info) {
NOTREACHED();
OnError(base::File::FILE_ERROR_SECURITY);
OnReadError(base::File::FILE_ERROR_SECURITY);
}

void FileWriterDelegate::OnCertificateRequested(
net::URLRequest* request,
net::SSLCertRequestInfo* cert_request_info) {
NOTREACHED();
OnError(base::File::FILE_ERROR_SECURITY);
OnReadError(base::File::FILE_ERROR_SECURITY);
}

void FileWriterDelegate::OnSSLCertificateError(net::URLRequest* request,
const net::SSLInfo& ssl_info,
bool fatal) {
NOTREACHED();
OnError(base::File::FILE_ERROR_SECURITY);
OnReadError(base::File::FILE_ERROR_SECURITY);
}

void FileWriterDelegate::OnResponseStarted(net::URLRequest* request,
Expand All @@ -98,7 +98,7 @@ void FileWriterDelegate::OnResponseStarted(net::URLRequest* request,
DCHECK_EQ(request_.get(), request);

if (net_error != net::OK || request->GetResponseCode() != 200) {
OnError(base::File::FILE_ERROR_FAILED);
OnReadError(base::File::FILE_ERROR_FAILED);
return;
}
Read();
Expand All @@ -110,7 +110,7 @@ void FileWriterDelegate::OnReadCompleted(net::URLRequest* request,
DCHECK_EQ(request_.get(), request);

if (bytes_read < 0) {
OnError(base::File::FILE_ERROR_FAILED);
OnReadError(base::File::FILE_ERROR_FAILED);
return;
}
OnDataReceived(bytes_read);
Expand All @@ -127,7 +127,7 @@ void FileWriterDelegate::Read() {
FROM_HERE, base::Bind(&FileWriterDelegate::OnDataReceived,
weak_factory_.GetWeakPtr(), bytes_read_));
} else {
OnError(base::File::FILE_ERROR_FAILED);
OnReadError(base::File::FILE_ERROR_FAILED);
}
}

Expand Down Expand Up @@ -157,11 +157,19 @@ void FileWriterDelegate::Write() {
FROM_HERE, base::Bind(&FileWriterDelegate::OnDataWritten,
weak_factory_.GetWeakPtr(), write_response));
} else if (net::ERR_IO_PENDING != write_response) {
OnError(NetErrorToFileError(write_response));
OnWriteError(NetErrorToFileError(write_response));
} else {
async_write_in_progress_ = true;
}
}

void FileWriterDelegate::OnDataWritten(int write_response) {
async_write_in_progress_ = false;
if (saved_read_error_ != base::File::FILE_OK) {
OnReadError(saved_read_error_);
return;
}

if (write_response > 0) {
OnProgress(write_response, false);
cursor_->DidConsume(write_response);
Expand All @@ -171,7 +179,7 @@ void FileWriterDelegate::OnDataWritten(int write_response) {
else
Write();
} else {
OnError(NetErrorToFileError(write_response));
OnWriteError(NetErrorToFileError(write_response));
}
}

Expand All @@ -180,7 +188,14 @@ FileWriterDelegate::GetCompletionStatusOnError() const {
return writing_started_ ? ERROR_WRITE_STARTED : ERROR_WRITE_NOT_STARTED;
}

void FileWriterDelegate::OnError(base::File::Error error) {
void FileWriterDelegate::OnReadError(base::File::Error error) {
if (async_write_in_progress_) {
// Error signaled by the URLRequest while writing. This will be processed
// when the write completes.
saved_read_error_ = error;
return;
}

// Destroy the request and invalidate weak ptrs to prevent pending callbacks.
request_.reset();
weak_factory_.InvalidateWeakPtrs();
Expand All @@ -191,6 +206,17 @@ void FileWriterDelegate::OnError(base::File::Error error) {
write_callback_.Run(error, 0, ERROR_WRITE_NOT_STARTED);
}

void FileWriterDelegate::OnWriteError(base::File::Error error) {
// Destroy the request and invalidate weak ptrs to prevent pending callbacks.
request_.reset();
weak_factory_.InvalidateWeakPtrs();

// Errors when writing are not recoverable, so don't bother flushing.
write_callback_.Run(
error, 0,
writing_started_ ? ERROR_WRITE_STARTED : ERROR_WRITE_NOT_STARTED);
}

void FileWriterDelegate::OnProgress(int bytes_written, bool done) {
DCHECK(bytes_written + bytes_written_backlog_ >= bytes_written_backlog_);
static const int kMinProgressDelayMS = 200;
Expand Down
10 changes: 8 additions & 2 deletions storage/browser/fileapi/file_writer_delegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,19 @@ class STORAGE_EXPORT FileWriterDelegate : public net::URLRequest::Delegate {
void OnResponseStarted(net::URLRequest* request, int net_error) override;
void OnReadCompleted(net::URLRequest* request, int bytes_read) override;

protected:
// Virtual for tests.
virtual void OnDataReceived(int bytes_read);

private:
void OnGetFileInfoAndStartRequest(std::unique_ptr<net::URLRequest> request,
base::File::Error error,
const base::File::Info& file_info);
void Read();
void OnDataReceived(int bytes_read);
void Write();
void OnDataWritten(int write_response);
void OnError(base::File::Error error);
void OnReadError(base::File::Error error);
void OnWriteError(base::File::Error error);
void OnProgress(int bytes_read, bool done);
void OnWriteCancelled(int status);
void MaybeFlushForCompletion(base::File::Error error,
Expand All @@ -93,6 +97,8 @@ class STORAGE_EXPORT FileWriterDelegate : public net::URLRequest::Delegate {
int bytes_written_backlog_;
int bytes_written_;
int bytes_read_;
bool async_write_in_progress_ = false;
base::File::Error saved_read_error_ = base::File::FILE_OK;
scoped_refptr<net::IOBufferWithSize> io_buffer_;
scoped_refptr<net::DrainableIOBuffer> cursor_;
std::unique_ptr<net::URLRequest> request_;
Expand Down
105 changes: 80 additions & 25 deletions storage/browser/fileapi/file_writer_delegate_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ class FileWriterDelegateTest : public PlatformTest {

int64_t GetFileSizeOnDisk(const char* test_file_path) {
// There might be in-flight flush/write.
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
base::Bind(&base::DoNothing));
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(&base::DoNothing));
base::RunLoop().RunUntilIdle();

FileSystemURL url = GetFileSystemURL(test_file_path);
Expand All @@ -123,23 +123,28 @@ class FileWriterDelegateTest : public PlatformTest {
kOrigin, kFileSystemType, base::FilePath().FromUTF8Unsafe(file_name));
}

FileWriterDelegate* CreateWriterDelegate(const char* test_file_path,
int64_t offset,
int64_t allowed_growth) {
storage::SandboxFileStreamWriter* writer =
new storage::SandboxFileStreamWriter(
file_system_context_.get(),
GetFileSystemURL(test_file_path),
offset,
*file_system_context_->GetUpdateObservers(kFileSystemType));
std::unique_ptr<storage::SandboxFileStreamWriter> CreateWriter(
const char* test_file_path,
int64_t offset,
int64_t allowed_growth) {
auto writer = std::make_unique<storage::SandboxFileStreamWriter>(
file_system_context_.get(), GetFileSystemURL(test_file_path), offset,
*file_system_context_->GetUpdateObservers(kFileSystemType));
writer->set_default_quota(allowed_growth);
return new FileWriterDelegate(
std::unique_ptr<storage::FileStreamWriter>(writer),
storage::FlushPolicy::FLUSH_ON_COMPLETION);
return writer;
}

std::unique_ptr<FileWriterDelegate> CreateWriterDelegate(
const char* test_file_path,
int64_t offset,
int64_t allowed_growth) {
auto writer = CreateWriter(test_file_path, offset, allowed_growth);
return std::make_unique<FileWriterDelegate>(
std::move(writer), storage::FlushPolicy::FLUSH_ON_COMPLETION);
}

FileWriterDelegate::DelegateWriteCallback GetWriteCallback(Result* result) {
return base::Bind(&Result::DidWrite, base::Unretained(result));
return base::BindRepeating(&Result::DidWrite, base::Unretained(result));
}

// Creates and sets up a FileWriterDelegate for writing the given |blob_url|,
Expand All @@ -148,8 +153,8 @@ class FileWriterDelegateTest : public PlatformTest {
const GURL& blob_url,
int64_t offset,
int64_t allowed_growth) {
file_writer_delegate_.reset(
CreateWriterDelegate(test_file_path, offset, allowed_growth));
file_writer_delegate_ =
CreateWriterDelegate(test_file_path, offset, allowed_growth);
request_ = empty_context_.CreateRequest(blob_url, net::DEFAULT_PRIORITY,
file_writer_delegate_.get(),
TRAFFIC_ANNOTATION_FOR_TESTS);
Expand Down Expand Up @@ -189,8 +194,9 @@ class FileWriterDelegateTestJob : public net::URLRequestJob {

void Start() override {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::Bind(&FileWriterDelegateTestJob::NotifyHeadersComplete,
weak_factory_.GetWeakPtr()));
FROM_HERE,
base::BindOnce(&FileWriterDelegateTestJob::NotifyHeadersComplete,
weak_factory_.GetWeakPtr()));
}

int ReadRawData(net::IOBuffer* buf, int buf_size) override {
Expand Down Expand Up @@ -233,8 +239,8 @@ class BlobURLRequestJobFactory : public net::URLRequestJobFactory {
const std::string& scheme,
net::URLRequest* request,
net::NetworkDelegate* network_delegate) const override {
return new FileWriterDelegateTestJob(
request, network_delegate, *content_data_);
return new FileWriterDelegateTestJob(request, network_delegate,
*content_data_);
}

net::URLRequestJob* MaybeInterceptRedirect(
Expand Down Expand Up @@ -274,7 +280,7 @@ void FileWriterDelegateTest::SetUp() {
ASSERT_EQ(base::File::FILE_OK,
AsyncFileTestHelper::CreateFile(file_system_context_.get(),
GetFileSystemURL("test")));
job_factory_.reset(new BlobURLRequestJobFactory(&content_));
job_factory_ = std::make_unique<BlobURLRequestJobFactory>(&content_);
empty_context_.set_job_factory(job_factory_.get());
}

Expand Down Expand Up @@ -379,9 +385,9 @@ TEST_F(FileWriterDelegateTest, WriteSuccessWithoutQuotaLimitConcurrent) {

PrepareForWrite("test", kBlobURL, 0, std::numeric_limits<int64_t>::max());

// Credate another FileWriterDelegate for concurrent write.
file_writer_delegate2.reset(
CreateWriterDelegate("test2", 0, std::numeric_limits<int64_t>::max()));
// Create another FileWriterDelegate for concurrent write.
file_writer_delegate2 =
CreateWriterDelegate("test2", 0, std::numeric_limits<int64_t>::max());
request2 = empty_context_.CreateRequest(kBlobURL2, net::DEFAULT_PRIORITY,
file_writer_delegate2.get(),
TRAFFIC_ANNOTATION_FOR_TESTS);
Expand Down Expand Up @@ -513,4 +519,53 @@ TEST_F(FileWriterDelegateTest, WritesWithQuotaAndOffset) {
}
}

class InterruptedFileWriterDelegate : public FileWriterDelegate {
public:
InterruptedFileWriterDelegate(
std::unique_ptr<storage::FileStreamWriter> file_writer,
storage::FlushPolicy flush_policy)
: FileWriterDelegate(std::move(file_writer), flush_policy) {}
~InterruptedFileWriterDelegate() override = default;

void OnDataReceived(int bytes_read) override {
// The base class will respond to OnDataReceived by performing an
// asynchronous write. Schedule a task now that will execute before the
// write completes which terminates the URLRequestJob.
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(IgnoreResult(&net::URLRequest::Cancel),
base::Unretained(request_)));
FileWriterDelegate::OnDataReceived(bytes_read);
}

void set_request(net::URLRequest* request) { request_ = request; }

private:
// The request is owned by the base class as a private member.
net::URLRequest* request_ = nullptr;
};

TEST_F(FileWriterDelegateTest, ReadFailureDuringAsyncWrite) {
const GURL kBlobURL("blob:async-fail");
content_ = kData;

auto writer = CreateWriter("test", 0, std::numeric_limits<int64_t>::max());
auto file_writer_delegate = std::make_unique<InterruptedFileWriterDelegate>(
std::move(writer), storage::FlushPolicy::FLUSH_ON_COMPLETION);
auto request = empty_context_.CreateRequest(kBlobURL, net::DEFAULT_PRIORITY,
file_writer_delegate.get(),
TRAFFIC_ANNOTATION_FOR_TESTS);
file_writer_delegate->set_request(request.get());

Result result;
file_writer_delegate->Start(std::move(request), GetWriteCallback(&result));
base::RunLoop().Run();

ASSERT_EQ(FileWriterDelegate::ERROR_WRITE_STARTED, result.write_status());
file_writer_delegate_.reset();

// The write should still have flushed.
ASSERT_EQ(kDataSize, usage());
EXPECT_EQ(GetFileSizeOnDisk("test"), usage());
}

} // namespace content

0 comments on commit ae92808

Please sign in to comment.