Skip to content

GitLab

T
tangled
Commit 86439789 authored by Austin Sullivan's avatar Austin Sullivan Committed by Chromium LUCI CQ
Browse files
Options

[FSA] Destroy FileWriter on Close() or Abort() 

Simplifies the lifetime of file writers. Previously, if the mojo pipe
outlived the usefulness of the writer (i.e. after the writer had been
either closed or aborted), the object would hang around until the pipe
closed.

Now, the writer is destroyed (and the mojo pipe closed) when the writer
is closed or aborted.

Bug: N/A
Change-Id: I69efecba1fe8d6c94f778defa6f0eaf91d088fc6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595905
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: default avatarMarijn Kruisselbrink <mek@chromium.org>
Reviewed-by: default avatarDaniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841513}
parent f83b17aa
Show whitespace changes
Inline Side-by-side
Showing with 76 additions and 172 deletions
content/browser/file_system_access/native_file_system_file_writer_impl.cc
View file @ 86439789
......@@ -160,21 +160,21 @@ NativeFileSystemFileWriterImpl::NativeFileSystemFileWriterImpl(
}
NativeFileSystemFileWriterImpl::~NativeFileSystemFileWriterImpl() {
if (can_purge()) {
DoFileSystemOperation(FROM_HERE, &FileSystemOperationRunner::RemoveFile,
// Purge the swap file. The swap file should be deleted after Close(), but
// we'll try to delete it anyways in case the writer wasn't closed cleanly.
DoFileSystemOperation(
FROM_HERE, &FileSystemOperationRunner::RemoveFile,
base::BindOnce(
[](const storage::FileSystemURL& swap_url,
base::File::Error result) {
if (result != base::File::FILE_OK) {
DLOG(ERROR)
<< "Error Deleting Swap File, status: "
[](const storage::FileSystemURL& swap_url, base::File::Error result) {
if (result != base::File::FILE_OK &&
result != base::File::FILE_ERROR_NOT_FOUND) {
DLOG(ERROR) << "Error Deleting Swap File, status: "
<< base::File::ErrorToString(result)
<< " path: " << swap_url.path();
}
},
swap_url()),
swap_url());
}
}
void NativeFileSystemFileWriterImpl::Write(
......@@ -355,24 +355,25 @@ class BlobReaderClient : public base::SupportsWeakPtr<BlobReaderClient>,
} // namespace
// Do not call this method if |close_callback_| is not set.
void NativeFileSystemFileWriterImpl::CallCloseCallbackAndMaybeDeleteThis(
void NativeFileSystemFileWriterImpl::CallCloseCallbackAndDeleteThis(
blink::mojom::FileSystemAccessErrorPtr result) {
std::move(close_callback_).Run(std::move(result));
if (!receiver_.is_bound()) {
// |this| is deleted after this call.
manager()->RemoveFileWriter(this);
}
}
void NativeFileSystemFileWriterImpl::OnDisconnect() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
receiver_.reset();
if (!close_callback_) {
if (!is_closed() && auto_close_) {
if (is_close_pending())
// Mojo connection lost while Close() in progress.
return;
if (auto_close_) {
// Close the Writer. |this| is deleted via
// CallCloseCallbackAndMaybeDeleteThis when Close finishes.
// CallCloseCallbackAndDeleteThis when Close() finishes.
Close(base::BindOnce([](blink::mojom::FileSystemAccessErrorPtr result) {
if (result->status != blink::mojom::FileSystemAccessStatus::kOk) {
DLOG(ERROR) << "AutoClose failed with result:"
......@@ -381,9 +382,9 @@ void NativeFileSystemFileWriterImpl::OnDisconnect() {
}));
return;
}
// |this| is deleted after this call.
// Mojo connection severed before Close() called. Destroy |this|.
manager()->RemoveFileWriter(this);
}
}
void NativeFileSystemFileWriterImpl::WriteImpl(
......@@ -394,11 +395,11 @@ void NativeFileSystemFileWriterImpl::WriteImpl(
DCHECK_EQ(GetWritePermissionStatus(),
blink::mojom::PermissionStatus::GRANTED);
if (is_closed()) {
if (is_close_pending()) {
std::move(callback).Run(
native_file_system_error::FromStatus(
FileSystemAccessStatus::kInvalidState,
"An attempt was made to write to a closed writer."),
"An attempt was made to write to a closing writer."),
/*bytes_written=*/0);
return;
}
......@@ -443,11 +444,11 @@ void NativeFileSystemFileWriterImpl::WriteStreamImpl(
DCHECK_EQ(GetWritePermissionStatus(),
blink::mojom::PermissionStatus::GRANTED);
if (is_closed()) {
if (is_close_pending()) {
std::move(callback).Run(
native_file_system_error::FromStatus(
FileSystemAccessStatus::kInvalidState,
"An attempt was made to write to a closed writer."),
"An attempt was made to write to a closing writer."),
/*bytes_written=*/0);
return;
}
......@@ -481,10 +482,10 @@ void NativeFileSystemFileWriterImpl::TruncateImpl(uint64_t length,
DCHECK_EQ(GetWritePermissionStatus(),
blink::mojom::PermissionStatus::GRANTED);
if (is_closed()) {
if (is_close_pending()) {
std::move(callback).Run(native_file_system_error::FromStatus(
FileSystemAccessStatus::kInvalidState,
"An attempt was made to write to a closed writer."));
"An attempt was made to write to a closing writer."));
return;
}
......@@ -503,15 +504,14 @@ void NativeFileSystemFileWriterImpl::CloseImpl(CloseCallback callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK_EQ(GetWritePermissionStatus(),
blink::mojom::PermissionStatus::GRANTED);
if (is_closed()) {
if (is_close_pending()) {
std::move(callback).Run(native_file_system_error::FromStatus(
FileSystemAccessStatus::kInvalidState,
"An attempt was made to close an already closed writer."));
"An attempt was made to close an already closing writer."));
return;
}
close_callback_ = std::move(callback);
state_ = State::kClosePending;
if (!RequireSecurityChecks() || !manager()->permission_context()) {
DidAfterWriteCheck(
......@@ -526,17 +526,19 @@ void NativeFileSystemFileWriterImpl::CloseImpl(CloseCallback callback) {
void NativeFileSystemFileWriterImpl::AbortImpl(AbortCallback callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (is_closed()) {
if (is_close_pending()) {
std::move(callback).Run(native_file_system_error::FromStatus(
FileSystemAccessStatus::kInvalidState,
"An attempt was made to abort an already closed writer."));
"An attempt was made to abort an already closing writer."));
return;
}
state_ = State::kClosed;
auto_close_ = false;
std::move(callback).Run(native_file_system_error::Ok());
// |this| is deleted after this call.
manager()->RemoveFileWriter(this);
}
// static
......@@ -551,7 +553,7 @@ void NativeFileSystemFileWriterImpl::DoAfterWriteCheck(
// callback.
manager()->operation_runner().PostTaskWithThisObject(
FROM_HERE, base::BindOnce(&RemoveSwapFile, swap_url()));
CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::FromStatus(
CallCloseCallbackAndDeleteThis(native_file_system_error::FromStatus(
FileSystemAccessStatus::kOperationAborted,
"Failed to perform Safe Browsing check."));
return;
......@@ -579,7 +581,7 @@ void NativeFileSystemFileWriterImpl::DidAfterWriteCheck(
// file and call the callback to report that close failed.
manager()->operation_runner().PostTaskWithThisObject(
FROM_HERE, base::BindOnce(&RemoveSwapFile, swap_url()));
CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::FromStatus(
CallCloseCallbackAndDeleteThis(native_file_system_error::FromStatus(
FileSystemAccessStatus::kOperationAborted,
"Write operation blocked by Safe Browsing."));
return;
......@@ -616,17 +618,15 @@ void NativeFileSystemFileWriterImpl::DidSwapFileSkipQuarantine(
base::File::Error result) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (result != base::File::FILE_OK) {
state_ = State::kCloseError;
DLOG(ERROR) << "Swap file move operation failed source: "
<< swap_url().path() << " dest: " << url().path()
<< " error: " << base::File::ErrorToString(result);
CallCloseCallbackAndMaybeDeleteThis(
CallCloseCallbackAndDeleteThis(
native_file_system_error::FromFileError(result));
return;
}
state_ = State::kClosed;
CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::Ok());
CallCloseCallbackAndDeleteThis(native_file_system_error::Ok());
}
void NativeFileSystemFileWriterImpl::DidSwapFileDoQuarantine(
......@@ -637,10 +637,9 @@ void NativeFileSystemFileWriterImpl::DidSwapFileDoQuarantine(
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (result != base::File::FILE_OK) {
state_ = State::kCloseError;
DLOG(ERROR) << "Swap file move operation failed dest: " << target_url.path()
<< " error: " << base::File::ErrorToString(result);
CallCloseCallbackAndMaybeDeleteThis(
CallCloseCallbackAndDeleteThis(
native_file_system_error::FromFileError(result));
return;
}
......@@ -697,7 +696,6 @@ void NativeFileSystemFileWriterImpl::DidAnnotateFile(
mojo::Remote<quarantine::mojom::Quarantine> quarantine_remote,
quarantine::mojom::QuarantineFileResult result) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
state_ = State::kClosed;
if (result != quarantine::mojom::QuarantineFileResult::OK &&
result != quarantine::mojom::QuarantineFileResult::ANNOTATION_FAILED) {
......@@ -705,13 +703,13 @@ void NativeFileSystemFileWriterImpl::DidAnnotateFile(
// file will be deleted at this point by AttachmentServices on Windows.
// There is nothing to do except to return the error message to the
// application.
CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::FromStatus(
CallCloseCallbackAndDeleteThis(native_file_system_error::FromStatus(
FileSystemAccessStatus::kOperationAborted,
"Write operation aborted due to security policy."));
return;
}
CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::Ok());
CallCloseCallbackAndDeleteThis(native_file_system_error::Ok());
}
void NativeFileSystemFileWriterImpl::ComputeHashForSwapFile(
......
content/browser/file_system_access/native_file_system_file_writer_impl.h
View file @ 86439789
......@@ -66,7 +66,9 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
WriteStreamCallback callback) override;
void Truncate(uint64_t length, TruncateCallback callback) override;
// The writer will be destroyed upon completion.
void Close(CloseCallback callback) override;
// The writer will be destroyed upon completion.
void Abort(AbortCallback callback) override;
using HashCallback = base::OnceCallback<
......@@ -82,10 +84,13 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
mojo::Receiver<blink::mojom::FileSystemAccessFileWriter> receiver_;
// If the mojo pipe is severed before either Close() or Abort() is invoked,
// the transaction is aborted from the OnDisconnect method. Otherwise, the
// writer will be destroyed upon completion of Close() or Abort().
void OnDisconnect();
// Delete the FileWriter after Close if the mojo pipe is unbound.
void CallCloseCallbackAndMaybeDeleteThis(
// Destroys the file writer after calling the close callback.
void CallCloseCallbackAndDeleteThis(
blink::mojom::FileSystemAccessErrorPtr result);
void WriteImpl(uint64_t offset,
......@@ -125,44 +130,13 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
void ComputeHashForSwapFile(HashCallback callback);
enum class State {
// The writer accepts write operations.
kOpen,
// The writer does not accept write operations and is in the process of
// closing.
kClosePending,
// The writer does not accept write operations and has entered an error
// state. A swap file may need to be purged.
kCloseError,
// The writer does not accept write operations. There should be no more swap
// file.
kClosed,
};
bool is_closed() const { return state_ != State::kOpen; }
// Returns whether the File Writer is in a state where any files can be
// deleted. We do not want to delete the files if there are clean-up
// operations in-flight.
bool can_purge() const {
return state_ == State::kOpen || state_ == State::kCloseError;
}
bool is_close_pending() const { return !close_callback_.is_null(); }
// We write using this file URL. When `Close()` is invoked, we
// execute a move operation from the swap URL to the target URL at `url_`. In
// most filesystems, this move operation is atomic.
storage::FileSystemURL swap_url_;
// NativeFileSystemWriter lifetime management has the following cases:
// 1) The mojo pipe is severed before Close() is invoked.
// - Abort the transaction from the OnDisconnect method.
// 2) The mojo pipe is severed before Close() finishes.
// - The Close() call is allowed to finish.
// - The Writer is destroyed immediately afterwards, via the
// CallCloseCallbackAndMaybeDeleteThis method.
// 3) The mojo pipe exists when Close() finishes.
// - The Writer will exist for as long as the mojo pipe is connected.
// - The Writer is destroyed via the OnDisconnect method.
State state_ = State::kOpen;
// This callback is non-null when the State is kClosePending or kCloseError.
CloseCallback close_callback_;
download::QuarantineConnectionCallback quarantine_connection_callback_;
......
content/browser/file_system_access/native_file_system_file_writer_impl_unittest.cc
View file @ 86439789
......@@ -549,7 +549,7 @@ TEST_F(NativeFileSystemFileWriterImplTest, TruncateGrow) {
EXPECT_EQ(std::string("abc\0\0", 5), ReadFile(test_file_url_));
}
TEST_F(NativeFileSystemFileWriterImplTest, CloseAfterCloseNotOK) {
TEST_F(NativeFileSystemFileWriterImplTest, WriterDestroyedAfterClose) {
uint64_t bytes_written;
FileSystemAccessStatus result = WriteSync(0, "abc", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
......@@ -557,49 +557,13 @@ TEST_F(NativeFileSystemFileWriterImplTest, CloseAfterCloseNotOK) {
result = CloseSync();
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
result = CloseSync();
EXPECT_EQ(result, FileSystemAccessStatus::kInvalidState);
}
TEST_F(NativeFileSystemFileWriterImplTest, TruncateAfterCloseNotOK) {
uint64_t bytes_written;
FileSystemAccessStatus result = WriteSync(0, "abc", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
EXPECT_EQ(bytes_written, 3u);
result = CloseSync();
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
result = TruncateSync(0);
EXPECT_EQ(result, FileSystemAccessStatus::kInvalidState);
}
TEST_P(NativeFileSystemFileWriterImplWriteTest, WriteAfterCloseNotOK) {
uint64_t bytes_written;
FileSystemAccessStatus result = WriteSync(0, "abc", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
EXPECT_EQ(bytes_written, 3u);
result = CloseSync();
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
result = WriteSync(0, "bcd", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kInvalidState);
}
TEST_F(NativeFileSystemFileWriterImplTest, AbortAfterCloseNotOK) {
uint64_t bytes_written;
FileSystemAccessStatus result = WriteSync(0, "abc", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
EXPECT_EQ(bytes_written, 3u);
result = CloseSync();
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
result = AbortSync();
EXPECT_EQ(result, FileSystemAccessStatus::kInvalidState);
EXPECT_TRUE(handle_.WasInvalidated());
EXPECT_FALSE(storage::AsyncFileTestHelper::FileExists(
file_system_context_.get(), test_swap_url_,
storage::AsyncFileTestHelper::kDontCheckSize));
}
TEST_F(NativeFileSystemFileWriterImplTest, AbortOK) {
TEST_F(NativeFileSystemFileWriterImplTest, WriterDestroyedAfterAbort) {
uint64_t bytes_written;
FileSystemAccessStatus result = WriteSync(0, "abc", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
......@@ -608,44 +572,10 @@ TEST_F(NativeFileSystemFileWriterImplTest, AbortOK) {
result = AbortSync();
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
EXPECT_EQ("", ReadFile(test_file_url_));
}
TEST_F(NativeFileSystemFileWriterImplTest, TruncateAfterAbortNotOK) {
uint64_t bytes_written;
FileSystemAccessStatus result = WriteSync(0, "abc", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
EXPECT_EQ(bytes_written, 3u);
result = AbortSync();
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
result = TruncateSync(0);
EXPECT_EQ(result, FileSystemAccessStatus::kInvalidState);
}
TEST_P(NativeFileSystemFileWriterImplWriteTest, WriteAfterAbortNotOK) {
uint64_t bytes_written;
FileSystemAccessStatus result = WriteSync(0, "abc", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
EXPECT_EQ(bytes_written, 3u);
result = AbortSync();
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
result = WriteSync(0, "bcd", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kInvalidState);
}
TEST_F(NativeFileSystemFileWriterImplTest, CloseAfterAbortNotOK) {
uint64_t bytes_written;
FileSystemAccessStatus result = WriteSync(0, "abc", &bytes_written);
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
EXPECT_EQ(bytes_written, 3u);
result = AbortSync();
EXPECT_EQ(result, FileSystemAccessStatus::kOk);
result = CloseSync();
EXPECT_EQ(result, FileSystemAccessStatus::kInvalidState);
EXPECT_TRUE(handle_.WasInvalidated());
EXPECT_FALSE(storage::AsyncFileTestHelper::FileExists(
file_system_context_.get(), test_swap_url_,
storage::AsyncFileTestHelper::kDontCheckSize));
}
// TODO(mek): More tests, particularly for error conditions.
......
third_party/blink/public/mojom/file_system_access/file_system_access_file_writer.mojom
View file @ 86439789
......@@ -32,13 +32,15 @@ interface FileSystemAccessFileWriter {
// Closes the file writer. This will materialize the writes operations on the
// intended file target in the case of atomic writes.
// The mojo pipe will be destroyed when Close() completes.
// Specify the |autoClose| flag to ensure Close() is automatically invoked
// when the mojo pipe closes.
// Returns whether the operation succeeded.
Close() => (FileSystemAccessError result);
// Aborts the write operation, resulting in the writes not being committed,
// even if autoClose is specified. All further operations will be rejected.
// even if |autoClose| is specified. The mojo pipe will be destroyed when
// Abort() completes.
// Returns whether the write operation was aborted successfully.
Abort() => (FileSystemAccessError result);
};
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment