Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11412101:
Provide an iOS message pump for IO implementation. (Closed)
|
Created:
8 years, 1 month ago by blundell Modified:
8 years ago CC:
chromium-reviews, erikwright+watch_chromium.org, sadrul, sail+watch_chromium.org, droger Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionProvide an iOS message pump for IO implementation.
The implementation is done as a subclass of MessagePumpNSRunLoop. It is conceptually quite similar to the libevent implementation (message_pump_libevent*), on which it is based.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170470
Patch Set 1 #Patch Set 2 : Undo mistaken change #Patch Set 3 : Formatting nit #
Total comments: 16
Patch Set 4 : Fixes #Patch Set 5 : Restore StopWatchingFileDescriptor signature #
Total comments: 21
Patch Set 6 : Response to review #
Total comments: 2
Patch Set 7 : Response to reviews #
Total comments: 7
Patch Set 8 : Response to review #
Total comments: 12
Patch Set 9 : Response to reviews #
Total comments: 4
Patch Set 10 : Response to review, bugfix #
Total comments: 30
Patch Set 11 : Back to old approach for maintaining safety #
Total comments: 6
Patch Set 12 : Address minor review comments #
Total comments: 5
Patch Set 13 : Response to wtc's review #
Messages
Total messages: 37 (0 generated)
Mark, As this class is used only on iOS, I put it in its own iOS-specific files rather than glomming it into message_pump_mac. Let me know if you think it's better the other way or if you have any naming suggestions. Thanks, Colin
https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... File base/mac/scoped_cffiledescriptorref.h (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... base/mac/scoped_cffiledescriptorref.h:17: // destruction, it will invalidate the file descriptor. It seems that ScopedCFTypeRef has evolved since this has been written. We may want to change that comment or (even better) make it true by merging the changes. https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... base/mac/scoped_cffiledescriptorref.h:32: if (fdref_) { Do we want to handle the case where fdref == fdref_? ScopedCFTypeRef does it. Or maybe simply DCHECK_NE(fdref, fdref_).
https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... File base/mac/scoped_cffiledescriptorref.h (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... base/mac/scoped_cffiledescriptorref.h:18: class ScopedCFFileDescriptorRef { Can this just extend ScopedCFTypeRef<CFFileDescriptorRef> and thus be simpler? https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.h:17: class MessagePumpIOSForIO : public MessagePumpNSRunLoop { Is there a reason you used the NSRunLoop flavor and not the CFRunLoop one? Just curious. https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.h:90: WATCH_READ_WRITE = WATCH_READ | WATCH_WRITE I don’t know that this needs to be distinct here. You can leave the read-write enum member in message_loop.h to maintain a consistent interface and have it OR the bits together in that header. I can’t justify having a distinct read-write enum value here when the implementation never uses this value, it just tests the read and write bits. https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... File base/message_pump_io_ios.mm (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.mm:22: bool This doesn’t need to be on its own line. Same on lines 51 and 60. https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.mm:25: return true; Does this ever return false?
Thanks for the reviews. PTAL. https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... File base/mac/scoped_cffiledescriptorref.h (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... base/mac/scoped_cffiledescriptorref.h:17: // destruction, it will invalidate the file descriptor. On 2012/11/20 14:41:28, droger wrote: > It seems that ScopedCFTypeRef has evolved since this has been written. > We may want to change that comment or (even better) make it true by merging the > changes. After discussion, droger and I realized that ScopedCFTypeRef has actually evolved *away* from ScopedCFFileDescriptorRef. ScopedCFTypeRef has evolved to support RETAIN semantics and copying, but it only makes sense for a file descriptor to have one owner, as the file descriptor will be invalidated on the destruction/reset of the first owner. https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... base/mac/scoped_cffiledescriptorref.h:18: class ScopedCFFileDescriptorRef { See the above comment; I could have this extend ScopedCFTypeRef<CFFileDescriptorRef>, but then would have to go to pains to ensure that RETAIN semantics, copying, etc. is never invoked. I don't think the end result would look simpler and this approach seems safer to me (less chance of the object mistakenly being used in a way it's not intended to be). What do you think? On 2012/11/20 14:47:58, Mark Mentovai wrote: > Can this just extend ScopedCFTypeRef<CFFileDescriptorRef> and thus be simpler? https://chromiumcodereview.appspot.com/11412101/diff/9/base/mac/scoped_cffile... base/mac/scoped_cffiledescriptorref.h:32: if (fdref_) { On 2012/11/20 14:41:28, droger wrote: > Do we want to handle the case where fdref == fdref_? > ScopedCFTypeRef does it. > > Or maybe simply DCHECK_NE(fdref, fdref_). Done. https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.h:17: class MessagePumpIOSForIO : public MessagePumpNSRunLoop { When we were originally bringing up this code, we experienced problems using the CFRunLoop flavor that disappeared when we moved to the NSRunLoop flavor. We didn't figure out what the root cause was at that time. I didn't revisit that decision when doing this work. :| On 2012/11/20 14:47:58, Mark Mentovai wrote: > Is there a reason you used the NSRunLoop flavor and not the CFRunLoop one? Just > curious. https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.h:90: WATCH_READ_WRITE = WATCH_READ | WATCH_WRITE Done. I noticed that the same argument could be applied to message_pump_libevent. Should I make that change as well? On 2012/11/20 14:47:58, Mark Mentovai wrote: > I don’t know that this needs to be distinct here. You can leave the read-write > enum member in message_loop.h to maintain a consistent interface and have it OR > the bits together in that header. I can’t justify having a distinct read-write > enum value here when the implementation never uses this value, it just tests the > read and write bits. https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... File base/message_pump_io_ios.mm (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.mm:22: bool On 2012/11/20 14:47:58, Mark Mentovai wrote: > This doesn’t need to be on its own line. Same on lines 51 and 60. Done. https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.mm:25: return true; No. The only reason that this was not void was for consistency with the message_pump_libevent interface, which doesn't seem particularly compelling. Changed. On 2012/11/20 14:47:58, Mark Mentovai wrote: > Does this ever return false?
https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... File base/message_pump_io_ios.mm (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.mm:25: return true; I spoke too soon; when I change this signature, compile fails on iOS due to files in net requiring the method to return a bool. It appears that these files all end with _libevent, so I'll take as followup work seeing whether we really need to be building these files on iOS, and if not, whether I can then change the signature of this method. On 2012/11/20 20:38:54, blundell wrote: > No. The only reason that this was not void was for consistency with the > message_pump_libevent interface, which doesn't seem particularly compelling. > Changed. > > On 2012/11/20 14:47:58, Mark Mentovai wrote: > > Does this ever return false? >
https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/9/base/message_pump_io_i... base/message_pump_io_ios.h:17: class MessagePumpIOSForIO : public MessagePumpNSRunLoop { On 2012/11/20 20:38:54, blundell wrote: > When we were originally bringing up this code, we experienced problems using the > CFRunLoop flavor that disappeared when we moved to the NSRunLoop flavor. We > didn't figure out what the root cause was at that time. I didn't revisit that > decision when doing this work. :| I remember I tracked that down and discussed the problem with Mark. We depend on something that we do in NSRunLoop but not in CFRunLoop. I seem to remember I wrote a proof-of-concept patch to make CFRunLoop work as well, but didn't pursue it further. Note: when I last worked on this, I became fascinated by the message loop code and spent a lot of time understanding it. I gradually realized that any issue I found could be "working as intended" or could take a lot of effort to explain to a code reviewer. Eventually I gave up.
I couldn't find my previous discussions with Mark on the CFRunLoop vs. NSRunLoop issue in my mail folder. I found two bugs I filed: https://code.google.com/p/chromium/issues/detail?id=87809 https://code.google.com/p/chromium/issues/detail?id=88806 It could be the second bug, or something I alluded to in there. By the way, you can use the first bug for this CL.
https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_loop.cc File base/message_loop.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_loop.cc#... base/message_loop.cc:792: static_cast<base::MessagePumpIOSForIO::Mode>(mode), No cast, the callee should be accepting int. See the next comment. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_loop.h File base/message_loop.h (right): https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_loop.h#n... base/message_loop.h:638: WATCH_READ_WRITE = base::MessagePumpLibevent::WATCH_READ_WRITE I think it would be good to fix this too. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.h:107: Mode mode, This should be an int argument. A variable of enum type can only legally store the values enumerated in the type. A compiler, assuming that those are the only possible values, might optimize away code that considers that mode might be READ | WRITE. If you accept this argument as an int or some other integral type, this isn’t a problem. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... File base/message_pump_io_ios.mm (right): https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:52: MessagePumpIOSForIO* pump) { Do you want to DCHECK that you’re really listening for read events if you enter this function? Likewise for write events in the next function. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:141: controller->callback_types_ |= callback_types; Does this “else” block ever get entered in practice? It’d be easier to just not support this. If this does need to happen in practice: Who documents that the right thing to do here is |= (add) instead of = (replace)? https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:180: CHECK_EQ(fdref, controller->fdref_); What justifies this being a CHECK and not a DCHECK? Same on line 194. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:184: if (callback_types & kCFFileDescriptorWriteCallBack) { Your style in this file has varied between (v & k) and ((v & k) != 0), as on lines 92 and 95. My preference is for this form, but it doesn’t matter what I like better as long as you’re consistent. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:193: // controller->persistent. |fdref| may have been invalidated. If you’re concerned that controller may be gone but you go ahead and re-enable callbacks anyway, then the next time the callback runs, controller will be, um, gone. Therefore I either don’t understand the paranoia that the comment expresses, or I don’t understand why similar paranoia hasn’t been applied to the code itself. I haven’t done a full analysis of the flow so this may be a silly question. You might be able to answer it just by explaining it, without having to change any code at all.
Thanks for the review. Comments addressed. Wan-Teh, can you comment on the discussion re: re-enabling the callback in HandleFdIOEvent? https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_loop.cc File base/message_loop.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_loop.cc#... base/message_loop.cc:792: static_cast<base::MessagePumpIOSForIO::Mode>(mode), On 2012/11/20 22:29:27, Mark Mentovai wrote: > No cast, the callee should be accepting int. See the next comment. Done. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_loop.h File base/message_loop.h (right): https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_loop.h#n... base/message_loop.h:638: WATCH_READ_WRITE = base::MessagePumpLibevent::WATCH_READ_WRITE On 2012/11/20 22:29:27, Mark Mentovai wrote: > I think it would be good to fix this too. Done. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.h:107: Mode mode, Thanks for the catch! Done. On 2012/11/20 22:29:27, Mark Mentovai wrote: > This should be an int argument. A variable of enum type can only legally store > the values enumerated in the type. A compiler, assuming that those are the only > possible values, might optimize away code that considers that mode might be READ > | WRITE. If you accept this argument as an int or some other integral type, this > isn’t a problem. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... File base/message_pump_io_ios.mm (right): https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:52: MessagePumpIOSForIO* pump) { On 2012/11/20 22:29:27, Mark Mentovai wrote: > Do you want to DCHECK that you’re really listening for read events if you enter > this function? Likewise for write events in the next function. Done. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:141: controller->callback_types_ |= callback_types; I don't know whether it ever gets entered in practice. These semantics match those of the corresponding function in message_pump_libevent, and are documented at the function declaration in message_pump_io_ios.h. It seems to me like it's worth keeping consistent semantics between the two implementations. On 2012/11/20 22:29:27, Mark Mentovai wrote: > Does this “else” block ever get entered in practice? It’d be easier to just not > support this. > > If this does need to happen in practice: > > Who documents that the right thing to do here is |= (add) instead of = > (replace)? https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:180: CHECK_EQ(fdref, controller->fdref_); Changed. On 2012/11/20 22:29:27, Mark Mentovai wrote: > What justifies this being a CHECK and not a DCHECK? Same on line 194. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:184: if (callback_types & kCFFileDescriptorWriteCallBack) { Changed to consistently use the (v & k) form both here and in message_pump_libevent. On 2012/11/20 22:29:27, Mark Mentovai wrote: > Your style in this file has varied between (v & k) and ((v & k) != 0), as on > lines 92 and 95. My preference is for this form, but it doesn’t matter what I > like better as long as you’re consistent. https://chromiumcodereview.appspot.com/11412101/diff/11/base/message_pump_io_... base/message_pump_io_ios.mm:193: // controller->persistent. |fdref| may have been invalidated. My understanding is that the |persistent| argument to |WatchFileDescriptor()| specifies whether the controller is persistent. If the controller is persistent, then it is safe to re-enable the callback. The problem is that if the controller is not persistent it might have already gone away by the time we do the check, hence the caching of the value. Unfortunately, that argument is not documented in either the header of this file or message_pump_libevent.h. Wan-Teh, can you confirm? On 2012/11/20 22:29:27, Mark Mentovai wrote: > If you’re concerned that controller may be gone but you go ahead and re-enable > callbacks anyway, then the next time the callback runs, controller will be, um, > gone. Therefore I either don’t understand the paranoia that the comment > expresses, or I don’t understand why similar paranoia hasn’t been applied to the > code itself. > > I haven’t done a full analysis of the flow so this may be a silly question. You > might be able to answer it just by explaining it, without having to change any > code at all.
https://codereview.chromium.org/11412101/diff/11/base/message_pump_io_ios.mm File base/message_pump_io_ios.mm (right): https://codereview.chromium.org/11412101/diff/11/base/message_pump_io_ios.mm#... base/message_pump_io_ios.mm:1: // Copyright 2012 The Chromium Authors. All rights reserved. This file is named message_pump_io_ios.mm, but I think it is pure C++ code. I suggest renaming it message_pump_io_ios.cc to make that clear. https://codereview.chromium.org/11412101/diff/11/base/message_pump_io_ios.mm#... base/message_pump_io_ios.mm:180: CHECK_EQ(fdref, controller->fdref_); On 2012/11/20 22:29:27, Mark Mentovai wrote: > What justifies this being a CHECK and not a DCHECK? Same on line 194. I may have used a CHECK to try to expose bugs using release builds. Since this code has been in use for a long time, it is fine to lower the CHECK to a DCHECK. https://codereview.chromium.org/11412101/diff/11/base/message_pump_io_ios.mm#... base/message_pump_io_ios.mm:188: controller->OnFileCanReadWithoutBlocking(fd, pump); I may have filed a bug report about this. |controller| may have been deleted during the controller->OnFileCanWriteWithoutBlocking() call above, so in general this could be problematic. I don't know what's the best solution, and why this hasn't affected us. willchan used the "weak pointer" to solve a similar problem in SSLClientSocketNSS. https://codereview.chromium.org/11412101/diff/11/base/message_pump_io_ios.mm#... base/message_pump_io_ios.mm:193: // controller->persistent. |fdref| may have been invalidated. At this point, |controller| may have been destroyed during the controller->OnFileCanWriteWithoutBlocking() or controller->OnFileCanReadWithoutBlocking() call. Note that even a persistent |controller| may have been destroyed, if its job is done. I think Mark pointed out a valid problem with this code (lines 195-196). I suspect that if CFFileDescriptorIsValid(fdref) is true, then |controller| is still alive, which is why it is valid to check |persistent|, and should be equally valid to just check |controller->persistent| directly. I just studied this code and I believe this is true. This can be verified with some debugging code. When I worked on this code, I added a |valid| member to |controller| and used that to check if |controller| was still alive. You can use the same technique. Proof: CFFileDescriptorInvalidate is only called by the destructor or the reset() method of the ScopedCFFileDescriptorRef class. ScopedCFFileDescriptorRef is only used as the fdref_ member of FileDescriptorWatcher (the |controller|). Therefore, if CFFileDescriptorIsValid(fdref) is true, it should imply that the containing |controller| has not called reset() or destroyed its fdref_ member, so |controller| should still be valid.
Review comments on patch set 6: You should point out (in the CL's description) that the three new files are based on message_pump_libevent*. If you use svn, consider using "svn copy" to create the three new files, so that we can see their differences from message_pump_libevent* more easily. Thank you for your work on this code! https://codereview.chromium.org/11412101/diff/9006/base/message_pump_libevent... File base/message_pump_libevent_unittest.cc (right): https://codereview.chromium.org/11412101/diff/9006/base/message_pump_libevent... base/message_pump_libevent_unittest.cc:133: MessagePumpLibevent::WATCH_READ | MessagePumpLibevent::WATCH_WRITE, This is very long. I think we can still define the enumerator WATCH_READ_WRITE, while declaring the |mode| argument of WatchFileDescriptor as int.
Thanks for your answers to those open questions, wtc. blundell, I think that with those final minor changes that were suggested, this should be good enough to LG.
https://codereview.chromium.org/11412101/diff/11/base/message_pump_io_ios.mm File base/message_pump_io_ios.mm (right): https://codereview.chromium.org/11412101/diff/11/base/message_pump_io_ios.mm#... base/message_pump_io_ios.mm:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2012/11/21 18:31:05, wtc wrote: > This file is named message_pump_io_ios.mm, but I think > it is pure C++ code. I suggest renaming it > message_pump_io_ios.cc to make that clear. AFAIK our policy has always been to name Mac (and for the same reason, iOS) files .mm, under the theory that a Mac/iOS specific file might pick up ObjC code at any time, and having to rename a file just to add a line of two of code is a colossal pain.
Not true for Mac Chrome. On the contrary, the rename is such a simple and lightweight operation that we don’t make files .mm unless they need to go Objective. Maybe iOS Chrome has a different policy.
On 2012/11/21 21:20:44, Mark Mentovai wrote: > Maybe iOS Chrome has a different policy. Or maybe I have a bad memory :) The rule I was thinking of is actually "use .mm instead of .m", not "use .mm instead of .cc".
Hi Mark and Wan-Teh, Thanks for your reviews. To be sure of our solution to the |controller| lifecycle problem, I ported the message_pump_libevent unittests that are designed to check these corner cases to message_pump_io_ios. Wan-Teh's solution unfortunately doesn't work, due to the fact that the |fdref| passed in to |MessagePumpIOSForIO::HandleFdIOEvent| is in fact owned by the |controller| and thus goes away if the controller goes away (when I brought up the tests, they immediately crashed for this reason :). I ended up using the same solution as that employed in |MessagePumpLibevent::OnLibeventNotification| : making |controller| a weak pointer. FWIW, it's not clear to me whether it's necessary to support this case (where |controller| is destroyed in the middle of executing one of its own callbacks). This case doesn't seem to occur on iOS in practice, because it would have caused crashes that we never saw. Moreover, looking at (e.g.) the |MessagePumpLibevent::FileDescriptorWatcher::OnFileCanWriteWithoutBlocking| function, there's nothing there to make me think that it would be safe for the delegate to delete the FileDescriptorWatcher instance in the middle of that function. That discussion would probably be outside of the scope of this CL. https://chromiumcodereview.appspot.com/11412101/diff/9006/base/message_pump_l... File base/message_pump_libevent_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/9006/base/message_pump_l... base/message_pump_libevent_unittest.cc:133: MessagePumpLibevent::WATCH_READ | MessagePumpLibevent::WATCH_WRITE, On 2012/11/21 18:48:42, wtc wrote: > > This is very long. I think we can still define the enumerator > WATCH_READ_WRITE, while declaring the |mode| argument of > WatchFileDescriptor as int. Done.
https://chromiumcodereview.appspot.com/11412101/diff/18/base/mac/scoped_cffil... File base/mac/scoped_cffiledescriptorref.h (right): https://chromiumcodereview.appspot.com/11412101/diff/18/base/mac/scoped_cffil... base/mac/scoped_cffiledescriptorref.h:16: // ScopedCFFileDescriptorRef is designed after ScopedCFTypeRef<>. On Due to the fact that I'm using git, this file becoming a copy rather than a straight add was an unavoidable side-effect of making the other files copies as desired (on git, whether a file is a copy is determined solely by looking at content similarity, with a global knob that you can set for the threshold of similarity necessary).
Review comments on patch set 7: Because of the renaming of base/message_pump_io_ios.cc, it is not possible to see the diffs of that file between patch sets 6 and 7. The rest of the diffs between patch sets 6 and 7 look correct. https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_io_... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_io_... base/message_pump_io_ios.cc:199: CFFileDescriptorIsValid(fdref)) Can you print the retain count of fdref at strategic points, in particular before and after the CFFileDescriptorEnableCallBacks and CFFileDescriptorDisableCallBacks calls? If those calls do not change the retain count of fdref, then I think this CFFileDescriptorIsValid(fdref) call here is wrong and should be removed. Note: I remember running into a destroyed 'controller' at this point. I was running this code on Mac OS X though (I manually modified Chrome to use this message pump on Mac OS X). I don't know what has changed since then. https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_lib... File base/message_pump_libevent.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_lib... base/message_pump_libevent.cc:154: mode == (WATCH_READ | WATCH_WRITE)); Can you use WATCH_READ_WRITE here?
https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_io_... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_io_... base/message_pump_io_ios.cc:178: int fd = CFFileDescriptorGetNativeDescriptor(fdref); The changes to message_pump_io_ios.{mm, cc} from patch 6 to patch 7 are to this function: - Maintaining |controller| as a weak pointer - Guarding the read callback against the controller having been deleted/the fd invalidated - Explicitly checking for deletion of the controller before re-enabling the callback - Changing comments accordingly with the above changes https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_io_... base/message_pump_io_ios.cc:199: CFFileDescriptorIsValid(fdref)) |CFFileDescriptorIsValid()| checks whether |CFFileDescriptorInvalidate()| has been called (e.g., via a call to |FileDescriptorWatcher::StopWatchingFileDescriptor()|. If |fdref| has been invalidated, then I would think that we shouldn't re-enable the callbacks. Note that if we omit the check for validity before performing the read callback, the |StopWatcher| unittest fails because |StopWatchingFileDescriptor()| resets internal state and hence the DCHECK that we are listening for read callbacks goes off. Does that make sense? However, your comment made me realize that there's still a problem with this code: if |StopWatchingFileDescriptor()| is called, |controller->fdref_| will be reset, and thus we shouldn't access |fdref| anymore either. In the newest patch, changed the code to check |controller->fdref_|. On 2012/11/26 22:17:42, wtc wrote: > > Can you print the retain count of fdref at strategic points, > in particular before and after the CFFileDescriptorEnableCallBacks > and CFFileDescriptorDisableCallBacks calls? If those calls > do not change the retain count of fdref, then I think this > CFFileDescriptorIsValid(fdref) call here is wrong and should > be removed. > > Note: I remember running into a destroyed 'controller' at > this point. I was running this code on Mac OS X though > (I manually modified Chrome to use this message pump on > Mac OS X). I don't know what has changed since then. https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_lib... File base/message_pump_libevent.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_lib... base/message_pump_libevent.cc:154: mode == (WATCH_READ | WATCH_WRITE)); On 2012/11/26 22:17:42, wtc wrote: > > Can you use WATCH_READ_WRITE here? Done.
Patch set 8 LGTM. Please commit this CL without waiting for my review. Just upload a new patch set before you commit it. Thanks. You can ignore my two "Nit" comments. https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_io_... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/18/base/message_pump_io_... base/message_pump_io_ios.cc:199: CFFileDescriptorIsValid(fdref)) On 2012/11/27 10:58:43, blundell wrote: > |CFFileDescriptorIsValid()| checks whether |CFFileDescriptorInvalidate()| has > been called (e.g., via a call to > |FileDescriptorWatcher::StopWatchingFileDescriptor()|. If |fdref| has been > invalidated, then I would think that we shouldn't re-enable the callbacks. Note > that if we omit the check for validity before performing the read callback, the > |StopWatcher| unittest fails because |StopWatchingFileDescriptor()| resets > internal state and hence the DCHECK that we are listening for read callbacks > goes off. Does that make sense? Yes, that makes sense. (My brain is not in its optimal state right now, so I am not confident that I fully understand this code. But your explanation makes sense.) https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_io_ios.cc:190: // not been deleted or |StopWatchingFileDescriptor()| been called within the This comment doesn't sound right. Perhaps we should change or |StopWatchingFileDescriptor()| been called to and |StopWatchingFileDescriptor()| has not been called ? Compare this with the similar comment on lines 198-199. https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_io_ios.cc:193: controller.get() && controller->fdref_) { Is the controller->fdref_ test intended to detect the case where the controller has not been deleted but StopWatchingFileDescriptor() has been called? Does the |StopWatcher| unit test exercise this code path? Do we have a unit test that deletes the controller? https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... File base/message_pump_io_ios_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_io_ios_unittest.cc:27: int err = pipe(pipefds_); Nit: the return value of pipe() is the standard Unix system call 0 or -1 status value, not an error code, which is reported using |errno|. https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_io_ios_unittest.cc:57: int pipefds_[2]; List pipefds_ before DISALLOW_COPY_AND_ASSIGN. Should the data members and DISALLOW_COPY_AND_ASSIGN still be private? https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... File base/message_pump_libevent.cc (left): https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_libevent.cc:198: if (event_add(evt.get(), NULL) != 0) { Nit: I believe Mark did not ask for these two changes. https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... File base/message_pump_libevent.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_libevent.cc:154: mode == (WATCH_READ_WRITE)); Remove the parantheses around WATCH_READ_WRITE. You can completely undo the change to this line.
Ugh. Turn down that global git knob for detecting copies for this one change.
Hi, Comments addressed. Via futzing around in depot_tools/, I managed to get just the message_pump_io_ios* files to be regarded as copies, as desired. Thanks! https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_io_ios.cc:190: // not been deleted or |StopWatchingFileDescriptor()| been called within the Changed the comment both for your suggestion and to add clarity to the |controller->fdref_| test. On 2012/11/28 00:25:26, wtc wrote: > > This comment doesn't sound right. Perhaps we should change > or |StopWatchingFileDescriptor()| been called > to > and |StopWatchingFileDescriptor()| has not been called > ? > > Compare this with the similar comment on lines 198-199. https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_io_ios.cc:193: controller.get() && controller->fdref_) { The controller->fdref_ test is intended to detect this case, and the |StopWatcher| test does exercise this code path. The |DeleteWatcher| test deletes the controller in the write callback. On 2012/11/28 00:25:26, wtc wrote: > > Is the controller->fdref_ test intended to detect the case > where the controller has not been deleted but > StopWatchingFileDescriptor() has been called? Does the > |StopWatcher| unit test exercise this code path? > > Do we have a unit test that deletes the controller? https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... File base/message_pump_io_ios_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_io_ios_unittest.cc:27: int err = pipe(pipefds_); On 2012/11/28 00:25:26, wtc wrote: > > Nit: the return value of pipe() is the standard Unix system > call 0 or -1 status value, not an error code, which is reported > using |errno|. Done. https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_io_ios_unittest.cc:57: int pipefds_[2]; Made everything private that could be made private. The things that are still protected are called in the TEST_F sections. On 2012/11/28 00:25:26, wtc wrote: > > List pipefds_ before DISALLOW_COPY_AND_ASSIGN. > > Should the data members and DISALLOW_COPY_AND_ASSIGN still > be private? https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... File base/message_pump_libevent.cc (left): https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_libevent.cc:198: if (event_add(evt.get(), NULL) != 0) { I extrapolated from his request for consistency on either using or not using "!= 0". On 2012/11/28 00:25:26, wtc wrote: > > Nit: I believe Mark did not ask for these two changes. https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... File base/message_pump_libevent.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/11058/base/message_pump_... base/message_pump_libevent.cc:154: mode == (WATCH_READ_WRITE)); On 2012/11/28 00:25:26, wtc wrote: > > Remove the parantheses around WATCH_READ_WRITE. You can > completely undo the change to this line. Done.
LGTM, although if you choose to refactor the test, let me know so we can do another round on those changes. https://chromiumcodereview.appspot.com/11412101/diff/3038/base/message_pump_i... File base/message_pump_io_ios_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/3038/base/message_pump_i... base/message_pump_io_ios_unittest.cc:5: #include "base/message_pump_io_ios.h" Now that I can see this file as a copy of the libevent unit test, I see just how much code is shared between the two. It’s almost all of it! If you can think of a good way to share the test implementations, it’s probably worthwhile. I think that a templatized test or macro-generated test are bad ideas that wouldn’t be readable, though, so it’d have to be something else. (Maybe just a templatized or macro-generated kernel, while leaving the rest of the test structured as-is with the test class, watcher classes, and TEST_Fs written normally?) This is totally optional, because it’s possible that any “improvement” in this area will make the test less readable. https://chromiumcodereview.appspot.com/11412101/diff/3038/base/message_pump_i... base/message_pump_io_ios_unittest.cc:23: virtual void SetUp() { The libevent test still calls this an OVERRIDE.
Hi Mark (and Wan-Teh, if you're still interested :), One last go-round on this please, due to a final corner case that I noticed while doing downstream testing. Thanks! https://chromiumcodereview.appspot.com/11412101/diff/3038/base/message_pump_i... File base/message_pump_io_ios_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/3038/base/message_pump_i... base/message_pump_io_ios_unittest.cc:5: #include "base/message_pump_io_ios.h" I will think about this, but would prefer to do this as a follow-up CL if I can think of a good way to do it. On 2012/11/28 14:59:39, Mark Mentovai wrote: > Now that I can see this file as a copy of the libevent unit test, I see just how > much code is shared between the two. It’s almost all of it! If you can think of > a good way to share the test implementations, it’s probably worthwhile. I think > that a templatized test or macro-generated test are bad ideas that wouldn’t be > readable, though, so it’d have to be something else. (Maybe just a templatized > or macro-generated kernel, while leaving the rest of the test structured as-is > with the test class, watcher classes, and TEST_Fs written normally?) > > This is totally optional, because it’s possible that any “improvement” in this > area will make the test less readable. https://chromiumcodereview.appspot.com/11412101/diff/3038/base/message_pump_i... base/message_pump_io_ios_unittest.cc:23: virtual void SetUp() { On 2012/11/28 14:59:39, Mark Mentovai wrote: > The libevent test still calls this an OVERRIDE. Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.cc:196: controller.get() && controller->fdref_ == fdref) { Made this change because I saw while testing downstream that it was possible for |StopWatchingFileDescriptor| and then |Init| to be successively called from within a callback, so that |controller->fdref_| wouldn't be NULL but nonetheless |fdref| would have been invalidated.
LGTM still, but that new logic is an indication of spaghetti. :(
Patch set 10 LGTM. I am definitely still interested in this CL. Recall that I revealed I was fascinated by this code when I last worked on it. Unfortunately I am not confident that I am "in the zone" for this code, so my long comment below may be unclear or contain mistakes. I did my best :-) https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.cc:193: // another call to |FileDescriptorWatcher::Init()| may have been made from Nit: this comment should mention the FileDescriptorWatcher::Init() call uses the same controller to watch a different fd. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.cc:196: controller.get() && controller->fdref_ == fdref) { On 2012/11/28 16:56:55, blundell wrote: > Made this change because I saw while testing downstream that it was possible for > |StopWatchingFileDescriptor| and then |Init| to be successively called from > within a callback, so that |controller->fdref_| wouldn't be NULL but nonetheless > |fdref| would have been invalidated. I confirm this can happen during TCPClientSocket::Connect. Is that where you observed this behavior? If a server has multiple IP addresses, TCPClientSocket::Connect will try connecting to the IP addresses of the server one by one. If the connection to the first IP address failed, TCPClientSocket will call StopWatchingFileDescriptor on the first (Unix) socket descriptor and open a new (Unix) socket to connect to the next IP address, while reusing the same |controller|. Hmm... Perhaps this is why I used to check if fdref is still valid at the end of this function. From patch set 1 of this CL: CHECK_GT(CFGetRetainCount(fdref), 0); if (CFFileDescriptorIsValid(fdref) && persistent) CFFileDescriptorEnableCallBacks(fdref, callback_types); I am worried that the controller->fdref_ == fdref test is still not robust in theory because the OS could reuse the fdref value. I think we should at least bring back the assertion CHECK_GT(CFGetRetainCount(fdref), 0); to ensure that fdref (the pointer value) could not have been freed and immediately reused. Perhaps this function should acquire a reference to fdref to ensure that it is not freed during this function, if we are not sure whether that is guaranteed by the OS.
blundell: I have a question about an earlier comment of yours. On 2012/11/26 16:02:23, blundell wrote: > Hi Mark and Wan-Teh, > > Thanks for your reviews. To be sure of our solution to the |controller| > lifecycle problem, I ported the message_pump_libevent unittests that are > designed to check these corner cases to message_pump_io_ios. Wan-Teh's solution > unfortunately doesn't work, due to the fact that the |fdref| passed in to > |MessagePumpIOSForIO::HandleFdIOEvent| is in fact owned by the |controller| and > thus goes away if the controller goes away (when I brought up the tests, they > immediately crashed for this reason :). Looking at my old CL for this work: https://codereview.chromium.org/7276045 I see that the DeleteWatcher test didn't exist back then. Perhaps this is why I didn't handle this case. Does your finding mean the OS does not acquire a reference to |fdref| while calling our callback (i.e., the only reference to |fdref| is owned by |controller|)?
Additional comments on patch set 10: 1. I spotted some differences between the new files and the _libevent files that they're based on. I suggested some changes below. 2. Would it be appropriate to add BUG=87809 to this CL? https://chromiumcodereview.appspot.com/11412101/diff/6017/base/mac/scoped_cff... File base/mac/scoped_cffiledescriptorref.h (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/mac/scoped_cff... base/mac/scoped_cffiledescriptorref.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. Do we need "(c)" after "Copyright"? This comment applies to all the new files. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.h:37: // a File Descriptor. Don't capitalize "File Descriptor". https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.h:40: virtual ~Watcher() {} In MessagePumpLibevent, ~Watcher() is protected. Should we do the same here? https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.h:142: #endif // BASE_MESSAGE_PUMP_IO_IOS_H_ Add a space before "//". https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... File base/message_pump_io_ios_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:23: virtual void SetUp() { Add OVERRIDE. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:73: virtual void OnFileCanWriteWithoutBlocking(int fd) {} Add OVERRIDE to these two methods. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:76: #if GTEST_HAS_DEATH_TEST Should we add && !defined(NDEBUG)? MessagePumpLibeventTest has it. If you add it, also add it to the comment on line 90. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:84: ASSERT_DEBUG_DEATH(io_loop()->WatchFileDescriptor( This may be related to the previous comment. MessagePumpLibeventTest uses ASSERT_DEATH. We should eliminate the difference. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:135: SetWatcherDelegate(watcher, &delegate); Remove SetWatcherDelegate. It is done at the end of pump->WatchFileDescriptor. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... File base/message_pump_libevent.h (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... base/message_pump_libevent.h:41: class FileDescriptorWatcher; Remove this forward declaration. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... base/message_pump_libevent.h:81: MessagePumpLibevent* pump() { return pump_; } Add 'const' to this getter method. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... File base/message_pump_libevent_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... base/message_pump_libevent_unittest.cc:57: int pipefds_[2]; Nit: should we also make ui_loop_ and io_thread_ private? (pipefds_ probably needs to remain protected as in the iOS version of this file.)
Mark and Wan-Teh, Thanks for all of your thorough reviews over this code. After each of your comments on the last iteration, I have gone back to and amended the previous approach of maintaining safety in |HandleFdIIOEvent|: checking |CFFileDescriptorIsValid(fdref)| together with a retain on |fdref| to ensure that it doesn't go out from under us in the middle of the function. I think that this is much cleaner, and it fixes the edge case that wtc had pointed out still existed with the approach that I was using in the previous iteration. Patch set 11 contains the above-described change (together with an added test for checking the problematic edge case). Patch set 12 contains all the other changes that wtc suggested (separated out from patch set 11 to make it easier for you to see the meaningful change here). wtc wrote "Does your finding mean the OS does not acquire a reference to |fdref| while calling our callback (i.e., the only reference to |fdref| is owned by |controller|)?" In response, I looked through all the documentation for CFFileDescriptor and could not find an answer to this question. I think that *in practice* the answer might be "the OS does acquire a reference to |fdref| in this case" but I think that given that this behavior is undocumented it is better to program defensively here. Please review these changes. Thanks again! https://chromiumcodereview.appspot.com/11412101/diff/6017/base/mac/scoped_cff... File base/mac/scoped_cffiledescriptorref.h (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/mac/scoped_cff... base/mac/scoped_cffiledescriptorref.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. We no longer put the "(c)" in new Chromium files (can't find the reference at the moment). On 2012/11/29 00:53:21, wtc wrote: > > Do we need "(c)" after "Copyright"? > > This comment applies to all the new files. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.cc:193: // another call to |FileDescriptorWatcher::Init()| may have been made from Changed the comment entirely (see below discussion). On 2012/11/28 23:59:47, wtc wrote: > > Nit: this comment should mention the FileDescriptorWatcher::Init() call > uses the same controller to watch a different fd. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.cc:196: controller.get() && controller->fdref_ == fdref) { After your comments and Mark's comments about the growing spaghetti nature of the code, I went back to the old approach of checking |CFFileDescriptorIsValid(fdref)| directly together with an added retain of |fdref| to make sure it doesn't go away from under us. I searched through documentation on |CFFileDescriptorRef| and could not find anything indicating whether the system guarantees that it retains |fdref| before passing it in to this callback, so I think that it is best to be defensive here. On 2012/11/28 23:59:47, wtc wrote: > > On 2012/11/28 16:56:55, blundell wrote: > > Made this change because I saw while testing downstream that it was possible > for > > |StopWatchingFileDescriptor| and then |Init| to be successively called from > > within a callback, so that |controller->fdref_| wouldn't be NULL but > nonetheless > > |fdref| would have been invalidated. > > I confirm this can happen during TCPClientSocket::Connect. > Is that where you observed this behavior? > > If a server has multiple IP addresses, TCPClientSocket::Connect > will try connecting to the IP addresses of the server one > by one. If the connection to the first IP address failed, > TCPClientSocket will call StopWatchingFileDescriptor on > the first (Unix) socket descriptor and open a new (Unix) > socket to connect to the next IP address, while reusing > the same |controller|. > > Hmm... Perhaps this is why I used to check if fdref is still > valid at the end of this function. From patch set 1 of this > CL: > > CHECK_GT(CFGetRetainCount(fdref), 0); > if (CFFileDescriptorIsValid(fdref) && persistent) > CFFileDescriptorEnableCallBacks(fdref, callback_types); > > I am worried that the controller->fdref_ == fdref test is > still not robust in theory because the OS could reuse the > fdref value. I think we should at least bring back the > assertion > CHECK_GT(CFGetRetainCount(fdref), 0); > to ensure that fdref (the pointer value) could not have > been freed and immediately reused. Perhaps this function > should acquire a reference to fdref to ensure that it is > not freed during this function, if we are not sure whether > that is guaranteed by the OS. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.h:37: // a File Descriptor. On 2012/11/29 00:53:21, wtc wrote: > > Don't capitalize "File Descriptor". Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.h:40: virtual ~Watcher() {} On 2012/11/29 00:53:21, wtc wrote: > > In MessagePumpLibevent, ~Watcher() is protected. Should we > do the same here? Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios.h:142: #endif // BASE_MESSAGE_PUMP_IO_IOS_H_ On 2012/11/29 00:53:21, wtc wrote: > > Add a space before "//". Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... File base/message_pump_io_ios_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:23: virtual void SetUp() { On 2012/11/29 00:53:21, wtc wrote: > > Add OVERRIDE. Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:73: virtual void OnFileCanWriteWithoutBlocking(int fd) {} On 2012/11/29 00:53:21, wtc wrote: > > Add OVERRIDE to these two methods. Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:76: #if GTEST_HAS_DEATH_TEST On 2012/11/29 00:53:21, wtc wrote: > > Should we add && !defined(NDEBUG)? MessagePumpLibeventTest > has it. If you add it, also add it to the comment on line 90. Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:84: ASSERT_DEBUG_DEATH(io_loop()->WatchFileDescriptor( On 2012/11/29 00:53:21, wtc wrote: > > This may be related to the previous comment. MessagePumpLibeventTest > uses ASSERT_DEATH. We should eliminate the difference. Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_i... base/message_pump_io_ios_unittest.cc:135: SetWatcherDelegate(watcher, &delegate); On 2012/11/29 00:53:21, wtc wrote: > > Remove SetWatcherDelegate. It is done at the end of > pump->WatchFileDescriptor. Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... File base/message_pump_libevent.h (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... base/message_pump_libevent.h:41: class FileDescriptorWatcher; On 2012/11/29 00:53:21, wtc wrote: > > Remove this forward declaration. Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... base/message_pump_libevent.h:81: MessagePumpLibevent* pump() { return pump_; } On 2012/11/29 00:53:21, wtc wrote: > > Add 'const' to this getter method. Done. https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... File base/message_pump_libevent_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/message_pump_l... base/message_pump_libevent_unittest.cc:57: int pipefds_[2]; On 2012/11/29 00:53:21, wtc wrote: > > Nit: should we also make ui_loop_ and io_thread_ private? > (pipefds_ probably needs to remain protected as in the iOS > version of this file.) Done.
https://chromiumcodereview.appspot.com/11412101/diff/6017/base/mac/scoped_cff... File base/mac/scoped_cffiledescriptorref.h (right): https://chromiumcodereview.appspot.com/11412101/diff/6017/base/mac/scoped_cff... base/mac/scoped_cffiledescriptorref.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. blundell wrote: > We no longer put the "(c)" in new Chromium files (can't find the reference at > the moment). https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8p4JKV76kig
Patch set 12 LGTM. Thanks. blundell,droger: can you confirm that it is still necessary for the message loop for IO to be based on a CFRunLoop on iOS? Does the IO Thread call some iOS functions that require a CFRunLoop? https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... base/message_pump_io_ios.cc:178: // called, either of which will cause |fdref| to be released. Nit: move the |controller| declaration before this comment because this comment mentions |controller|. Nit: you can continue to use |fdref| for brevity in the rest of this function, and use scoped_fdref just for retaining |fdref|. https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... File base/message_pump_io_ios_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... base/message_pump_io_ios_unittest.cc:152: int fd_to_start_watching = 0) 1. Remove "explicit". 2. Nit: the native fd 0 is a valid fd (for standard input). It may be better to use -1 as the default argument. Our Style Guide recommends against default arguments. I am not sure if it is appropriate here. You can avoid this issue by using a different subclass of BaseWatcher for the StopWatcherAndWatchSomethingElse test, rather than trying to share StopWatcher between two tests. 3. The |pump| argument is not necessary. We can just use controller_->pump(). https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... base/message_pump_io_ios_unittest.cc:161: if (fd_to_start_watching_) { If you use -1 as the default value, this test should become if (fd_to_start_watching_ != -1) or if (fd_to_start_watching_ >= 0) https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... base/message_pump_io_ios.cc:176: // Ensure that |fdref| will remain live for the duration of this function Can you get the retain count of |fdref| on entry to this function? If the retain count is only 1, then |controller| is the sole owner of |fdref|, and it is necessary for this function to retain |fdref| to ensure it remains live. If the retain count is > 1, then the OS must have retained |fdref| as we suspected, and our use of scoped_fdref is defensive programming. https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... base/message_pump_io_ios.h:46: Nit: remove this blank line.
Hi, All comments addressed. Mark, I am waiting for a last LGTM from you to push this, as the code has changed since your last LGTM. Wan-Teh, as you might recall, the original concrete motivation for doing this work (1+ years ago) was to be able use CFSockets in a straightforward way on iOS. While we are not currently doing that at the moment, it is not clear whether we might do that (or use some other piece of Apple functionality) in the future. I have filed https://code.google.com/p/chromium/issues/detail?id=163570 for investigating the feasibility (and doing the cost/benefit analysis) of using message_pump_libevent on iOS. At the current time, we would like to avoid changing horses from what we have been using for the past year+. Thanks, Colin https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... base/message_pump_io_ios.cc:178: // called, either of which will cause |fdref| to be released. On 2012/11/29 20:30:34, wtc wrote: > > Nit: move the |controller| declaration before this comment > because this comment mentions |controller|. > > Nit: you can continue to use |fdref| for brevity in the > rest of this function, and use scoped_fdref just for retaining > |fdref|. Done. https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... File base/message_pump_io_ios_unittest.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... base/message_pump_io_ios_unittest.cc:152: int fd_to_start_watching = 0) 1. Done. 2. Changed the value of the default argument to -1. Thanks! Re: having the default argument, I couldn't find anything in the Chromium style guide (let me know if I missed it), but the Google C++ style guide says the following (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments): "One specific exception is when the function is a static function (or in an unnamed namespace) in a .cc file. In this case, the cons don't apply since the function's use is so localized." 3. |pump()| is a private method, and I can't make |StopWatcher| a friend class of |FileDescriptorWatcher| as it's in an unnamed namespace here. On 2012/11/29 20:30:34, wtc wrote: > > 1. Remove "explicit". > > 2. Nit: the native fd 0 is a valid fd (for standard input). > It may be better to use -1 as the default argument. > > Our Style Guide recommends against default arguments. I am > not sure if it is appropriate here. > > You can avoid this issue by using a different subclass of > BaseWatcher for the StopWatcherAndWatchSomethingElse test, > rather than trying to share StopWatcher between two tests. > > 3. The |pump| argument is not necessary. We can just use > controller_->pump(). https://chromiumcodereview.appspot.com/11412101/diff/13027/base/message_pump_... base/message_pump_io_ios_unittest.cc:161: if (fd_to_start_watching_) { On 2012/11/29 20:30:34, wtc wrote: > > If you use -1 as the default value, this test should become > if (fd_to_start_watching_ != -1) > or > if (fd_to_start_watching_ >= 0) Done. https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... base/message_pump_io_ios.cc:176: // Ensure that |fdref| will remain live for the duration of this function The retain count is 2, so in fact the system is retaining |fdref| in practice. Nonetheless, I would like to avoid relying on this undocumented behavior. On 2012/11/29 20:30:34, wtc wrote: > > Can you get the retain count of |fdref| on entry to this > function? > > If the retain count is only 1, then |controller| is the sole > owner of |fdref|, and it is necessary for this function to > retain |fdref| to ensure it remains live. > > If the retain count is > 1, then the OS must have retained > |fdref| as we suspected, and our use of scoped_fdref is > defensive programming. https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... File base/message_pump_io_ios.h (right): https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... base/message_pump_io_ios.h:46: On 2012/11/29 20:30:34, wtc wrote: > > Nit: remove this blank line. Done.
Patch set 13 LGTM. https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... File base/message_pump_io_ios.cc (right): https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... base/message_pump_io_ios.cc:176: // Ensure that |fdref| will remain live for the duration of this function On 2012/11/30 11:19:52, blundell wrote: > The retain count is 2, so in fact the system is retaining > |fdref| in practice. In https://chromiumcodereview.appspot.com/11412101/#msg16, you reported that "the |fdref| passed in to |MessagePumpIOSForIO::HandleFdIOEvent| is in fact owned by the |controller| and thus goes away if the controller goes away". If the retain count is 2, |fdref| should not go away when the controller goes away. Can you find out exactly what happened when you ran the DeleteWatcher test? Perhaps the test crashed when calling controller->OnFileCanReadWithoutBlocking rather than when reading controller->is_persistent? As an experiment, if you remove the scoped_fdref variable, do the tests still pass? > Nonetheless, I would like to avoid relying on this > undocumented behavior. Agreed. I just wanted to fully understand this code. Thanks.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/11412101/4022
On 2012/11/30 15:14:20, wtc wrote: > Patch set 13 LGTM. > > https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... > File base/message_pump_io_ios.cc (right): > > https://chromiumcodereview.appspot.com/11412101/diff/7037/base/message_pump_i... > base/message_pump_io_ios.cc:176: // Ensure that |fdref| will remain live for the > duration of this function > > On 2012/11/30 11:19:52, blundell wrote: > > The retain count is 2, so in fact the system is retaining > > |fdref| in practice. > > In https://chromiumcodereview.appspot.com/11412101/#msg16, > you reported that "the |fdref| passed in to > |MessagePumpIOSForIO::HandleFdIOEvent| is in fact owned by > the |controller| and thus goes away if the controller goes > away". If the retain count is 2, |fdref| should not go away > when the controller goes away. Can you find out exactly what > happened when you ran the DeleteWatcher test? Perhaps the > test crashed when calling controller->OnFileCanReadWithoutBlocking > rather than when reading controller->is_persistent? > > As an experiment, if you remove the scoped_fdref variable, > do the tests still pass? > Wan-Teh, The tests don't do an extra retain of |fdref| anywhere, hence the reason for the crash if |HandleFdIOEvent| does not retain it. I had erroneously jumped to the conclusion that the system did not either, which as we've seen is not true in practice. We could of course change the tests to retain |fdref|, but as it is they effectively serve to test that |HandleFdIOEvent| does not rely on its caller retaining |fdref|, which is nice. My apologies for the earlier misleading comment :). > > Nonetheless, I would like to avoid relying on this > > undocumented behavior. > > Agreed. I just wanted to fully understand this code. Thanks.
Message was sent while issue was closed.
Change committed as 170470 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
