Turn FileDescriptorInfo a collection class

Turn FileDescriptorInfo a collection class

This is a part of "kiling FileDescriptor" effort.
This CL turns vector<FileDescriptorInfo> into FileDescriptorInfo,
and makes the class a container of file descriptor and
associated ID.

The goal is to make ownership of file descriptor easy to
reason. Now the lifetime is tracked by ScopedFD and it's clear
who is responsible for that.

R=jam@chromium.org, jln@chromium.org
BUG=415294
Committed: https://crrev.com/d95714f1db8dd1fe6138579db3971da4e947a5bf
Cr-Commit-Position: refs/heads/master@{#297584}

[jln (very slow on Chromium), 2014-09-22 17:51:24]

jln@chromium.org changed reviewers:
+ mdempsky@chromium.org

[jln (very slow on Chromium), 2014-09-22 17:54:15]

(I only looked at the Zygote part). This looks ok, but having multiple file
descriptor sets to "remember" to close manually seems to defeat the purpose a
little.

Adding Matthew who re-factored this code before.

https://chromiumcodereview.appspot.com/585203002/diff/1/content/browser/zygot...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/585203002/diff/1/content/browser/zygot...
content/browser/zygote_host/zygote_host_impl_linux.cc:333: mapping.reset();
I find this slightly worse. Having one autoclose_fds variable made it easier to
know what we close.

[mdempsky, 2014-09-22 18:30:51]

https://codereview.chromium.org/585203002/diff/1/content/browser/child_proces...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/585203002/diff/1/content/browser/child_proces...
content/browser/child_process_launcher.cc:255: fds_to_map[i].second +=
base::GlobalDescriptors::kBaseDescriptor;
I know FileHandleMappingVector predates your CL, but I feel like this is a
really bad API: "first" and "second" are both ints, so from just reading the new
code in isolation, it's entirely unobvious what's going on here.

It also at first strikes me initially as a mistake that you've made a copy for
fds_to_map, and also that you're using +=.  I feel like the new logic is subtle
enough to warrant a better comment.  Something like "Make a copy of the file
descriptor mapping vector and adjust the IDs by kBaseDescriptor."

https://codereview.chromium.org/585203002/diff/1/content/browser/zygote_host/...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://codereview.chromium.org/585203002/diff/1/content/browser/zygote_host/...
content/browser/zygote_host/zygote_host_impl_linux.cc:333: mapping.reset();
On 2014/09/22 17:54:15, jln (slow on IPC reviews) wrote:
> I find this slightly worse. Having one autoclose_fds variable made it easier
to
> know what we close.

Agreed, though it's overall fewer lines of code, so I'm okay with it as a
net-win towards simplicity.

https://codereview.chromium.org/585203002/diff/1/content/public/browser/file_...
File content/public/browser/file_descriptor_info.cc (right):

https://codereview.chromium.org/585203002/diff/1/content/public/browser/file_...
content/public/browser/file_descriptor_info.cc:33: DCHECK(descriptors_.end() ==
std::find(descriptors_.begin(),
This check allows duplicate mappings for a single |id| as long as they map to
different |fd|s. Is that intentional?

https://codereview.chromium.org/585203002/diff/1/content/public/browser/file_...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/585203002/diff/1/content/public/browser/file_...
content/public/browser/file_descriptor_info.h:8: #include <map>
Why?

https://codereview.chromium.org/585203002/diff/1/content/public/browser/file_...
content/public/browser/file_descriptor_info.h:19: // It also provides a way to
know if the actual file descriptor should be
I think this sentence should just be removed: this class no longer provides a
way for users to "know" whether a descriptor was added via Transfer vs Share, as
that responsibility is now handled internally.

https://codereview.chromium.org/585203002/diff/1/content/public/browser/file_...
content/public/browser/file_descriptor_info.h:22: class CONTENT_EXPORT
FileDescriptorInfo {
No unit tests?

https://codereview.chromium.org/585203002/diff/1/content/public/browser/file_...
content/public/browser/file_descriptor_info.h:27: void Transfer(int id,
base::ScopedFD fd);
Comments/documentation?

https://codereview.chromium.org/585203002/diff/1/content/public/browser/file_...
content/public/browser/file_descriptor_info.h:29: void
GetMapping(base::FileHandleMappingVector* mapping) const;
GetMapping() doesn't seem to be implemented.

[Hajime Morrita, 2014-09-22 22:59:26]

Thanks for the thorough review!
I updated the CL. PTAL?

https://chromiumcodereview.appspot.com/585203002/diff/1/content/browser/child...
File content/browser/child_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/585203002/diff/1/content/browser/child...
content/browser/child_process_launcher.cc:255: fds_to_map[i].second +=
base::GlobalDescriptors::kBaseDescriptor;
On 2014/09/22 18:30:51, mdempsky wrote:
> I know FileHandleMappingVector predates your CL, but I feel like this is a
> really bad API: "first" and "second" are both ints, so from just reading the
new
> code in isolation, it's entirely unobvious what's going on here.
> 
> It also at first strikes me initially as a mistake that you've made a copy for
> fds_to_map, and also that you're using +=.  I feel like the new logic is
subtle
> enough to warrant a better comment.  Something like "Make a copy of the file
> descriptor mapping vector and adjust the IDs by kBaseDescriptor."
Agreed. I cannot kill FileHandleMappingVector so let me hide the weird part
behind a FileDescriporInfo API.

https://chromiumcodereview.appspot.com/585203002/diff/1/content/browser/zygot...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/585203002/diff/1/content/browser/zygot...
content/browser/zygote_host/zygote_host_impl_linux.cc:333: mapping.reset();
On 2014/09/22 18:30:51, mdempsky wrote:
> On 2014/09/22 17:54:15, jln (slow on IPC reviews) wrote:
> > I find this slightly worse. Having one autoclose_fds variable made it easier
> to
> > know what we close.
> 
> Agreed, though it's overall fewer lines of code, so I'm okay with it as a
> net-win towards simplicity.
Let me have some addition to FileDescriptorInfo to simplify this.

https://chromiumcodereview.appspot.com/585203002/diff/1/content/public/browse...
File content/public/browser/file_descriptor_info.h (right):

https://chromiumcodereview.appspot.com/585203002/diff/1/content/public/browse...
content/public/browser/file_descriptor_info.h:19: // It also provides a way to
know if the actual file descriptor should be
Rewrite to match what we have.

https://chromiumcodereview.appspot.com/585203002/diff/1/content/public/browse...
content/public/browser/file_descriptor_info.h:22: class CONTENT_EXPORT
FileDescriptorInfo {
On 2014/09/22 18:30:51, mdempsky wrote:
> No unit tests?

Done.

https://chromiumcodereview.appspot.com/585203002/diff/1/content/public/browse...
content/public/browser/file_descriptor_info.h:27: void Transfer(int id,
base::ScopedFD fd);
On 2014/09/22 18:30:51, mdempsky wrote:
> Comments/documentation?

Done.

https://chromiumcodereview.appspot.com/585203002/diff/1/content/public/browse...
content/public/browser/file_descriptor_info.h:29: void
GetMapping(base::FileHandleMappingVector* mapping) const;
On 2014/09/22 18:30:51, mdempsky wrote:
> GetMapping() doesn't seem to be implemented.

Acknowledged.

[jam, 2014-09-23 05:16:09]

https://codereview.chromium.org/585203002/diff/40001/content/public/browser/f...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/585203002/diff/40001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:15: // FileDescriptorInfo is a
collection of file descriptors which is
this doesn't conform to the content api
http://www.chromium.org/developers/content-module/content-api

[Hajime Morrita, 2014-09-23 21:20:42]

Thanks for the review John!

On 2014/09/23 05:16:09, jam wrote:
>
https://codereview.chromium.org/585203002/diff/40001/content/public/browser/f...
> File content/public/browser/file_descriptor_info.h (right):
> 
>
https://codereview.chromium.org/585203002/diff/40001/content/public/browser/f...
> content/public/browser/file_descriptor_info.h:15: // FileDescriptorInfo is a
> collection of file descriptors which is
> this doesn't conform to the content api
> http://www.chromium.org/developers/content-module/content-api

Right. Updated to conform to that. PTAL?

[jam, 2014-09-24 20:45:47]

lgtm

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:14: // needed to launch a process.
You should to tell FileDescriptorInfo
nit: You should tell

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:24: // Add and FD associated with
an ID, without delegating the ownerhip
nit: "Add an"

also leave a blank line between each method to make things easier to read

[mdempsky, 2014-09-24 21:16:45]

overall lgtm too, with a handful of remaining nits

https://codereview.chromium.org/585203002/diff/80001/content/browser/android/...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/585203002/diff/80001/content/browser/android/...
content/browser/android/child_process_launcher_android.cc:137: file_fds[i] =
dup(files_to_register->GetFDAt(i));
Needing to dup() even file descriptors that are marked as auto_close seems
unfortunate.  That could maybe be fixed with a TakeOrDupFDAt() function, but I'm
not sure I like that either.  Shrug.

https://codereview.chromium.org/585203002/diff/80001/content/browser/android/...
content/browser/android/child_process_launcher_android.cc:138: CHECK(0 <=
file_fds[i]);
FYI, dup() sets errno, so you can use PCHECK() to automatically include the
failure reason in the logs.

https://codereview.chromium.org/585203002/diff/80001/content/browser/file_des...
File content/browser/file_descriptor_info_impl.cc (right):

https://codereview.chromium.org/585203002/diff/80001/content/browser/file_des...
content/browser/file_descriptor_info_impl.cc:46: DCHECK(mapping_.end() ==
I still suspect that we should be checking for duplicate ids, not just for
duplicate (id, fd) pairs.

https://codereview.chromium.org/585203002/diff/80001/content/browser/file_des...
File content/browser/file_descriptor_info_impl_unittest.cc (right):

https://codereview.chromium.org/585203002/diff/80001/content/browser/file_des...
content/browser/file_descriptor_info_impl_unittest.cc:53:
ASSERT_TRUE(VerifyClosed(raw_fd));
FYI, a slightly more robust way to test that a FD was closed is to create a
pipe, do your test using one end of the pipe, then check that it was closed by
using the other end; e.g., read()'ing from a pipe will return EOF if the writing
end was successfully closed.  (A socket pair would also work.)

But I think this test is fine as is for now.

https://codereview.chromium.org/585203002/diff/80001/content/browser/zygote_h...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://codereview.chromium.org/585203002/diff/80001/content/browser/zygote_h...
content/browser/zygote_host/zygote_host_impl_linux.cc:318:
mapping->TransferWithoutMapping(peer_sock.Pass());
This feels a little awkward to me actually.  It seems like a pretty special case
operation, so users otherwise familiar with FileDescriptorInfo probably won't
know what it does off hand. It also means ForkRequest() has to modify the
mapping it's given, not just use it as input.

I'd probably suggest going back to just manually calling peer_sock.reset() below
next to mapping.reset().  That will be the same number of lines here, but it
means you can simplify FileDescriptorInfo's interface/implementation somewhat.

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:15: // which FD should be closed
and which isn't so that it can take care
nit: "and which *shouldn't*"

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:33: // A vecotr-backed map from
registered ID-FD pair.
nit: "A *vector*-backed map *of* registered ID-FD *pairs*."

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:40: // API for iterating registerd
ID-FD pair.
nit: "registered" and "pairs"

[Hajime Morrita, 2014-09-24 23:08:57]

Thanks for the review! Landing.

https://codereview.chromium.org/585203002/diff/80001/content/browser/android/...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/585203002/diff/80001/content/browser/android/...
content/browser/android/child_process_launcher_android.cc:137: file_fds[i] =
dup(files_to_register->GetFDAt(i));
On 2014/09/24 21:16:44, mdempsky wrote:
> Needing to dup() even file descriptors that are marked as auto_close seems
> unfortunate.  That could maybe be fixed with a TakeOrDupFDAt() function, but
I'm
> not sure I like that either.  Shrug.
Well, lifetime management with different languages is tricky :-(
We could try to simplify FileDescriptInfo.java though.

https://codereview.chromium.org/585203002/diff/80001/content/browser/android/...
content/browser/android/child_process_launcher_android.cc:138: CHECK(0 <=
file_fds[i]);
On 2014/09/24 21:16:44, mdempsky wrote:
> FYI, dup() sets errno, so you can use PCHECK() to automatically include the
> failure reason in the logs.

Done.

https://codereview.chromium.org/585203002/diff/80001/content/browser/file_des...
File content/browser/file_descriptor_info_impl.cc (right):

https://codereview.chromium.org/585203002/diff/80001/content/browser/file_des...
content/browser/file_descriptor_info_impl.cc:46: DCHECK(mapping_.end() ==
On 2014/09/24 21:16:44, mdempsky wrote:
> I still suspect that we should be checking for duplicate ids, not just for
> duplicate (id, fd) pairs.

Done.

https://codereview.chromium.org/585203002/diff/80001/content/browser/file_des...
File content/browser/file_descriptor_info_impl_unittest.cc (right):

https://codereview.chromium.org/585203002/diff/80001/content/browser/file_des...
content/browser/file_descriptor_info_impl_unittest.cc:53:
ASSERT_TRUE(VerifyClosed(raw_fd));
On 2014/09/24 21:16:44, mdempsky wrote:
> FYI, a slightly more robust way to test that a FD was closed is to create a
> pipe, do your test using one end of the pipe, then check that it was closed by
> using the other end; e.g., read()'ing from a pipe will return EOF if the
writing
> end was successfully closed.  (A socket pair would also work.)
> 
> But I think this test is fine as is for now.

Acknowledged.

https://codereview.chromium.org/585203002/diff/80001/content/browser/zygote_h...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://codereview.chromium.org/585203002/diff/80001/content/browser/zygote_h...
content/browser/zygote_host/zygote_host_impl_linux.cc:318:
mapping->TransferWithoutMapping(peer_sock.Pass());
On 2014/09/24 21:16:44, mdempsky wrote:
> This feels a little awkward to me actually.  It seems like a pretty special
case
> operation, so users otherwise familiar with FileDescriptorInfo probably won't
> know what it does off hand. It also means ForkRequest() has to modify the
> mapping it's given, not just use it as input.
> 
> I'd probably suggest going back to just manually calling peer_sock.reset()
below
> next to mapping.reset().  That will be the same number of lines here, but it
> means you can simplify FileDescriptorInfo's interface/implementation somewhat.
OK.

Ideally, we should change the zygote IPC protocol to simplify this. But I don't
want to have such a tricky change with this...

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:14: // needed to launch a process.
You should to tell FileDescriptorInfo
On 2014/09/24 20:45:47, jam wrote:
> nit: You should tell

Done.

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:15: // which FD should be closed
and which isn't so that it can take care
On 2014/09/24 21:16:45, mdempsky wrote:
> nit: "and which *shouldn't*"

Done.

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:24: // Add and FD associated with
an ID, without delegating the ownerhip
On 2014/09/24 20:45:47, jam wrote:
> nit: "Add an"
> 
> also leave a blank line between each method to make things easier to read

Done.

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:33: // A vecotr-backed map from
registered ID-FD pair.
On 2014/09/24 21:16:44, mdempsky wrote:
> nit: "A *vector*-backed map *of* registered ID-FD *pairs*."

Done.

https://codereview.chromium.org/585203002/diff/80001/content/public/browser/f...
content/public/browser/file_descriptor_info.h:40: // API for iterating registerd
ID-FD pair.
On 2014/09/24 21:16:45, mdempsky wrote:
> nit: "registered" and "pairs"

Done.

[Hajime Morrita, 2014-09-25 16:10:13]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-09-25 16:11:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/585203002/100001

[commit-bot: I haz the power, 2014-09-25 16:28:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-25 16:28:50]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Hajime Morrita, 2014-10-01 01:13:29]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-10-01 01:14:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/585203002/120001

[commit-bot: I haz the power, 2014-10-01 02:37:39]

Committed patchset #7 (id:120001) as 401a2d3c2f391042d98d3f1d43e9f3ab0cf9e162

[commit-bot: I haz the power, 2014-10-01 02:38:23]

Patchset 7 (id:??) landed as
https://crrev.com/d95714f1db8dd1fe6138579db3971da4e947a5bf
Cr-Commit-Position: refs/heads/master@{#297584}