Implement SharedMemory::NewAnonymousReadOnly(contents).

base/memory/shared_memory_posix.cc

Index: base/memory/shared_memory_posix.cc
diff --git a/base/memory/shared_memory_posix.cc b/base/memory/shared_memory_posix.cc
index efb0caf5bc9a29d805700c3abdad18ad6671a199..cb6ae1e4de9a4f6a908386c3c8144c098c391ea5 100644
--- a/base/memory/shared_memory_posix.cc
+++ b/base/memory/shared_memory_posix.cc
@@ -101,6 +101,51 @@ size_t SharedMemory::GetHandleLimit() {
   return base::GetMaxFds();
 }
 
+// static
+scoped_ptr<SharedMemory> SharedMemory::NewAnonymousReadOnly(
+    StringPiece contents) {
+  // This function theoretically can block on the disk, but realistically
+  // the temporary files we create will just go into the buffer cache
+  // and be deleted before they ever make it out to disk.
+  base::ThreadRestrictions::ScopedAllowIO allow_io;

[Jeffrey Yasskin, 2013/10/15 23:03:52]

This causes a presubmit error:

Banned functions were used.
    base/memory/shared_memory_posix.cc:110:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.


Let me know if there's something else I should do instead.

[jln (very slow on Chromium), 2013/10/16 00:16:39]

I would hope that the presubmit warning doesn't concern base/

+
+  FilePath path;
+  file_util::ScopedFILE writable_fp(
+      file_util::CreateAndOpenTemporaryShmemFile(&path, /*executable=*/false));
+  if (!writable_fp) {
+    LOG(ERROR) << "Failed to create temp shmem file.";
+    return scoped_ptr<SharedMemory>();

[jln (very slow on Chromium), 2013/10/16 00:16:39]

Should we CHECK here instead? (Or LOG(FATAL)). It looks error-prone.

[Jeffrey Yasskin, 2013/10/16 01:16:39]

Create() only LOG(FATAL)s when /dev/shm is unwritable, so I'm reluctant to make
this API crash in more cases. If callers use the result without checking,
they'll get a NULL dereference anyway, which is better than Create().

If we want to ratchet up the crashes here (which is plausible), I think we
should do it in a sequence of subsequent CLs, to reduce the risk of this one.

If you really think this needs to be done in this CL for one or more of these
NULL returns, let me know.

+  }
+
+  int readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+  if (readonly_fd < 0) {
+    LOG(ERROR) << "Failed to reopen temp shmem file readonly.";

[jln (very slow on Chromium), 2013/10/16 00:16:39]

Use PLOG instead.

[Jeffrey Yasskin, 2013/10/16 01:16:39]

Oh yeah, done.

+    return scoped_ptr<SharedMemory>();

[jln (very slow on Chromium), 2013/10/16 00:16:39]

Same remark.

+  }
+  file_util::ScopedFD readonly_fd_closer(&readonly_fd);
+
+  if (unlink(path.value().c_str()))
+    PLOG(WARNING) << "unlink";

[jln (very slow on Chromium), 2013/10/16 00:16:39]

I would personally add a DCHECK. Your choice.

[Jeffrey Yasskin, 2013/10/16 01:16:39]

I'm imitating Create(), so I'd leave this for a subsequent CL also.

+
+  if (fwrite(contents.data(), 1, contents.size(), writable_fp.get()) !=
+      contents.size()) {
+    LOG(ERROR) << "Failed to write " << contents.size()
+               << " bytes to memory block.";
+    return scoped_ptr<SharedMemory>();

[jln (very slow on Chromium), 2013/10/16 00:16:39]

Same remark about error handling.

+  }
+
+  writable_fp.reset();

[Jeffrey Yasskin, 2013/10/16 01:16:39]

I've also changed this to check the fclose() result and return NULL if it fails.

+
+  // Protect against people accidentally reopening as read/write.  It's still
+  // possible to intentionally fchmod() the FD back and then open it from /dev,
+  // but the renderer can't open from a filesystem path.
+  fchmod(readonly_fd, S_IRUSR);

[jln (very slow on Chromium), 2013/10/16 00:16:39]

Please, at least DCHECK this. It's too easy to silently have no-ops.

[Jeffrey Yasskin, 2013/10/16 01:16:39]

Done.

+
+  return make_scoped_ptr(new SharedMemory(
+      FileDescriptor(*readonly_fd_closer.release(), /*iauto_close=*/false),
+      /*read_only=*/true));
+}
+
 bool SharedMemory::CreateAndMapAnonymous(size_t size) {
   return CreateAnonymous(size) && Map(size);
 }