1440199 - Remove unused functionality from Chromium IPC shared memory, including named shared memory

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P1)

Description

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

There are a few things in base::SharedMemory that aren't currently being used — locks, named segments, and POSIX-only IDs (really inode numbers).  We could remove a few hundred lines of code, and I have patches which do this and pass Try.

This would also make it easier to make improvements to what remains of shared memory, like the sandbox integration proposed in bug 1439057, or maybe using shm_open() on POSIX platforms instead of direct filesystem access.

(It might also help with possibly simplifying the class hierarchy — there are (at least?) four different classes representing various aspects of “a shared memory segment”, and I don't think the code really needs to be that difficult to follow.)

Comment 1

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

Attachment: Bug 1440199 - Part 1: Remove Chromium shared memory locks.

This deletes some dead code and removes a dependency on the shared
memory object's name, which will be removed in the next patch (and is
always empty in our usage).

Review commit: https://reviewboard.mozilla.org/r/229606/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229606/

Comment 2

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

Attachment: Bug 1440199 - Part 2: Remove named mode from IPC shared memory.

We're not using named shared memory, and supporting only anonymous
shared memory allows using other backends that are more compatible
with preventing a process from accessing any shared memory it wasn't
explicitly granted (i.e., sandboxing).

This patch also fixes some comments in what remains of
SharedMemory::Create.

Review commit: https://reviewboard.mozilla.org/r/229608/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229608/

Comment 3

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

Attachment: Bug 1440199 - Part 3: Remove IPC shared memory IDs.

This code isn't blocking anything, but it's dead and I don't think we
have any plans to use it.

Review commit: https://reviewboard.mozilla.org/r/229610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229610/

Comment 4

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

I wasn't sure who else among the IPC peers would be a good reviewer for shmem; redirect if needed.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8960849 [details]
Bug 1440199 - Part 1: Remove Chromium shared memory locks.

https://reviewboard.mozilla.org/r/229606/#review235518

I would think we do want some version of this at some point, right?  With potentially placing things in shared memory for multiple content processes, we theoretically need some sort of cross-thread/process lock?  But maybe this facility is not yet good enough and we should design something better.

Regardless, unused code is deleted code.  r+!

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8960850 [details]
Bug 1440199 - Part 2: Remove named mode from IPC shared memory.

https://reviewboard.mozilla.org/r/229608/#review235524

r=me assuming my understanding below is correct.

::: ipc/chromium/src/base/shared_memory.h
(Diff revision 1)
> -  // If read_only is true, opens the memory as read-only.
> -  // If open_existing is true, and the shared memory already exists,
> -  // opens the existing shared memory and ignores the size parameter.
> -  // If name is the empty string, use a unique name.

Why are we also removing `read_only` and `open_existing` too?  I can totally understand removing `name`, but the booleans seem like they would be helpful things to have?  Oh, I suppose they don't make sense for anonymous shared memory segments, so much; read-onlyness is pointless, and opening an existing one doesn't make much sense because you don't have names.  Correct?

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8960850 [details]
Bug 1440199 - Part 2: Remove named mode from IPC shared memory.

https://reviewboard.mozilla.org/r/229608/#review235526

Sigh, let's set the r+ flag.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8960851 [details]
Bug 1440199 - Part 3: Remove IPC shared memory IDs.

https://reviewboard.mozilla.org/r/229610/#review235530

Comment 9

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

(In reply to Nathan Froyd [:froydnj] from comment #6)
> Why are we also removing `read_only` and `open_existing` too?  I can totally
> understand removing `name`, but the booleans seem like they would be helpful
> things to have?  Oh, I suppose they don't make sense for anonymous shared
> memory segments, so much; read-onlyness is pointless, and opening an
> existing one doesn't make much sense because you don't have names.  Correct?

Exactly.  But I'll add that to the commit message.

Comment 10

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

Comment on attachment 8960849 [details]
Bug 1440199 - Part 1: Remove Chromium shared memory locks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229606/diff/1-2/

Comment 11

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

Comment on attachment 8960850 [details]
Bug 1440199 - Part 2: Remove named mode from IPC shared memory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229608/diff/1-2/

Comment 12

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

Comment on attachment 8960851 [details]
Bug 1440199 - Part 3: Remove IPC shared memory IDs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229610/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17c89b6a5972
Part 1: Remove Chromium shared memory locks. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9561bdb8c479
Part 2: Remove named mode from IPC shared memory. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/5b55e19c6ce1
Part 3: Remove IPC shared memory IDs. r=froydnj

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/17c89b6a5972
https://hg.mozilla.org/mozilla-central/rev/9561bdb8c479
https://hg.mozilla.org/mozilla-central/rev/5b55e19c6ce1