1167280 - Simplify posix shared memory for small perf improvement

Status: RESOLVED WONTFIX

(Core :: IPC, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_posix.patch

OVer in bug 1161166 I wound up with a patch for generic posix shared memory that improves performance slight. Splitting this bug off to handle that.

Comment 1

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

Comment on attachment 8608873 [details] [diff] [review]
SharedMemoryBasic_posix.patch

Review of attachment 8608873 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/SharedMemoryBasic_posix.cpp
@@ +60,2 @@
>    // Carve a new instance off of /dev/ashmem
> +  int shmfd = mkstemp(const_cast<char*>(path.value().c_str()));

When is this file deleted?

@@ -59,2 @@
>    // Carve a new instance off of /dev/ashmem
> -  int shmfd = open("/" ASHMEM_NAME_DEF, O_RDWR, 0600);

This is going to delay bug 930258 if it lands.  Opening /dev/ashmem is easy to intercept and remote at the system call level; handling temporary file creation is harder.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8608873 [details] [diff] [review]
SharedMemoryBasic_posix.patch

Review of attachment 8608873 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not super-excited about this change. The Chromium code is well-tested, so it would be nice to continue using it. Could we just improve it instead? It looks like the first fstat in the Chromium code can be skipped if name == "". And the second one can be removed if we get rid of the id() method on base::SharedMemory, which we don't appear to need. Can you make that change instead?

::: ipc/glue/SharedMemoryBasic_posix.cpp
@@ +60,2 @@
>    // Carve a new instance off of /dev/ashmem
> +  int shmfd = mkstemp(const_cast<char*>(path.value().c_str()));

Yeah, looks like this needs to be fixed.

@@ -59,2 @@
>    // Carve a new instance off of /dev/ashmem
> -  int shmfd = open("/" ASHMEM_NAME_DEF, O_RDWR, 0600);

Note that we don't use this code currently. We use shared_memory_posix.cc from the Chromium code. It basically does the same thing as Brad's new version except it has two extra fstat calls (and it correctly deletes the temp file).

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jed pointed out that ANDROID is defined for b2g, so his other concern about sandboxing is valid.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, it seems like this will probably break Android. I assume it needs the ashmem thing.

Comment 5

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

The fstat()s are gone as of bug 1440199, as foretold in comment #2.  The current plan for base::SharedMemory is to use shm_open on generic POSIX and ashmem on Android (bug 1447867), but on Linux ≥3.17 we can use memfd_create (bug 1440203) which is a single syscall that doesn't touch the filesystem and is sandbox-safe, so that's probably about as performant as it's going to get.