1480324 - Get rid of base::FileDescriptor

Status: RESOLVED DUPLICATE of bug 1732343

(Core :: IPC, enhancement, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

base::FileDescriptor is an odd class (or rather struct): it looks like it should be RAII, with the auto_close member, but it's not.  In fact auto_close controls only what happens to fds when they're in a FileDescriptorSet that's destroyed or consumed by IPC.  And you generally don't want to set it to false, because messages are sent asynchronously.  Received messages do set it to false, and I'm not entirely convinced that's right.

Meanwhile, in bug 1479960 I'll have a patch to move mozilla::ipc::FileDescriptor::UniquePlatformHandle into mfbt/UniquePtrExtensions.h so other things can use it, because I finally got tired of having to manually close fds on error paths (and sometimes forgetting).

So, base::FileDescriptor could be replaced with that UniqueFileHandle, and FileDescriptorSet with a `mutable` (sigh) vector of them, and this should all work and hopefully be less code and simpler code.  There are a few supporting pieces, which I'm writing down so I don't forget them:

1. base::SharedMemory: UniqueFileHandle can be used instead.

2. TransportDescriptor: can use ipc::FileDescriptor; ParamTraits<ipc::FD> is implementable on POSIX (but not Windows).  Using IPDLParamTraits might simplify Endpoint<> somewhat, now that it exists, but that's beyond the scope of this bug.

3. ipc/glue/SharedMemoryBasic_chromium: the SharedMemory* stack uses ParamTraits, but this can probably just use ipc::FileDescriptor as the handle type because we don't use it on Windows; same situation as TransportDescriptor.

4. ipc::FileDescriptor: I already have a patch for this, which also merges ParamTraits<base::FileDescriptor> into IPDLParamTraits<ipc::FileDescriptor>, but ParamTraits<base::FileDescriptor> can't be removed until the previous ones are done.  Basically ipc::FileDescriptor becomes UniqueFileHandle plus copying and interprocess transferring.

Comment 1

[Kris Maglione [:kmag]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #0)
> Meanwhile, in bug 1479960 I'll have a patch to move
> mozilla::ipc::FileDescriptor::UniquePlatformHandle into
> mfbt/UniquePtrExtensions.h so other things can use it, because I finally got
> tired of having to manually close fds on error paths (and sometimes
> forgetting).

(Note we already have another RAII type for this, albeit with different and slightly weirder semantics: https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/xpcom/glue/FileUtils.h#52)

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Kris Maglione [:kmag] from comment #1)
> (Note we already have another RAII type for this, albeit with different and
> slightly weirder semantics:
> https://searchfox.org/mozilla-central/rev/
> d0a0ae30d534dc55e712bd3eedc1b4553ba56323/xpcom/glue/FileUtils.h#52)

Interesting.  It also has a double-close bug if it's interrupted; see bug 1278361.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #2)
> (In reply to Kris Maglione [:kmag] from comment #1)
> > (Note we already have another RAII type for this, albeit with different and
> > slightly weirder semantics:
> > https://searchfox.org/mozilla-central/rev/
> > d0a0ae30d534dc55e712bd3eedc1b4553ba56323/xpcom/glue/FileUtils.h#52)
> 
> Interesting.  It also has a double-close bug if it's interrupted; see bug
> 1278361.

Heh. Well that's good to know.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

base::FileDescriptor is being removed as part of bug 1732343