chore: cherry-pick 6bb320d134b1 from chromium (#32009)

* chore: cherry-pick 6bb320d134b1 from chromium

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: Electron Bot <electron@github.com>
This commit is contained in:
Pedro Pontes
2022-01-06 15:47:14 +01:00
committed by GitHub
parent 98cf0e5dff
commit cdf661939d
2 changed files with 766 additions and 0 deletions

View File

@@ -121,6 +121,7 @@ cherry-pick-1a8af2da50e4.patch
cherry-pick-a5f54612590d.patch
cachestorage_store_partial_opaque_responses.patch
fix_aspect_ratio_with_max_size.patch
cherry-pick-6bb320d134b1.patch
cherry-pick-109fde1088be.patch
m96_fileapi_move_origin_checks_in_bloburlstore_sooner.patch
cherry-pick-f781748dcb3c.patch

View File

@@ -0,0 +1,765 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Victor Costan <pwnall@chromium.org>
Date: Thu, 11 Nov 2021 00:22:30 +0000
Subject: M96: Storage Foundation: Share FileState ownership with I/O threads.
blink::NativeIOFile methods implementing the Storage Foundation
JavaScript API pass raw pointers to NativeIOFile::FileState instances to
their corresponding blink::NativeIOFile::Do*() methods, which rely on
that CrossThreadPersistent<NativeIOFile> arguments to keep the
underlying NativeIOFile::FileState instances alive.
CrossThreadPersistent can be used across threads to keep a garbage
collected object alive, together with any non-garbage-collected objects
that it owns. However, relying on CrossThreadPersistent existence to
access the owned objects on a different thread is not safe.
cppgc::subtle::CrossThreadPersistent (blink::CrossThreadPersistent is an
alias to that) has comments explaining that the garbage collected heap
can go away while the CrossThreadPersistent instance exists.
This CL fixes the problem by having the ownership of
NativeIOFile::FileState be shared between the corresponding NativeIOFile
instance and any threads doing I/O on the FileState.
(cherry picked from commit 7dc02206707362f3f92cea93f8eb2fa4af0d375f)
Bug: 1240593
Change-Id: I5c9c818bcb23316fe1fd5afa57ed9c3fdb034377
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3269947
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#940130}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3272672
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4664@{#945}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}
diff --git a/third_party/blink/renderer/modules/native_io/native_io_file.cc b/third_party/blink/renderer/modules/native_io/native_io_file.cc
index 4d5aa4efa13930aea4886bac0fd8ba892ce8b5a5..615c1d3a20cba732a3d981bf6b2df181e56d2727 100644
--- a/third_party/blink/renderer/modules/native_io/native_io_file.cc
+++ b/third_party/blink/renderer/modules/native_io/native_io_file.cc
@@ -9,6 +9,7 @@
#include "base/check.h"
#include "base/files/file.h"
#include "base/location.h"
+#include "base/memory/scoped_refptr.h"
#include "base/numerics/checked_math.h"
#include "base/sequenced_task_runner.h"
#include "base/task/thread_pool.h"
@@ -47,31 +48,167 @@
namespace blink {
-struct NativeIOFile::FileState {
- explicit FileState(base::File file) : file(std::move(file)) {}
+// State and logic for performing file I/O off the JavaScript thread.
+//
+// Instances are allocated on the PartitionAlloc heap. Instances cannot be
+// garbage-collected, because garbage collected heaps get deallocated when the
+// underlying threads are terminated, and we need a guarantee that each
+// instance remains alive while it is used by a thread performing file I/O.
+//
+// Instances are initially constructed on a Blink thread that executes
+// JavaScript, which can be Blink's main thread, or a worker thread. Afterwards,
+// instances are (mostly*) only accessed on dedicated threads that do blocking
+// file I/O.
+//
+// Mostly*: On macOS < 10.15, SetLength() synchronously accesses FileState on
+// the JavaScript thread. This could be fixed with extra thread hopping. We're
+// not currently planning to invest in the fix.
+class NativeIOFile::FileState
+ : public base::RefCountedThreadSafe<NativeIOFile::FileState> {
+ public:
+ explicit FileState(base::File file) : file_(std::move(file)) {
+ DCHECK(file_.IsValid());
+ }
FileState(const FileState&) = delete;
FileState& operator=(const FileState&) = delete;
~FileState() = default;
+ // Returns true until Close() is called. Returns false afterwards.
+ //
+ // On macOS < 10.15, returns false between a TakeFile() call and the
+ // corresponding SetFile() call.
+ bool IsValid() {
+ DCHECK(!IsMainThread());
+
+ WTF::MutexLocker locker(mutex_);
+ return file_.IsValid();
+ }
+
+ void Close() {
+ DCHECK(!IsMainThread());
+
+ WTF::MutexLocker locker(mutex_);
+ DCHECK(file_.IsValid()) << __func__ << " called on invalid file";
+
+ file_.Close();
+ }
+
+ // Returns {length, base::File::FILE_OK} in case of success.
+ // Returns {invalid number, error} in case of failure.
+ std::pair<int64_t, base::File::Error> GetLength() {
+ DCHECK(!IsMainThread());
+
+ WTF::MutexLocker mutex_locker(mutex_);
+ DCHECK(file_.IsValid()) << __func__ << " called on invalid file";
+
+ int64_t length = file_.GetLength();
+ base::File::Error error =
+ (length < 0) ? file_.GetLastFileError() : base::File::FILE_OK;
+
+ return {length, error};
+ }
+
+ // Returns {expected_length, base::File::FILE_OK} in case of success.
+ // Returns {actual file length, error} in case of failure.
+ std::pair<int64_t, base::File::Error> SetLength(int64_t expected_length) {
+ DCHECK(!IsMainThread());
+ DCHECK_GE(expected_length, 0);
+
+ WTF::MutexLocker mutex_locker(mutex_);
+ DCHECK(file_.IsValid()) << __func__ << " called on invalid file";
+
+ bool success = file_.SetLength(expected_length);
+ base::File::Error error =
+ success ? base::File::FILE_OK : file_.GetLastFileError();
+ int64_t actual_length = success ? expected_length : file_.GetLength();
+
+ return {actual_length, error};
+ }
+
+#if defined(OS_MAC)
+ // Used to implement browser-side SetLength() on macOS < 10.15.
+ base::File TakeFile() {
+ WTF::MutexLocker mutex_locker(mutex_);
+ DCHECK(file_.IsValid()) << __func__ << " called on invalid file";
+
+ return std::move(file_);
+ }
+
+ // Used to implement browser-side SetLength() on macOS < 10.15.
+ void SetFile(base::File file) {
+ WTF::MutexLocker locker(mutex_);
+ DCHECK(!file_.IsValid()) << __func__ << " called on valid file";
+
+ file_ = std::move(file);
+ }
+#endif // defined(OS_MAC)
+
+ // Returns {read byte count, base::File::FILE_OK} in case of success.
+ // Returns {invalid number, error} in case of failure.
+ std::pair<int, base::File::Error> Read(NativeIODataBuffer* buffer,
+ int64_t file_offset,
+ int read_size) {
+ DCHECK(!IsMainThread());
+ DCHECK(buffer);
+ DCHECK_GE(file_offset, 0);
+ DCHECK_GE(read_size, 0);
+
+ WTF::MutexLocker mutex_locker(mutex_);
+ DCHECK(file_.IsValid()) << __func__ << " called on invalid file";
+
+ int read_bytes = file_.Read(file_offset, buffer->Data(), read_size);
+ base::File::Error error =
+ (read_bytes < 0) ? file_.GetLastFileError() : base::File::FILE_OK;
+
+ return {read_bytes, error};
+ }
+
+ // Returns {0, write_size, base::File::FILE_OK} in case of success.
+ // Returns {actual file length, written bytes, base::File::OK} in case of a
+ // short write.
+ // Returns {actual file length, invalid number, error} in case of failure.
+ std::tuple<int64_t, int, base::File::Error> Write(NativeIODataBuffer* buffer,
+ int64_t file_offset,
+ int write_size) {
+ DCHECK(!IsMainThread());
+ DCHECK(buffer);
+ DCHECK_GE(file_offset, 0);
+ DCHECK_GE(write_size, 0);
+
+ WTF::MutexLocker mutex_locker(mutex_);
+ DCHECK(file_.IsValid()) << __func__ << " called on invalid file";
+
+ int written_bytes = file_.Write(file_offset, buffer->Data(), write_size);
+ base::File::Error error =
+ (written_bytes < 0) ? file_.GetLastFileError() : base::File::FILE_OK;
+ int64_t actual_file_length_on_failure = 0;
+ if (written_bytes < write_size || error != base::File::FILE_OK) {
+ actual_file_length_on_failure = file_.GetLength();
+ if (actual_file_length_on_failure < 0 && error != base::File::FILE_OK)
+ error = file_.GetLastFileError();
+ }
+
+ return {actual_file_length_on_failure, written_bytes, error};
+ }
+
+ base::File::Error Flush() {
+ DCHECK(!IsMainThread());
+
+ WTF::MutexLocker mutex_locker(mutex_);
+ DCHECK(file_.IsValid()) << __func__ << " called on invalid file";
+
+ bool success = file_.Flush();
+ return success ? base::File::FILE_OK : file_.GetLastFileError();
+ }
+
+ private:
// Lock coordinating cross-thread access to the state.
- WTF::Mutex mutex;
+ WTF::Mutex mutex_;
+
// The file on disk backing this NativeIOFile.
- //
- // The mutex is there to protect us against using the file after it was
- // closed, and against OS-specific behavior around concurrent file access. It
- // should never cause the main (JS) thread to block. This is because the mutex
- // is only taken on the main thread in CloseBackingFile(), which is called
- // when the NativeIOFile is destroyed (which implies there's no pending I/O
- // operation, because all I/O operations hold onto a Persistent<NativeIOFile>)
- // and when the mojo pipe is closed, which currently only happens when the JS
- // context is being torn down.
- //
- // TODO(rstz): Is it possible and worthwhile to remove the mutex and rely
- // exclusively on |NativeIOFile::io_pending_|, or remove
- // |NativeIOFile::io_pending_| in favor of the mutex (might be harder)?
- base::File file GUARDED_BY(mutex);
+ base::File file_ GUARDED_BY(mutex_);
};
NativeIOFile::NativeIOFile(
@@ -81,7 +218,7 @@ NativeIOFile::NativeIOFile(
NativeIOCapacityTracker* capacity_tracker,
ExecutionContext* execution_context)
: file_length_(backing_file_length),
- file_state_(std::make_unique<FileState>(std::move(backing_file))),
+ file_state_(base::MakeRefCounted<FileState>(std::move(backing_file))),
// TODO(pwnall): Get a dedicated queue when the specification matures.
resolver_task_runner_(
execution_context->GetTaskRunner(TaskType::kMiscPlatformAPI)),
@@ -94,7 +231,7 @@ NativeIOFile::NativeIOFile(
}
NativeIOFile::~NativeIOFile() {
- // Needed to avoid having the base::File destructor close the file descriptor
+ // Needed to avoid having the FileState destructor close the file descriptor
// synchronously on the main thread.
CloseBackingFile();
}
@@ -114,6 +251,9 @@ ScriptPromise NativeIOFile::close(ScriptState* script_state) {
queued_close_resolver_ = resolver;
if (!io_pending_) {
+ DCHECK(file_state_)
+ << "file_state_ nulled out without setting closed_ or io_pending_";
+
// Pretend that a close() promise was queued behind an I/O operation, and
// the operation just finished. This is less logic than handling the
// non-queued case separately.
@@ -138,18 +278,15 @@ ScriptPromise NativeIOFile::getLength(ScriptState* script_state,
"The file was already closed"));
return ScriptPromise();
}
- io_pending_ = true;
+ DCHECK(file_state_)
+ << "file_state_ nulled out without setting closed_ or io_pending_";
+ io_pending_ = true;
auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
- // CrossThreadUnretained() is safe here because the NativeIOFile::FileState
- // instance is owned by this NativeIOFile, which is also passed to the task
- // via WrapCrossThreadPersistent. Therefore, the FileState instance is
- // guaranteed to remain alive during the task's execution.
worker_pool::PostTask(
FROM_HERE, {base::MayBlock()},
CrossThreadBindOnce(&DoGetLength, WrapCrossThreadPersistent(this),
- WrapCrossThreadPersistent(resolver),
- CrossThreadUnretained(file_state_.get()),
+ WrapCrossThreadPersistent(resolver), file_state_,
resolver_task_runner_));
return resolver->Promise();
}
@@ -175,6 +312,9 @@ ScriptPromise NativeIOFile::setLength(ScriptState* script_state,
"The file was already closed"));
return ScriptPromise();
}
+ DCHECK(file_state_)
+ << "file_state_ nulled out without setting closed_ or io_pending_";
+
int64_t expected_length = base::as_signed(new_length);
DCHECK_GE(expected_length, 0);
@@ -201,8 +341,8 @@ ScriptPromise NativeIOFile::setLength(ScriptState* script_state,
}
file_length_ = expected_length;
}
- io_pending_ = true;
+ io_pending_ = true;
auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
#if defined(OS_MAC)
@@ -217,26 +357,19 @@ ScriptPromise NativeIOFile::setLength(ScriptState* script_state,
// To preserve this invariant, we pass this file's handle to the browser
// process during the SetLength() mojo call, and the browser passes it back
// when the call completes.
- {
- WTF::MutexLocker locker(file_state_->mutex);
- backend_file_->SetLength(
- expected_length, std::move(file_state_->file),
- WTF::Bind(&NativeIOFile::DidSetLengthIpc, WrapPersistent(this),
- WrapPersistent(resolver)));
- }
+ base::File file = file_state_->TakeFile();
+ backend_file_->SetLength(
+ expected_length, std::move(file),
+ WTF::Bind(&NativeIOFile::DidSetLengthIpc, WrapPersistent(this),
+ WrapPersistent(resolver)));
return resolver->Promise();
}
#endif // defined(OS_MAC)
- // CrossThreadUnretained() is safe here because the NativeIOFile::FileState
- // instance is owned by this NativeIOFile, which is also passed to the task
- // via WrapCrossThreadPersistent. Therefore, the FileState instance is
- // guaranteed to remain alive during the task's execution.
worker_pool::PostTask(
FROM_HERE, {base::MayBlock()},
CrossThreadBindOnce(&DoSetLength, WrapCrossThreadPersistent(this),
- WrapCrossThreadPersistent(resolver),
- CrossThreadUnretained(file_state_.get()),
+ WrapCrossThreadPersistent(resolver), file_state_,
resolver_task_runner_, expected_length));
return resolver->Promise();
}
@@ -258,6 +391,8 @@ ScriptPromise NativeIOFile::read(ScriptState* script_state,
"The file was already closed"));
return ScriptPromise();
}
+ DCHECK(file_state_)
+ << "file_state_ nulled out without setting closed_ or io_pending_";
// TODO(pwnall): This assignment should move right before the
// worker_pool::PostTask() call.
@@ -281,16 +416,10 @@ ScriptPromise NativeIOFile::read(ScriptState* script_state,
DCHECK(buffer->IsDetached());
auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
- // The first CrossThreadUnretained() is safe here because the
- // NativeIOFile::FileState instance is owned by this NativeIOFile, which is
- // also passed to the task via WrapCrossThreadPersistent. Therefore, the
- // FileState instance is guaranteed to remain alive during the task's
- // execution.
worker_pool::PostTask(
FROM_HERE, {base::MayBlock()},
CrossThreadBindOnce(&DoRead, WrapCrossThreadPersistent(this),
- WrapCrossThreadPersistent(resolver),
- CrossThreadUnretained(file_state_.get()),
+ WrapCrossThreadPersistent(resolver), file_state_,
resolver_task_runner_, std::move(result_buffer_data),
file_offset, read_size));
return resolver->Promise();
@@ -313,6 +442,8 @@ ScriptPromise NativeIOFile::write(ScriptState* script_state,
"The file was already closed"));
return ScriptPromise();
}
+ DCHECK(file_state_)
+ << "file_state_ nulled out without setting closed_ or io_pending_";
int write_size = NativeIOOperationSize(*buffer);
int64_t write_end_offset;
@@ -346,6 +477,14 @@ ScriptPromise NativeIOFile::write(ScriptState* script_state,
file_length_ = write_end_offset;
}
+ // TODO(pwnall): This assignment should move right before the
+ // worker_pool::PostTask() call.
+ //
+ // `io_pending_` should only be set to true when we know for sure we'll post a
+ // task that eventually results in getting `io_pending_` set back to false.
+ // Having `io_pending_` set to true in an early return case (rejecting with an
+ // exception) leaves the NativeIOFile "stuck" in a state where all future I/O
+ // method calls will reject.
io_pending_ = true;
std::unique_ptr<NativeIODataBuffer> result_buffer_data =
@@ -358,16 +497,10 @@ ScriptPromise NativeIOFile::write(ScriptState* script_state,
DCHECK(buffer->IsDetached());
auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
- // The first CrossThreadUnretained() is safe here because the
- // NativeIOFile::FileState instance is owned by this NativeIOFile, which is
- // also passed to the task via WrapCrossThreadPersistent. Therefore, the
- // FileState instance is guaranteed to remain alive during the task's
- // execution.
worker_pool::PostTask(
FROM_HERE, {base::MayBlock()},
CrossThreadBindOnce(&DoWrite, WrapCrossThreadPersistent(this),
- WrapCrossThreadPersistent(resolver),
- CrossThreadUnretained(file_state_.get()),
+ WrapCrossThreadPersistent(resolver), file_state_,
resolver_task_runner_, std::move(result_buffer_data),
file_offset, write_size));
return resolver->Promise();
@@ -391,18 +524,15 @@ ScriptPromise NativeIOFile::flush(ScriptState* script_state,
"The file was already closed"));
return ScriptPromise();
}
- io_pending_ = true;
+ DCHECK(file_state_)
+ << "file_state_ nulled out without setting closed_ or io_pending_";
+ io_pending_ = true;
auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
- // CrossThreadUnretained() is safe here because the NativeIOFile::FileState
- // instance is owned by this NativeIOFile, which is also passed to the task
- // via WrapCrossThreadPersistent. Therefore, the FileState instance is
- // guaranteed to remain alive during the task's execution.
worker_pool::PostTask(
FROM_HERE, {base::MayBlock()},
CrossThreadBindOnce(&DoFlush, WrapCrossThreadPersistent(this),
- WrapCrossThreadPersistent(resolver),
- CrossThreadUnretained(file_state_.get()),
+ WrapCrossThreadPersistent(resolver), file_state_,
resolver_task_runner_));
return resolver->Promise();
}
@@ -430,28 +560,29 @@ void NativeIOFile::DispatchQueuedClose() {
ScriptPromiseResolver* resolver = queued_close_resolver_;
queued_close_resolver_ = nullptr;
+ scoped_refptr<FileState> file_state = std::move(file_state_);
+ DCHECK(!file_state_);
+
worker_pool::PostTask(
FROM_HERE, {base::MayBlock()},
CrossThreadBindOnce(&DoClose, WrapCrossThreadPersistent(this),
WrapCrossThreadPersistent(resolver),
- CrossThreadUnretained(file_state_.get()),
- resolver_task_runner_));
+ std::move(file_state), resolver_task_runner_));
}
// static
void NativeIOFile::DoClose(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> resolver_task_runner) {
DCHECK(!IsMainThread()) << "File I/O should not happen on the main thread";
+ DCHECK(file_state);
+ DCHECK(file_state->IsValid())
+ << "File I/O operation queued after file closed";
+ DCHECK(resolver_task_runner);
- {
- WTF::MutexLocker locker(file_state->mutex);
- DCHECK(file_state->file.IsValid())
- << "file I/O operation queued after file closed";
- file_state->file.Close();
- }
+ file_state->Close();
PostCrossThreadTask(
*resolver_task_runner, FROM_HERE,
@@ -482,19 +613,17 @@ void NativeIOFile::DidClose(
void NativeIOFile::DoGetLength(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> resolver_task_runner) {
DCHECK(!IsMainThread()) << "File I/O should not happen on the main thread";
+ DCHECK(file_state);
+ DCHECK(file_state->IsValid())
+ << "File I/O operation queued after file closed";
+ DCHECK(resolver_task_runner);
+
+ int64_t length;
base::File::Error get_length_error;
- int64_t length = -1;
- {
- WTF::MutexLocker mutex_locker(file_state->mutex);
- DCHECK(file_state->file.IsValid())
- << "file I/O operation queued after file closed";
- length = file_state->file.GetLength();
- get_length_error = (length < 0) ? file_state->file.GetLastFileError()
- : base::File::FILE_OK;
- }
+ std::tie(length, get_length_error) = file_state->GetLength();
PostCrossThreadTask(
*resolver_task_runner, FROM_HERE,
@@ -541,22 +670,20 @@ void NativeIOFile::DidGetLength(
void NativeIOFile::DoSetLength(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> resolver_task_runner,
int64_t expected_length) {
DCHECK(!IsMainThread()) << "File I/O should not happen on the main thread";
+ DCHECK(file_state);
+ DCHECK(file_state->IsValid())
+ << "File I/O operation queued after file closed";
+ DCHECK(resolver_task_runner);
+ DCHECK_GE(expected_length, 0);
- base::File::Error set_length_error;
int64_t actual_length;
- {
- WTF::MutexLocker mutex_locker(file_state->mutex);
- DCHECK(file_state->file.IsValid())
- << "file I/O operation queued after file closed";
- bool success = file_state->file.SetLength(expected_length);
- set_length_error =
- success ? base::File::FILE_OK : file_state->file.GetLastFileError();
- actual_length = success ? expected_length : file_state->file.GetLength();
- }
+ base::File::Error set_length_error;
+ std::tie(actual_length, set_length_error) =
+ file_state->SetLength(expected_length);
PostCrossThreadTask(
*resolver_task_runner, FROM_HERE,
@@ -619,10 +746,7 @@ void NativeIOFile::DidSetLengthIpc(
int64_t actual_length,
mojom::blink::NativeIOErrorPtr set_length_error) {
DCHECK(backing_file.IsValid()) << "browser returned closed file";
- {
- WTF::MutexLocker locker(file_state_->mutex);
- file_state_->file = std::move(backing_file);
- }
+ file_state_->SetFile(std::move(backing_file));
ScriptState* script_state = resolver->GetScriptState();
DCHECK(io_pending_) << "I/O operation performed without io_pending_ set";
@@ -673,13 +797,15 @@ void NativeIOFile::DidSetLengthIpc(
void NativeIOFile::DoRead(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> resolver_task_runner,
std::unique_ptr<NativeIODataBuffer> result_buffer_data,
uint64_t file_offset,
int read_size) {
DCHECK(!IsMainThread()) << "File I/O should not happen on the main thread";
-
+ DCHECK(file_state);
+ DCHECK(file_state->IsValid())
+ << "File I/O operation queued after file closed";
DCHECK(resolver_task_runner);
DCHECK(result_buffer_data);
DCHECK(result_buffer_data->IsValid());
@@ -690,15 +816,8 @@ void NativeIOFile::DoRead(
int read_bytes;
base::File::Error read_error;
- {
- WTF::MutexLocker mutex_locker(file_state->mutex);
- DCHECK(file_state->file.IsValid())
- << "file I/O operation queued after file closed";
- read_bytes = file_state->file.Read(file_offset, result_buffer_data->Data(),
- read_size);
- read_error = (read_bytes < 0) ? file_state->file.GetLastFileError()
- : base::File::FILE_OK;
- }
+ std::tie(read_bytes, read_error) =
+ file_state->Read(result_buffer_data.get(), file_offset, read_size);
PostCrossThreadTask(
*resolver_task_runner, FROM_HERE,
@@ -743,12 +862,15 @@ void NativeIOFile::DidRead(
void NativeIOFile::DoWrite(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> resolver_task_runner,
std::unique_ptr<NativeIODataBuffer> result_buffer_data,
uint64_t file_offset,
int write_size) {
DCHECK(!IsMainThread()) << "File I/O should not happen on the main thread";
+ DCHECK(file_state);
+ DCHECK(file_state->IsValid())
+ << "File I/O operation queued after file closed";
DCHECK(resolver_task_runner);
DCHECK(result_buffer_data);
DCHECK(result_buffer_data->IsValid());
@@ -760,22 +882,8 @@ void NativeIOFile::DoWrite(
int written_bytes;
int64_t actual_file_length_on_failure = 0;
base::File::Error write_error;
- {
- WTF::MutexLocker mutex_locker(file_state->mutex);
- DCHECK(file_state->file.IsValid())
- << "file I/O operation queued after file closed";
- written_bytes = file_state->file.Write(
- file_offset, result_buffer_data->Data(), write_size);
- write_error = (written_bytes < 0) ? file_state->file.GetLastFileError()
- : base::File::FILE_OK;
- if (written_bytes < write_size || write_error != base::File::FILE_OK) {
- actual_file_length_on_failure = file_state->file.GetLength();
- if (actual_file_length_on_failure < 0 &&
- write_error != base::File::FILE_OK) {
- write_error = file_state->file.GetLastFileError();
- }
- }
- }
+ std::tie(actual_file_length_on_failure, written_bytes, write_error) =
+ file_state->Write(result_buffer_data.get(), file_offset, write_size);
PostCrossThreadTask(
*resolver_task_runner, FROM_HERE,
@@ -846,18 +954,14 @@ void NativeIOFile::DidWrite(
void NativeIOFile::DoFlush(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> resolver_task_runner) {
DCHECK(!IsMainThread()) << "File I/O should not happen on the main thread";
- base::File::Error flush_error;
- {
- WTF::MutexLocker mutex_locker(file_state->mutex);
- DCHECK(file_state->file.IsValid())
- << "file I/O operation queued after file closed";
- bool success = file_state->file.Flush();
- flush_error =
- success ? base::File::FILE_OK : file_state->file.GetLastFileError();
- }
+ DCHECK(file_state);
+ DCHECK(file_state->IsValid())
+ << "File I/O operation queued after file closed";
+
+ base::File::Error flush_error = file_state->Flush();
PostCrossThreadTask(
*resolver_task_runner, FROM_HERE,
@@ -887,20 +991,23 @@ void NativeIOFile::DidFlush(
void NativeIOFile::CloseBackingFile() {
closed_ = true;
- file_state_->mutex.lock();
- base::File backing_file = std::move(file_state_->file);
- file_state_->mutex.unlock();
- if (!backing_file.IsValid()) {
+ if (!file_state_) {
// Avoid posting a cross-thread task if the file is already closed. This is
// the expected path.
return;
}
- worker_pool::PostTask(
- FROM_HERE, {base::MayBlock()},
- CrossThreadBindOnce([](base::File file) { file.Close(); },
- std::move(backing_file)));
+ scoped_refptr<FileState> file_state = std::move(file_state_);
+ DCHECK(!file_state_);
+
+ worker_pool::PostTask(FROM_HERE, {base::MayBlock()},
+ CrossThreadBindOnce(
+ [](scoped_refptr<FileState> file_state) {
+ DCHECK(file_state);
+ file_state->Close();
+ },
+ std::move(file_state)));
}
} // namespace blink
diff --git a/third_party/blink/renderer/modules/native_io/native_io_file.h b/third_party/blink/renderer/modules/native_io/native_io_file.h
index 8ae49ebc2d36d547d152d4e56192e30f8cacd641..95d2da4ac3e1859a0abc21ea15d269758eed1681 100644
--- a/third_party/blink/renderer/modules/native_io/native_io_file.h
+++ b/third_party/blink/renderer/modules/native_io/native_io_file.h
@@ -67,12 +67,7 @@ class NativeIOFile final : public ScriptWrappable {
void Trace(Visitor* visitor) const override;
private:
- // Data accessed on the threads that do file I/O.
- //
- // Instances are allocated on the PartitionAlloc heap. Instances are initially
- // constructed on Blink's main thread, or on a worker thread. Afterwards,
- // instances are only accessed on dedicated threads that do blocking file I/O.
- struct FileState;
+ class FileState;
// Called when the mojo backend disconnects.
void OnBackendDisconnect();
@@ -84,7 +79,7 @@ class NativeIOFile final : public ScriptWrappable {
static void DoClose(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> file_task_runner);
// Performs the post file I/O part of close(), on the main thread.
void DidClose(CrossThreadPersistent<ScriptPromiseResolver> resolver);
@@ -93,7 +88,7 @@ class NativeIOFile final : public ScriptWrappable {
static void DoGetLength(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> file_task_runner);
// Performs the post file I/O part of getLength(), on the main thread.
void DidGetLength(CrossThreadPersistent<ScriptPromiseResolver> resolver,
@@ -104,7 +99,7 @@ class NativeIOFile final : public ScriptWrappable {
static void DoSetLength(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> file_task_runner,
int64_t expected_length);
// Performs the post file I/O part of setLength(), on the main thread.
@@ -128,7 +123,7 @@ class NativeIOFile final : public ScriptWrappable {
// Performs the file I/O part of read(), off the main thread.
static void DoRead(CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> file_task_runner,
std::unique_ptr<NativeIODataBuffer> result_buffer_data,
uint64_t file_offset,
@@ -143,7 +138,7 @@ class NativeIOFile final : public ScriptWrappable {
static void DoWrite(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> resolver_task_runner,
std::unique_ptr<NativeIODataBuffer> result_buffer_data,
uint64_t file_offset,
@@ -163,7 +158,7 @@ class NativeIOFile final : public ScriptWrappable {
static void DoFlush(
CrossThreadPersistent<NativeIOFile> native_io_file,
CrossThreadPersistent<ScriptPromiseResolver> resolver,
- NativeIOFile::FileState* file_state,
+ scoped_refptr<NativeIOFile::FileState> file_state,
scoped_refptr<base::SequencedTaskRunner> file_task_runner);
// Performs the post file-I/O part of flush(), on the main thread.
void DidFlush(CrossThreadPersistent<ScriptPromiseResolver> resolver,
@@ -210,8 +205,13 @@ class NativeIOFile final : public ScriptWrappable {
// TODO(rstz): Consider moving this variable into `file_state_`
int64_t file_length_ = 0;
- // See NativeIOFile::FileState, declared above.
- const std::unique_ptr<FileState> file_state_;
+ // Points to a NativeIOFile::FileState while the underlying file is open.
+ //
+ // When the underlying file is closed, this pointer is nulled out, and the
+ // FileState instance is passed to a different thread, where the closing
+ // happens. This avoids having any I/O performed by the base::File::Close()
+ // jank the JavaScript thread that owns this NativeIOFile instance.
+ scoped_refptr<FileState> file_state_;
// Schedules resolving Promises with file I/O results.
const scoped_refptr<base::SequencedTaskRunner> resolver_task_runner_;