diff options
Diffstat (limited to 'base/files')
33 files changed, 628 insertions, 1458 deletions
diff --git a/base/files/dir_reader_posix_unittest.cc b/base/files/dir_reader_posix_unittest.cc index 5d7fd8b139..a75858feeb 100644 --- a/base/files/dir_reader_posix_unittest.cc +++ b/base/files/dir_reader_posix_unittest.cc @@ -29,7 +29,7 @@ TEST(DirReaderPosixUnittest, Read) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - const char* dir = temp_dir.GetPath().value().c_str(); + const char* dir = temp_dir.path().value().c_str(); ASSERT_TRUE(dir); const int prev_wd = open(".", O_RDONLY | O_DIRECTORY); diff --git a/base/files/file.cc b/base/files/file.cc index 1b2224e323..ab05630062 100644 --- a/base/files/file.cc +++ b/base/files/file.cc @@ -138,4 +138,12 @@ std::string File::ErrorToString(Error error) { return ""; } +bool File::Flush() { + ElapsedTimer timer; + SCOPED_FILE_TRACE("Flush"); + bool return_value = DoFlush(); + UMA_HISTOGRAM_TIMES("PlatformFile.FlushTime", timer.Elapsed()); + return return_value; +} + } // namespace base diff --git a/base/files/file.h b/base/files/file.h index 94a9d5cf49..ae2bd1b61b 100644 --- a/base/files/file.h +++ b/base/files/file.h @@ -63,31 +63,28 @@ class BASE_EXPORT File { // FLAG_EXCLUSIVE_(READ|WRITE) only grant exclusive access to the file on // creation on POSIX; for existing files, consider using Lock(). enum Flags { - FLAG_OPEN = 1 << 0, // Opens a file, only if it exists. - FLAG_CREATE = 1 << 1, // Creates a new file, only if it does not - // already exist. - FLAG_OPEN_ALWAYS = 1 << 2, // May create a new file. - FLAG_CREATE_ALWAYS = 1 << 3, // May overwrite an old file. - FLAG_OPEN_TRUNCATED = 1 << 4, // Opens a file and truncates it, only if it - // exists. + FLAG_OPEN = 1 << 0, // Opens a file, only if it exists. + FLAG_CREATE = 1 << 1, // Creates a new file, only if it does not + // already exist. + FLAG_OPEN_ALWAYS = 1 << 2, // May create a new file. + FLAG_CREATE_ALWAYS = 1 << 3, // May overwrite an old file. + FLAG_OPEN_TRUNCATED = 1 << 4, // Opens a file and truncates it, only if it + // exists. FLAG_READ = 1 << 5, FLAG_WRITE = 1 << 6, FLAG_APPEND = 1 << 7, - FLAG_EXCLUSIVE_READ = 1 << 8, // EXCLUSIVE is opposite of Windows SHARE. + FLAG_EXCLUSIVE_READ = 1 << 8, // EXCLUSIVE is opposite of Windows SHARE. FLAG_EXCLUSIVE_WRITE = 1 << 9, FLAG_ASYNC = 1 << 10, - FLAG_TEMPORARY = 1 << 11, // Used on Windows only. - FLAG_HIDDEN = 1 << 12, // Used on Windows only. + FLAG_TEMPORARY = 1 << 11, // Used on Windows only. + FLAG_HIDDEN = 1 << 12, // Used on Windows only. FLAG_DELETE_ON_CLOSE = 1 << 13, - FLAG_WRITE_ATTRIBUTES = 1 << 14, // Used on Windows only. - FLAG_SHARE_DELETE = 1 << 15, // Used on Windows only. - FLAG_TERMINAL_DEVICE = 1 << 16, // Serial port flags. - FLAG_BACKUP_SEMANTICS = 1 << 17, // Used on Windows only. - FLAG_EXECUTE = 1 << 18, // Used on Windows only. - FLAG_SEQUENTIAL_SCAN = 1 << 19, // Used on Windows only. - FLAG_CAN_DELETE_ON_CLOSE = 1 << 20, // Requests permission to delete a file - // via DeleteOnClose() (Windows only). - // See DeleteOnClose() for details. + FLAG_WRITE_ATTRIBUTES = 1 << 14, // Used on Windows only. + FLAG_SHARE_DELETE = 1 << 15, // Used on Windows only. + FLAG_TERMINAL_DEVICE = 1 << 16, // Serial port flags. + FLAG_BACKUP_SEMANTICS = 1 << 17, // Used on Windows only. + FLAG_EXECUTE = 1 << 18, // Used on Windows only. + FLAG_SEQUENTIAL_SCAN = 1 << 19, // Used on Windows only. }; // This enum has been recorded in multiple histograms. If the order of the @@ -293,41 +290,11 @@ class BASE_EXPORT File { // object that was created or initialized with this flag will have unlinked // the underlying file when it was created or opened. On Windows, the // underlying file is deleted when the last handle to it is closed. - File Duplicate() const; + File Duplicate(); bool async() const { return async_; } #if defined(OS_WIN) - // Sets or clears the DeleteFile disposition on the handle. Returns true if - // the disposition was set or cleared, as indicated by |delete_on_close|. - // - // Microsoft Windows deletes a file only when the last handle to the - // underlying kernel object is closed when the DeleteFile disposition has been - // set by any handle holder. This disposition is be set by: - // - Calling the Win32 DeleteFile function with the path to a file. - // - Opening/creating a file with FLAG_DELETE_ON_CLOSE. - // - Opening/creating a file with FLAG_CAN_DELETE_ON_CLOSE and subsequently - // calling DeleteOnClose(true). - // - // In all cases, all pre-existing handles to the file must have been opened - // with FLAG_SHARE_DELETE. - // - // So: - // - Use FLAG_SHARE_DELETE when creating/opening a file to allow another - // entity on the system to cause it to be deleted when it is closed. (Note: - // another entity can delete the file the moment after it is closed, so not - // using this permission doesn't provide any protections.) - // - Use FLAG_DELETE_ON_CLOSE for any file that is to be deleted after use. - // The OS will ensure it is deleted even in the face of process termination. - // - Use FLAG_CAN_DELETE_ON_CLOSE in conjunction with DeleteOnClose() to alter - // the DeleteFile disposition on an open handle. This fine-grained control - // allows for marking a file for deletion during processing so that it is - // deleted in the event of untimely process termination, and then clearing - // this state once the file is suitable for persistence. - bool DeleteOnClose(bool delete_on_close); -#endif - -#if defined(OS_WIN) static Error OSErrorToFileError(DWORD last_error); #elif defined(OS_POSIX) static Error OSErrorToFileError(int saved_errno); @@ -343,6 +310,10 @@ class BASE_EXPORT File { // traversal ('..') components. void DoInitialize(const FilePath& path, uint32_t flags); + // TODO(tnagel): Reintegrate into Flush() once histogram isn't needed anymore, + // cf. issue 473337. + bool DoFlush(); + void SetPlatformFile(PlatformFile file); #if defined(OS_WIN) diff --git a/base/files/file_descriptor_watcher_posix.cc b/base/files/file_descriptor_watcher_posix.cc deleted file mode 100644 index 9746e35ea7..0000000000 --- a/base/files/file_descriptor_watcher_posix.cc +++ /dev/null @@ -1,210 +0,0 @@ -// Copyright 2016 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "base/files/file_descriptor_watcher_posix.h" - -#include "base/bind.h" -#include "base/lazy_instance.h" -#include "base/logging.h" -#include "base/memory/ptr_util.h" -#include "base/sequenced_task_runner.h" -#include "base/single_thread_task_runner.h" -#include "base/threading/sequenced_task_runner_handle.h" -#include "base/threading/thread_checker.h" -#include "base/threading/thread_local.h" - -namespace base { - -namespace { - -// MessageLoopForIO used to watch file descriptors for which callbacks are -// registered from a given thread. -LazyInstance<ThreadLocalPointer<MessageLoopForIO>>::Leaky - tls_message_loop_for_io = LAZY_INSTANCE_INITIALIZER; - -} // namespace - -FileDescriptorWatcher::Controller::~Controller() { - DCHECK(sequence_checker_.CalledOnValidSequence()); - - // Delete |watcher_| on the MessageLoopForIO. - // - // If the MessageLoopForIO is deleted before Watcher::StartWatching() runs, - // |watcher_| is leaked. If the MessageLoopForIO is deleted after - // Watcher::StartWatching() runs but before the DeleteSoon task runs, - // |watcher_| is deleted from Watcher::WillDestroyCurrentMessageLoop(). - message_loop_for_io_task_runner_->DeleteSoon(FROM_HERE, watcher_.release()); - - // Since WeakPtrs are invalidated by the destructor, RunCallback() won't be - // invoked after this returns. -} - -class FileDescriptorWatcher::Controller::Watcher - : public MessageLoopForIO::Watcher, - public MessageLoop::DestructionObserver { - public: - Watcher(WeakPtr<Controller> controller, MessageLoopForIO::Mode mode, int fd); - ~Watcher() override; - - void StartWatching(); - - private: - friend class FileDescriptorWatcher; - - // MessageLoopForIO::Watcher: - void OnFileCanReadWithoutBlocking(int fd) override; - void OnFileCanWriteWithoutBlocking(int fd) override; - - // MessageLoop::DestructionObserver: - void WillDestroyCurrentMessageLoop() override; - - // Used to instruct the MessageLoopForIO to stop watching the file descriptor. - MessageLoopForIO::FileDescriptorWatcher file_descriptor_watcher_; - - // Runs tasks on the sequence on which this was instantiated (i.e. the - // sequence on which the callback must run). - const scoped_refptr<SequencedTaskRunner> callback_task_runner_ = - SequencedTaskRunnerHandle::Get(); - - // The Controller that created this Watcher. - WeakPtr<Controller> controller_; - - // Whether this Watcher is notified when |fd_| becomes readable or writable - // without blocking. - const MessageLoopForIO::Mode mode_; - - // The watched file descriptor. - const int fd_; - - // Except for the constructor, every method of this class must run on the same - // MessageLoopForIO thread. - ThreadChecker thread_checker_; - - // Whether this Watcher was registered as a DestructionObserver on the - // MessageLoopForIO thread. - bool registered_as_destruction_observer_ = false; - - DISALLOW_COPY_AND_ASSIGN(Watcher); -}; - -FileDescriptorWatcher::Controller::Watcher::Watcher( - WeakPtr<Controller> controller, - MessageLoopForIO::Mode mode, - int fd) - : file_descriptor_watcher_(FROM_HERE), - controller_(controller), - mode_(mode), - fd_(fd) { - DCHECK(callback_task_runner_); - thread_checker_.DetachFromThread(); -} - -FileDescriptorWatcher::Controller::Watcher::~Watcher() { - DCHECK(thread_checker_.CalledOnValidThread()); - MessageLoopForIO::current()->RemoveDestructionObserver(this); -} - -void FileDescriptorWatcher::Controller::Watcher::StartWatching() { - DCHECK(thread_checker_.CalledOnValidThread()); - - MessageLoopForIO::current()->WatchFileDescriptor( - fd_, false, mode_, &file_descriptor_watcher_, this); - - if (!registered_as_destruction_observer_) { - MessageLoopForIO::current()->AddDestructionObserver(this); - registered_as_destruction_observer_ = true; - } -} - -void FileDescriptorWatcher::Controller::Watcher::OnFileCanReadWithoutBlocking( - int fd) { - DCHECK_EQ(fd_, fd); - DCHECK_EQ(MessageLoopForIO::WATCH_READ, mode_); - DCHECK(thread_checker_.CalledOnValidThread()); - - // Run the callback on the sequence on which the watch was initiated. - callback_task_runner_->PostTask(FROM_HERE, - Bind(&Controller::RunCallback, controller_)); -} - -void FileDescriptorWatcher::Controller::Watcher::OnFileCanWriteWithoutBlocking( - int fd) { - DCHECK_EQ(fd_, fd); - DCHECK_EQ(MessageLoopForIO::WATCH_WRITE, mode_); - DCHECK(thread_checker_.CalledOnValidThread()); - - // Run the callback on the sequence on which the watch was initiated. - callback_task_runner_->PostTask(FROM_HERE, - Bind(&Controller::RunCallback, controller_)); -} - -void FileDescriptorWatcher::Controller::Watcher:: - WillDestroyCurrentMessageLoop() { - DCHECK(thread_checker_.CalledOnValidThread()); - - // A Watcher is owned by a Controller. When the Controller is deleted, it - // transfers ownership of the Watcher to a delete task posted to the - // MessageLoopForIO. If the MessageLoopForIO is deleted before the delete task - // runs, the following line takes care of deleting the Watcher. - delete this; -} - -FileDescriptorWatcher::Controller::Controller(MessageLoopForIO::Mode mode, - int fd, - const Closure& callback) - : callback_(callback), - message_loop_for_io_task_runner_( - tls_message_loop_for_io.Get().Get()->task_runner()), - weak_factory_(this) { - DCHECK(!callback_.is_null()); - DCHECK(message_loop_for_io_task_runner_); - watcher_ = MakeUnique<Watcher>(weak_factory_.GetWeakPtr(), mode, fd); - StartWatching(); -} - -void FileDescriptorWatcher::Controller::StartWatching() { - DCHECK(sequence_checker_.CalledOnValidSequence()); - // It is safe to use Unretained() below because |watcher_| can only be deleted - // by a delete task posted to |message_loop_for_io_task_runner_| by this - // Controller's destructor. Since this delete task hasn't been posted yet, it - // can't run before the task posted below. - message_loop_for_io_task_runner_->PostTask( - FROM_HERE, Bind(&Watcher::StartWatching, Unretained(watcher_.get()))); -} - -void FileDescriptorWatcher::Controller::RunCallback() { - DCHECK(sequence_checker_.CalledOnValidSequence()); - - WeakPtr<Controller> weak_this = weak_factory_.GetWeakPtr(); - - callback_.Run(); - - // If |this| wasn't deleted, re-enable the watch. - if (weak_this) - StartWatching(); -} - -FileDescriptorWatcher::FileDescriptorWatcher( - MessageLoopForIO* message_loop_for_io) { - DCHECK(message_loop_for_io); - DCHECK(!tls_message_loop_for_io.Get().Get()); - tls_message_loop_for_io.Get().Set(message_loop_for_io); -} - -FileDescriptorWatcher::~FileDescriptorWatcher() { - tls_message_loop_for_io.Get().Set(nullptr); -} - -std::unique_ptr<FileDescriptorWatcher::Controller> -FileDescriptorWatcher::WatchReadable(int fd, const Closure& callback) { - return WrapUnique(new Controller(MessageLoopForIO::WATCH_READ, fd, callback)); -} - -std::unique_ptr<FileDescriptorWatcher::Controller> -FileDescriptorWatcher::WatchWritable(int fd, const Closure& callback) { - return WrapUnique( - new Controller(MessageLoopForIO::WATCH_WRITE, fd, callback)); -} - -} // namespace base diff --git a/base/files/file_descriptor_watcher_posix.h b/base/files/file_descriptor_watcher_posix.h deleted file mode 100644 index 6cc011bb3e..0000000000 --- a/base/files/file_descriptor_watcher_posix.h +++ /dev/null @@ -1,99 +0,0 @@ -// Copyright 2016 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef BASE_FILES_FILE_DESCRIPTOR_WATCHER_POSIX_H_ -#define BASE_FILES_FILE_DESCRIPTOR_WATCHER_POSIX_H_ - -#include <memory> - -#include "base/base_export.h" -#include "base/callback.h" -#include "base/macros.h" -#include "base/memory/ref_counted.h" -#include "base/memory/weak_ptr.h" -#include "base/message_loop/message_loop.h" -#include "base/sequence_checker.h" - -namespace base { - -class SingleThreadTaskRunner; - -// The FileDescriptorWatcher API allows callbacks to be invoked when file -// descriptors are readable or writable without blocking. -class BASE_EXPORT FileDescriptorWatcher { - public: - // Instantiated and returned by WatchReadable() or WatchWritable(). The - // constructor registers a callback to be invoked when a file descriptor is - // readable or writable without blocking and the destructor unregisters it. - class Controller { - public: - // Unregisters the callback registered by the constructor. - ~Controller(); - - private: - friend class FileDescriptorWatcher; - class Watcher; - - // Registers |callback| to be invoked when |fd| is readable or writable - // without blocking (depending on |mode|). - Controller(MessageLoopForIO::Mode mode, int fd, const Closure& callback); - - // Starts watching the file descriptor. - void StartWatching(); - - // Runs |callback_|. - void RunCallback(); - - // The callback to run when the watched file descriptor is readable or - // writable without blocking. - Closure callback_; - - // TaskRunner associated with the MessageLoopForIO that watches the file - // descriptor. - const scoped_refptr<SingleThreadTaskRunner> - message_loop_for_io_task_runner_; - - // Notified by the MessageLoopForIO associated with - // |message_loop_for_io_task_runner_| when the watched file descriptor is - // readable or writable without blocking. Posts a task to run RunCallback() - // on the sequence on which the Controller was instantiated. When the - // Controller is deleted, ownership of |watcher_| is transfered to a delete - // task posted to the MessageLoopForIO. This ensures that |watcher_| isn't - // deleted while it is being used by the MessageLoopForIO. - std::unique_ptr<Watcher> watcher_; - - // Validates that the Controller is used on the sequence on which it was - // instantiated. - SequenceChecker sequence_checker_; - - WeakPtrFactory<Controller> weak_factory_; - - DISALLOW_COPY_AND_ASSIGN(Controller); - }; - - // Registers |message_loop_for_io| to watch file descriptors for which - // callbacks are registered from the current thread via WatchReadable() or - // WatchWritable(). |message_loop_for_io| may run on another thread. The - // constructed FileDescriptorWatcher must not outlive |message_loop_for_io|. - FileDescriptorWatcher(MessageLoopForIO* message_loop_for_io); - ~FileDescriptorWatcher(); - - // Registers |callback| to be invoked on the current sequence when |fd| is - // readable or writable without blocking. |callback| is unregistered when the - // returned Controller is deleted (deletion must happen on the current - // sequence). To call these methods, a FileDescriptorWatcher must have been - // instantiated on the current thread and SequencedTaskRunnerHandle::IsSet() - // must return true. - static std::unique_ptr<Controller> WatchReadable(int fd, - const Closure& callback); - static std::unique_ptr<Controller> WatchWritable(int fd, - const Closure& callback); - - private: - DISALLOW_COPY_AND_ASSIGN(FileDescriptorWatcher); -}; - -} // namespace base - -#endif // BASE_FILES_FILE_DESCRIPTOR_WATCHER_POSIX_H_ diff --git a/base/files/file_descriptor_watcher_posix_unittest.cc b/base/files/file_descriptor_watcher_posix_unittest.cc deleted file mode 100644 index 7ff40c5fee..0000000000 --- a/base/files/file_descriptor_watcher_posix_unittest.cc +++ /dev/null @@ -1,318 +0,0 @@ -// Copyright 2016 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "base/files/file_descriptor_watcher_posix.h" - -#include <unistd.h> - -#include <memory> - -#include "base/bind.h" -#include "base/files/file_util.h" -#include "base/macros.h" -#include "base/memory/ptr_util.h" -#include "base/message_loop/message_loop.h" -#include "base/posix/eintr_wrapper.h" -#include "base/run_loop.h" -#include "base/test/test_timeouts.h" -#include "base/threading/platform_thread.h" -#include "base/threading/thread.h" -#include "base/threading/thread_checker_impl.h" -#include "build/build_config.h" -#include "testing/gmock/include/gmock/gmock.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace base { - -namespace { - -class Mock { - public: - Mock() = default; - - MOCK_METHOD0(ReadableCallback, void()); - MOCK_METHOD0(WritableCallback, void()); - - private: - DISALLOW_COPY_AND_ASSIGN(Mock); -}; - -enum class FileDescriptorWatcherTestType { - MESSAGE_LOOP_FOR_IO_ON_MAIN_THREAD, - MESSAGE_LOOP_FOR_IO_ON_OTHER_THREAD, -}; - -class FileDescriptorWatcherTest - : public testing::TestWithParam<FileDescriptorWatcherTestType> { - public: - FileDescriptorWatcherTest() - : message_loop_(GetParam() == FileDescriptorWatcherTestType:: - MESSAGE_LOOP_FOR_IO_ON_MAIN_THREAD - ? new MessageLoopForIO - : new MessageLoop), - other_thread_("FileDescriptorWatcherTest_OtherThread") {} - ~FileDescriptorWatcherTest() override = default; - - void SetUp() override { - ASSERT_EQ(0, pipe(pipe_fds_)); - - MessageLoop* message_loop_for_io; - if (GetParam() == - FileDescriptorWatcherTestType::MESSAGE_LOOP_FOR_IO_ON_OTHER_THREAD) { - Thread::Options options; - options.message_loop_type = MessageLoop::TYPE_IO; - ASSERT_TRUE(other_thread_.StartWithOptions(options)); - message_loop_for_io = other_thread_.message_loop(); - } else { - message_loop_for_io = message_loop_.get(); - } - - ASSERT_TRUE(message_loop_for_io->IsType(MessageLoop::TYPE_IO)); - file_descriptor_watcher_ = MakeUnique<FileDescriptorWatcher>( - static_cast<MessageLoopForIO*>(message_loop_for_io)); - } - - void TearDown() override { - if (GetParam() == - FileDescriptorWatcherTestType::MESSAGE_LOOP_FOR_IO_ON_MAIN_THREAD && - message_loop_) { - // Allow the delete task posted by the Controller's destructor to run. - base::RunLoop().RunUntilIdle(); - } - - EXPECT_EQ(0, IGNORE_EINTR(close(pipe_fds_[0]))); - EXPECT_EQ(0, IGNORE_EINTR(close(pipe_fds_[1]))); - } - - protected: - int read_file_descriptor() const { return pipe_fds_[0]; } - int write_file_descriptor() const { return pipe_fds_[1]; } - - // Waits for a short delay and run pending tasks. - void WaitAndRunPendingTasks() { - PlatformThread::Sleep(TestTimeouts::tiny_timeout()); - RunLoop().RunUntilIdle(); - } - - // Registers ReadableCallback() to be called on |mock_| when - // read_file_descriptor() is readable without blocking. - std::unique_ptr<FileDescriptorWatcher::Controller> WatchReadable() { - std::unique_ptr<FileDescriptorWatcher::Controller> controller = - FileDescriptorWatcher::WatchReadable( - read_file_descriptor(), - Bind(&Mock::ReadableCallback, Unretained(&mock_))); - EXPECT_TRUE(controller); - - // Unless read_file_descriptor() was readable before the callback was - // registered, this shouldn't do anything. - WaitAndRunPendingTasks(); - - return controller; - } - - // Registers WritableCallback() to be called on |mock_| when - // write_file_descriptor() is writable without blocking. - std::unique_ptr<FileDescriptorWatcher::Controller> WatchWritable() { - std::unique_ptr<FileDescriptorWatcher::Controller> controller = - FileDescriptorWatcher::WatchWritable( - read_file_descriptor(), - Bind(&Mock::WritableCallback, Unretained(&mock_))); - EXPECT_TRUE(controller); - return controller; - } - - void WriteByte() { - constexpr char kByte = '!'; - ASSERT_TRUE( - WriteFileDescriptor(write_file_descriptor(), &kByte, sizeof(kByte))); - } - - void ReadByte() { - // This is always called as part of the WatchReadable() callback, which - // should run on the main thread. - EXPECT_TRUE(thread_checker_.CalledOnValidThread()); - - char buffer; - ASSERT_TRUE(ReadFromFD(read_file_descriptor(), &buffer, sizeof(buffer))); - } - - // Mock on wich callbacks are invoked. - testing::StrictMock<Mock> mock_; - - // MessageLoop bound to the main thread. - std::unique_ptr<MessageLoop> message_loop_; - - // Thread running a MessageLoopForIO. Used when the test type is - // MESSAGE_LOOP_FOR_IO_ON_OTHER_THREAD. - Thread other_thread_; - - private: - // Determines which MessageLoopForIO is used to watch file descriptors for - // which callbacks are registered on the main thread. - std::unique_ptr<FileDescriptorWatcher> file_descriptor_watcher_; - - // Watched file descriptors. - int pipe_fds_[2]; - - // Used to verify that callbacks run on the thread on which they are - // registered. - ThreadCheckerImpl thread_checker_; - - DISALLOW_COPY_AND_ASSIGN(FileDescriptorWatcherTest); -}; - -} // namespace - -TEST_P(FileDescriptorWatcherTest, WatchWritable) { - auto controller = WatchWritable(); - -// On Mac and iOS, the write end of a newly created pipe is writable without -// blocking. -#if defined(OS_MACOSX) - RunLoop run_loop; - EXPECT_CALL(mock_, WritableCallback()) - .WillOnce(testing::Invoke(&run_loop, &RunLoop::Quit)); - run_loop.Run(); -#endif // defined(OS_MACOSX) -} - -TEST_P(FileDescriptorWatcherTest, WatchReadableOneByte) { - auto controller = WatchReadable(); - - // Write 1 byte to the pipe, making it readable without blocking. Expect one - // call to ReadableCallback() which will read 1 byte from the pipe. - WriteByte(); - RunLoop run_loop; - EXPECT_CALL(mock_, ReadableCallback()) - .WillOnce(testing::Invoke([this, &run_loop]() { - ReadByte(); - run_loop.Quit(); - })); - run_loop.Run(); - testing::Mock::VerifyAndClear(&mock_); - - // No more call to ReadableCallback() is expected. - WaitAndRunPendingTasks(); -} - -TEST_P(FileDescriptorWatcherTest, WatchReadableTwoBytes) { - auto controller = WatchReadable(); - - // Write 2 bytes to the pipe. Expect two calls to ReadableCallback() which - // will each read 1 byte from the pipe. - WriteByte(); - WriteByte(); - RunLoop run_loop; - EXPECT_CALL(mock_, ReadableCallback()) - .WillOnce(testing::Invoke([this]() { ReadByte(); })) - .WillOnce(testing::Invoke([this, &run_loop]() { - ReadByte(); - run_loop.Quit(); - })); - run_loop.Run(); - testing::Mock::VerifyAndClear(&mock_); - - // No more call to ReadableCallback() is expected. - WaitAndRunPendingTasks(); -} - -TEST_P(FileDescriptorWatcherTest, WatchReadableByteWrittenFromCallback) { - auto controller = WatchReadable(); - - // Write 1 byte to the pipe. Expect one call to ReadableCallback() from which - // 1 byte is read and 1 byte is written to the pipe. Then, expect another call - // to ReadableCallback() from which the remaining byte is read from the pipe. - WriteByte(); - RunLoop run_loop; - EXPECT_CALL(mock_, ReadableCallback()) - .WillOnce(testing::Invoke([this]() { - ReadByte(); - WriteByte(); - })) - .WillOnce(testing::Invoke([this, &run_loop]() { - ReadByte(); - run_loop.Quit(); - })); - run_loop.Run(); - testing::Mock::VerifyAndClear(&mock_); - - // No more call to ReadableCallback() is expected. - WaitAndRunPendingTasks(); -} - -TEST_P(FileDescriptorWatcherTest, DeleteControllerFromCallback) { - auto controller = WatchReadable(); - - // Write 1 byte to the pipe. Expect one call to ReadableCallback() from which - // |controller| is deleted. - WriteByte(); - RunLoop run_loop; - EXPECT_CALL(mock_, ReadableCallback()) - .WillOnce(testing::Invoke([&run_loop, &controller]() { - controller = nullptr; - run_loop.Quit(); - })); - run_loop.Run(); - testing::Mock::VerifyAndClear(&mock_); - - // Since |controller| has been deleted, no call to ReadableCallback() is - // expected even though the pipe is still readable without blocking. - WaitAndRunPendingTasks(); -} - -TEST_P(FileDescriptorWatcherTest, - DeleteControllerBeforeFileDescriptorReadable) { - auto controller = WatchReadable(); - - // Cancel the watch. - controller = nullptr; - - // Write 1 byte to the pipe to make it readable without blocking. - WriteByte(); - - // No call to ReadableCallback() is expected. - WaitAndRunPendingTasks(); -} - -TEST_P(FileDescriptorWatcherTest, DeleteControllerAfterFileDescriptorReadable) { - auto controller = WatchReadable(); - - // Write 1 byte to the pipe to make it readable without blocking. - WriteByte(); - - // Cancel the watch. - controller = nullptr; - - // No call to ReadableCallback() is expected. - WaitAndRunPendingTasks(); -} - -TEST_P(FileDescriptorWatcherTest, DeleteControllerAfterDeleteMessageLoopForIO) { - auto controller = WatchReadable(); - - // Delete the MessageLoopForIO. - if (GetParam() == - FileDescriptorWatcherTestType::MESSAGE_LOOP_FOR_IO_ON_MAIN_THREAD) { - message_loop_ = nullptr; - } else { - other_thread_.Stop(); - } - - // Deleting |controller| shouldn't crash even though that causes a task to be - // posted to the MessageLoopForIO thread. - controller = nullptr; -} - -INSTANTIATE_TEST_CASE_P( - MessageLoopForIOOnMainThread, - FileDescriptorWatcherTest, - ::testing::Values( - FileDescriptorWatcherTestType::MESSAGE_LOOP_FOR_IO_ON_MAIN_THREAD)); -INSTANTIATE_TEST_CASE_P( - MessageLoopForIOOnOtherThread, - FileDescriptorWatcherTest, - ::testing::Values( - FileDescriptorWatcherTestType::MESSAGE_LOOP_FOR_IO_ON_OTHER_THREAD)); - -} // namespace base diff --git a/base/files/file_path.cc b/base/files/file_path.cc index 9f67f9bc49..29f12a80aa 100644 --- a/base/files/file_path.cc +++ b/base/files/file_path.cc @@ -176,7 +176,6 @@ FilePath::FilePath() { FilePath::FilePath(const FilePath& that) : path_(that.path_) { } -FilePath::FilePath(FilePath&& that) = default; FilePath::FilePath(StringPieceType path) { path.CopyToString(&path_); @@ -193,8 +192,6 @@ FilePath& FilePath::operator=(const FilePath& that) { return *this; } -FilePath& FilePath::operator=(FilePath&& that) = default; - bool FilePath::operator==(const FilePath& that) const { #if defined(FILE_PATH_USES_DRIVE_LETTERS) return EqualDriveLetterCaseInsensitive(this->path_, that.path_); diff --git a/base/files/file_path.h b/base/files/file_path.h index 02846f6892..3234df7bfb 100644 --- a/base/files/file_path.h +++ b/base/files/file_path.h @@ -182,13 +182,6 @@ class BASE_EXPORT FilePath { ~FilePath(); FilePath& operator=(const FilePath& that); - // Constructs FilePath with the contents of |that|, which is left in valid but - // unspecified state. - FilePath(FilePath&& that); - // Replaces the contents with those of |that|, which is left in valid but - // unspecified state. - FilePath& operator=(FilePath&& that); - bool operator==(const FilePath& that) const; bool operator!=(const FilePath& that) const; diff --git a/base/files/file_path_unittest.cc b/base/files/file_path_unittest.cc index a091e62dd1..d8c5969513 100644 --- a/base/files/file_path_unittest.cc +++ b/base/files/file_path_unittest.cc @@ -9,6 +9,7 @@ #include "base/files/file_path.h" #include "base/macros.h" #include "base/strings/utf_string_conversions.h" +#include "base/test/scoped_locale.h" #include "build/build_config.h" #include "testing/gtest/include/gtest/gtest.h" #include "testing/platform_test.h" diff --git a/base/files/file_path_watcher.cc b/base/files/file_path_watcher.cc index 245bd8efe2..a4624ab609 100644 --- a/base/files/file_path_watcher.cc +++ b/base/files/file_path_watcher.cc @@ -8,16 +8,22 @@ #include "base/files/file_path_watcher.h" #include "base/logging.h" +#include "base/message_loop/message_loop.h" #include "build/build_config.h" namespace base { FilePathWatcher::~FilePathWatcher() { - DCHECK(sequence_checker_.CalledOnValidSequence()); impl_->Cancel(); } // static +void FilePathWatcher::CancelWatch( + const scoped_refptr<PlatformDelegate>& delegate) { + delegate->CancelOnMessageLoopThread(); +} + +// static bool FilePathWatcher::RecursiveWatchAvailable() { #if (defined(OS_MACOSX) && !defined(OS_IOS)) || defined(OS_WIN) || \ defined(OS_LINUX) || defined(OS_ANDROID) @@ -38,7 +44,6 @@ FilePathWatcher::PlatformDelegate::~PlatformDelegate() { bool FilePathWatcher::Watch(const FilePath& path, bool recursive, const Callback& callback) { - DCHECK(sequence_checker_.CalledOnValidSequence()); DCHECK(path.IsAbsolute()); return impl_->Watch(path, recursive, callback); } diff --git a/base/files/file_path_watcher.h b/base/files/file_path_watcher.h index 9e29d0a9d5..d5c6db1acf 100644 --- a/base/files/file_path_watcher.h +++ b/base/files/file_path_watcher.h @@ -7,15 +7,12 @@ #ifndef BASE_FILES_FILE_PATH_WATCHER_H_ #define BASE_FILES_FILE_PATH_WATCHER_H_ -#include <memory> - #include "base/base_export.h" #include "base/callback.h" #include "base/files/file_path.h" #include "base/macros.h" #include "base/memory/ref_counted.h" -#include "base/sequence_checker.h" -#include "base/sequenced_task_runner.h" +#include "base/single_thread_task_runner.h" namespace base { @@ -28,8 +25,6 @@ namespace base { // detect the creation and deletion of files in a watched directory, but will // not detect modifications to those files. See file_path_watcher_kqueue.cc for // details. -// -// Must be destroyed on the sequence that invokes Watch(). class BASE_EXPORT FilePathWatcher { public: // Callback type for Watch(). |path| points to the file that was updated, @@ -38,10 +33,9 @@ class BASE_EXPORT FilePathWatcher { typedef base::Callback<void(const FilePath& path, bool error)> Callback; // Used internally to encapsulate different members on different platforms. - class PlatformDelegate { + class PlatformDelegate : public base::RefCountedThreadSafe<PlatformDelegate> { public: PlatformDelegate(); - virtual ~PlatformDelegate(); // Start watching for the given |path| and notify |delegate| about changes. virtual bool Watch(const FilePath& path, @@ -50,16 +44,25 @@ class BASE_EXPORT FilePathWatcher { // Stop watching. This is called from FilePathWatcher's dtor in order to // allow to shut down properly while the object is still alive. + // It can be called from any thread. virtual void Cancel() = 0; protected: + friend class base::RefCountedThreadSafe<PlatformDelegate>; friend class FilePathWatcher; - scoped_refptr<SequencedTaskRunner> task_runner() const { + virtual ~PlatformDelegate(); + + // Stop watching. This is only called on the thread of the appropriate + // message loop. Since it can also be called more than once, it should + // check |is_cancelled()| to avoid duplicate work. + virtual void CancelOnMessageLoopThread() = 0; + + scoped_refptr<base::SingleThreadTaskRunner> task_runner() const { return task_runner_; } - void set_task_runner(scoped_refptr<SequencedTaskRunner> runner) { + void set_task_runner(scoped_refptr<base::SingleThreadTaskRunner> runner) { task_runner_ = std::move(runner); } @@ -73,34 +76,32 @@ class BASE_EXPORT FilePathWatcher { } private: - scoped_refptr<SequencedTaskRunner> task_runner_; + scoped_refptr<base::SingleThreadTaskRunner> task_runner_; bool cancelled_; - - DISALLOW_COPY_AND_ASSIGN(PlatformDelegate); }; FilePathWatcher(); - ~FilePathWatcher(); + virtual ~FilePathWatcher(); + + // A callback that always cleans up the PlatformDelegate, either when executed + // or when deleted without having been executed at all, as can happen during + // shutdown. + static void CancelWatch(const scoped_refptr<PlatformDelegate>& delegate); // Returns true if the platform and OS version support recursive watches. static bool RecursiveWatchAvailable(); // Invokes |callback| whenever updates to |path| are detected. This should be - // called at most once. Set |recursive| to true to watch |path| and its - // children. The callback will be invoked on the same sequence. Returns true - // on success. - // - // On POSIX, this must be called from a thread that supports - // FileDescriptorWatcher. + // called at most once, and from a MessageLoop of TYPE_IO. Set |recursive| to + // true, to watch |path| and its children. The callback will be invoked on + // the same loop. Returns true on success. // // Recursive watch is not supported on all platforms and file systems. // Watch() will return false in the case of failure. bool Watch(const FilePath& path, bool recursive, const Callback& callback); private: - std::unique_ptr<PlatformDelegate> impl_; - - SequenceChecker sequence_checker_; + scoped_refptr<PlatformDelegate> impl_; DISALLOW_COPY_AND_ASSIGN(FilePathWatcher); }; diff --git a/base/files/file_path_watcher_fsevents.cc b/base/files/file_path_watcher_fsevents.cc index e9a87b0e05..e9d25080e7 100644 --- a/base/files/file_path_watcher_fsevents.cc +++ b/base/files/file_path_watcher_fsevents.cc @@ -13,8 +13,10 @@ #include "base/lazy_instance.h" #include "base/logging.h" #include "base/mac/scoped_cftyperef.h" +#include "base/macros.h" +#include "base/message_loop/message_loop.h" #include "base/strings/stringprintf.h" -#include "base/threading/sequenced_task_runner_handle.h" +#include "base/threading/thread_task_runner_handle.h" namespace base { @@ -68,21 +70,16 @@ FilePath ResolvePath(const FilePath& path) { FilePathWatcherFSEvents::FilePathWatcherFSEvents() : queue_(dispatch_queue_create( - base::StringPrintf("org.chromium.base.FilePathWatcher.%p", this) - .c_str(), + base::StringPrintf( + "org.chromium.base.FilePathWatcher.%p", this).c_str(), DISPATCH_QUEUE_SERIAL)), - fsevent_stream_(nullptr), - weak_factory_(this) {} - -FilePathWatcherFSEvents::~FilePathWatcherFSEvents() { - DCHECK(!task_runner() || task_runner()->RunsTasksOnCurrentThread()); - DCHECK(callback_.is_null()) - << "Cancel() must be called before FilePathWatcher is destroyed."; + fsevent_stream_(nullptr) { } bool FilePathWatcherFSEvents::Watch(const FilePath& path, bool recursive, const FilePathWatcher::Callback& callback) { + DCHECK(MessageLoopForIO::current()); DCHECK(!callback.is_null()); DCHECK(callback_.is_null()); @@ -91,7 +88,7 @@ bool FilePathWatcherFSEvents::Watch(const FilePath& path, if (!recursive) return false; - set_task_runner(SequencedTaskRunnerHandle::Get()); + set_task_runner(ThreadTaskRunnerHandle::Get()); callback_ = callback; FSEventStreamEventId start_event = FSEventsGetCurrentEventId(); @@ -110,15 +107,11 @@ void FilePathWatcherFSEvents::Cancel() { set_cancelled(); callback_.Reset(); - // Switch to the dispatch queue to tear down the event stream. As the queue is - // owned by |this|, and this method is called from the destructor, execute the - // block synchronously. + // Switch to the dispatch queue to tear down the event stream. As the queue + // is owned by this object, and this method is called from the destructor, + // execute the block synchronously. dispatch_sync(queue_, ^{ - if (fsevent_stream_) { - DestroyEventStream(); - target_.clear(); - resolved_target_.clear(); - } + CancelOnMessageLoopThread(); }); } @@ -149,40 +142,31 @@ void FilePathWatcherFSEvents::FSEventsCallback( // the directory to be watched gets created. if (root_changed) { // Resetting the event stream from within the callback fails (FSEvents spews - // bad file descriptor errors), so do the reset asynchronously. - // - // We can't dispatch_async a call to UpdateEventStream() directly because - // there would be no guarantee that |watcher| still exists when it runs. - // - // Instead, bounce on task_runner() and use a WeakPtr to verify that - // |watcher| still exists. If it does, dispatch_async a call to - // UpdateEventStream(). Because the destructor of |watcher| runs on - // task_runner() and calls dispatch_sync, it is guaranteed that |watcher| - // still exists when UpdateEventStream() runs. - watcher->task_runner()->PostTask( - FROM_HERE, Bind( - [](WeakPtr<FilePathWatcherFSEvents> weak_watcher, - FSEventStreamEventId root_change_at) { - if (!weak_watcher) - return; - FilePathWatcherFSEvents* watcher = weak_watcher.get(); - dispatch_async(watcher->queue_, ^{ - watcher->UpdateEventStream(root_change_at); - }); - }, - watcher->weak_factory_.GetWeakPtr(), root_change_at)); + // bad file descriptor errors), so post a task to do the reset. + dispatch_async(watcher->queue_, ^{ + watcher->UpdateEventStream(root_change_at); + }); } watcher->OnFilePathsChanged(paths); } +FilePathWatcherFSEvents::~FilePathWatcherFSEvents() { + // This method may be called on either the libdispatch or task_runner() + // thread. Checking callback_ on the libdispatch thread here is safe because + // it is executing in a task posted by Cancel() which first reset callback_. + // PostTask forms a sufficient memory barrier to ensure that the value is + // consistent on the target thread. + DCHECK(callback_.is_null()) + << "Cancel() must be called before FilePathWatcher is destroyed."; +} + void FilePathWatcherFSEvents::OnFilePathsChanged( const std::vector<FilePath>& paths) { DCHECK(!resolved_target_.empty()); task_runner()->PostTask( - FROM_HERE, - Bind(&FilePathWatcherFSEvents::DispatchEvents, weak_factory_.GetWeakPtr(), - paths, target_, resolved_target_)); + FROM_HERE, Bind(&FilePathWatcherFSEvents::DispatchEvents, this, paths, + target_, resolved_target_)); } void FilePathWatcherFSEvents::DispatchEvents(const std::vector<FilePath>& paths, @@ -203,6 +187,18 @@ void FilePathWatcherFSEvents::DispatchEvents(const std::vector<FilePath>& paths, } } +void FilePathWatcherFSEvents::CancelOnMessageLoopThread() { + // For all other implementations, the "message loop thread" is the IO thread, + // as returned by task_runner(). This implementation, however, needs to + // cancel pending work on the Dispatch Queue thread. + + if (fsevent_stream_) { + DestroyEventStream(); + target_.clear(); + resolved_target_.clear(); + } +} + void FilePathWatcherFSEvents::UpdateEventStream( FSEventStreamEventId start_event) { // It can happen that the watcher gets canceled while tasks that call this @@ -238,9 +234,8 @@ void FilePathWatcherFSEvents::UpdateEventStream( FSEventStreamSetDispatchQueue(fsevent_stream_, queue_); if (!FSEventStreamStart(fsevent_stream_)) { - task_runner()->PostTask(FROM_HERE, - Bind(&FilePathWatcherFSEvents::ReportError, - weak_factory_.GetWeakPtr(), target_)); + task_runner()->PostTask( + FROM_HERE, Bind(&FilePathWatcherFSEvents::ReportError, this, target_)); } } @@ -249,9 +244,8 @@ bool FilePathWatcherFSEvents::ResolveTargetPath() { bool changed = resolved != resolved_target_; resolved_target_ = resolved; if (resolved_target_.empty()) { - task_runner()->PostTask(FROM_HERE, - Bind(&FilePathWatcherFSEvents::ReportError, - weak_factory_.GetWeakPtr(), target_)); + task_runner()->PostTask( + FROM_HERE, Bind(&FilePathWatcherFSEvents::ReportError, this, target_)); } return changed; } diff --git a/base/files/file_path_watcher_fsevents.h b/base/files/file_path_watcher_fsevents.h index dcdf2fbf9d..cfbe020b51 100644 --- a/base/files/file_path_watcher_fsevents.h +++ b/base/files/file_path_watcher_fsevents.h @@ -14,7 +14,6 @@ #include "base/files/file_path_watcher.h" #include "base/mac/scoped_dispatch_object.h" #include "base/macros.h" -#include "base/memory/weak_ptr.h" namespace base { @@ -27,7 +26,6 @@ namespace base { class FilePathWatcherFSEvents : public FilePathWatcher::PlatformDelegate { public: FilePathWatcherFSEvents(); - ~FilePathWatcherFSEvents() override; // FilePathWatcher::PlatformDelegate overrides. bool Watch(const FilePath& path, @@ -43,6 +41,8 @@ class FilePathWatcherFSEvents : public FilePathWatcher::PlatformDelegate { const FSEventStreamEventFlags flags[], const FSEventStreamEventId event_ids[]); + ~FilePathWatcherFSEvents() override; + // Called from FSEventsCallback whenever there is a change to the paths. void OnFilePathsChanged(const std::vector<FilePath>& paths); @@ -53,6 +53,9 @@ class FilePathWatcherFSEvents : public FilePathWatcher::PlatformDelegate { const FilePath& target, const FilePath& resolved_target); + // Cleans up and stops the event stream. + void CancelOnMessageLoopThread() override; + // (Re-)Initialize the event stream to start reporting events from // |start_event|. void UpdateEventStream(FSEventStreamEventId start_event); @@ -89,8 +92,6 @@ class FilePathWatcherFSEvents : public FilePathWatcher::PlatformDelegate { // (Only accessed from the libdispatch queue.) FSEventStreamRef fsevent_stream_; - WeakPtrFactory<FilePathWatcherFSEvents> weak_factory_; - DISALLOW_COPY_AND_ASSIGN(FilePathWatcherFSEvents); }; diff --git a/base/files/file_path_watcher_kqueue.cc b/base/files/file_path_watcher_kqueue.cc index a28726acb0..6d034cd9a2 100644 --- a/base/files/file_path_watcher_kqueue.cc +++ b/base/files/file_path_watcher_kqueue.cc @@ -12,7 +12,7 @@ #include "base/files/file_util.h" #include "base/logging.h" #include "base/strings/stringprintf.h" -#include "base/threading/sequenced_task_runner_handle.h" +#include "base/threading/thread_task_runner_handle.h" // On some platforms these are not defined. #if !defined(EV_RECEIPT) @@ -26,9 +26,7 @@ namespace base { FilePathWatcherKQueue::FilePathWatcherKQueue() : kqueue_(-1) {} -FilePathWatcherKQueue::~FilePathWatcherKQueue() { - DCHECK(!task_runner() || task_runner()->RunsTasksOnCurrentThread()); -} +FilePathWatcherKQueue::~FilePathWatcherKQueue() {} void FilePathWatcherKQueue::ReleaseEvent(struct kevent& event) { CloseFileDescriptor(&event.ident); @@ -38,6 +36,7 @@ void FilePathWatcherKQueue::ReleaseEvent(struct kevent& event) { } int FilePathWatcherKQueue::EventsForPath(FilePath path, EventVector* events) { + DCHECK(MessageLoopForIO::current()); // Make sure that we are working with a clean slate. DCHECK(events->empty()); @@ -231,74 +230,9 @@ bool FilePathWatcherKQueue::UpdateWatches(bool* target_file_affected) { return true; } -bool FilePathWatcherKQueue::Watch(const FilePath& path, - bool recursive, - const FilePathWatcher::Callback& callback) { - DCHECK(target_.value().empty()); // Can only watch one path. - DCHECK(!callback.is_null()); - DCHECK_EQ(kqueue_, -1); - // Recursive watch is not supported using kqueue. - DCHECK(!recursive); - - callback_ = callback; - target_ = path; - - set_task_runner(SequencedTaskRunnerHandle::Get()); - - kqueue_ = kqueue(); - if (kqueue_ == -1) { - DPLOG(ERROR) << "kqueue"; - return false; - } - - int last_entry = EventsForPath(target_, &events_); - DCHECK_NE(last_entry, 0); - - EventVector responses(last_entry); - - int count = HANDLE_EINTR(kevent(kqueue_, &events_[0], last_entry, - &responses[0], last_entry, NULL)); - if (!AreKeventValuesValid(&responses[0], count)) { - // Calling Cancel() here to close any file descriptors that were opened. - // This would happen in the destructor anyways, but FilePathWatchers tend to - // be long lived, and if an error has occurred, there is no reason to waste - // the file descriptors. - Cancel(); - return false; - } - - // It's safe to use Unretained() because the watch is cancelled and the - // callback cannot be invoked after |kqueue_watch_controller_| (which is a - // member of |this|) has been deleted. - kqueue_watch_controller_ = FileDescriptorWatcher::WatchReadable( - kqueue_, - Bind(&FilePathWatcherKQueue::OnKQueueReadable, Unretained(this))); - - return true; -} - -void FilePathWatcherKQueue::Cancel() { - if (!task_runner()) { - set_cancelled(); - return; - } - - DCHECK(task_runner()->RunsTasksOnCurrentThread()); - if (!is_cancelled()) { - set_cancelled(); - kqueue_watch_controller_.reset(); - if (IGNORE_EINTR(close(kqueue_)) != 0) { - DPLOG(ERROR) << "close kqueue"; - } - kqueue_ = -1; - std::for_each(events_.begin(), events_.end(), ReleaseEvent); - events_.clear(); - callback_.Reset(); - } -} - -void FilePathWatcherKQueue::OnKQueueReadable() { - DCHECK(task_runner()->RunsTasksOnCurrentThread()); +void FilePathWatcherKQueue::OnFileCanReadWithoutBlocking(int fd) { + DCHECK(MessageLoopForIO::current()); + DCHECK_EQ(fd, kqueue_); DCHECK(events_.size()); // Request the file system update notifications that have occurred and return @@ -369,4 +303,89 @@ void FilePathWatcherKQueue::OnKQueueReadable() { } } +void FilePathWatcherKQueue::OnFileCanWriteWithoutBlocking(int /* fd */) { + NOTREACHED(); +} + +void FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() { + CancelOnMessageLoopThread(); +} + +bool FilePathWatcherKQueue::Watch(const FilePath& path, + bool recursive, + const FilePathWatcher::Callback& callback) { + DCHECK(MessageLoopForIO::current()); + DCHECK(target_.value().empty()); // Can only watch one path. + DCHECK(!callback.is_null()); + DCHECK_EQ(kqueue_, -1); + + if (recursive) { + // Recursive watch is not supported using kqueue. + NOTIMPLEMENTED(); + return false; + } + + callback_ = callback; + target_ = path; + + MessageLoop::current()->AddDestructionObserver(this); + io_task_runner_ = ThreadTaskRunnerHandle::Get(); + + kqueue_ = kqueue(); + if (kqueue_ == -1) { + DPLOG(ERROR) << "kqueue"; + return false; + } + + int last_entry = EventsForPath(target_, &events_); + DCHECK_NE(last_entry, 0); + + EventVector responses(last_entry); + + int count = HANDLE_EINTR(kevent(kqueue_, &events_[0], last_entry, + &responses[0], last_entry, NULL)); + if (!AreKeventValuesValid(&responses[0], count)) { + // Calling Cancel() here to close any file descriptors that were opened. + // This would happen in the destructor anyways, but FilePathWatchers tend to + // be long lived, and if an error has occurred, there is no reason to waste + // the file descriptors. + Cancel(); + return false; + } + + return MessageLoopForIO::current()->WatchFileDescriptor( + kqueue_, true, MessageLoopForIO::WATCH_READ, &kqueue_watcher_, this); +} + +void FilePathWatcherKQueue::Cancel() { + SingleThreadTaskRunner* task_runner = io_task_runner_.get(); + if (!task_runner) { + set_cancelled(); + return; + } + if (!task_runner->BelongsToCurrentThread()) { + task_runner->PostTask(FROM_HERE, + base::Bind(&FilePathWatcherKQueue::Cancel, this)); + return; + } + CancelOnMessageLoopThread(); +} + +void FilePathWatcherKQueue::CancelOnMessageLoopThread() { + DCHECK(MessageLoopForIO::current()); + if (!is_cancelled()) { + set_cancelled(); + kqueue_watcher_.StopWatchingFileDescriptor(); + if (IGNORE_EINTR(close(kqueue_)) != 0) { + DPLOG(ERROR) << "close kqueue"; + } + kqueue_ = -1; + std::for_each(events_.begin(), events_.end(), ReleaseEvent); + events_.clear(); + io_task_runner_ = NULL; + MessageLoop::current()->RemoveDestructionObserver(this); + callback_.Reset(); + } +} + } // namespace base diff --git a/base/files/file_path_watcher_kqueue.h b/base/files/file_path_watcher_kqueue.h index ef79be5596..d9db8c2587 100644 --- a/base/files/file_path_watcher_kqueue.h +++ b/base/files/file_path_watcher_kqueue.h @@ -6,14 +6,13 @@ #define BASE_FILES_FILE_PATH_WATCHER_KQUEUE_H_ #include <sys/event.h> - -#include <memory> #include <vector> -#include "base/files/file_descriptor_watcher_posix.h" #include "base/files/file_path.h" #include "base/files/file_path_watcher.h" #include "base/macros.h" +#include "base/message_loop/message_loop.h" +#include "base/single_thread_task_runner.h" namespace base { @@ -28,10 +27,18 @@ namespace base { // detect the creation and deletion of files, just not the modification of // files. It does however detect the attribute changes that the FSEvents impl // would miss. -class FilePathWatcherKQueue : public FilePathWatcher::PlatformDelegate { +class FilePathWatcherKQueue : public FilePathWatcher::PlatformDelegate, + public MessageLoopForIO::Watcher, + public MessageLoop::DestructionObserver { public: FilePathWatcherKQueue(); - ~FilePathWatcherKQueue() override; + + // MessageLoopForIO::Watcher overrides. + void OnFileCanReadWithoutBlocking(int fd) override; + void OnFileCanWriteWithoutBlocking(int fd) override; + + // MessageLoop::DestructionObserver overrides. + void WillDestroyCurrentMessageLoop() override; // FilePathWatcher::PlatformDelegate overrides. bool Watch(const FilePath& path, @@ -39,6 +46,9 @@ class FilePathWatcherKQueue : public FilePathWatcher::PlatformDelegate { const FilePathWatcher::Callback& callback) override; void Cancel() override; + protected: + ~FilePathWatcherKQueue() override; + private: class EventData { public: @@ -50,8 +60,8 @@ class FilePathWatcherKQueue : public FilePathWatcher::PlatformDelegate { typedef std::vector<struct kevent> EventVector; - // Called when data is available in |kqueue_|. - void OnKQueueReadable(); + // Can only be called on |io_task_runner_|'s thread. + void CancelOnMessageLoopThread() override; // Returns true if the kevent values are error free. bool AreKeventValuesValid(struct kevent* kevents, int count); @@ -109,14 +119,12 @@ class FilePathWatcherKQueue : public FilePathWatcher::PlatformDelegate { } EventVector events_; + scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; + MessageLoopForIO::FileDescriptorWatcher kqueue_watcher_; FilePathWatcher::Callback callback_; FilePath target_; int kqueue_; - // Throughout the lifetime of this, OnKQueueReadable() will be called when - // data is available in |kqueue_|. - std::unique_ptr<FileDescriptorWatcher::Controller> kqueue_watch_controller_; - DISALLOW_COPY_AND_ASSIGN(FilePathWatcherKQueue); }; diff --git a/base/files/file_path_watcher_linux.cc b/base/files/file_path_watcher_linux.cc index 1dc833dc88..87bddd3dea 100644 --- a/base/files/file_path_watcher_linux.cc +++ b/base/files/file_path_watcher_linux.cc @@ -28,14 +28,12 @@ #include "base/location.h" #include "base/logging.h" #include "base/macros.h" -#include "base/memory/ptr_util.h" -#include "base/memory/weak_ptr.h" #include "base/posix/eintr_wrapper.h" #include "base/single_thread_task_runner.h" #include "base/stl_util.h" #include "base/synchronization/lock.h" -#include "base/threading/sequenced_task_runner_handle.h" #include "base/threading/thread.h" +#include "base/threading/thread_task_runner_handle.h" #include "base/trace_event/trace_event.h" namespace base { @@ -63,14 +61,12 @@ class InotifyReader { void OnInotifyEvent(const inotify_event* event); private: - friend struct LazyInstanceTraitsBase<InotifyReader>; + friend struct DefaultLazyInstanceTraits<InotifyReader>; typedef std::set<FilePathWatcherImpl*> WatcherSet; InotifyReader(); - // There is no destructor because |g_inotify_reader| is a - // base::LazyInstace::Leaky object. Having a destructor causes build - // issues with GCC 6 (http://crbug.com/636346). + ~InotifyReader(); // We keep track of which delegates want to be notified on which watches. hash_map<Watch, WatcherSet> watchers_; @@ -84,16 +80,19 @@ class InotifyReader { // File descriptor returned by inotify_init. const int inotify_fd_; + // Use self-pipe trick to unblock select during shutdown. + int shutdown_pipe_[2]; + // Flag set to true when startup was successful. bool valid_; DISALLOW_COPY_AND_ASSIGN(InotifyReader); }; -class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { +class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate, + public MessageLoop::DestructionObserver { public: FilePathWatcherImpl(); - ~FilePathWatcherImpl() override; // Called for each event coming from the watch. |fired_watch| identifies the // watch that fired, |child| indicates what has changed, and is relative to @@ -108,13 +107,10 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { bool deleted, bool is_dir); - private: - void OnFilePathChangedOnOriginSequence(InotifyReader::Watch fired_watch, - const FilePath::StringType& child, - bool created, - bool deleted, - bool is_dir); + protected: + ~FilePathWatcherImpl() override {} + private: // Start watching |path| for changes and notify |delegate| on each change. // Returns true if watch for |path| has been added successfully. bool Watch(const FilePath& path, @@ -124,6 +120,14 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { // Cancel the watch. This unregisters the instance with InotifyReader. void Cancel() override; + // Cleans up and stops observing the message_loop() thread. + void CancelOnMessageLoopThread() override; + + // Deletion of the FilePathWatcher will call Cancel() to dispose of this + // object in the right thread. This also observes destruction of the required + // cleanup thread, in case it quits before Cancel() is called. + void WillDestroyCurrentMessageLoop() override; + // Inotify watches are installed for all directory components of |target_|. // A WatchEntry instance holds: // - |watch|: the watch descriptor for a component. @@ -187,15 +191,16 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { hash_map<InotifyReader::Watch, FilePath> recursive_paths_by_watch_; std::map<FilePath, InotifyReader::Watch> recursive_watches_by_path_; - WeakPtrFactory<FilePathWatcherImpl> weak_factory_; - DISALLOW_COPY_AND_ASSIGN(FilePathWatcherImpl); }; -void InotifyReaderCallback(InotifyReader* reader, int inotify_fd) { +void InotifyReaderCallback(InotifyReader* reader, int inotify_fd, + int shutdown_fd) { // Make sure the file descriptors are good for use with select(). CHECK_LE(0, inotify_fd); CHECK_GT(FD_SETSIZE, inotify_fd); + CHECK_LE(0, shutdown_fd); + CHECK_GT(FD_SETSIZE, shutdown_fd); trace_event::TraceLog::GetInstance()->SetCurrentThreadBlocksMessageLoop(); @@ -203,15 +208,20 @@ void InotifyReaderCallback(InotifyReader* reader, int inotify_fd) { fd_set rfds; FD_ZERO(&rfds); FD_SET(inotify_fd, &rfds); + FD_SET(shutdown_fd, &rfds); // Wait until some inotify events are available. int select_result = - HANDLE_EINTR(select(inotify_fd + 1, &rfds, NULL, NULL, NULL)); + HANDLE_EINTR(select(std::max(inotify_fd, shutdown_fd) + 1, + &rfds, NULL, NULL, NULL)); if (select_result < 0) { DPLOG(WARNING) << "select failed"; return; } + if (FD_ISSET(shutdown_fd, &rfds)) + return; + // Adjust buffer size to current event queue size. int buffer_size; int ioctl_result = HANDLE_EINTR(ioctl(inotify_fd, FIONREAD, @@ -253,14 +263,33 @@ InotifyReader::InotifyReader() if (inotify_fd_ < 0) PLOG(ERROR) << "inotify_init() failed"; - if (inotify_fd_ >= 0 && thread_.Start()) { + shutdown_pipe_[0] = -1; + shutdown_pipe_[1] = -1; + if (inotify_fd_ >= 0 && pipe(shutdown_pipe_) == 0 && thread_.Start()) { thread_.task_runner()->PostTask( FROM_HERE, - Bind(&InotifyReaderCallback, this, inotify_fd_)); + Bind(&InotifyReaderCallback, this, inotify_fd_, shutdown_pipe_[0])); valid_ = true; } } +InotifyReader::~InotifyReader() { + if (valid_) { + // Write to the self-pipe so that the select call in InotifyReaderTask + // returns. + ssize_t ret = HANDLE_EINTR(write(shutdown_pipe_[1], "", 1)); + DPCHECK(ret > 0); + DCHECK_EQ(ret, 1); + thread_.Stop(); + } + if (inotify_fd_ >= 0) + close(inotify_fd_); + if (shutdown_pipe_[0] >= 0) + close(shutdown_pipe_[0]); + if (shutdown_pipe_[1] >= 0) + close(shutdown_pipe_[1]); +} + InotifyReader::Watch InotifyReader::AddWatch( const FilePath& path, FilePathWatcherImpl* watcher) { if (!valid_) @@ -314,10 +343,7 @@ void InotifyReader::OnInotifyEvent(const inotify_event* event) { } FilePathWatcherImpl::FilePathWatcherImpl() - : recursive_(false), weak_factory_(this) {} - -FilePathWatcherImpl::~FilePathWatcherImpl() { - DCHECK(!task_runner() || task_runner()->RunsTasksOnCurrentThread()); + : recursive_(false) { } void FilePathWatcherImpl::OnFilePathChanged(InotifyReader::Watch fired_watch, @@ -325,25 +351,22 @@ void FilePathWatcherImpl::OnFilePathChanged(InotifyReader::Watch fired_watch, bool created, bool deleted, bool is_dir) { - DCHECK(!task_runner()->RunsTasksOnCurrentThread()); - - // This method is invoked on the Inotify thread. Switch to task_runner() to - // access |watches_| safely. Use a WeakPtr to prevent the callback from - // running after |this| is destroyed (i.e. after the watch is cancelled). - task_runner()->PostTask( - FROM_HERE, Bind(&FilePathWatcherImpl::OnFilePathChangedOnOriginSequence, - weak_factory_.GetWeakPtr(), fired_watch, child, created, - deleted, is_dir)); -} + if (!task_runner()->BelongsToCurrentThread()) { + // Switch to task_runner() to access |watches_| safely. + task_runner()->PostTask(FROM_HERE, + Bind(&FilePathWatcherImpl::OnFilePathChanged, this, + fired_watch, child, created, deleted, is_dir)); + return; + } -void FilePathWatcherImpl::OnFilePathChangedOnOriginSequence( - InotifyReader::Watch fired_watch, - const FilePath::StringType& child, - bool created, - bool deleted, - bool is_dir) { - DCHECK(task_runner()->RunsTasksOnCurrentThread()); - DCHECK(!watches_.empty()); + // Check to see if CancelOnMessageLoopThread() has already been called. + // May happen when code flow reaches here from the PostTask() above. + if (watches_.empty()) { + DCHECK(target_.empty()); + return; + } + + DCHECK(MessageLoopForIO::current()); DCHECK(HasValidWatchVector()); // Used below to avoid multiple recursive updates. @@ -428,11 +451,13 @@ bool FilePathWatcherImpl::Watch(const FilePath& path, bool recursive, const FilePathWatcher::Callback& callback) { DCHECK(target_.empty()); + DCHECK(MessageLoopForIO::current()); - set_task_runner(SequencedTaskRunnerHandle::Get()); + set_task_runner(ThreadTaskRunnerHandle::Get()); callback_ = callback; target_ = path; recursive_ = recursive; + MessageLoop::current()->AddDestructionObserver(this); std::vector<FilePath::StringType> comps; target_.GetComponents(&comps); @@ -445,29 +470,47 @@ bool FilePathWatcherImpl::Watch(const FilePath& path, } void FilePathWatcherImpl::Cancel() { - if (!callback_) { - // Watch() was never called. + if (callback_.is_null()) { + // Watch was never called, or the message_loop() thread is already gone. set_cancelled(); return; } - DCHECK(task_runner()->RunsTasksOnCurrentThread()); - DCHECK(!is_cancelled()); + // Switch to the message_loop() if necessary so we can access |watches_|. + if (!task_runner()->BelongsToCurrentThread()) { + task_runner()->PostTask(FROM_HERE, Bind(&FilePathWatcher::CancelWatch, + make_scoped_refptr(this))); + } else { + CancelOnMessageLoopThread(); + } +} +void FilePathWatcherImpl::CancelOnMessageLoopThread() { + DCHECK(task_runner()->BelongsToCurrentThread()); set_cancelled(); - callback_.Reset(); + + if (!callback_.is_null()) { + MessageLoop::current()->RemoveDestructionObserver(this); + callback_.Reset(); + } for (size_t i = 0; i < watches_.size(); ++i) g_inotify_reader.Get().RemoveWatch(watches_[i].watch, this); watches_.clear(); target_.clear(); - RemoveRecursiveWatches(); + + if (recursive_) + RemoveRecursiveWatches(); +} + +void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() { + CancelOnMessageLoopThread(); } void FilePathWatcherImpl::UpdateWatches() { - // Ensure this runs on the task_runner() exclusively in order to avoid + // Ensure this runs on the message_loop() exclusively in order to avoid // concurrency issues. - DCHECK(task_runner()->RunsTasksOnCurrentThread()); + DCHECK(task_runner()->BelongsToCurrentThread()); DCHECK(HasValidWatchVector()); // Walk the list of watches and update them as we go. @@ -498,8 +541,6 @@ void FilePathWatcherImpl::UpdateWatches() { void FilePathWatcherImpl::UpdateRecursiveWatches( InotifyReader::Watch fired_watch, bool is_dir) { - DCHECK(HasValidWatchVector()); - if (!recursive_) return; @@ -510,8 +551,7 @@ void FilePathWatcherImpl::UpdateRecursiveWatches( // Check to see if this is a forced update or if some component of |target_| // has changed. For these cases, redo the watches for |target_| and below. - if (!ContainsKey(recursive_paths_by_watch_, fired_watch) && - fired_watch != watches_.back().watch) { + if (!ContainsKey(recursive_paths_by_watch_, fired_watch)) { UpdateRecursiveWatchesForPath(target_); return; } @@ -520,10 +560,7 @@ void FilePathWatcherImpl::UpdateRecursiveWatches( if (!is_dir) return; - const FilePath& changed_dir = - ContainsKey(recursive_paths_by_watch_, fired_watch) ? - recursive_paths_by_watch_[fired_watch] : - target_; + const FilePath& changed_dir = recursive_paths_by_watch_[fired_watch]; std::map<FilePath, InotifyReader::Watch>::iterator start_it = recursive_watches_by_path_.lower_bound(changed_dir); @@ -646,8 +683,7 @@ bool FilePathWatcherImpl::HasValidWatchVector() const { } // namespace FilePathWatcher::FilePathWatcher() { - sequence_checker_.DetachFromSequence(); - impl_ = MakeUnique<FilePathWatcherImpl>(); + impl_ = new FilePathWatcherImpl(); } } // namespace base diff --git a/base/files/file_path_watcher_mac.cc b/base/files/file_path_watcher_mac.cc new file mode 100644 index 0000000000..7338eafa44 --- /dev/null +++ b/base/files/file_path_watcher_mac.cc @@ -0,0 +1,61 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/files/file_path_watcher.h" +#include "base/files/file_path_watcher_kqueue.h" +#include "build/build_config.h" + +#if !defined(OS_IOS) +#include "base/files/file_path_watcher_fsevents.h" +#endif + +namespace base { + +namespace { + +class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { + public: + bool Watch(const FilePath& path, + bool recursive, + const FilePathWatcher::Callback& callback) override { + // Use kqueue for non-recursive watches and FSEvents for recursive ones. + DCHECK(!impl_.get()); + if (recursive) { + if (!FilePathWatcher::RecursiveWatchAvailable()) + return false; +#if !defined(OS_IOS) + impl_ = new FilePathWatcherFSEvents(); +#endif // OS_IOS + } else { + impl_ = new FilePathWatcherKQueue(); + } + DCHECK(impl_.get()); + return impl_->Watch(path, recursive, callback); + } + + void Cancel() override { + if (impl_.get()) + impl_->Cancel(); + set_cancelled(); + } + + void CancelOnMessageLoopThread() override { + if (impl_.get()) + impl_->Cancel(); + set_cancelled(); + } + + protected: + ~FilePathWatcherImpl() override {} + + scoped_refptr<PlatformDelegate> impl_; +}; + +} // namespace + +FilePathWatcher::FilePathWatcher() { + impl_ = new FilePathWatcherImpl(); +} + +} // namespace base diff --git a/base/files/file_path_watcher_unittest.cc b/base/files/file_path_watcher_unittest.cc index d2ec37bbec..a40e4858b4 100644 --- a/base/files/file_path_watcher_unittest.cc +++ b/base/files/file_path_watcher_unittest.cc @@ -28,6 +28,7 @@ #include "base/synchronization/waitable_event.h" #include "base/test/test_file_util.h" #include "base/test/test_timeouts.h" +#include "base/threading/thread.h" #include "base/threading/thread_task_runner_handle.h" #include "build/build_config.h" #include "testing/gtest/include/gtest/gtest.h" @@ -36,10 +37,6 @@ #include "base/android/path_utils.h" #endif // defined(OS_ANDROID) -#if defined(OS_POSIX) -#include "base/files/file_descriptor_watcher_posix.h" -#endif // defined(OS_POSIX) - namespace base { namespace { @@ -134,19 +131,30 @@ class TestDelegate : public TestDelegateBase { DISALLOW_COPY_AND_ASSIGN(TestDelegate); }; +void SetupWatchCallback(const FilePath& target, + FilePathWatcher* watcher, + TestDelegateBase* delegate, + bool recursive_watch, + bool* result, + base::WaitableEvent* completion) { + *result = watcher->Watch(target, recursive_watch, + base::Bind(&TestDelegateBase::OnFileChanged, + delegate->AsWeakPtr())); + completion->Signal(); +} + class FilePathWatcherTest : public testing::Test { public: FilePathWatcherTest() -#if defined(OS_POSIX) - : file_descriptor_watcher_(&loop_) -#endif - { - } + : file_thread_("FilePathWatcherTest") {} ~FilePathWatcherTest() override {} protected: void SetUp() override { + // Create a separate file thread in order to test proper thread usage. + base::Thread::Options options(MessageLoop::TYPE_IO, 0); + ASSERT_TRUE(file_thread_.StartWithOptions(options)); #if defined(OS_ANDROID) // Watching files is only permitted when all parent directories are // accessible, which is not the case for the default temp directory @@ -163,12 +171,16 @@ class FilePathWatcherTest : public testing::Test { void TearDown() override { RunLoop().RunUntilIdle(); } + void DeleteDelegateOnFileThread(TestDelegate* delegate) { + file_thread_.task_runner()->DeleteSoon(FROM_HERE, delegate); + } + FilePath test_file() { - return temp_dir_.GetPath().AppendASCII("FilePathWatcherTest"); + return temp_dir_.path().AppendASCII("FilePathWatcherTest"); } FilePath test_link() { - return temp_dir_.GetPath().AppendASCII("FilePathWatcherTest.lnk"); + return temp_dir_.path().AppendASCII("FilePathWatcherTest.lnk"); } // Write |content| to |file|. Returns true on success. @@ -184,23 +196,18 @@ class FilePathWatcherTest : public testing::Test { bool WaitForEvents() WARN_UNUSED_RESULT { collector_->Reset(); - - RunLoop run_loop; // Make sure we timeout if we don't get notified. - ThreadTaskRunnerHandle::Get()->PostDelayedTask( - FROM_HERE, run_loop.QuitWhenIdleClosure(), - TestTimeouts::action_timeout()); - run_loop.Run(); + loop_.PostDelayedTask(FROM_HERE, + MessageLoop::QuitWhenIdleClosure(), + TestTimeouts::action_timeout()); + RunLoop().Run(); return collector_->Success(); } NotificationCollector* collector() { return collector_.get(); } - MessageLoopForIO loop_; -#if defined(OS_POSIX) - FileDescriptorWatcher file_descriptor_watcher_; -#endif - + MessageLoop loop_; + base::Thread file_thread_; ScopedTempDir temp_dir_; scoped_refptr<NotificationCollector> collector_; @@ -212,9 +219,14 @@ bool FilePathWatcherTest::SetupWatch(const FilePath& target, FilePathWatcher* watcher, TestDelegateBase* delegate, bool recursive_watch) { - return watcher->Watch( - target, recursive_watch, - base::Bind(&TestDelegateBase::OnFileChanged, delegate->AsWeakPtr())); + base::WaitableEvent completion(WaitableEvent::ResetPolicy::AUTOMATIC, + WaitableEvent::InitialState::NOT_SIGNALED); + bool result; + file_thread_.task_runner()->PostTask( + FROM_HERE, base::Bind(SetupWatchCallback, target, watcher, delegate, + recursive_watch, &result, &completion)); + completion.Wait(); + return result; } // Basic test: Create the file and verify that we notice. @@ -225,6 +237,7 @@ TEST_F(FilePathWatcherTest, NewFile) { ASSERT_TRUE(WriteFile(test_file(), "content")); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that modifying the file is caught. @@ -238,11 +251,12 @@ TEST_F(FilePathWatcherTest, ModifiedFile) { // Now make sure we get notified if the file is modified. ASSERT_TRUE(WriteFile(test_file(), "new content")); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that moving the file into place is caught. TEST_F(FilePathWatcherTest, MovedFile) { - FilePath source_file(temp_dir_.GetPath().AppendASCII("source")); + FilePath source_file(temp_dir_.path().AppendASCII("source")); ASSERT_TRUE(WriteFile(source_file, "content")); FilePathWatcher watcher; @@ -252,6 +266,7 @@ TEST_F(FilePathWatcherTest, MovedFile) { // Now make sure we get notified if the file is modified. ASSERT_TRUE(base::Move(source_file, test_file())); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } TEST_F(FilePathWatcherTest, DeletedFile) { @@ -264,6 +279,7 @@ TEST_F(FilePathWatcherTest, DeletedFile) { // Now make sure we get notified if the file is deleted. base::DeleteFile(test_file(), false); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Used by the DeleteDuringNotify test below. @@ -311,9 +327,11 @@ TEST_F(FilePathWatcherTest, DeleteDuringNotify) { // Flaky on MacOS (and ARM linux): http://crbug.com/85930 TEST_F(FilePathWatcherTest, DISABLED_DestroyWithPendingNotification) { std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); - FilePathWatcher watcher; - ASSERT_TRUE(SetupWatch(test_file(), &watcher, delegate.get(), false)); + FilePathWatcher* watcher = new FilePathWatcher; + ASSERT_TRUE(SetupWatch(test_file(), watcher, delegate.get(), false)); ASSERT_TRUE(WriteFile(test_file(), "content")); + file_thread_.task_runner()->DeleteSoon(FROM_HERE, watcher); + DeleteDelegateOnFileThread(delegate.release()); } TEST_F(FilePathWatcherTest, MultipleWatchersSingleFile) { @@ -325,13 +343,15 @@ TEST_F(FilePathWatcherTest, MultipleWatchersSingleFile) { ASSERT_TRUE(WriteFile(test_file(), "content")); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate1.release()); + DeleteDelegateOnFileThread(delegate2.release()); } // Verify that watching a file whose parent directory doesn't exist yet works if // the directory and file are created eventually. TEST_F(FilePathWatcherTest, NonExistentDirectory) { FilePathWatcher watcher; - FilePath dir(temp_dir_.GetPath().AppendASCII("dir")); + FilePath dir(temp_dir_.path().AppendASCII("dir")); FilePath file(dir.AppendASCII("file")); std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); ASSERT_TRUE(SetupWatch(file, &watcher, delegate.get(), false)); @@ -350,12 +370,13 @@ TEST_F(FilePathWatcherTest, NonExistentDirectory) { ASSERT_TRUE(base::DeleteFile(file, false)); VLOG(1) << "Waiting for file deletion"; ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Exercises watch reconfiguration for the case that directories on the path // are rapidly created. TEST_F(FilePathWatcherTest, DirectoryChain) { - FilePath path(temp_dir_.GetPath()); + FilePath path(temp_dir_.path()); std::vector<std::string> dir_names; for (int i = 0; i < 20; i++) { std::string dir(base::StringPrintf("d%d", i)); @@ -368,7 +389,7 @@ TEST_F(FilePathWatcherTest, DirectoryChain) { std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); ASSERT_TRUE(SetupWatch(file, &watcher, delegate.get(), false)); - FilePath sub_path(temp_dir_.GetPath()); + FilePath sub_path(temp_dir_.path()); for (std::vector<std::string>::const_iterator d(dir_names.begin()); d != dir_names.end(); ++d) { sub_path = sub_path.AppendASCII(*d); @@ -382,6 +403,7 @@ TEST_F(FilePathWatcherTest, DirectoryChain) { ASSERT_TRUE(WriteFile(file, "content v2")); VLOG(1) << "Waiting for file modification"; ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } #if defined(OS_MACOSX) @@ -390,7 +412,7 @@ TEST_F(FilePathWatcherTest, DirectoryChain) { #endif TEST_F(FilePathWatcherTest, DisappearingDirectory) { FilePathWatcher watcher; - FilePath dir(temp_dir_.GetPath().AppendASCII("dir")); + FilePath dir(temp_dir_.path().AppendASCII("dir")); FilePath file(dir.AppendASCII("file")); ASSERT_TRUE(base::CreateDirectory(dir)); ASSERT_TRUE(WriteFile(file, "content")); @@ -399,6 +421,7 @@ TEST_F(FilePathWatcherTest, DisappearingDirectory) { ASSERT_TRUE(base::DeleteFile(dir, true)); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Tests that a file that is deleted and reappears is tracked correctly. @@ -415,11 +438,12 @@ TEST_F(FilePathWatcherTest, DeleteAndRecreate) { ASSERT_TRUE(WriteFile(test_file(), "content")); VLOG(1) << "Waiting for file creation"; ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } TEST_F(FilePathWatcherTest, WatchDirectory) { FilePathWatcher watcher; - FilePath dir(temp_dir_.GetPath().AppendASCII("dir")); + FilePath dir(temp_dir_.path().AppendASCII("dir")); FilePath file1(dir.AppendASCII("file1")); FilePath file2(dir.AppendASCII("file2")); std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); @@ -447,13 +471,14 @@ TEST_F(FilePathWatcherTest, WatchDirectory) { ASSERT_TRUE(WriteFile(file2, "content")); VLOG(1) << "Waiting for file2 creation"; ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } TEST_F(FilePathWatcherTest, MoveParent) { FilePathWatcher file_watcher; FilePathWatcher subdir_watcher; - FilePath dir(temp_dir_.GetPath().AppendASCII("dir")); - FilePath dest(temp_dir_.GetPath().AppendASCII("dest")); + FilePath dir(temp_dir_.path().AppendASCII("dir")); + FilePath dest(temp_dir_.path().AppendASCII("dest")); FilePath subdir(dir.AppendASCII("subdir")); FilePath file(subdir.AppendASCII("file")); std::unique_ptr<TestDelegate> file_delegate(new TestDelegate(collector())); @@ -472,15 +497,18 @@ TEST_F(FilePathWatcherTest, MoveParent) { base::Move(dir, dest); VLOG(1) << "Waiting for directory move"; ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(file_delegate.release()); + DeleteDelegateOnFileThread(subdir_delegate.release()); } TEST_F(FilePathWatcherTest, RecursiveWatch) { FilePathWatcher watcher; - FilePath dir(temp_dir_.GetPath().AppendASCII("dir")); + FilePath dir(temp_dir_.path().AppendASCII("dir")); std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); bool setup_result = SetupWatch(dir, &watcher, delegate.get(), true); if (!FilePathWatcher::RecursiveWatchAvailable()) { ASSERT_FALSE(setup_result); + DeleteDelegateOnFileThread(delegate.release()); return; } ASSERT_TRUE(setup_result); @@ -536,6 +564,7 @@ TEST_F(FilePathWatcherTest, RecursiveWatch) { // Delete "$dir/subdir/subdir_child_dir/child_dir_file1". ASSERT_TRUE(base::DeleteFile(child_dir_file1, false)); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } #if defined(OS_POSIX) @@ -552,14 +581,14 @@ TEST_F(FilePathWatcherTest, RecursiveWithSymLink) { return; FilePathWatcher watcher; - FilePath test_dir(temp_dir_.GetPath().AppendASCII("test_dir")); + FilePath test_dir(temp_dir_.path().AppendASCII("test_dir")); ASSERT_TRUE(base::CreateDirectory(test_dir)); FilePath symlink(test_dir.AppendASCII("symlink")); std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); ASSERT_TRUE(SetupWatch(symlink, &watcher, delegate.get(), true)); // Link creation. - FilePath target1(temp_dir_.GetPath().AppendASCII("target1")); + FilePath target1(temp_dir_.path().AppendASCII("target1")); ASSERT_TRUE(base::CreateSymbolicLink(target1, symlink)); ASSERT_TRUE(WaitForEvents()); @@ -573,7 +602,7 @@ TEST_F(FilePathWatcherTest, RecursiveWithSymLink) { ASSERT_TRUE(WaitForEvents()); // Link change. - FilePath target2(temp_dir_.GetPath().AppendASCII("target2")); + FilePath target2(temp_dir_.path().AppendASCII("target2")); ASSERT_TRUE(base::CreateDirectory(target2)); ASSERT_TRUE(base::DeleteFile(symlink, false)); ASSERT_TRUE(base::CreateSymbolicLink(target2, symlink)); @@ -583,16 +612,18 @@ TEST_F(FilePathWatcherTest, RecursiveWithSymLink) { FilePath target2_file(target2.AppendASCII("file")); ASSERT_TRUE(WriteFile(target2_file, "content")); ASSERT_TRUE(WaitForEvents()); + + DeleteDelegateOnFileThread(delegate.release()); } #endif // OS_POSIX TEST_F(FilePathWatcherTest, MoveChild) { FilePathWatcher file_watcher; FilePathWatcher subdir_watcher; - FilePath source_dir(temp_dir_.GetPath().AppendASCII("source")); + FilePath source_dir(temp_dir_.path().AppendASCII("source")); FilePath source_subdir(source_dir.AppendASCII("subdir")); FilePath source_file(source_subdir.AppendASCII("file")); - FilePath dest_dir(temp_dir_.GetPath().AppendASCII("dest")); + FilePath dest_dir(temp_dir_.path().AppendASCII("dest")); FilePath dest_subdir(dest_dir.AppendASCII("subdir")); FilePath dest_file(dest_subdir.AppendASCII("file")); @@ -609,6 +640,8 @@ TEST_F(FilePathWatcherTest, MoveChild) { // Move the directory into place, s.t. the watched file appears. ASSERT_TRUE(base::Move(source_dir, dest_dir)); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(file_delegate.release()); + DeleteDelegateOnFileThread(subdir_delegate.release()); } // Verify that changing attributes on a file is caught @@ -629,6 +662,7 @@ TEST_F(FilePathWatcherTest, FileAttributesChanged) { // Now make sure we get notified if the file is modified. ASSERT_TRUE(base::MakeFileUnreadable(test_file())); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } #if defined(OS_LINUX) @@ -644,6 +678,7 @@ TEST_F(FilePathWatcherTest, CreateLink) { // Note that test_file() doesn't have to exist. ASSERT_TRUE(CreateSymbolicLink(test_file(), test_link())); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that deleting a symlink is caught. @@ -659,6 +694,7 @@ TEST_F(FilePathWatcherTest, DeleteLink) { // Now make sure we get notified if the link is deleted. ASSERT_TRUE(base::DeleteFile(test_link(), false)); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that modifying a target file that a link is pointing to @@ -674,6 +710,7 @@ TEST_F(FilePathWatcherTest, ModifiedLinkedFile) { // Now make sure we get notified if the file is modified. ASSERT_TRUE(WriteFile(test_file(), "new content")); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that creating a target file that a link is pointing to @@ -688,6 +725,7 @@ TEST_F(FilePathWatcherTest, CreateTargetLinkedFile) { // Now make sure we get notified if the target file is created. ASSERT_TRUE(WriteFile(test_file(), "content")); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that deleting a target file that a link is pointing to @@ -703,14 +741,15 @@ TEST_F(FilePathWatcherTest, DeleteTargetLinkedFile) { // Now make sure we get notified if the target file is deleted. ASSERT_TRUE(base::DeleteFile(test_file(), false)); ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that watching a file whose parent directory is a link that // doesn't exist yet works if the symlink is created eventually. TEST_F(FilePathWatcherTest, LinkedDirectoryPart1) { FilePathWatcher watcher; - FilePath dir(temp_dir_.GetPath().AppendASCII("dir")); - FilePath link_dir(temp_dir_.GetPath().AppendASCII("dir.lnk")); + FilePath dir(temp_dir_.path().AppendASCII("dir")); + FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk")); FilePath file(dir.AppendASCII("file")); FilePath linkfile(link_dir.AppendASCII("file")); std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); @@ -731,14 +770,15 @@ TEST_F(FilePathWatcherTest, LinkedDirectoryPart1) { ASSERT_TRUE(base::DeleteFile(file, false)); VLOG(1) << "Waiting for file deletion"; ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that watching a file whose parent directory is a // dangling symlink works if the directory is created eventually. TEST_F(FilePathWatcherTest, LinkedDirectoryPart2) { FilePathWatcher watcher; - FilePath dir(temp_dir_.GetPath().AppendASCII("dir")); - FilePath link_dir(temp_dir_.GetPath().AppendASCII("dir.lnk")); + FilePath dir(temp_dir_.path().AppendASCII("dir")); + FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk")); FilePath file(dir.AppendASCII("file")); FilePath linkfile(link_dir.AppendASCII("file")); std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); @@ -760,14 +800,15 @@ TEST_F(FilePathWatcherTest, LinkedDirectoryPart2) { ASSERT_TRUE(base::DeleteFile(file, false)); VLOG(1) << "Waiting for file deletion"; ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } // Verify that watching a file with a symlink on the path // to the file works. TEST_F(FilePathWatcherTest, LinkedDirectoryPart3) { FilePathWatcher watcher; - FilePath dir(temp_dir_.GetPath().AppendASCII("dir")); - FilePath link_dir(temp_dir_.GetPath().AppendASCII("dir.lnk")); + FilePath dir(temp_dir_.path().AppendASCII("dir")); + FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk")); FilePath file(dir.AppendASCII("file")); FilePath linkfile(link_dir.AppendASCII("file")); std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector())); @@ -787,6 +828,7 @@ TEST_F(FilePathWatcherTest, LinkedDirectoryPart3) { ASSERT_TRUE(base::DeleteFile(file, false)); VLOG(1) << "Waiting for file deletion"; ASSERT_TRUE(WaitForEvents()); + DeleteDelegateOnFileThread(delegate.release()); } #endif // OS_LINUX @@ -837,8 +879,7 @@ bool ChangeFilePermissions(const FilePath& path, Permission perm, bool allow) { // Verify that changing attributes on a directory works. TEST_F(FilePathWatcherTest, DirAttributesChanged) { - FilePath test_dir1( - temp_dir_.GetPath().AppendASCII("DirAttributesChangedDir1")); + FilePath test_dir1(temp_dir_.path().AppendASCII("DirAttributesChangedDir1")); FilePath test_dir2(test_dir1.AppendASCII("DirAttributesChangedDir2")); FilePath test_file(test_dir2.AppendASCII("DirAttributesChangedFile")); // Setup a directory hierarchy. @@ -864,6 +905,7 @@ TEST_F(FilePathWatcherTest, DirAttributesChanged) { ASSERT_TRUE(ChangeFilePermissions(test_dir1, Execute, false)); ASSERT_TRUE(WaitForEvents()); ASSERT_TRUE(ChangeFilePermissions(test_dir1, Execute, true)); + DeleteDelegateOnFileThread(delegate.release()); } #endif // OS_MACOSX diff --git a/base/files/file_posix.cc b/base/files/file_posix.cc index 2738d6c45c..12f80c4f8f 100644 --- a/base/files/file_posix.cc +++ b/base/files/file_posix.cc @@ -11,7 +11,7 @@ #include <unistd.h> #include "base/logging.h" -#include "base/metrics/histogram_macros.h" +#include "base/metrics/sparse_histogram.h" #include "base/posix/eintr_wrapper.h" #include "base/strings/utf_string_conversions.h" #include "base/threading/thread_restrictions.h" @@ -323,7 +323,7 @@ int64_t File::GetLength() { stat_wrapper_t file_info; if (CallFstat(file_.get(), &file_info)) - return -1; + return false; return file_info.st_size; } @@ -372,7 +372,7 @@ File::Error File::Unlock() { return CallFcntlFlock(file_.get(), false); } -File File::Duplicate() const { +File File::Duplicate() { if (!IsValid()) return File(); @@ -513,10 +513,9 @@ void File::DoInitialize(const FilePath& path, uint32_t flags) { } #endif // !defined(OS_NACL) -bool File::Flush() { +bool File::DoFlush() { ThreadRestrictions::AssertIOAllowed(); DCHECK(IsValid()); - SCOPED_FILE_TRACE("Flush"); #if defined(OS_NACL) NOTIMPLEMENTED(); // NaCl doesn't implement fsync. diff --git a/base/files/file_tracing.cc b/base/files/file_tracing.cc index 48f57412f9..6d11cbc746 100644 --- a/base/files/file_tracing.cc +++ b/base/files/file_tracing.cc @@ -4,62 +4,47 @@ #include "base/files/file_tracing.h" -#include "base/atomicops.h" #include "base/files/file.h" -using base::subtle::AtomicWord; - namespace base { namespace { -AtomicWord g_provider; -} - -FileTracing::Provider* GetProvider() { - AtomicWord provider = base::subtle::Acquire_Load(&g_provider); - return reinterpret_cast<FileTracing::Provider*>(provider); +FileTracing::Provider* g_provider = nullptr; } // static bool FileTracing::IsCategoryEnabled() { - FileTracing::Provider* provider = GetProvider(); - return provider && provider->FileTracingCategoryIsEnabled(); + return g_provider && g_provider->FileTracingCategoryIsEnabled(); } // static void FileTracing::SetProvider(FileTracing::Provider* provider) { - base::subtle::Release_Store(&g_provider, - reinterpret_cast<AtomicWord>(provider)); + g_provider = provider; } FileTracing::ScopedEnabler::ScopedEnabler() { - FileTracing::Provider* provider = GetProvider(); - if (provider) - provider->FileTracingEnable(this); + if (g_provider) + g_provider->FileTracingEnable(this); } FileTracing::ScopedEnabler::~ScopedEnabler() { - FileTracing::Provider* provider = GetProvider(); - if (provider) - provider->FileTracingDisable(this); + if (g_provider) + g_provider->FileTracingDisable(this); } FileTracing::ScopedTrace::ScopedTrace() : id_(nullptr) {} FileTracing::ScopedTrace::~ScopedTrace() { - if (id_) { - FileTracing::Provider* provider = GetProvider(); - if (provider) - provider->FileTracingEventEnd(name_, id_); - } + if (id_ && g_provider) + g_provider->FileTracingEventEnd(name_, id_); } void FileTracing::ScopedTrace::Initialize(const char* name, - const File* file, + File* file, int64_t size) { id_ = &file->trace_enabler_; name_ = name; - GetProvider()->FileTracingEventBegin(name_, id_, file->tracing_path_, size); + g_provider->FileTracingEventBegin(name_, id_, file->tracing_path_, size); } } // namespace base diff --git a/base/files/file_tracing.h b/base/files/file_tracing.h index 1fbfcd4498..bedd7be64b 100644 --- a/base/files/file_tracing.h +++ b/base/files/file_tracing.h @@ -37,21 +37,21 @@ class BASE_EXPORT FileTracing { virtual bool FileTracingCategoryIsEnabled() const = 0; // Enables file tracing for |id|. Must be called before recording events. - virtual void FileTracingEnable(const void* id) = 0; + virtual void FileTracingEnable(void* id) = 0; // Disables file tracing for |id|. - virtual void FileTracingDisable(const void* id) = 0; + virtual void FileTracingDisable(void* id) = 0; // Begins an event for |id| with |name|. |path| tells where in the directory // structure the event is happening (and may be blank). |size| is the number // of bytes involved in the event. virtual void FileTracingEventBegin(const char* name, - const void* id, + void* id, const FilePath& path, int64_t size) = 0; // Ends an event for |id| with |name|. - virtual void FileTracingEventEnd(const char* name, const void* id) = 0; + virtual void FileTracingEventEnd(const char* name, void* id) = 0; }; // Sets a global file tracing provider to query categories and record events. @@ -73,12 +73,12 @@ class BASE_EXPORT FileTracing { // event to trace (e.g. "Read", "Write") and must have an application // lifetime (e.g. static or literal). |file| is the file being traced; must // outlive this class. |size| is the size (in bytes) of this event. - void Initialize(const char* name, const File* file, int64_t size); + void Initialize(const char* name, File* file, int64_t size); private: // The ID of this trace. Based on the |file| passed to |Initialize()|. Must // outlive this class. - const void* id_; + void* id_; // The name of the event to trace (e.g. "Read", "Write"). Prefixed with // "File". diff --git a/base/files/file_unittest.cc b/base/files/file_unittest.cc index 66c312b60d..2445f7e128 100644 --- a/base/files/file_unittest.cc +++ b/base/files/file_unittest.cc @@ -9,7 +9,6 @@ #include <utility> #include "base/files/file_util.h" -#include "base/files/memory_mapped_file.h" #include "base/files/scoped_temp_dir.h" #include "base/time/time.h" #include "build/build_config.h" @@ -21,7 +20,7 @@ using base::FilePath; TEST(FileTest, Create) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("create_file_1"); + FilePath file_path = temp_dir.path().AppendASCII("create_file_1"); { // Don't create a File at all. @@ -93,7 +92,7 @@ TEST(FileTest, Create) { { // Create a delete-on-close file. - file_path = temp_dir.GetPath().AppendASCII("create_file_2"); + file_path = temp_dir.path().AppendASCII("create_file_2"); File file(file_path, base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_READ | base::File::FLAG_DELETE_ON_CLOSE); @@ -108,7 +107,7 @@ TEST(FileTest, Create) { TEST(FileTest, Async) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("create_file"); + FilePath file_path = temp_dir.path().AppendASCII("create_file"); { File file(file_path, base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_ASYNC); @@ -126,7 +125,7 @@ TEST(FileTest, Async) { TEST(FileTest, DeleteOpenFile) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("create_file_1"); + FilePath file_path = temp_dir.path().AppendASCII("create_file_1"); // Create a file. File file(file_path, @@ -153,7 +152,7 @@ TEST(FileTest, DeleteOpenFile) { TEST(FileTest, ReadWrite) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("read_write_file"); + FilePath file_path = temp_dir.path().AppendASCII("read_write_file"); File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_READ | base::File::FLAG_WRITE); @@ -225,7 +224,7 @@ TEST(FileTest, ReadWrite) { TEST(FileTest, Append) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("append_file"); + FilePath file_path = temp_dir.path().AppendASCII("append_file"); File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_APPEND); ASSERT_TRUE(file.IsValid()); @@ -273,7 +272,7 @@ TEST(FileTest, Append) { TEST(FileTest, Length) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("truncate_file"); + FilePath file_path = temp_dir.path().AppendASCII("truncate_file"); File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_READ | base::File::FLAG_WRITE); @@ -325,7 +324,7 @@ TEST(FileTest, DISABLED_TouchGetInfo) { #endif base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - File file(temp_dir.GetPath().AppendASCII("touch_get_info_file"), + File file(temp_dir.path().AppendASCII("touch_get_info_file"), base::File::FLAG_CREATE | base::File::FLAG_WRITE | base::File::FLAG_WRITE_ATTRIBUTES); ASSERT_TRUE(file.IsValid()); @@ -388,8 +387,7 @@ TEST(FileTest, DISABLED_TouchGetInfo) { TEST(FileTest, ReadAtCurrentPosition) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = - temp_dir.GetPath().AppendASCII("read_at_current_position"); + FilePath file_path = temp_dir.path().AppendASCII("read_at_current_position"); File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_READ | base::File::FLAG_WRITE); @@ -413,8 +411,7 @@ TEST(FileTest, ReadAtCurrentPosition) { TEST(FileTest, WriteAtCurrentPosition) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = - temp_dir.GetPath().AppendASCII("write_at_current_position"); + FilePath file_path = temp_dir.path().AppendASCII("write_at_current_position"); File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_READ | base::File::FLAG_WRITE); @@ -437,7 +434,7 @@ TEST(FileTest, WriteAtCurrentPosition) { TEST(FileTest, Seek) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("seek_file"); + FilePath file_path = temp_dir.path().AppendASCII("seek_file"); File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_READ | base::File::FLAG_WRITE); @@ -454,7 +451,7 @@ TEST(FileTest, Seek) { TEST(FileTest, Duplicate) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); + FilePath file_path = temp_dir.path().AppendASCII("file"); File file(file_path,(base::File::FLAG_CREATE | base::File::FLAG_READ | base::File::FLAG_WRITE)); @@ -481,7 +478,7 @@ TEST(FileTest, Duplicate) { TEST(FileTest, DuplicateDeleteOnClose) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); + FilePath file_path = temp_dir.path().AppendASCII("file"); File file(file_path,(base::File::FLAG_CREATE | base::File::FLAG_READ | base::File::FLAG_WRITE | @@ -498,8 +495,7 @@ TEST(FileTest, DuplicateDeleteOnClose) { TEST(FileTest, GetInfoForDirectory) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath empty_dir = - temp_dir.GetPath().Append(FILE_PATH_LITERAL("gpfi_test")); + FilePath empty_dir = temp_dir.path().Append(FILE_PATH_LITERAL("gpfi_test")); ASSERT_TRUE(CreateDirectory(empty_dir)); base::File dir( @@ -518,158 +514,4 @@ TEST(FileTest, GetInfoForDirectory) { EXPECT_FALSE(info.is_symbolic_link); EXPECT_EQ(0, info.size); } - -TEST(FileTest, DeleteNoop) { - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); - - // Creating and closing a file with DELETE perms should do nothing special. - File file(file_path, - (base::File::FLAG_CREATE | base::File::FLAG_READ | - base::File::FLAG_WRITE | base::File::FLAG_CAN_DELETE_ON_CLOSE)); - ASSERT_TRUE(file.IsValid()); - file.Close(); - ASSERT_TRUE(base::PathExists(file_path)); -} - -TEST(FileTest, Delete) { - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); - - // Creating a file with DELETE and then marking for delete on close should - // delete it. - File file(file_path, - (base::File::FLAG_CREATE | base::File::FLAG_READ | - base::File::FLAG_WRITE | base::File::FLAG_CAN_DELETE_ON_CLOSE)); - ASSERT_TRUE(file.IsValid()); - ASSERT_TRUE(file.DeleteOnClose(true)); - file.Close(); - ASSERT_FALSE(base::PathExists(file_path)); -} - -TEST(FileTest, DeleteThenRevoke) { - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); - - // Creating a file with DELETE, marking it for delete, then clearing delete on - // close should not delete it. - File file(file_path, - (base::File::FLAG_CREATE | base::File::FLAG_READ | - base::File::FLAG_WRITE | base::File::FLAG_CAN_DELETE_ON_CLOSE)); - ASSERT_TRUE(file.IsValid()); - ASSERT_TRUE(file.DeleteOnClose(true)); - ASSERT_TRUE(file.DeleteOnClose(false)); - file.Close(); - ASSERT_TRUE(base::PathExists(file_path)); -} - -TEST(FileTest, IrrevokableDeleteOnClose) { - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); - - // DELETE_ON_CLOSE cannot be revoked by this opener. - File file( - file_path, - (base::File::FLAG_CREATE | base::File::FLAG_READ | - base::File::FLAG_WRITE | base::File::FLAG_DELETE_ON_CLOSE | - base::File::FLAG_SHARE_DELETE | base::File::FLAG_CAN_DELETE_ON_CLOSE)); - ASSERT_TRUE(file.IsValid()); - // https://msdn.microsoft.com/library/windows/desktop/aa364221.aspx says that - // setting the dispositon has no effect if the handle was opened with - // FLAG_DELETE_ON_CLOSE. Do not make the test's success dependent on whether - // or not SetFileInformationByHandle indicates success or failure. (It happens - // to indicate success on Windows 10.) - file.DeleteOnClose(false); - file.Close(); - ASSERT_FALSE(base::PathExists(file_path)); -} - -TEST(FileTest, IrrevokableDeleteOnCloseOther) { - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); - - // DELETE_ON_CLOSE cannot be revoked by another opener. - File file( - file_path, - (base::File::FLAG_CREATE | base::File::FLAG_READ | - base::File::FLAG_WRITE | base::File::FLAG_DELETE_ON_CLOSE | - base::File::FLAG_SHARE_DELETE | base::File::FLAG_CAN_DELETE_ON_CLOSE)); - ASSERT_TRUE(file.IsValid()); - - File file2( - file_path, - (base::File::FLAG_OPEN | base::File::FLAG_READ | base::File::FLAG_WRITE | - base::File::FLAG_SHARE_DELETE | base::File::FLAG_CAN_DELETE_ON_CLOSE)); - ASSERT_TRUE(file2.IsValid()); - - file2.DeleteOnClose(false); - file2.Close(); - ASSERT_TRUE(base::PathExists(file_path)); - file.Close(); - ASSERT_FALSE(base::PathExists(file_path)); -} - -TEST(FileTest, DeleteWithoutPermission) { - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); - - // It should not be possible to mark a file for deletion when it was not - // created/opened with DELETE. - File file(file_path, (base::File::FLAG_CREATE | base::File::FLAG_READ | - base::File::FLAG_WRITE)); - ASSERT_TRUE(file.IsValid()); - ASSERT_FALSE(file.DeleteOnClose(true)); - file.Close(); - ASSERT_TRUE(base::PathExists(file_path)); -} - -TEST(FileTest, UnsharedDeleteOnClose) { - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); - - // Opening with DELETE_ON_CLOSE when a previous opener hasn't enabled sharing - // will fail. - File file(file_path, (base::File::FLAG_CREATE | base::File::FLAG_READ | - base::File::FLAG_WRITE)); - ASSERT_TRUE(file.IsValid()); - File file2( - file_path, - (base::File::FLAG_OPEN | base::File::FLAG_READ | base::File::FLAG_WRITE | - base::File::FLAG_DELETE_ON_CLOSE | base::File::FLAG_SHARE_DELETE)); - ASSERT_FALSE(file2.IsValid()); - - file.Close(); - ASSERT_TRUE(base::PathExists(file_path)); -} - -TEST(FileTest, NoDeleteOnCloseWithMappedFile) { - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - FilePath file_path = temp_dir.GetPath().AppendASCII("file"); - - // Mapping a file into memory blocks DeleteOnClose. - File file(file_path, - (base::File::FLAG_CREATE | base::File::FLAG_READ | - base::File::FLAG_WRITE | base::File::FLAG_CAN_DELETE_ON_CLOSE)); - ASSERT_TRUE(file.IsValid()); - ASSERT_EQ(5, file.WriteAtCurrentPos("12345", 5)); - - { - base::MemoryMappedFile mapping; - ASSERT_TRUE(mapping.Initialize(file.Duplicate())); - ASSERT_EQ(5U, mapping.length()); - - EXPECT_FALSE(file.DeleteOnClose(true)); - } - - file.Close(); - ASSERT_TRUE(base::PathExists(file_path)); -} #endif // defined(OS_WIN) diff --git a/base/files/file_util.h b/base/files/file_util.h index 5ada35f9a4..420dcaee61 100644 --- a/base/files/file_util.h +++ b/base/files/file_util.h @@ -294,6 +294,10 @@ BASE_EXPORT bool DevicePathToDriveLetterPath(const FilePath& device_path, // be resolved with this function. BASE_EXPORT bool NormalizeToNativeFilePath(const FilePath& path, FilePath* nt_path); + +// Given an existing file in |path|, returns whether this file is on a network +// drive or not. If |path| does not exist, this function returns false. +BASE_EXPORT bool IsOnNetworkDrive(const base::FilePath& path); #endif // This function will return if the given file is a symlink or not. @@ -307,9 +311,7 @@ BASE_EXPORT bool TouchFile(const FilePath& path, const Time& last_accessed, const Time& last_modified); -// Wrapper for fopen-like calls. Returns non-NULL FILE* on success. The -// underlying file descriptor (POSIX) or handle (Windows) is unconditionally -// configured to not be propagated to child processes. +// Wrapper for fopen-like calls. Returns non-NULL FILE* on success. BASE_EXPORT FILE* OpenFile(const FilePath& filename, const char* mode); // Closes file opened by OpenFile. Returns true on success. @@ -363,17 +365,6 @@ BASE_EXPORT int GetUniquePathNumber(const FilePath& path, BASE_EXPORT bool SetNonBlocking(int fd); #if defined(OS_POSIX) -// Creates a non-blocking, close-on-exec pipe. -// This creates a non-blocking pipe that is not intended to be shared with any -// child process. This will be done atomically if the operating system supports -// it. Returns true if it was able to create the pipe, otherwise false. -BASE_EXPORT bool CreateLocalNonBlockingPipe(int fds[2]); - -// Sets the given |fd| to close-on-exec mode. -// Returns true if it was able to set it in the close-on-exec mode, otherwise -// false. -BASE_EXPORT bool SetCloseOnExec(int fd); - // Test that |path| can only be changed by a given user and members of // a given set of groups. // Specifically, test that all parts of |path| under (and including) |base|: diff --git a/base/files/file_util_mac.mm b/base/files/file_util_mac.mm index 5a99aa0e81..e9c6c65159 100644 --- a/base/files/file_util_mac.mm +++ b/base/files/file_util_mac.mm @@ -4,9 +4,8 @@ #include "base/files/file_util.h" -#import <Foundation/Foundation.h> #include <copyfile.h> -#include <stdlib.h> +#import <Foundation/Foundation.h> #include "base/files/file_path.h" #include "base/mac/foundation_util.h" @@ -24,15 +23,6 @@ bool CopyFile(const FilePath& from_path, const FilePath& to_path) { } bool GetTempDir(base::FilePath* path) { - // In order to facilitate hermetic runs on macOS, first check $TMPDIR. - // NOTE: $TMPDIR is ALMOST ALWAYS set on macOS (unless the user un-set it). - const char* env_tmpdir = getenv("TMPDIR"); - if (env_tmpdir) { - *path = base::FilePath(env_tmpdir); - return true; - } - - // If we didn't find it, fall back to the native function. NSString* tmp = NSTemporaryDirectory(); if (tmp == nil) return false; diff --git a/base/files/file_util_posix.cc b/base/files/file_util_posix.cc index a03ca8d8d8..85a1b41d46 100644 --- a/base/files/file_util_posix.cc +++ b/base/files/file_util_posix.cc @@ -185,19 +185,6 @@ bool DetermineDevShmExecutable() { #endif // defined(OS_LINUX) #endif // !defined(OS_NACL_NONSFI) -#if !defined(OS_MACOSX) -// Appends |mode_char| to |mode| before the optional character set encoding; see -// https://www.gnu.org/software/libc/manual/html_node/Opening-Streams.html for -// details. -std::string AppendModeCharacter(StringPiece mode, char mode_char) { - std::string result(mode.as_string()); - size_t comma_pos = result.find(','); - result.insert(comma_pos == std::string::npos ? result.length() : comma_pos, 1, - mode_char); - return result; -} -#endif - } // namespace #if !defined(OS_NACL_NONSFI) @@ -289,8 +276,11 @@ bool CopyDirectory(const FilePath& from_path, FilePath real_from_path = MakeAbsoluteFilePath(from_path); if (real_from_path.empty()) return false; - if (real_to_path == real_from_path || real_from_path.IsParent(real_to_path)) + if (real_to_path.value().size() >= real_from_path.value().size() && + real_to_path.value().compare(0, real_from_path.value().size(), + real_from_path.value()) == 0) { return false; + } int traverse_type = FileEnumerator::FILES | FileEnumerator::SHOW_SYM_LINKS; if (recursive) @@ -361,29 +351,6 @@ bool CopyDirectory(const FilePath& from_path, } #endif // !defined(OS_NACL_NONSFI) -bool CreateLocalNonBlockingPipe(int fds[2]) { -#if defined(OS_LINUX) - return pipe2(fds, O_CLOEXEC | O_NONBLOCK) == 0; -#else - int raw_fds[2]; - if (pipe(raw_fds) != 0) - return false; - ScopedFD fd_out(raw_fds[0]); - ScopedFD fd_in(raw_fds[1]); - if (!SetCloseOnExec(fd_out.get())) - return false; - if (!SetCloseOnExec(fd_in.get())) - return false; - if (!SetNonBlocking(fd_out.get())) - return false; - if (!SetNonBlocking(fd_in.get())) - return false; - fds[0] = fd_out.release(); - fds[1] = fd_in.release(); - return true; -#endif -} - bool SetNonBlocking(int fd) { const int flags = fcntl(fd, F_GETFL); if (flags == -1) @@ -395,21 +362,6 @@ bool SetNonBlocking(int fd) { return true; } -bool SetCloseOnExec(int fd) { -#if defined(OS_NACL_NONSFI) - const int flags = 0; -#else - const int flags = fcntl(fd, F_GETFD); - if (flags == -1) - return false; - if (flags & FD_CLOEXEC) - return true; -#endif // defined(OS_NACL_NONSFI) - if (HANDLE_EINTR(fcntl(fd, F_SETFD, flags | FD_CLOEXEC)) == -1) - return false; - return true; -} - bool PathExists(const FilePath& path) { ThreadRestrictions::AssertIOAllowed(); #if defined(OS_ANDROID) @@ -725,29 +677,11 @@ bool GetFileInfo(const FilePath& file_path, File::Info* results) { #endif // !defined(OS_NACL_NONSFI) FILE* OpenFile(const FilePath& filename, const char* mode) { - // 'e' is unconditionally added below, so be sure there is not one already - // present before a comma in |mode|. - DCHECK( - strchr(mode, 'e') == nullptr || - (strchr(mode, ',') != nullptr && strchr(mode, 'e') > strchr(mode, ','))); ThreadRestrictions::AssertIOAllowed(); FILE* result = NULL; -#if defined(OS_MACOSX) - // macOS does not provide a mode character to set O_CLOEXEC; see - // https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/fopen.3.html. - const char* the_mode = mode; -#else - std::string mode_with_e(AppendModeCharacter(mode, 'e')); - const char* the_mode = mode_with_e.c_str(); -#endif do { - result = fopen(filename.value().c_str(), the_mode); + result = fopen(filename.value().c_str(), mode); } while (!result && errno == EINTR); -#if defined(OS_MACOSX) - // Mark the descriptor as close-on-exec. - if (result) - SetCloseOnExec(fileno(result)); -#endif return result; } diff --git a/base/files/important_file_writer.cc b/base/files/important_file_writer.cc index b46846277b..28550ad52f 100644 --- a/base/files/important_file_writer.cc +++ b/base/files/important_file_writer.cc @@ -18,7 +18,7 @@ #include "base/files/file_util.h" #include "base/logging.h" #include "base/macros.h" -#include "base/metrics/histogram_macros.h" +#include "base/metrics/histogram.h" #include "base/numerics/safe_conversions.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_util.h" @@ -57,18 +57,9 @@ void LogFailure(const FilePath& path, TempFileFailure failure_code, // Helper function to call WriteFileAtomically() with a // std::unique_ptr<std::string>. -void WriteScopedStringToFileAtomically( - const FilePath& path, - std::unique_ptr<std::string> data, - Closure before_write_callback, - Callback<void(bool success)> after_write_callback) { - if (!before_write_callback.is_null()) - before_write_callback.Run(); - - bool result = ImportantFileWriter::WriteFileAtomically(path, *data); - - if (!after_write_callback.is_null()) - after_write_callback.Run(result); +bool WriteScopedStringToFileAtomically(const FilePath& path, + std::unique_ptr<std::string> data) { + return ImportantFileWriter::WriteFileAtomically(path, *data); } } // namespace @@ -103,7 +94,6 @@ bool ImportantFileWriter::WriteFileAtomically(const FilePath& path, File tmp_file(tmp_file_path, File::FLAG_OPEN | File::FLAG_WRITE); if (!tmp_file.IsValid()) { LogFailure(path, FAILED_OPENING, "could not open temporary file"); - DeleteFile(tmp_file_path, false); return false; } @@ -178,11 +168,8 @@ void ImportantFileWriter::WriteNow(std::unique_ptr<std::string> data) { if (HasPendingWrite()) timer_.Stop(); - Closure task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data), - Passed(&before_next_write_callback_), - Passed(&after_next_write_callback_)); - - if (!task_runner_->PostTask(FROM_HERE, MakeCriticalClosure(task))) { + auto task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data)); + if (!PostWriteTask(task)) { // Posting the task to background message loop is not expected // to fail, but if it does, avoid losing data and just hit the disk // on the current thread. @@ -216,11 +203,37 @@ void ImportantFileWriter::DoScheduledWrite() { serializer_ = nullptr; } -void ImportantFileWriter::RegisterOnNextWriteCallbacks( - const Closure& before_next_write_callback, - const Callback<void(bool success)>& after_next_write_callback) { - before_next_write_callback_ = before_next_write_callback; - after_next_write_callback_ = after_next_write_callback; +void ImportantFileWriter::RegisterOnNextSuccessfulWriteCallback( + const Closure& on_next_successful_write) { + DCHECK(on_next_successful_write_.is_null()); + on_next_successful_write_ = on_next_successful_write; +} + +bool ImportantFileWriter::PostWriteTask(const Callback<bool()>& task) { + // TODO(gab): This code could always use PostTaskAndReplyWithResult and let + // ForwardSuccessfulWrite() no-op if |on_next_successful_write_| is null, but + // PostTaskAndReply causes memory leaks in tests (crbug.com/371974) and + // suppressing all of those is unrealistic hence we avoid most of them by + // using PostTask() in the typical scenario below. + if (!on_next_successful_write_.is_null()) { + return PostTaskAndReplyWithResult( + task_runner_.get(), + FROM_HERE, + MakeCriticalClosure(task), + Bind(&ImportantFileWriter::ForwardSuccessfulWrite, + weak_factory_.GetWeakPtr())); + } + return task_runner_->PostTask( + FROM_HERE, + MakeCriticalClosure(Bind(IgnoreResult(task)))); +} + +void ImportantFileWriter::ForwardSuccessfulWrite(bool result) { + DCHECK(CalledOnValidThread()); + if (result && !on_next_successful_write_.is_null()) { + on_next_successful_write_.Run(); + on_next_successful_write_.Reset(); + } } } // namespace base diff --git a/base/files/important_file_writer.h b/base/files/important_file_writer.h index f154b043b2..0bd8a7fd35 100644 --- a/base/files/important_file_writer.h +++ b/base/files/important_file_writer.h @@ -20,21 +20,24 @@ namespace base { class SequencedTaskRunner; +class Thread; -// Helper for atomically writing a file to ensure that it won't be corrupted by -// *application* crash during write (implemented as create, flush, rename). +// Helper to ensure that a file won't be corrupted by the write (for example on +// application crash). Consider a naive way to save an important file F: // -// As an added benefit, ImportantFileWriter makes it less likely that the file -// is corrupted by *system* crash, though even if the ImportantFileWriter call -// has already returned at the time of the crash it is not specified which -// version of the file (old or new) is preserved. And depending on system -// configuration (hardware and software) a significant likelihood of file -// corruption may remain, thus using ImportantFileWriter is not a valid -// substitute for file integrity checks and recovery codepaths for malformed -// files. +// 1. Open F for writing, truncating it. +// 2. Write new data to F. // -// Also note that ImportantFileWriter can be *really* slow (cf. File::Flush() -// for details) and thus please don't block shutdown on ImportantFileWriter. +// It's good when it works, but it gets very bad if step 2. doesn't complete. +// It can be caused by a crash, a computer hang, or a weird I/O error. And you +// end up with a broken file. +// +// To be safe, we don't start with writing directly to F. Instead, we write to +// to a temporary file. Only after that write is successful, we rename the +// temporary file to target filename. +// +// If you want to know more about this approach and ext3/ext4 fsync issues, see +// http://blog.valerieaurora.org/2009/04/16/dont-panic-fsync-ext34-and-your-data/ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe { public: // Used by ScheduleSave to lazily provide the data to be saved. Allows us @@ -50,9 +53,8 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe { virtual ~DataSerializer() {} }; - // Save |data| to |path| in an atomic manner. Blocks and writes data on the - // current thread. Does not guarantee file integrity across system crash (see - // the class comment above). + // Save |data| to |path| in an atomic manner (see the class comment above). + // Blocks and writes data on the current thread. static bool WriteFileAtomically(const FilePath& path, StringPiece data); // Initialize the writer. @@ -93,26 +95,25 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe { // Serialize data pending to be saved and execute write on backend thread. void DoScheduledWrite(); - // Registers |before_next_write_callback| and |after_next_write_callback| to - // be synchronously invoked from WriteFileAtomically() before its next write - // and after its next write, respectively. The boolean passed to - // |after_next_write_callback| indicates whether the write was successful. - // Both callbacks must be thread safe as they will be called on |task_runner_| - // and may be called during Chrome shutdown. - // If called more than once before a write is scheduled on |task_runner|, the - // latest callbacks clobber the others. - void RegisterOnNextWriteCallbacks( - const Closure& before_next_write_callback, - const Callback<void(bool success)>& after_next_write_callback); + // Registers |on_next_successful_write| to be called once, on the next + // successful write event. Only one callback can be set at once. + void RegisterOnNextSuccessfulWriteCallback( + const Closure& on_next_successful_write); TimeDelta commit_interval() const { return commit_interval_; } private: - // Invoked synchronously on the next write event. - Closure before_next_write_callback_; - Callback<void(bool success)> after_next_write_callback_; + // Helper method for WriteNow(). + bool PostWriteTask(const Callback<bool()>& task); + + // If |result| is true and |on_next_successful_write_| is set, invokes + // |on_successful_write_| and then resets it; no-ops otherwise. + void ForwardSuccessfulWrite(bool result); + + // Invoked once and then reset on the next successful write event. + Closure on_next_successful_write_; // Path being written to. const FilePath path_; diff --git a/base/files/important_file_writer_unittest.cc b/base/files/important_file_writer_unittest.cc index 9b8dcfd4e3..43e051ebcf 100644 --- a/base/files/important_file_writer_unittest.cc +++ b/base/files/important_file_writer_unittest.cc @@ -46,61 +46,39 @@ class DataSerializer : public ImportantFileWriter::DataSerializer { const std::string data_; }; -enum WriteCallbackObservationState { - NOT_CALLED, - CALLED_WITH_ERROR, - CALLED_WITH_SUCCESS, -}; - -class WriteCallbacksObserver { +class SuccessfulWriteObserver { public: - WriteCallbacksObserver() = default; + SuccessfulWriteObserver() : successful_write_observed_(false) {} - // Register OnBeforeWrite() and OnAfterWrite() to be called on the next write + // Register on_successful_write() to be called on the next successful write // of |writer|. - void ObserveNextWriteCallbacks(ImportantFileWriter* writer); + void ObserveNextSuccessfulWrite(ImportantFileWriter* writer); - // Returns the |WriteCallbackObservationState| which was observed, then resets - // it to |NOT_CALLED|. - WriteCallbackObservationState GetAndResetObservationState(); + // Returns true if a successful write was observed via on_successful_write() + // and resets the observation state to false regardless. + bool GetAndResetObservationState(); private: - void OnBeforeWrite() { - EXPECT_FALSE(before_write_called_); - before_write_called_ = true; - } - - void OnAfterWrite(bool success) { - EXPECT_EQ(NOT_CALLED, after_write_observation_state_); - after_write_observation_state_ = - success ? CALLED_WITH_SUCCESS : CALLED_WITH_ERROR; + void on_successful_write() { + EXPECT_FALSE(successful_write_observed_); + successful_write_observed_ = true; } - bool before_write_called_ = false; - WriteCallbackObservationState after_write_observation_state_ = NOT_CALLED; + bool successful_write_observed_; - DISALLOW_COPY_AND_ASSIGN(WriteCallbacksObserver); + DISALLOW_COPY_AND_ASSIGN(SuccessfulWriteObserver); }; -void WriteCallbacksObserver::ObserveNextWriteCallbacks( +void SuccessfulWriteObserver::ObserveNextSuccessfulWrite( ImportantFileWriter* writer) { - writer->RegisterOnNextWriteCallbacks( - base::Bind(&WriteCallbacksObserver::OnBeforeWrite, - base::Unretained(this)), - base::Bind(&WriteCallbacksObserver::OnAfterWrite, - base::Unretained(this))); + writer->RegisterOnNextSuccessfulWriteCallback(base::Bind( + &SuccessfulWriteObserver::on_successful_write, base::Unretained(this))); } -WriteCallbackObservationState -WriteCallbacksObserver::GetAndResetObservationState() { - EXPECT_EQ(after_write_observation_state_ != NOT_CALLED, before_write_called_) - << "The before-write callback should always be called before the " - "after-write callback"; - - WriteCallbackObservationState state = after_write_observation_state_; - before_write_called_ = false; - after_write_observation_state_ = NOT_CALLED; - return state; +bool SuccessfulWriteObserver::GetAndResetObservationState() { + bool was_successful_write_observed = successful_write_observed_; + successful_write_observed_ = false; + return was_successful_write_observed; } } // namespace @@ -110,11 +88,11 @@ class ImportantFileWriterTest : public testing::Test { ImportantFileWriterTest() { } void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); - file_ = temp_dir_.GetPath().AppendASCII("test-file"); + file_ = temp_dir_.path().AppendASCII("test-file"); } protected: - WriteCallbacksObserver write_callback_observer_; + SuccessfulWriteObserver successful_write_observer_; FilePath file_; MessageLoop loop_; @@ -125,102 +103,49 @@ class ImportantFileWriterTest : public testing::Test { TEST_F(ImportantFileWriterTest, Basic) { ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get()); EXPECT_FALSE(PathExists(writer.path())); - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); - writer.WriteNow(MakeUnique<std::string>("foo")); + EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); + writer.WriteNow(WrapUnique(new std::string("foo"))); RunLoop().RunUntilIdle(); - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); + EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); ASSERT_TRUE(PathExists(writer.path())); EXPECT_EQ("foo", GetFileContent(writer.path())); } -TEST_F(ImportantFileWriterTest, WriteWithObserver) { +TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) { ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get()); EXPECT_FALSE(PathExists(writer.path())); - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); - - // Confirm that the observer is invoked. - write_callback_observer_.ObserveNextWriteCallbacks(&writer); - writer.WriteNow(MakeUnique<std::string>("foo")); + EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); + successful_write_observer_.ObserveNextSuccessfulWrite(&writer); + writer.WriteNow(WrapUnique(new std::string("foo"))); RunLoop().RunUntilIdle(); - EXPECT_EQ(CALLED_WITH_SUCCESS, - write_callback_observer_.GetAndResetObservationState()); + // Confirm that the observer is invoked. + EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState()); ASSERT_TRUE(PathExists(writer.path())); EXPECT_EQ("foo", GetFileContent(writer.path())); // Confirm that re-installing the observer works for another write. - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); - write_callback_observer_.ObserveNextWriteCallbacks(&writer); - writer.WriteNow(MakeUnique<std::string>("bar")); + EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); + successful_write_observer_.ObserveNextSuccessfulWrite(&writer); + writer.WriteNow(WrapUnique(new std::string("bar"))); RunLoop().RunUntilIdle(); - EXPECT_EQ(CALLED_WITH_SUCCESS, - write_callback_observer_.GetAndResetObservationState()); + EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState()); ASSERT_TRUE(PathExists(writer.path())); EXPECT_EQ("bar", GetFileContent(writer.path())); // Confirm that writing again without re-installing the observer doesn't // result in a notification. - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); - writer.WriteNow(MakeUnique<std::string>("baz")); + EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); + writer.WriteNow(WrapUnique(new std::string("baz"))); RunLoop().RunUntilIdle(); - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); + EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); ASSERT_TRUE(PathExists(writer.path())); EXPECT_EQ("baz", GetFileContent(writer.path())); } -TEST_F(ImportantFileWriterTest, FailedWriteWithObserver) { - // Use an invalid file path (relative paths are invalid) to get a - // FILE_ERROR_ACCESS_DENIED error when trying to write the file. - ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"), - ThreadTaskRunnerHandle::Get()); - EXPECT_FALSE(PathExists(writer.path())); - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); - write_callback_observer_.ObserveNextWriteCallbacks(&writer); - writer.WriteNow(MakeUnique<std::string>("foo")); - RunLoop().RunUntilIdle(); - - // Confirm that the write observer was invoked with its boolean parameter set - // to false. - EXPECT_EQ(CALLED_WITH_ERROR, - write_callback_observer_.GetAndResetObservationState()); - EXPECT_FALSE(PathExists(writer.path())); -} - -TEST_F(ImportantFileWriterTest, CallbackRunsOnWriterThread) { - base::Thread file_writer_thread("ImportantFileWriter test thread"); - file_writer_thread.Start(); - ImportantFileWriter writer(file_, file_writer_thread.task_runner()); - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); - - // Block execution on |file_writer_thread| to verify that callbacks are - // executed on it. - base::WaitableEvent wait_helper( - base::WaitableEvent::ResetPolicy::MANUAL, - base::WaitableEvent::InitialState::NOT_SIGNALED); - file_writer_thread.task_runner()->PostTask( - FROM_HERE, - base::Bind(&base::WaitableEvent::Wait, base::Unretained(&wait_helper))); - - write_callback_observer_.ObserveNextWriteCallbacks(&writer); - writer.WriteNow(MakeUnique<std::string>("foo")); - RunLoop().RunUntilIdle(); - - // Expect the callback to not have been executed before the - // |file_writer_thread| is unblocked. - EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); - - wait_helper.Signal(); - file_writer_thread.FlushForTesting(); - - EXPECT_EQ(CALLED_WITH_SUCCESS, - write_callback_observer_.GetAndResetObservationState()); - ASSERT_TRUE(PathExists(writer.path())); - EXPECT_EQ("foo", GetFileContent(writer.path())); -} - TEST_F(ImportantFileWriterTest, ScheduleWrite) { ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get(), diff --git a/base/files/memory_mapped_file_posix.cc b/base/files/memory_mapped_file_posix.cc index 90ba6f49c1..4899cf0cda 100644 --- a/base/files/memory_mapped_file_posix.cc +++ b/base/files/memory_mapped_file_posix.cc @@ -31,7 +31,7 @@ bool MemoryMappedFile::MapFileRegionToMemory( if (region == MemoryMappedFile::Region::kWholeFile) { int64_t file_len = file_.GetLength(); - if (file_len < 0) { + if (file_len == -1) { DPLOG(ERROR) << "fstat " << file_.GetPlatformFile(); return false; } @@ -78,12 +78,7 @@ bool MemoryMappedFile::MapFileRegionToMemory( // POSIX won't auto-extend the file when it is written so it must first // be explicitly extended to the maximum size. Zeros will fill the new // space. - auto file_len = file_.GetLength(); - if (file_len < 0) { - DPLOG(ERROR) << "fstat " << file_.GetPlatformFile(); - return false; - } - file_.SetLength(std::max(file_len, region.offset + region.size)); + file_.SetLength(std::max(file_.GetLength(), region.offset + region.size)); flags |= PROT_READ | PROT_WRITE; break; } diff --git a/base/files/scoped_file.cc b/base/files/scoped_file.cc index 78d4ca5263..8ce45b8ba3 100644 --- a/base/files/scoped_file.cc +++ b/base/files/scoped_file.cc @@ -37,14 +37,6 @@ void ScopedFDCloseTraits::Free(int fd) { int close_errno = errno; base::debug::Alias(&close_errno); -#if defined(OS_LINUX) - // NB: Some file descriptors can return errors from close() e.g. network - // filesystems such as NFS and Linux input devices. On Linux, errors from - // close other than EBADF do not indicate failure to actually close the fd. - if (ret != 0 && errno != EBADF) - ret = 0; -#endif - PCHECK(0 == ret); } diff --git a/base/files/scoped_temp_dir.cc b/base/files/scoped_temp_dir.cc index 26815217c6..27b758ed90 100644 --- a/base/files/scoped_temp_dir.cc +++ b/base/files/scoped_temp_dir.cc @@ -76,11 +76,6 @@ FilePath ScopedTempDir::Take() { return ret; } -const FilePath& ScopedTempDir::GetPath() const { - DCHECK(!path_.empty()) << "Did you call CreateUniqueTempDir* before?"; - return path_; -} - bool ScopedTempDir::IsValid() const { return !path_.empty() && DirectoryExists(path_); } diff --git a/base/files/scoped_temp_dir.h b/base/files/scoped_temp_dir.h index a5aaf84362..b1f2f5b874 100644 --- a/base/files/scoped_temp_dir.h +++ b/base/files/scoped_temp_dir.h @@ -47,9 +47,7 @@ class BASE_EXPORT ScopedTempDir { // when this object goes out of scope. FilePath Take(); - // Returns the path to the created directory. Call one of the - // CreateUniqueTempDir* methods before getting the path. - const FilePath& GetPath() const; + const FilePath& path() const { return path_; } // Returns true if path_ is non-empty and exists. bool IsValid() const; diff --git a/base/files/scoped_temp_dir_unittest.cc b/base/files/scoped_temp_dir_unittest.cc index 024b438aa0..3b2f28e50e 100644 --- a/base/files/scoped_temp_dir_unittest.cc +++ b/base/files/scoped_temp_dir_unittest.cc @@ -53,7 +53,7 @@ TEST(ScopedTempDir, TempDir) { { ScopedTempDir dir; EXPECT_TRUE(dir.CreateUniqueTempDir()); - test_path = dir.GetPath(); + test_path = dir.path(); EXPECT_TRUE(DirectoryExists(test_path)); FilePath tmp_dir; EXPECT_TRUE(base::GetTempDir(&tmp_dir)); @@ -72,7 +72,7 @@ TEST(ScopedTempDir, UniqueTempDirUnderPath) { { ScopedTempDir dir; EXPECT_TRUE(dir.CreateUniqueTempDirUnderPath(base_path)); - test_path = dir.GetPath(); + test_path = dir.path(); EXPECT_TRUE(DirectoryExists(test_path)); EXPECT_TRUE(base_path.IsParent(test_path)); EXPECT_TRUE(test_path.value().find(base_path.value()) != std::string::npos); @@ -99,12 +99,12 @@ TEST(ScopedTempDir, MultipleInvocations) { TEST(ScopedTempDir, LockedTempDir) { ScopedTempDir dir; EXPECT_TRUE(dir.CreateUniqueTempDir()); - base::File file(dir.GetPath().Append(FILE_PATH_LITERAL("temp")), + base::File file(dir.path().Append(FILE_PATH_LITERAL("temp")), base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE); EXPECT_TRUE(file.IsValid()); EXPECT_EQ(base::File::FILE_OK, file.error_details()); EXPECT_FALSE(dir.Delete()); // We should not be able to delete. - EXPECT_FALSE(dir.GetPath().empty()); // We should still have a valid path. + EXPECT_FALSE(dir.path().empty()); // We should still have a valid path. file.Close(); // Now, we should be able to delete. EXPECT_TRUE(dir.Delete()); |