Remove handler from MessagePumpForIO::IOContext.

base/message_loop/message_pump_win.cc

Index: base/message_loop/message_pump_win.cc
diff --git a/base/message_loop/message_pump_win.cc b/base/message_loop/message_pump_win.cc
index 836c1d111b094a06cc35598bd5ad72e61e152f3c..1d35050558a384f9c9f1d855b375e27f39272056 100644
--- a/base/message_loop/message_pump_win.cc
+++ b/base/message_loop/message_pump_win.cc
@@ -420,13 +420,16 @@ bool MessagePumpForUI::ProcessPumpReplacementMessage() {
 //-----------------------------------------------------------------------------
 // MessagePumpForIO public:
 
+MessagePumpForIO::IOContext::IOContext() {
+  memset(&overlapped, 0, sizeof(overlapped));
+}
+
 MessagePumpForIO::MessagePumpForIO() {
   port_.Set(CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, NULL, 1));
   DCHECK(port_.IsValid());
 }
 
-MessagePumpForIO::~MessagePumpForIO() {
-}
+MessagePumpForIO::~MessagePumpForIO() = default;
 
 void MessagePumpForIO::ScheduleWork() {
   if (InterlockedExchange(&have_work_, 1))
@@ -456,19 +459,15 @@ void MessagePumpForIO::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
 
 void MessagePumpForIO::RegisterIOHandler(HANDLE file_handle,
                                          IOHandler* handler) {
-  ULONG_PTR key = HandlerToKey(handler, true);
-  HANDLE port = CreateIoCompletionPort(file_handle, port_.Get(), key, 1);
+  HANDLE port = CreateIoCompletionPort(file_handle, port_.Get(),
+                                       reinterpret_cast<ULONG_PTR>(handler), 1);
   DPCHECK(port);
 }
 
 bool MessagePumpForIO::RegisterJobObject(HANDLE job_handle,
                                          IOHandler* handler) {
-  // Job object notifications use the OVERLAPPED pointer to carry the message
-  // data. Mark the completion key correspondingly, so we will not try to
-  // convert OVERLAPPED* to IOContext*.
-  ULONG_PTR key = HandlerToKey(handler, false);
   JOBOBJECT_ASSOCIATE_COMPLETION_PORT info;
-  info.CompletionKey = reinterpret_cast<void*>(key);
+  info.CompletionKey = reinterpret_cast<void*>(handler);

[Lei Zhang, 2016/05/09 22:41:30]

reinterpret_cast no longer needed?

[fdoray, 2016/05/10 16:33:17]

Done.

[Lei Zhang, 2016/05/10 19:15:25]

Nope. :-P

[fdoray, 2016/05/10 19:40:20]

Done for realz.

   info.CompletionPort = port_.Get();
   return SetInformationJobObject(job_handle,
                                  JobObjectAssociateCompletionPortInformation,
@@ -542,22 +541,12 @@ bool MessagePumpForIO::WaitForIOCompletion(DWORD timeout, IOHandler* filter) {
       return true;
   }
 
-  // If |item.has_valid_io_context| is false then |item.context| does not point
-  // to a context structure, and so should not be dereferenced, although it may
-  // still hold valid non-pointer data.
-  if (!item.has_valid_io_context || item.context->handler) {

[Lei Zhang, 2016/05/09 22:41:30]

So I didn't write this code and I'm only looking at it for the first time ever.
Are you saying |item.context->handler| is always initialized to be non-NULL, and
nobody sets it to NULL like in example #1 in
base/message_loop/message_pump_win.h? Thus conditional always evaluates true,
and everything else falls in place?

[fdoray, 2016/05/10 16:33:17]

yes, it's always non-null

-    if (filter && item.handler != filter) {
-      // Save this item for later
-      completed_io_.push_back(item);
-    } else {
-      DCHECK(!item.has_valid_io_context ||
-             (item.context->handler == item.handler));
-      item.handler->OnIOCompleted(item.context, item.bytes_transfered,
-                                  item.error);
-    }
+  if (filter && item.handler != filter) {
+    // Save this item for later
+    completed_io_.push_back(item);
   } else {
-    // The handler must be gone by now, just cleanup the mess.
-    delete item.context;
+    item.handler->OnIOCompleted(item.context, item.bytes_transfered,
+                                item.error);
   }
   return true;
 }
@@ -575,7 +564,7 @@ bool MessagePumpForIO::GetIOItem(DWORD timeout, IOItem* item) {
     item->bytes_transfered = 0;
   }
 
-  item->handler = KeyToHandler(key, &item->has_valid_io_context);
+  item->handler = reinterpret_cast<IOHandler*>(key);
   item->context = reinterpret_cast<IOContext*>(overlapped);
   return true;
 }
@@ -605,28 +594,4 @@ bool MessagePumpForIO::MatchCompletedIOItem(IOHandler* filter, IOItem* item) {
   return false;
 }
 
-// static
-ULONG_PTR MessagePumpForIO::HandlerToKey(IOHandler* handler,
-                                         bool has_valid_io_context) {
-  ULONG_PTR key = reinterpret_cast<ULONG_PTR>(handler);
-
-  // |IOHandler| is at least pointer-size aligned, so the lowest two bits are
-  // always cleared. We use the lowest bit to distinguish completion keys with
-  // and without the associated |IOContext|.
-  DCHECK_EQ(key & 1, 0u);
-
-  // Mark the completion key as context-less.
-  if (!has_valid_io_context)
-    key = key | 1;
-  return key;
-}
-
-// static
-MessagePumpForIO::IOHandler* MessagePumpForIO::KeyToHandler(
-    ULONG_PTR key,
-    bool* has_valid_io_context) {
-  *has_valid_io_context = ((key & 1) == 0);
-  return reinterpret_cast<IOHandler*>(key & ~static_cast<ULONG_PTR>(1));
-}
-
 }  // namespace base