Enable MessageLoop unittests on iOS and fix MessagePumpIOForIOS bug.

Enable MessageLoop unittests on iOS and fix MessagePumpIOForIOS bug.

This CL does follow-up work to https://chromiumcodereview.appspot.com/11412101/ :

1. Turns on MessageLoop unittests on iOS.
2. Fixes a bug that the |FileDescriptorWatcherOutlivesMessageLoop| test
   uncovered wherein |FileDescriptorWatcher| needs to retain its pump to ensure
   that it can safely call |RemoveRunLoopSource()| on the pump at destruction
   if necessary.
3. Stops message_pump_libevent being built on iOS.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170965

[blundell, 2012-12-03 12:14:25]

Hi Wan-Teh and Mark,

Bit of follow-up work on the message pump for IO on iOS work.

Thanks!

[Mark Mentovai, 2012-12-03 14:04:43]

LGTM

[wtc, 2012-12-03 19:27:18]

Patch set 1 LGTM. I ask whether there is a better solution
below.

https://chromiumcodereview.appspot.com/11299301/diff/1/base/message_pump_io_i...
File base/message_pump_io_ios.h (right):

https://chromiumcodereview.appspot.com/11299301/diff/1/base/message_pump_io_i...
base/message_pump_io_ios.h:85: scoped_refptr<MessagePumpIOSForIO> pump_;

Do you know why the FileDescriptorWatcher class in
message_pump_libevent.h doesn't need to do this?
What is the solution used over there? Can we copy
that solution here?

I studied the code and may have found the answers to
my own questions.

It seems that an important difference between the libevent and
iOS versions of the
FileDescriptorWatcher::StopWatchingFileDescriptor function
is that the libevent version does not use |pump_|, so
|pump_| can be an invalid pointer, whereas the iOS version
needs a valid |pump_| for the
pump_->RemoveRunLoopSource(fd_source_) call to succeed.

Another solution would be to make the FileDescriptorWatcher
objects "observe" the |pump_| object, so that the |pump_|
object can notify the FileDescriptorWatcher objects that it
is going away. Alternatively, make |pump_| a weak pointer.
These alternative solutions would allow |pump_| to be
destroyed sooner. I don't know which solution is better though.

[blundell, 2012-12-03 20:04:48]

(from the right email address this time)

Hi Wan-Teh,

I did study the message_pump_libevent code first to determine whether they had a
solution that would work for us, and found the difference that you described
between the two implementations. FWIW, it's not exactly clear to me how the
libevent code works correctly in this corner case: in that implementation,
StopWatchingFileDescriptor calls |del_event()|, which comments at the top of
|message_pump_libevent.cc| say removes the event from the |event_base| that is
associated with the message pump (and is freed when the message pump is
destroyed). I am not an expert on the libevent API :), so there is likely some
subtlety here that is making the corner case work safely in that implementation
(as it seems to be, since the test passes there).

I thought about the two other solutions that you mention while bringing up this
CL, and the solution in this CL seemed to me the best of the three for the
following reasons.

- Making |pump_| a weak pointer: The call to
|pump_->RemoveRunLoopSource(fd_source_)| calls |CFRunLoopRemoveSource()| on the
backing CFRunLoop. My thought was that we want to guarantee that the latter
method is always called as part of the FileDescriptorWatcher stopping watching
the file descriptor, i.e., we don't want to end up potentially leaving a
dangling source there. Let me know if you think differently.

- Having the FileDescriptorWatcher objects observe the |pump_| object: This
solution would clearly be more fiddly code-wise than the current one. As you
point out, the benefit would be that |pump_| would be destroyed sooner in the
case where there *are* file descriptor watchers that outlive their |pump_|. My
assumption is that that case is extremely uncommon in practice (in particular,
we've never seen a crash in practice resulting from the bug that this CL fixes),
and so I decided that it was best to opt for code clarity. What do you think?

Thanks,

Colin

[blundell1, 2012-12-04 08:08:50]

Hi Wan-Teh,

I did study the message_pump_libevent code first to determine whether they
had a solution that would work for us, and found the difference that you
described between the two implementations. FWIW, it's not exactly clear to
me how the libevent code works correctly in this corner case: in that
implementation, StopWatchingFileDescriptor calls |del_event()|, which
comments at the top of |message_pump_libevent.cc| say removes the event
from the |event_base| that is associated with the message pump (and is
freed when the message pump is destroyed). I am not an expert on the
libevent API :), so there is likely some subtlety here that is making the
corner case work safely in that implementation (as it seems to be, since
the test passes there).

I thought about the two other solutions that you mention while bringing up
this CL, and the solution in this CL seemed to me the best of the three for
the following reasons.

- Making |pump_| a weak pointer: The call to
|pump_->RemoveRunLoopSource(fd_source_)| calls |CFRunLoopRemoveSource()| on
the backing CFRunLoop. My thought was that we want to guarantee that the
latter method is always called as part of the FileDescriptorWatcher
stopping watching the file descriptor, i.e., we don't want to end up
potentially leaving a dangling source there. Let me know if you think
differently.

- Having the FileDescriptorWatcher objects observe the |pump_| object: This
solution would clearly be more fiddly code-wise than the current one. As
you point out, the benefit would be that |pump_| would be destroyed sooner
in the case where there *are* file descriptor watchers that outlive their
|pump_|. My assumption is that that case is extremely uncommon in practice
(in particular, we've never seen a crash in practice resulting from the bug
that this CL fixes), and so I decided that it was best to opt for code
clarity. What do you think?

Thanks,

Colin


On Mon, Dec 3, 2012 at 8:27 PM, <wtc@chromium.org> wrote:

