Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2705743002:
Introduce base::ScopedPlatformHandle (Closed)
|
Created:
3 years, 10 months ago by Ken Rockot(use gerrit already) Modified:
3 years, 9 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce base::ScopedPlatformHandle
ScopedPlatformHandle encapsulates ownership of one of various types
of platform-specific resource handles including POSIX file descriptors,
and Windows handles.
The purpose of this type is to have a consistent representation of such
resources for the sake of safe ownership semantics across various API
boundaries, relegating platform-specific details to either the code which
initially acquires the resource or the code which actually uses it.
BUG=None
Review-Url: https://codereview.chromium.org/2705743002
Cr-Commit-Position: refs/heads/master@{#453242}
Committed: https://chromium.googlesource.com/chromium/src/+/a42cfdc27822fb57673106a4b7a953d6c8cda233
Patch Set 1 #
Total comments: 1
Patch Set 2 : union #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 2
Patch Set 6 : . #Patch Set 7 : . #
Total comments: 3
Patch Set 8 : . #
Messages
Total messages: 69 (41 generated)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Description was changed from ========== Introduce base::ScopedPlatformHandle ScopedPlatformHandle encapsulates ownership of one of various types of platform-specific resource handles including POSIX file descriptors, Windows handles, and Mach ports. The purpose of this type is to have a consistent representation of such resources for the sake of safe ownership semantics, relegating platform-specific details to either the code which initially acquires the resource or the code which actually uses it. BUG=None ========== to ========== Introduce base::ScopedPlatformHandle ScopedPlatformHandle encapsulates ownership of one of various types of platform-specific resource handles including POSIX file descriptors, Windows handles, and Mach ports. The purpose of this type is to have a consistent representation of such resources for the sake of safe ownership semantics across various API boundaries, relegating platform-specific details to either the code which initially acquires the resource or the code which actually uses it. BUG=None ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rockot@chromium.org changed reviewers: + dcheng@chromium.org
an attempt at doing the thing
On 2017/02/17 at 23:51:44, Ken Rockot wrote: > an attempt at doing the thing (WIP still at least need to write mach port sanity tests)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
https://codereview.chromium.org/2705743002/diff/1/base/files/scoped_platform_... File base/files/scoped_platform_handle.h (right): https://codereview.chromium.org/2705743002/diff/1/base/files/scoped_platform_... base/files/scoped_platform_handle.h:148: // individual platform handle types. A C++11 union should still allow the existing scoping behavior to just work, right? And it'll make the handle size smaller on Mac.
On Fri, Feb 17, 2017 at 4:14 PM, <dcheng@chromium.org> wrote: > > https://codereview.chromium.org/2705743002/diff/1/base/ > files/scoped_platform_handle.h > File base/files/scoped_platform_handle.h (right): > > https://codereview.chromium.org/2705743002/diff/1/base/ > files/scoped_platform_handle.h#newcode148 > base/files/scoped_platform_handle.h:148: // individual platform handle > types. > A C++11 union should still allow the existing scoping behavior to just > work, right? And it'll make the handle size smaller on Mac. > Yes, I think so. I'm happy to go that route if you think it's reasonable. I avoided it because I thought the code would be slightly easier to get right and understand, e.g. no manual destructor invocations. > https://codereview.chromium.org/2705743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/18 00:20:46, Ken Rockot wrote: > On Fri, Feb 17, 2017 at 4:14 PM, <mailto:dcheng@chromium.org> wrote: > > > > > https://codereview.chromium.org/2705743002/diff/1/base/ > > files/scoped_platform_handle.h > > File base/files/scoped_platform_handle.h (right): > > > > https://codereview.chromium.org/2705743002/diff/1/base/ > > files/scoped_platform_handle.h#newcode148 > > base/files/scoped_platform_handle.h:148: // individual platform handle > > types. > > A C++11 union should still allow the existing scoping behavior to just > > work, right? And it'll make the handle size smaller on Mac. > > > > Yes, I think so. I'm happy to go that route if you think it's reasonable. I > avoided it because I thought the code would be slightly easier to get right > and understand, e.g. no manual destructor invocations. I'd be OK with it; it'd make the scoper a bit smaller on platforms like Mac. > > > > https://codereview.chromium.org/2705743002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
unionized!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... File base/files/scoped_platform_handle.h (right): https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... base/files/scoped_platform_handle.h:43: INVALID, How much benefit do we get from having this scoper handle mach ports as well? I'm assuming this is probably valuable for Mojo, but I'd like to better understand: having to support four types on Mac makes this a bit more complex than I'd like. If we only had to worry about WINDOWS_HANDLE vs FILE_DESCRIPTOR, then we wouldn't need a type enum at all.
https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... File base/files/scoped_platform_handle.h (right): https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... base/files/scoped_platform_handle.h:43: INVALID, On 2017/02/22 at 08:22:16, dcheng wrote: > How much benefit do we get from having this scoper handle mach ports as well? I'm assuming this is probably valuable for Mojo, but I'd like to better understand: having to support four types on Mac makes this a bit more complex than I'd like. If we only had to worry about WINDOWS_HANDLE vs FILE_DESCRIPTOR, then we wouldn't need a type enum at all. As far as I can tell, no immediate benefit outside of Mojo internals. I think it would be reasonable to start with what you suggest since it's useful enough just to unify HANDLE and fd, and if later we find enough compelling reasons to add Mach port support we can do it at that time. WDYT?
On 2017/02/22 20:30:11, Ken Rockot wrote: > https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... > File base/files/scoped_platform_handle.h (right): > > https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... > base/files/scoped_platform_handle.h:43: INVALID, > On 2017/02/22 at 08:22:16, dcheng wrote: > > How much benefit do we get from having this scoper handle mach ports as well? > I'm assuming this is probably valuable for Mojo, but I'd like to better > understand: having to support four types on Mac makes this a bit more complex > than I'd like. If we only had to worry about WINDOWS_HANDLE vs FILE_DESCRIPTOR, > then we wouldn't need a type enum at all. > > As far as I can tell, no immediate benefit outside of Mojo internals. I think it > would be reasonable to start with what you suggest since it's useful enough just > to unify HANDLE and fd, and if later we find enough compelling reasons to add > Mach port support we can do it at that time. WDYT? Sorry for the delayed response. I think that plan makes sense: I suspect most code doesn't ever need to deal with mach ports.
On 2017/02/23 at 05:32:55, dcheng wrote: > On 2017/02/22 20:30:11, Ken Rockot wrote: > > https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... > > File base/files/scoped_platform_handle.h (right): > > > > https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... > > base/files/scoped_platform_handle.h:43: INVALID, > > On 2017/02/22 at 08:22:16, dcheng wrote: > > > How much benefit do we get from having this scoper handle mach ports as well? > > I'm assuming this is probably valuable for Mojo, but I'd like to better > > understand: having to support four types on Mac makes this a bit more complex > > than I'd like. If we only had to worry about WINDOWS_HANDLE vs FILE_DESCRIPTOR, > > then we wouldn't need a type enum at all. > > > > As far as I can tell, no immediate benefit outside of Mojo internals. I think it > > would be reasonable to start with what you suggest since it's useful enough just > > to unify HANDLE and fd, and if later we find enough compelling reasons to add > > Mach port support we can do it at that time. WDYT? > > Sorry for the delayed response. I think that plan makes sense: I suspect most code doesn't ever need to deal with mach ports. So removing the type enum is nice, but it sort of fundamentally changes the API. Namely, the Get/Take/Release methods then have no legitimate reason to return a bool if the enclosed handle type can only be one kind of thing, and this will affect callers' assumptions in a way that might make it harder to change the API later. How would you feel about leaving the API as-is (including the Type enum), but just dropping the union storage and Mach port support for now?
On 2017/02/23 16:25:17, Ken Rockot wrote: > On 2017/02/23 at 05:32:55, dcheng wrote: > > On 2017/02/22 20:30:11, Ken Rockot wrote: > > > > https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... > > > File base/files/scoped_platform_handle.h (right): > > > > > > > https://codereview.chromium.org/2705743002/diff/80001/base/files/scoped_platf... > > > base/files/scoped_platform_handle.h:43: INVALID, > > > On 2017/02/22 at 08:22:16, dcheng wrote: > > > > How much benefit do we get from having this scoper handle mach ports as > well? > > > I'm assuming this is probably valuable for Mojo, but I'd like to better > > > understand: having to support four types on Mac makes this a bit more > complex > > > than I'd like. If we only had to worry about WINDOWS_HANDLE vs > FILE_DESCRIPTOR, > > > then we wouldn't need a type enum at all. > > > > > > As far as I can tell, no immediate benefit outside of Mojo internals. I > think it > > > would be reasonable to start with what you suggest since it's useful enough > just > > > to unify HANDLE and fd, and if later we find enough compelling reasons to > add > > > Mach port support we can do it at that time. WDYT? > > > > Sorry for the delayed response. I think that plan makes sense: I suspect most > code doesn't ever need to deal with mach ports. > > So removing the type enum is nice, but it sort of fundamentally changes the API. > Namely, the Get/Take/Release methods then have no legitimate reason to return a > bool if the enclosed handle type can only be one kind of thing, and this will > affect callers' assumptions in a way that might make it harder to change the API > later. How would you feel about leaving the API as-is (including the Type enum), > but just dropping the union storage and Mach port support for now? I'm starting to think Mach ports are a different enough abstraction that I'm not sure they should be covered by the same abstraction. The common case in IPC seems to be passing handles to files/shared memory/mmaped things, so as long as this supports both, I think that's probably sufficient for 99% of use cases. In that case, I think simplifying the API does make sense.
On Thu, Feb 23, 2017 at 12:05 PM, <dcheng@chromium.org> wrote: > On 2017/02/23 16:25:17, Ken Rockot wrote: > > On 2017/02/23 at 05:32:55, dcheng wrote: > > > On 2017/02/22 20:30:11, Ken Rockot wrote: > > > > > > > https://codereview.chromium.org/2705743002/diff/80001/ > base/files/scoped_platform_handle.h > > > > File base/files/scoped_platform_handle.h (right): > > > > > > > > > > > https://codereview.chromium.org/2705743002/diff/80001/ > base/files/scoped_platform_handle.h#newcode43 > > > > base/files/scoped_platform_handle.h:43: INVALID, > > > > On 2017/02/22 at 08:22:16, dcheng wrote: > > > > > How much benefit do we get from having this scoper handle mach > ports as > > well? > > > > I'm assuming this is probably valuable for Mojo, but I'd like to > better > > > > understand: having to support four types on Mac makes this a bit more > > complex > > > > than I'd like. If we only had to worry about WINDOWS_HANDLE vs > > FILE_DESCRIPTOR, > > > > then we wouldn't need a type enum at all. > > > > > > > > As far as I can tell, no immediate benefit outside of Mojo > internals. I > > think it > > > > would be reasonable to start with what you suggest since it's useful > enough > > just > > > > to unify HANDLE and fd, and if later we find enough compelling > reasons to > > add > > > > Mach port support we can do it at that time. WDYT? > > > > > > Sorry for the delayed response. I think that plan makes sense: I > suspect > most > > code doesn't ever need to deal with mach ports. > > > > So removing the type enum is nice, but it sort of fundamentally changes > the > API. > > Namely, the Get/Take/Release methods then have no legitimate reason to > return > a > > bool if the enclosed handle type can only be one kind of thing, and this > will > > affect callers' assumptions in a way that might make it harder to change > the > API > > later. How would you feel about leaving the API as-is (including the Type > enum), > > but just dropping the union storage and Mach port support for now? > > I'm starting to think Mach ports are a different enough abstraction that > I'm not > sure they should be covered by the same abstraction. The common case in IPC > seems to be passing handles to files/shared memory/mmaped things, so as > long as > this supports both, I think that's probably sufficient for 99% of use > cases. In > that case, I think simplifying the API does make sense. > Note that shared memory on Mac may use Mach Ports OR file descriptors, which is ultimately the motivation for having this level of abstraction inside Mojo. But I suppose having a separate (sanely scoped) handle type for shared memory objects could be treated as a separate problem in its own right. > > https://codereview.chromium.org/2705743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/23 20:08:41, Ken Rockot wrote: > On Thu, Feb 23, 2017 at 12:05 PM, <mailto:dcheng@chromium.org> wrote: > > > On 2017/02/23 16:25:17, Ken Rockot wrote: > > > On 2017/02/23 at 05:32:55, dcheng wrote: > > > > On 2017/02/22 20:30:11, Ken Rockot wrote: > > > > > > > > > > https://codereview.chromium.org/2705743002/diff/80001/ > > base/files/scoped_platform_handle.h > > > > > File base/files/scoped_platform_handle.h (right): > > > > > > > > > > > > > > > https://codereview.chromium.org/2705743002/diff/80001/ > > base/files/scoped_platform_handle.h#newcode43 > > > > > base/files/scoped_platform_handle.h:43: INVALID, > > > > > On 2017/02/22 at 08:22:16, dcheng wrote: > > > > > > How much benefit do we get from having this scoper handle mach > > ports as > > > well? > > > > > I'm assuming this is probably valuable for Mojo, but I'd like to > > better > > > > > understand: having to support four types on Mac makes this a bit more > > > complex > > > > > than I'd like. If we only had to worry about WINDOWS_HANDLE vs > > > FILE_DESCRIPTOR, > > > > > then we wouldn't need a type enum at all. > > > > > > > > > > As far as I can tell, no immediate benefit outside of Mojo > > internals. I > > > think it > > > > > would be reasonable to start with what you suggest since it's useful > > enough > > > just > > > > > to unify HANDLE and fd, and if later we find enough compelling > > reasons to > > > add > > > > > Mach port support we can do it at that time. WDYT? > > > > > > > > Sorry for the delayed response. I think that plan makes sense: I > > suspect > > most > > > code doesn't ever need to deal with mach ports. > > > > > > So removing the type enum is nice, but it sort of fundamentally changes > > the > > API. > > > Namely, the Get/Take/Release methods then have no legitimate reason to > > return > > a > > > bool if the enclosed handle type can only be one kind of thing, and this > > will > > > affect callers' assumptions in a way that might make it harder to change > > the > > API > > > later. How would you feel about leaving the API as-is (including the Type > > enum), > > > but just dropping the union storage and Mach port support for now? > > > > I'm starting to think Mach ports are a different enough abstraction that > > I'm not > > sure they should be covered by the same abstraction. The common case in IPC > > seems to be passing handles to files/shared memory/mmaped things, so as > > long as > > this supports both, I think that's probably sufficient for 99% of use > > cases. In > > that case, I think simplifying the API does make sense. > > > > Note that shared memory on Mac may use Mach Ports OR file descriptors, > which is ultimately the motivation for having this level of abstraction > inside Mojo. But I suppose having a separate (sanely scoped) handle type > for shared memory objects could be treated as a separate problem in its own > right. Doh. OK... maybe shared memory will have to be separate too then =( > > > > > > https://codereview.chromium.org/2705743002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK, much simpler now, PTAL
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+erikchen as FYI as well, since he did a ton of work with the ipc attachment broker https://codereview.chromium.org/2705743002/diff/160001/base/files/scoped_plat... File base/files/scoped_platform_handle.cc (right): https://codereview.chromium.org/2705743002/diff/160001/base/files/scoped_plat... base/files/scoped_platform_handle.cc:26: ScopedPlatformHandle&& other) { If you want, this and the move ctor can be = default in the .cc. https://codereview.chromium.org/2705743002/diff/160001/base/files/scoped_plat... base/files/scoped_platform_handle.cc:32: #if defined(OS_WIN) Most of the time we have files like this, we just have scoped_platform_handle_win.cc, scoped_platform_handle_posix.cc rather than conditionally compiling each method that's platform specific.
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
Tracking bug? The reason I didn't do this is because it requires fixing the ownership semantics for IPC [and rewriting the existing system for decoding IPC messages]. We might be able to avoid this with pure Mojo IPC [haven't looked in detail], but certainly not for Chrome IPC layered on Mojo IPC. I just want to make sure you've appropriately scoped the problem before jumping into a very deep pool. https://codereview.chromium.org/2705743002/diff/160001/base/files/scoped_plat... File base/files/scoped_platform_handle.h (right): https://codereview.chromium.org/2705743002/diff/160001/base/files/scoped_plat... base/files/scoped_platform_handle.h:36: using ScopedHandleType = ScopedFD; Mach Ports?
On Thu, Feb 23, 2017 at 1:52 PM, <erikchen@chromium.org> wrote: > Tracking bug? The reason I didn't do this is because it requires fixing the > ownership semantics for IPC [and rewriting the existing system for > decoding IPC > messages]. We might be able to avoid this with pure Mojo IPC [haven't > looked in > detail], but certainly not for Chrome IPC layered on Mojo IPC. > This was sort of an off-the-cuff effort based on the relatively common desire to have some kind of handle scoper which is typemapped for convenient mojom IPC without having to do an awkward platform-specific dance at every call site. > > I just want to make sure you've appropriately scoped the problem before > jumping > into a very deep pool. > Can you elaborate on the complexity of ownership semantics insofar as this might be problematic? For Mojo IPC, everything is pretty straightforward - ownership is always transferred, and we only ever work with scoped handle objects. I don't think there's any value in optimizing new APIs for new uses of the legacy IPC system. > > https://codereview.chromium.org/2705743002/diff/160001/ > base/files/scoped_platform_handle.h > File base/files/scoped_platform_handle.h (right): > > https://codereview.chromium.org/2705743002/diff/160001/ > base/files/scoped_platform_handle.h#newcode36 > base/files/scoped_platform_handle.h:36: using ScopedHandleType = > ScopedFD; > Mach Ports? > Heh. Please see earlier patch sets and discussion on this CL. :) > > https://codereview.chromium.org/2705743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/23 21:52:40, erikchen wrote: > Tracking bug? The reason I didn't do this is because it requires fixing the > ownership semantics for IPC [and rewriting the existing system for decoding IPC > messages]. We might be able to avoid this with pure Mojo IPC [haven't looked in > detail], but certainly not for Chrome IPC layered on Mojo IPC. It's a lot easier with Mojo, since mojo has move support, unlike legacy IPC. I tried to hack this in at one point in the past, and it got way too complicated. It should be feasible with Mojo. > > I just want to make sure you've appropriately scoped the problem before jumping > into a very deep pool. > > https://codereview.chromium.org/2705743002/diff/160001/base/files/scoped_plat... > File base/files/scoped_platform_handle.h (right): > > https://codereview.chromium.org/2705743002/diff/160001/base/files/scoped_plat... > base/files/scoped_platform_handle.h:36: using ScopedHandleType = ScopedFD; > Mach Ports?
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Can you elaborate on the complexity of ownership semantics insofar as this > might be problematic? For Mojo IPC, everything is pretty straightforward - > ownership is always transferred, and we only ever work with scoped handle > objects. > > I don't think there's any value in optimizing new APIs for new uses of the > legacy IPC system. In Chrome IPC, ownership is implicitly passed to the deserializer of the IPC message, which has to explicitly retain the resource. Random example: https://cs.chromium.org/chromium/src/content/child/resource_dispatcher.cc?typ... 1) This means that if a message fails to be processed, we need to explicitly release the resources anyways, somehow, or leak. There's a bunch of this logic scattered around. From same file: https://cs.chromium.org/chromium/src/content/child/resource_dispatcher.cc?typ... 2) There are places [NaCl, and I believe others] where we deserialize an IPC message more than once. That won't work with explicit ownership semantics. Switching to explicit ownership semantics in Chrome IPC means finding and fixing all sites that do (1) and (2), in additional to updating semantics for every IPC. [Slightly more abstract: switching to ownership semantics for Chrome IPC means that Message deserialization must be stateful. It's currently stateless in Chrome].
OK thanks for the clarification. I don't think Chrome IPC attachment ownership semantics should be considered relevant for this type then. Do you have any thoughts regarding whether it actually makes sense (or not) to include Mach port rights in this abstraction? In practice it doesn't look like there are any places that conflate port rights and file descriptors *except* for shared memory handles, presumably only for legacy reasons, and all the relevant code therein already has to effectively special-case every operation based on the shared memory handle type. I am so unfamiliar with Mac as a platform that I can't tell if this lack of use cases in chromium indicates some fundamental conceptual difference between the handle types, or if in practice we just end up using POSIX APIs in many places where we might eventually be better served by mach APIs. On Thu, Feb 23, 2017 at 3:22 PM, <erikchen@chromium.org> wrote: > > Can you elaborate on the complexity of ownership semantics insofar as > this > > might be problematic? For Mojo IPC, everything is pretty straightforward > - > > ownership is always transferred, and we only ever work with scoped handle > > objects. > > > > I don't think there's any value in optimizing new APIs for new uses of > the > > legacy IPC system. > > In Chrome IPC, ownership is implicitly passed to the deserializer of the > IPC > message, which has to explicitly retain the resource. Random example: > https://cs.chromium.org/chromium/src/content/child/ > resource_dispatcher.cc?type=cs&l=215 > > 1) This means that if a message fails to be processed, we need to > explicitly > release the resources anyways, somehow, or leak. There's a bunch of this > logic > scattered around. From same file: > https://cs.chromium.org/chromium/src/content/child/ > resource_dispatcher.cc?type=cs&l=809 > 2) There are places [NaCl, and I believe others] where we deserialize an > IPC > message more than once. That won't work with explicit ownership semantics. > > Switching to explicit ownership semantics in Chrome IPC means finding and > fixing > all sites that do (1) and (2), in additional to updating semantics for > every > IPC. > > [Slightly more abstract: switching to ownership semantics for Chrome IPC > means > that Message deserialization must be stateful. It's currently stateless in > Chrome]. > > https://codereview.chromium.org/2705743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/23 23:45:43, Ken Rockot wrote: > OK thanks for the clarification. I don't think Chrome IPC attachment > ownership semantics should be considered relevant for this type then. > > Do you have any thoughts regarding whether it actually makes sense (or not) > to include Mach port rights in this abstraction? In practice it doesn't > look like there are any places that conflate port rights and file > descriptors *except* for shared memory handles, presumably only for legacy > reasons, and all the relevant code therein already has to effectively > special-case every operation based on the shared memory handle type. > > I am so unfamiliar with Mac as a platform that I can't tell if this lack of > use cases in chromium indicates some fundamental conceptual difference > between the handle types, or if in practice we just end up using POSIX APIs > in many places where we might eventually be better served by mach APIs. SharedMemoryHandle is severely constrained by legacy consumers. Mach Ports have the same semantics as Windows HANDLEs, and I imagine would be transported by ScopedPlatformHandle [which is why I was surprised that it wasn't there].
On Thu, Feb 23, 2017 at 4:09 PM, <erikchen@chromium.org> wrote: > On 2017/02/23 23:45:43, Ken Rockot wrote: > > OK thanks for the clarification. I don't think Chrome IPC attachment > > ownership semantics should be considered relevant for this type then. > > > > Do you have any thoughts regarding whether it actually makes sense (or > not) > > to include Mach port rights in this abstraction? In practice it doesn't > > look like there are any places that conflate port rights and file > > descriptors *except* for shared memory handles, presumably only for > legacy > > reasons, and all the relevant code therein already has to effectively > > special-case every operation based on the shared memory handle type. > > > > I am so unfamiliar with Mac as a platform that I can't tell if this lack > of > > use cases in chromium indicates some fundamental conceptual difference > > between the handle types, or if in practice we just end up using POSIX > APIs > > in many places where we might eventually be better served by mach APIs. > > SharedMemoryHandle is severely constrained by legacy consumers. Mach Ports > have > the same semantics as Windows HANDLEs, and I imagine would be transported > by > ScopedPlatformHandle [which is why I was surprised that it wasn't there]. > It was, and I removed it because it added complexity to the implementation without having any obvious uses in Chromium code today. However if conceptually it really does make sense to include here, I think we should take care to have the right API here from the start. At this point I think I will leave the decision between Daniel as a base OWNER and you as a Mac expert. I'm happy to leave this CL as-is or take it back to something closer to PS#5. > > https://codereview.chromium.org/2705743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
ScopedMachPort is the counterpart to ScopedFD/ScopedHandle https://cs.chromium.org/chromium/src/base/mac/scoped_mach_port.h Without a tracking bug, it's really hard to say what the appropriate way forward is. If you're looking just to clean up some posix/win code, then sure, this is fine. But if you plan on fixing IPC semantics, shared memory makes up the bulk of IPC handles, so you can't avoid dealing with mach ports.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/24 at 00:21:29, erikchen wrote: > ScopedMachPort is the counterpart to ScopedFD/ScopedHandle > https://cs.chromium.org/chromium/src/base/mac/scoped_mach_port.h > > Without a tracking bug, it's really hard to say what the appropriate way forward is. If you're looking just to clean up some posix/win code, then sure, this is fine. But if you plan on fixing IPC semantics, shared memory makes up the bulk of IPC handles, so you can't avoid dealing with mach ports. It's not clear to me what a tracking bug can add to the context here (unless you just want a better place to persist discussion?), but FWIW I did file http://crbug.com/695490 earlier yesterday. As a thought experiment, could you imagine a shared memory handle type -- assuming a new, safely scoped one, not in any way relevant to or targeted toward usage with legacy Chrome IPC -- that would benefit from the existence of a ScopedPlatformHandle as defined in PS#5 of this CL, i.e. one which could hold a file descriptor OR a Mach send right? What about any other similar use cases where we might replace some file descriptor usage on OSX today with a Mach port in the future? Is it conceivable that we would have the ability and motivation to improve network/file/device IO on OS X by using Mach APIs instead of POSIX APIs? To be clear, Mojo IPC does not have a problem here. We already seamlessly and safely carry Mach ports as well as any other type of handle over message pipes. We do so using a similar ScopedPlatformHandle abstraction internal to Mojo which does encapsulate Mach ports. This abstraction is at least partially useful outside of Mojo as well, hence this CL. The question here is whether or not Mach ports have a place in the base version of the abstraction.
OK Daniel, I spoke with Erik offline and there is consensus that we should leave Mach ports out of this abstraction. Shared memory is essentially an isolated edge case worthy of its own dedicated scoping abstraction. Ready for review!
lgtm (I assume there are concrete plans to use this and switch away from things like base::FileDescriptor as we continue mojofying things)
The CQ bit was checked by rockot@chromium.org
Description was changed from ========== Introduce base::ScopedPlatformHandle ScopedPlatformHandle encapsulates ownership of one of various types of platform-specific resource handles including POSIX file descriptors, Windows handles, and Mach ports. The purpose of this type is to have a consistent representation of such resources for the sake of safe ownership semantics across various API boundaries, relegating platform-specific details to either the code which initially acquires the resource or the code which actually uses it. BUG=None ========== to ========== Introduce base::ScopedPlatformHandle ScopedPlatformHandle encapsulates ownership of one of various types of platform-specific resource handles including POSIX file descriptors, and Windows handles. The purpose of this type is to have a consistent representation of such resources for the sake of safe ownership semantics across various API boundaries, relegating platform-specific details to either the code which initially acquires the resource or the code which actually uses it. BUG=None ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488212371356460,
"parent_rev": "e6c32ed57fd0f31d1f197e239e17c7b5780f47fc", "commit_rev":
"a42cfdc27822fb57673106a4b7a953d6c8cda233"}
Message was sent while issue was closed.
Description was changed from ========== Introduce base::ScopedPlatformHandle ScopedPlatformHandle encapsulates ownership of one of various types of platform-specific resource handles including POSIX file descriptors, and Windows handles. The purpose of this type is to have a consistent representation of such resources for the sake of safe ownership semantics across various API boundaries, relegating platform-specific details to either the code which initially acquires the resource or the code which actually uses it. BUG=None ========== to ========== Introduce base::ScopedPlatformHandle ScopedPlatformHandle encapsulates ownership of one of various types of platform-specific resource handles including POSIX file descriptors, and Windows handles. The purpose of this type is to have a consistent representation of such resources for the sake of safe ownership semantics across various API boundaries, relegating platform-specific details to either the code which initially acquires the resource or the code which actually uses it. BUG=None Review-Url: https://codereview.chromium.org/2705743002 Cr-Commit-Position: refs/heads/master@{#453242} Committed: https://chromium.googlesource.com/chromium/src/+/a42cfdc27822fb57673106a4b7a9... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a42cfdc27822fb57673106a4b7a9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
