diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 37232100acda5..0610aeb22a8af 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -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 diff --git a/patches/chromium/cherry-pick-6bb320d134b1.patch b/patches/chromium/cherry-pick-6bb320d134b1.patch new file mode 100644 index 0000000000000..e4fdcf090cfe9 --- /dev/null +++ b/patches/chromium/cherry-pick-6bb320d134b1.patch @@ -0,0 +1,765 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Victor Costan +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 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 +Reviewed-by: Austin Sullivan +Reviewed-by: Marijn Kruisselbrink +Reviewed-by: enne +Cr-Original-Commit-Position: refs/heads/main@{#940130} +Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3272672 +Bot-Commit: Rubber Stamper +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 { ++ 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 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 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 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 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) +- // 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(std::move(backing_file))), ++ file_state_(base::MakeRefCounted(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(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(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(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 result_buffer_data = +@@ -358,16 +497,10 @@ ScriptPromise NativeIOFile::write(ScriptState* script_state, + DCHECK(buffer->IsDetached()); + + auto* resolver = MakeGarbageCollected(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(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 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 native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr 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 native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr 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 native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr 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 native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr resolver_task_runner, + std::unique_ptr 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 native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr resolver_task_runner, + std::unique_ptr 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 native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr 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 file_state = std::move(file_state_); ++ DCHECK(!file_state_); ++ ++ worker_pool::PostTask(FROM_HERE, {base::MayBlock()}, ++ CrossThreadBindOnce( ++ [](scoped_refptr 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 native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr file_task_runner); + // Performs the post file I/O part of close(), on the main thread. + void DidClose(CrossThreadPersistent resolver); +@@ -93,7 +88,7 @@ class NativeIOFile final : public ScriptWrappable { + static void DoGetLength( + CrossThreadPersistent native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr file_task_runner); + // Performs the post file I/O part of getLength(), on the main thread. + void DidGetLength(CrossThreadPersistent resolver, +@@ -104,7 +99,7 @@ class NativeIOFile final : public ScriptWrappable { + static void DoSetLength( + CrossThreadPersistent native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr 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 native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr file_task_runner, + std::unique_ptr result_buffer_data, + uint64_t file_offset, +@@ -143,7 +138,7 @@ class NativeIOFile final : public ScriptWrappable { + static void DoWrite( + CrossThreadPersistent native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr resolver_task_runner, + std::unique_ptr result_buffer_data, + uint64_t file_offset, +@@ -163,7 +158,7 @@ class NativeIOFile final : public ScriptWrappable { + static void DoFlush( + CrossThreadPersistent native_io_file, + CrossThreadPersistent resolver, +- NativeIOFile::FileState* file_state, ++ scoped_refptr file_state, + scoped_refptr file_task_runner); + // Performs the post file-I/O part of flush(), on the main thread. + void DidFlush(CrossThreadPersistent 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 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 file_state_; + + // Schedules resolving Promises with file I/O results. + const scoped_refptr resolver_task_runner_;