> Patch set 1 LGTM. I ask whether there is a better solution
> below.
>
>
> https://chromiumcodereview.**appspot.com/11299301/diff/1/**
>
base/message_pump_io_ios.h<https://chromiumcodereview.appspot.com/11299301/diff/1/base/message_pump_io_ios.h>
> File base/message_pump_io_ios.h (right):
>
> https://chromiumcodereview.**appspot.com/11299301/diff/1/**
>
base/message_pump_io_ios.h#**newcode85<https://chromiumcodereview.appspot.com/11299301/diff/1/base/message_pump_io_ios.h#newcode85>
> base/message_pump_io_ios.h:85: scoped_refptr<**MessagePumpIOSForIO> pump_;
>
> Do you know why the FileDescriptorWatcher class in
> message_pump_libevent.h doesn't need to do this?
> What is the solution used over there? Can we copy
> that solution here?
>
> I studied the code and may have found the answers to
> my own questions.
>
> It seems that an important difference between the libevent and
> iOS versions of the
> FileDescriptorWatcher::**StopWatchingFileDescriptor function
> is that the libevent version does not use |pump_|, so
> |pump_| can be an invalid pointer, whereas the iOS version
> needs a valid |pump_| for the
> pump_->RemoveRunLoopSource(fd_**source_) call to succeed.
>
> Another solution would be to make the FileDescriptorWatcher
> objects "observe" the |pump_| object, so that the |pump_|
> object can notify the FileDescriptorWatcher objects that it
> is going away. Alternatively, make |pump_| a weak pointer.
> These alternative solutions would allow |pump_| to be
> destroyed sooner. I don't know which solution is better though.
>
>
https://chromiumcodereview.**appspot.com/11299301/<https://chromiumcodereview...
>

[blundell, 2012-12-04 13:07:40]

wtc had the below comments via email:

> I did study the message_pump_libevent code first to determine whether they
> had a
> solution that would work for us, and found the difference that you described
> between the two implementations. FWIW, it's not exactly clear to me how the
> libevent code works correctly in this corner case: in that implementation,
> StopWatchingFileDescriptor calls |del_event()|, which comments at the top of
> |message_pump_libevent.cc| say removes the event from the |event_base| that
> is
> associated with the message pump (and is freed when the message pump is
> destroyed). I am not an expert on the libevent API :), so there is likely
> some
> subtlety here that is making the corner case work safely in that
> implementation
> (as it seems to be, since the test passes there).

It's only working safely if the test passes under valgrind and ASAN.
Your findings are a little worrisome.

> - Making |pump_| a weak pointer: The call to
> |pump_->RemoveRunLoopSource(fd_source_)| calls |CFRunLoopRemoveSource()| on
> the
> backing CFRunLoop. My thought was that we want to guarantee that the latter
> method is always called as part of the FileDescriptorWatcher stopping
> watching
> the file descriptor, i.e., we don't want to end up potentially leaving a
> dangling source there. Let me know if you think differently.

Yes, you are right.

> - Having the FileDescriptorWatcher objects observe the |pump_| object: This
> solution would clearly be more fiddly code-wise than the current one. As you
> point out, the benefit would be that |pump_| would be destroyed sooner in
> the
> case where there *are* file descriptor watchers that outlive their |pump_|.
> My
> assumption is that that case is extremely uncommon in practice (in
> particular,
> we've never seen a crash in practice resulting from the bug that this CL
> fixes),
> and so I decided that it was best to opt for code clarity. What do you
> think?

Agreed.

Another way to look at this is that perhaps the
|FileDescriptorWatcherOutlivesMessageLoop| test is unrealistic and
imposes unnecessary constraints on MessageLoop implementations. But
the comment at the beginning of that test says the scenario it's
testing could happen. So I guess we will need to deal with it.

UPDATE: after I wrote the above, I studied the
message_pump_libevent.{h,cc} and third_party/libevent/event.c code. I
think I understand why the MessagePumpLibevent code works safely. The
relevant libevent functions to study are:
- event_base_free(): called by the MessagePumpLibevent destructor
- event_del(): called by StopWatchingFileDescriptor(), but ALSO by
event_base_free().
- event_queue_remove(): called by event_del().

The ev->ev_base member is the problematic member. I believe that it
does become invalid after the MessageLoop is destroyed. However, I
noticed that ev->ev_base is only dereferenced after checking
ev->ev_flags, and I believe that event_base_free() will cause all the
outstanding 'event' objects to have an empty ev_flags field, so that
their ev_base field won't be dereferenced even when
StopWatchingFileDescriptor() calls event_del().

To copy this solution to MessagePumpIOForIOS, we would need to
implement the "observers" approach (or a moral equivalent) I suggested
earlier. When the MessagePumpIOForIOS broadcasts to the observers that
it is going away, all the file descriptor watchers would need to do
the equivalent of StopWatchingFileDescriptor(). (In libevent this is
done by the event_base itself because it knows about all the
outstanding events.) I also don't know if this is worth doing.

In summary, we should not make |pump_| a weak pointer. But the
"observer" approach may be worth considering. But I don't know this
code well enough to decide.

Caveat: I am not familiar with libevent, so I may have misunderstood its code.

Wan-Teh

[blundell, 2012-12-04 13:14:10]

Hi Wan-Teh,

Thanks for your response. I studied the libevent code that you pointed to and I
agree with your conclusions wrt why message_pump_libevent is correct in this
corner case.

I'm going to push this CL as-is: I think that this case is extremely unlikely in
practice as evidenced by the fact that we've never seen a crash due to the bug
fixed here, and hence I prefer this simpler solution to the approach of having
the FileDescriptorWatchers be observers of the pump.

[commit-bot: I haz the power, 2012-12-04 13:14:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/11299301/1

[commit-bot: I haz the power, 2012-12-04 15:44:17]

Change committed as 170965