Make MessagePumpForIO::RegisterIOHandler return an error code

RegisterIOHandler calls CreateIoCompletionPort and will currently fail silently. This adds a return code and propagates it up. Bug: 845612 Change-Id: Ie0685cb7fb6d21a5e23489337158ee0ba0b75154 Reviewed-on: https://chromium-review.googlesource.com/1101644 Commit-Queue: Robbie McElrath <rmcelrath@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#568570}
qweri0p · Jun 19, 2018 · addae86 · addae86
1 parent 03ffa71
commit addae86
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 8 deletions.
diff --git a/base/message_loop/message_loop_current.cc b/base/message_loop/message_loop_current.cc
@@ -201,11 +201,11 @@ bool MessageLoopCurrentForIO::IsSet() {
 #if !defined(OS_NACL_SFI)
 
 #if defined(OS_WIN)
-void MessageLoopCurrentForIO::RegisterIOHandler(
+HRESULT MessageLoopCurrentForIO::RegisterIOHandler(
     HANDLE file,
     MessagePumpForIO::IOHandler* handler) {
   DCHECK_CALLED_ON_VALID_THREAD(current_->bound_thread_checker_);
-  pump_->RegisterIOHandler(file, handler);
+  return pump_->RegisterIOHandler(file, handler);
 }
 
 bool MessageLoopCurrentForIO::RegisterJobObject(

diff --git a/base/message_loop/message_loop_current.h b/base/message_loop/message_loop_current.h
@@ -265,7 +265,7 @@ class BASE_EXPORT MessageLoopCurrentForIO : public MessageLoopCurrent {
 
 #if defined(OS_WIN)
   // Please see MessagePumpWin for definitions of these methods.
-  void RegisterIOHandler(HANDLE file, MessagePumpForIO::IOHandler* handler);
+  HRESULT RegisterIOHandler(HANDLE file, MessagePumpForIO::IOHandler* handler);
   bool RegisterJobObject(HANDLE job, MessagePumpForIO::IOHandler* handler);
   bool WaitForIOCompletion(DWORD timeout, MessagePumpForIO::IOHandler* filter);
 #elif defined(OS_POSIX) || defined(OS_FUCHSIA)

diff --git a/base/message_loop/message_pump_win.cc b/base/message_loop/message_pump_win.cc
@@ -443,11 +443,11 @@ void MessagePumpForIO::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
   delayed_work_time_ = delayed_work_time;
 }
 
-void MessagePumpForIO::RegisterIOHandler(HANDLE file_handle,
-                                         IOHandler* handler) {
+HRESULT MessagePumpForIO::RegisterIOHandler(HANDLE file_handle,
+                                            IOHandler* handler) {
   HANDLE port = CreateIoCompletionPort(file_handle, port_.Get(),
                                        reinterpret_cast<ULONG_PTR>(handler), 1);
-  DPCHECK(port);
+  return (port != nullptr) ? S_OK : HRESULT_FROM_WIN32(GetLastError());
 }
 
 bool MessagePumpForIO::RegisterJobObject(HANDLE job_handle,

diff --git a/base/message_loop/message_pump_win.h b/base/message_loop/message_pump_win.h
@@ -209,7 +209,7 @@ class BASE_EXPORT MessagePumpForIO : public MessagePumpWin {
   // Register the handler to be used when asynchronous IO for the given file
   // completes. The registration persists as long as |file_handle| is valid, so
   // |handler| must be valid as long as there is pending IO for the given file.
-  void RegisterIOHandler(HANDLE file_handle, IOHandler* handler);
+  HRESULT RegisterIOHandler(HANDLE file_handle, IOHandler* handler);
 
   // Register the handler to be used to process job events. The registration
   // persists as long as the job object is live, so |handler| must be valid

diff --git a/net/base/file_stream_context_win.cc b/net/base/file_stream_context_win.cc
@@ -119,8 +119,10 @@ FileStream::Context::IOResult FileStream::Context::SeekFileImpl(
 }
 
 void FileStream::Context::OnFileOpened() {
-  base::MessageLoopCurrentForIO::Get()->RegisterIOHandler(
+  HRESULT hr = base::MessageLoopCurrentForIO::Get()->RegisterIOHandler(
       file_.GetPlatformFile(), this);
+  if (!SUCCEEDED(hr))
+    file_.Close();
 }
 
 void FileStream::Context::IOCompletionIsPending(CompletionOnceCallback callback,

diff --git a/net/base/file_stream_unittest.cc b/net/base/file_stream_unittest.cc
@@ -21,6 +21,7 @@
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "build/build_config.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
@@ -805,6 +806,27 @@ TEST_F(FileStreamTest, ReadError) {
   base::RunLoop().RunUntilIdle();
 }
 
+#if defined(OS_WIN)
+// Verifies that a FileStream will close itself if it receives a File whose
+// async flag doesn't match the async state of the underlying handle.
+TEST_F(FileStreamTest, AsyncFlagMismatch) {
+  // Open the test file without async, then make a File with the same sync
+  // handle but with the async flag set to true.
+  uint32_t flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
+  base::File file(temp_file_path(), flags);
+  base::File lying_file =
+      base::File::CreateForAsyncHandle(file.TakePlatformFile());
+  ASSERT_TRUE(lying_file.IsValid());
+
+  FileStream stream(std::move(lying_file), base::ThreadTaskRunnerHandle::Get());
+  ASSERT_FALSE(stream.IsOpen());
+  TestCompletionCallback callback;
+  scoped_refptr<IOBufferWithSize> buf = new IOBufferWithSize(4);
+  int rv = stream.Read(buf.get(), buf->size(), callback.callback());
+  EXPECT_THAT(callback.GetResult(rv), IsError(ERR_UNEXPECTED));
+}
+#endif
+
 #if defined(OS_ANDROID)
 TEST_F(FileStreamTest, ContentUriRead) {
   base::FilePath test_dir;