Change MessagePumpLibevent::Run to handle IO events before doing idle

Change MessagePumpLibevent::Run to process I/O events before doing idle
work.

R=agl@chromium.org,jar@chromium.org,mark@chromium.org,rvargas@chromium.org
BUG=87707
TEST=covered by existing unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91137

[wtc, 2011-06-25 00:39:45]

jar, rvargas: just FYI.  This is the MessagePumpLibevent
problem I told you about today.

http://codereview.chromium.org/7262011/diff/1/base/message_pump_libevent.cc
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/7262011/diff/1/base/message_pump_libevent.cc#n...
base/message_pump_libevent.cc:249: event_base_loop(event_base_, EVLOOP_ONCE);
The original code calls event_base_loop here and on line
259 below, after the delegate->DoIdleWork() call.

http://codereview.chromium.org/7262011/diff/1/third_party/libevent/event.c
File third_party/libevent/event.c (right):

http://codereview.chromium.org/7262011/diff/1/third_party/libevent/event.c#ne...
third_party/libevent/event.c:538: return (processed_active ? 2 : 0);
The return value 0 means success.  1 means no events are
registered.  So I need to use 2 for the new meaning.
I'll find a better solution.

[wtc, 2011-06-28 02:39:54]

jar, rvargas: I came up with a good solution.  Please review
Patch Set 2.

[rvargas (doing something else), 2011-06-28 18:14:45]

Looks good to me, but you should get a reviewer familiar with the libevent
message pump (I'm not). For instance, I see DidProcessIOEvent being called only
from OnFileCanReadWithoutBlocking (and Write); is that enough?

[wtc, 2011-06-28 22:01:54]

agl: can you review this patch for MessagePumpLibevent?

Please compare MessagePumpLibevent::Run() with
essagePumpForIO::DoRunLoop() in base/message_pump_win.cc.
You will see that MessagePumpLibevent::Run() is missing
the code equivalent to

    more_work_is_plausible |= WaitForIOCompletion(0, NULL);
    if (state_->should_quit)
      break;

which is what this patch adds.  Thanks.

[agl, 2011-06-28 22:19:30]

LGTM.

I'm happy that DidProcessIOEvent is being called in the right places and that
there aren't any timer issues that this could kick up.

[jar (doing other things), 2011-06-29 04:20:41]

I think this LGTM.

One plausible gotcha would appear if nested message loops were used.  In that
case, we *might* have to be more careful about how we pass the flag around...
but even then I'm not sure.  I could hope that the introduction of the nested
message loop would be "work done," and no amount of fumbling of the new slot
would hurt us.  Similarly, the termination of a nested message loop would have
to be in the context of finishing up some work... so again I think we'd get the
work_done flag set.

I think (thankfully) that we don't use nested message loops on an IO thread (and
that such evil only appears on the UI thread??).  There is a small chance that
(evil) nested message loops do appear in IPC scenarios on the IO thread to
support (sigh) synchronous IPC requests.  In that case, I *think* we'd have to
live with the argument of the first paragraph.

Bottom line: I think it LGTM... but I wanted to get folks thinking about this
subtle element.  I also feel confident this is a VAST improvement, so even if it
is not perfect (I think it might be perfect), is is well worth landing!!

[commit-bot: I haz the power, 2011-06-30 12:32:31]

Presubmit check for 7262011-4001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
base/message_pump_libevent.cc,base/message_pump_libevent.h

[wtc, 2011-06-30 12:35:08]

mark: please review this CL.  Thanks.

[Mark Mentovai, 2011-06-30 13:38:39]

LGTM for base approval.

I agree with jar, this probably isn’t strictly perfect under a nested loop
scenario.

[commit-bot: I haz the power, 2011-06-30 15:39:22]

Change committed as 91137