Make AsyncHandleWaiter reenterancy-safe

ipc/mojo/async_handle_waiter.cc

Index: ipc/mojo/async_handle_waiter.cc
diff --git a/ipc/mojo/async_handle_waiter.cc b/ipc/mojo/async_handle_waiter.cc
index 7b199ed95e835eec7dae4533ef4474f400030bf6..ceea3b07b3ed107d0885ab899e1dc2e7efeb7011 100644
--- a/ipc/mojo/async_handle_waiter.cc
+++ b/ipc/mojo/async_handle_waiter.cc
@@ -32,7 +32,7 @@ class AsyncHandleWaiter::Context
       : io_runner_(base::MessageLoopForIO::current()->task_runner()),
         waiter_(waiter),
         last_result_(MOJO_RESULT_INTERNAL),
-        processing_(false),
+        processing_(0),
         should_invoke_callback_(false) {
     base::MessageLoopForIO::current()->AddIOObserver(this);
   }
@@ -67,7 +67,7 @@ class AsyncHandleWaiter::Context
       return false;
     if (loop->task_runner() != io_runner_)
       return false;
-    return processing_;
+    return 0 < processing_;

[viettrungluu, 2015/03/03 22:03:33]

nit: "processing_ > 0" would probably be more readable for most people

   }
 
   // Called from |io_runner_| thus safe to touch |waiter_|.
@@ -82,18 +82,24 @@ class AsyncHandleWaiter::Context
 
   void WillProcessIOEvent() override {
     DCHECK(!should_invoke_callback_);
-    DCHECK(!processing_);
-    processing_ = true;
+    DCHECK_GE(processing_, 0);
+    processing_++;
   }
 
   void DidProcessIOEvent() override {
-    DCHECK(processing_);
+    DCHECK_GE(processing_, 1);
+
+    // Leaving a nested loop.
+    if (1 < processing_) {

[viettrungluu, 2015/03/03 22:03:33]

processing_ > 1

[Hajime Morrita, 2015/03/04 23:16:08]

Done.

+      processing_--;
+      return;

[viettrungluu, 2015/03/03 22:03:33]

Why do we not want to do any of the stuff below in this case?

[Hajime Morrita, 2015/03/04 23:16:08]

It can cause nested IO callback, which isn't what we want here.
It would be safe to let the outermost handler take it.

+    }
 
     // The zero |waiter_| indicates that |this| have lost the owner and can be
     // under destruction. So we cannot wrap it with a |scoped_refptr| anymore.
     if (!waiter_) {
       should_invoke_callback_ = false;
-      processing_ = false;
+      processing_--;
       return;
     }
 
@@ -105,7 +111,7 @@ class AsyncHandleWaiter::Context
       InvokeWaiterCallback();
     }
 
-    processing_ = false;
+    processing_--;
   }
 
   // Only |io_runner_| is accessed from arbitrary threads.  Others are touched
@@ -114,7 +120,7 @@ class AsyncHandleWaiter::Context
 
   const base::WeakPtr<AsyncHandleWaiter> waiter_;
   MojoResult last_result_;
-  bool processing_;
+  int processing_;

[viettrungluu, 2015/03/03 22:03:33]

nit: Maybe rename this to something slightly more descriptive, now that it's no
longer a bool?

(I'm not sure what though. process_io_event_enter_count_?
processing_enter_count_?)

[Hajime Morrita, 2015/03/04 23:16:08]

Done.

   bool should_invoke_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(Context);