Remove handler from MessagePumpForIO::IOContext.

base/message_loop/message_pump_win.cc

 // Copyright (c) 2012 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/message_loop/message_pump_win.h"
 
 #include <math.h>
 #include <stdint.h>
 
 #include <limits>
@@ skipping 402 matching lines @@
   // windows code.   This ScheduleWork() may hurt performance a tiny bit when
   // tasks appear very infrequently, but when the event queue is busy, the
   // kMsgHaveWork events get (percentage wise) rarer and rarer.
   ScheduleWork();
   return ProcessMessageHelper(msg);
 }
 
 //-----------------------------------------------------------------------------
 // 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))
     return;  // Someone else continued the pumping.
 
   // Make sure the MessagePump does some work for us.
   BOOL ret = PostQueuedCompletionStatus(port_.Get(), 0,
                                         reinterpret_cast<ULONG_PTR>(this),
                                         reinterpret_cast<OVERLAPPED*>(this));
   if (ret)
     return;  // Post worked perfectly.
 
   // See comment in MessagePumpForUI::ScheduleWork() for this error recovery.
   InterlockedExchange(&have_work_, 0);  // Clarify that we didn't succeed.
   UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", COMPLETION_POST_ERROR,
                             MESSAGE_LOOP_PROBLEM_MAX);
   state_->schedule_work_error_count++;
   state_->last_schedule_work_error_time = Time::Now();
 }
 
 void MessagePumpForIO::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
   // We know that we can't be blocked right now since this method can only be
   // called on the same thread as Run, so we only need to update our record of
   // how long to sleep when we do sleep.
   delayed_work_time_ = 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,
                                  &info,
                                  sizeof(info)) != FALSE;
 }
 
 //-----------------------------------------------------------------------------
 // MessagePumpForIO private:
 
@@ skipping 53 matching lines @@
   IOItem item;
   if (completed_io_.empty() || !MatchCompletedIOItem(filter, &item)) {
     // We have to ask the system for another IO completion.
     if (!GetIOItem(timeout, &item))
       return false;
 
     if (ProcessInternalIOItem(item))
       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;
 }
 
 // Asks the OS for another IO completion result.
 bool MessagePumpForIO::GetIOItem(DWORD timeout, IOItem* item) {
   memset(item, 0, sizeof(*item));
   ULONG_PTR key = NULL;
   OVERLAPPED* overlapped = NULL;
   if (!GetQueuedCompletionStatus(port_.Get(), &item->bytes_transfered, &key,
                                  &overlapped, timeout)) {
     if (!overlapped)
       return false;  // Nothing in the queue.
     item->error = GetLastError();
     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;
 }
 
 bool MessagePumpForIO::ProcessInternalIOItem(const IOItem& item) {
   if (reinterpret_cast<void*>(this) == reinterpret_cast<void*>(item.context) &&
       reinterpret_cast<void*>(this) == reinterpret_cast<void*>(item.handler)) {
     // This is our internal completion.
     DCHECK(!item.bytes_transfered);
     InterlockedExchange(&have_work_, 0);
     return true;
   }
   return false;
 }
 
 // Returns a completion item that was previously received.
 bool MessagePumpForIO::MatchCompletedIOItem(IOHandler* filter, IOItem* item) {
   DCHECK(!completed_io_.empty());
   for (std::list<IOItem>::iterator it = completed_io_.begin();
        it != completed_io_.end(); ++it) {
     if (!filter || it->handler == filter) {
       *item = *it;
       completed_io_.erase(it);
       return true;
     }
   }
   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