content: Pass IOSurface references using Mach IPC.

content: Pass IOSurface references using Mach IPC.

This removes the use of global IOSurfaces and instead passes
ownership between processes using Mach IPC.

The IOSurface GpuMemoryBuffer factory instance in the GPU
process sends a synchronous Mach message to the browser
process to register each IOSurface it creates. IOSurface
registration messages are handled by the
BrowserIOSurfaceManager class and child processes can use
a Mach message to acquire a reference to an IOSurface that
has been registered with the manager.

The BrowserIOSurfaceManager class keeps track of the
ownership of each IOSurface and prevents a child process
from acquiring a reference to an IOSurface that it doesn't
own. A unique unguessable token is generated for each child
process that is allowed to use IOSurfaces. The token
restricts what IOSurfaces a child process has access to
and prevents a malicious process from gaining access to
IOSurfaces it doesn't own.

Security Considerations
-----------------------

In general, this is a major improvement to security as it
provides proper sand-boxing of IOSurfaces. Prior to this
change, IOSurfaces were global and any process on the
system (including all renderer processes) had access to
all IOSurfaces. The renderer who owns the IOSurface is
the only process (except for the browser and GPU) that
has access to the IOSurface as a result of this change.

Passing of IOSurface references to child processes require
a Mach port to be open in the child process sandbox for
sending messages to the browser. As a result, Mach message
handling in the browser process
(BrowserIOSurfaceManager::Handle*Request) requires
validation and proper error handling to prevent a
malicious renderer from exploiting this channel.

BUG=323304
TEST=content_unittests --gtest_filter=GpuMemoryBuffer*/1, content_unittests --gtest_filter=BrowserIOSurfaceManagerTest.*, content_shell --enable-native-gpu-memory-buffers

Committed: https://crrev.com/7c45b308edffa29d63d752fef7f7bf759a465a64
Cr-Commit-Position: refs/heads/master@{#332757}

[reveman, 2015-05-07 20:36:43]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org

[Daniele Castagna, 2015-05-08 18:58:30]

As far as I understand, it lgtm.
Someone with better knowledge of the Mach and IOSurfaces will definitely have
take a better look at it.

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
File content/browser/mach_broker_mac.h (right):

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
content/browser/mach_broker_mac.h:58: // process.
What happens if it doesn't succeed? Does it return MACH_PORT_NULL?

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
content/browser/mach_broker_mac.h:116: bool
RegisterIOSurface(base::ProcessHandle pid,
Are there annotations used in chromium for suggesting that a methods require a
locks?

Something like:
 bool RegisterIOSurface(...) EXCLUSIVE_LOCKS_REQUIRED(lock_);

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
content/browser/mach_broker_mac.mm:33: const mach_msg_id_t kMachBroker_ChildId =
1;
What about moving the ids inside the structs they identify (e.g:
MachBroker_ChildMsg::Id)

https://codereview.chromium.org/1137453002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1137453002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:145:
IOSurfaceMap::iterator it = io_surfaces_.find(key);
io_surfaces_.erase(IOSurfaceMapKey(id, client_id));

[reveman, 2015-05-09 15:54:53]

reveman@chromium.org changed reviewers:
+ avi@chromium.org

[reveman, 2015-05-09 15:54:56]

+avi

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
File content/browser/mach_broker_mac.h (right):

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
content/browser/mach_broker_mac.h:58: // process.
On 2015/05/08 at 18:58:30, Daniele Castagna wrote:
> What happens if it doesn't succeed? Does it return MACH_PORT_NULL?

Yes, added this to the comment.

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
content/browser/mach_broker_mac.h:116: bool
RegisterIOSurface(base::ProcessHandle pid,
On 2015/05/08 at 18:58:30, Daniele Castagna wrote:
> Are there annotations used in chromium for suggesting that a methods require a
locks?
> 
> Something like:
>  bool RegisterIOSurface(...) EXCLUSIVE_LOCKS_REQUIRED(lock_);

No, that that I know. That'd be nice but I'm afraid the comment will have to do.

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/1137453002/diff/20001/content/browser/mach_br...
content/browser/mach_broker_mac.mm:33: const mach_msg_id_t kMachBroker_ChildId =
1;
On 2015/05/08 at 18:58:30, Daniele Castagna wrote:
> What about moving the ids inside the structs they identify (e.g:
MachBroker_ChildMsg::Id)

Added MACH_MESSAGE_ID macro to latest patch that uses the line number as a
unique ID for each message. This is consistent with how our regular IPC system
generates IDs.

https://codereview.chromium.org/1137453002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1137453002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:145:
IOSurfaceMap::iterator it = io_surfaces_.find(key);
On 2015/05/08 at 18:58:30, Daniele Castagna wrote:
> io_surfaces_.erase(IOSurfaceMapKey(id, client_id));

https://codereview.chromium.org/1131463004

[Avi (use Gerrit), 2015-05-09 18:00:28]

I have some objections to the placement of code, but I am not qualified to
review the Mach code. Ask Mark or Robert.

https://codereview.chromium.org/1137453002/diff/40001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1137453002/diff/40001/content/app/content_mai...
content/app/content_main_runner.cc:132: class IOSurfaceManagerImpl : public
IOSurfaceManager {
If the MachBroker is basically backing this, put this implementation in
MachBroker and have MachBroker vend this. Having this class implementation in
content_main_runner.cc makes little sense to me.

https://codereview.chromium.org/1137453002/diff/40001/content/browser/mach_br...
File content/browser/mach_broker_mac.h (right):

https://codereview.chromium.org/1137453002/diff/40001/content/browser/mach_br...
content/browser/mach_broker_mac.h:60: 
You might as well keep these private and have MachBroker vend the
IOSurfaceManagerImpl.

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
content/test/content_test_suite.cc:101: class IOSurfaceManagerImpl : public
IOSurfaceManager {
Is this a simplified test implementation? Can you stick the name "test" in there
somewhere? I don't like having two different IOSurfaceManagerImpl classes.

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
content/test/content_test_suite.cc:170: IOSurfaceManager::InitInstance(new
IOSurfaceManagerImpl);
We're also calling InitInstance at the end of ContentMainRunnerImpl::Initialize.
What is the relationship here? This gets called after?

[reveman, 2015-05-09 19:24:24]

https://codereview.chromium.org/1137453002/diff/40001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1137453002/diff/40001/content/app/content_mai...
content/app/content_main_runner.cc:132: class IOSurfaceManagerImpl : public
IOSurfaceManager {
On 2015/05/09 at 18:00:27, Avi wrote:
> If the MachBroker is basically backing this, put this implementation in
MachBroker and have MachBroker vend this. Having this class implementation in
content_main_runner.cc makes little sense to me.

Ok, I can move this code to mach_broker_mac.cc. Just to be clear, there's no
MachBroker instance in child processes so this would still be a separate class.

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
content/test/content_test_suite.cc:101: class IOSurfaceManagerImpl : public
IOSurfaceManager {
On 2015/05/09 at 18:00:28, Avi wrote:
> Is this a simplified test implementation? Can you stick the name "test" in
there somewhere? I don't like having two different IOSurfaceManagerImpl classes.

Sure, let me just land a patch that changes SurfaceTextureManagerImpl above
first so things are consistent.

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
content/test/content_test_suite.cc:170: IOSurfaceManager::InitInstance(new
IOSurfaceManagerImpl);
On 2015/05/09 at 18:00:28, Avi wrote:
> We're also calling InitInstance at the end of
ContentMainRunnerImpl::Initialize. What is the relationship here? This gets
called after?

Pretty sure ContentMainRunnerImpl::Initialize is not called in the case where
this is used as that would cause the DCHECK in InitInstance to fail.

[Avi (use Gerrit), 2015-05-09 20:41:02]

https://codereview.chromium.org/1137453002/diff/40001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1137453002/diff/40001/content/app/content_mai...
content/app/content_main_runner.cc:132: class IOSurfaceManagerImpl : public
IOSurfaceManager {
On 2015/05/09 19:24:23, reveman wrote:
> On 2015/05/09 at 18:00:27, Avi wrote:
> > If the MachBroker is basically backing this, put this implementation in
> MachBroker and have MachBroker vend this. Having this class implementation in
> content_main_runner.cc makes little sense to me.
> 
> Ok, I can move this code to mach_broker_mac.cc. Just to be clear, there's no
> MachBroker instance in child processes so this would still be a separate
class.

Then maybe its own file? It just feels weird to be dumped here.

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
content/test/content_test_suite.cc:170: IOSurfaceManager::InitInstance(new
IOSurfaceManagerImpl);
On 2015/05/09 19:24:23, reveman wrote:
> On 2015/05/09 at 18:00:28, Avi wrote:
> > We're also calling InitInstance at the end of
> ContentMainRunnerImpl::Initialize. What is the relationship here? This gets
> called after?
> 
> Pretty sure ContentMainRunnerImpl::Initialize is not called in the case where
> this is used as that would cause the DCHECK in InitInstance to fail.

That makes sense.

[reveman, 2015-05-10 18:45:08]

PTAL

https://codereview.chromium.org/1137453002/diff/40001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1137453002/diff/40001/content/app/content_mai...
content/app/content_main_runner.cc:132: class IOSurfaceManagerImpl : public
IOSurfaceManager {
On 2015/05/09 20:41:01, Avi wrote:
> On 2015/05/09 19:24:23, reveman wrote:
> > On 2015/05/09 at 18:00:27, Avi wrote:
> > > If the MachBroker is basically backing this, put this implementation in
> > MachBroker and have MachBroker vend this. Having this class implementation
in
> > content_main_runner.cc makes little sense to me.
> > 
> > Ok, I can move this code to mach_broker_mac.cc. Just to be clear, there's no
> > MachBroker instance in child processes so this would still be a separate
> class.
> 
> Then maybe its own file? It just feels weird to be dumped here.

Moved it to mach_broker_mac.cc in latest patch. I think it cleaned things up
nicely and it's also somewhat consistent with how this is done for
SurfaceTextures on Android where the child process SurfaceTextureManager
implementation lives in app/android/child_process_service.cc.

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1137453002/diff/40001/content/test/content_te...
content/test/content_test_suite.cc:101: class IOSurfaceManagerImpl : public
IOSurfaceManager {
On 2015/05/09 at 19:24:23, reveman wrote:
> On 2015/05/09 at 18:00:28, Avi wrote:
> > Is this a simplified test implementation? Can you stick the name "test" in
there somewhere? I don't like having two different IOSurfaceManagerImpl classes.
> 
> Sure, let me just land a patch that changes SurfaceTextureManagerImpl above
first so things are consistent.

Done.

[Avi (use Gerrit), 2015-05-10 20:00:04]

I'm cool with the organization now, but find a Mach reviewer. I'll stamp when
they're happy.

[reveman, 2015-05-11 15:03:17]

reveman@chromium.org changed reviewers:
+ rsesek@chromium.org

[reveman, 2015-05-11 15:03:18]

+rsesek for everything Mach related

fyi, design doc:
https://docs.google.com/a/chromium.org/document/d/1L8wZtEDtQ2S6dLfNcZHopMgZVz...

[Robert Sesek, 2015-05-12 23:15:25]

Still looking, but I have a high-level question:

Is there a strong need to co-mingle the existing MachBroker logic with the new
IOSurface logic? MachBroker is currently only handling the PID -> task_port
translation, and it'd be preferable to not add unrelated functionality to this
class (if the two are actually unrelated).

My (jet lagged) reading of the code is that the MachBroker is used only because
it's already vending a Mach server accessible to all the child processes. If
that's the primary motivator, then I'd consider vending another Mach service,
rather than piggybacking on top of the MachBroker logic. We can avoid creating
another dedicated thread for any new Mach server by scheduling this it on a
dispatch queue. I've been meaning to migrate the MachBroker to use a dispatch
queue instead of a dedicated thread, anyways.

https://codereview.chromium.org/1137453002/diff/140001/content/common/mac/io_...
File content/common/mac/io_surface_manager.h (right):

https://codereview.chromium.org/1137453002/diff/140001/content/common/mac/io_...
content/common/mac/io_surface_manager.h:14: class CONTENT_EXPORT
IOSurfaceManager {
This needs a class-level comment.

https://codereview.chromium.org/1137453002/diff/140001/content/common/mac/io_...
content/common/mac/io_surface_manager.h:17: static void
InitInstance(IOSurfaceManager* instance);
SetInstance is probably a more appropriate name?

https://codereview.chromium.org/1137453002/diff/140001/content/common/mac/io_...
content/common/mac/io_surface_manager.h:19: // Register an IO surface for use in
another process.
How does an io_surface_id differ from a client_id?

[reveman, 2015-05-15 14:48:58]

https://codereview.chromium.org/1137453002/diff/140001/content/common/mac/io_...
File content/common/mac/io_surface_manager.h (right):

https://codereview.chromium.org/1137453002/diff/140001/content/common/mac/io_...
content/common/mac/io_surface_manager.h:14: class CONTENT_EXPORT
IOSurfaceManager {
On 2015/05/12 at 23:15:25, Robert Sesek wrote:
> This needs a class-level comment.

Done.

https://codereview.chromium.org/1137453002/diff/140001/content/common/mac/io_...
content/common/mac/io_surface_manager.h:17: static void
InitInstance(IOSurfaceManager* instance);
On 2015/05/12 at 23:15:25, Robert Sesek wrote:
> SetInstance is probably a more appropriate name?

Done. Was InitInstance to be consistent with SurfaceTextureManager on Android
but I'll fix in a different patch as SetInstance makes more sense.

https://codereview.chromium.org/1137453002/diff/140001/content/common/mac/io_...
content/common/mac/io_surface_manager.h:19: // Register an IO surface for use in
another process.
On 2015/05/12 at 23:15:25, Robert Sesek wrote:
> How does an io_surface_id differ from a client_id?

client_id specifies what GPU client is allowed access the IOSurface. The mach
message handling in the browser process will enforce this. io_surface_id is a
unique ID that identifies each IOSurface and allows GPU clients to use multiple
IOSurfaces.

[Robert Sesek, 2015-05-15 21:45:05]

Nice, definitely a lot cleaner than before.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:7: #include <bsm/libbsm.h>
Unused.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:37:
DCHECK(io_surfaces_.find(key) == io_surfaces_.end());
This DCHECK will continue in release mode (same bug you spotted in MachBroker).
Should this be a return false, too?

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:48:
DCHECK(io_surfaces_.find(key) != io_surfaces_.end());
Same.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:91: :
gpu_process_mailbox_(gpu::Mailbox::Generate()) {
DCHECK(gpu_process_mailbox_.Verify()) ?

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:124:
IOSurfaceManagerHostMsg_RegisterIOSurface msg;
This entire struct is the message. The struct declarations should have the
header, body, and mailbox fields in them.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:126: } register_io_surface;
These message structs should be in a common file, so that the client and server
can use the same declaration. It's easier to maintain that way.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:224: }
Registration gets a reply but unregistration does not? Why?

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:40: // acquire IOSurface
references.
|client_id| is the ___.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:46: // Get a unique unguessable
mailbox name that each GPU process can
a -> the, otherwise it's hard to understand why this shouldn't be named
"Generate". Also, isn't there only one GPU process, why "each"?

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:81: typedef std::pair<int, int>
IOSurfaceMapKey;
using IOSurfaceMapKey = std::pair<int, int>;

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:82: typedef
base::ScopedPtrHashMap<IOSurfaceMapKey,
using IOSurfaceMap = ...;

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:7: #include <bsm/libbsm.h>
Unused.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:30: base::mac::ScopedMachSendRight
scoped_io_surface_right(
Do this after allocating the reply, since that can fail and this cannot.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:33: mach_port_t receive_port;
naming: reply_port

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:42: union {
I'd avoid a union here.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:156: "%s.iosurfacemgr.%d",
base::mac::BaseBundleID(), getppid());
This could be centralized to IOSurfaceManager as a static, just like MachBroker
does. That way the same string literal does not need to be kept in two places.
It could take a bool is_child argument.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:162: return;
If this fails, then an instance of this class will continue to try and send
messages to a NULL port.

https://codereview.chromium.org/1137453002/diff/210001/content/common/mac/io_...
File content/common/mac/io_surface_manager_messages.h (right):

https://codereview.chromium.org/1137453002/diff/210001/content/common/mac/io_...
content/common/mac/io_surface_manager_messages.h:19: struct
IOSurfaceManagerHostMsg_RegisterIOSurface {
Each of these messages should have the

mach_msg_header_t header
mach_msg_body_t body
gpu::Mailbox mailbox
<data>

Rather than declaring those where you're calling mach_msg.

https://codereview.chromium.org/1137453002/diff/210001/content/test/content_t...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1137453002/diff/210001/content/test/content_t...
content/test/content_test_suite.cc:120: typedef std::map<int,
linked_ptr<base::mac::ScopedMachSendRight>>
Why is this a linked_ptr?

[reveman, 2015-05-18 18:15:39]

PTAL

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:7: #include <bsm/libbsm.h>
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> Unused.

Done.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:37:
DCHECK(io_surfaces_.find(key) == io_surfaces_.end());
On 2015/05/15 at 21:45:04, Robert Sesek wrote:
> This DCHECK will continue in release mode (same bug you spotted in
MachBroker). Should this be a return false, too?

This is for in-process use only. e.g. the browser compositor. it's a browser bug
if this ever fails. A renderer process can never trigger this.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:48:
DCHECK(io_surfaces_.find(key) != io_surfaces_.end());
On 2015/05/15 at 21:45:04, Robert Sesek wrote:
> Same.

Same reply.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:91: :
gpu_process_mailbox_(gpu::Mailbox::Generate()) {
On 2015/05/15 at 21:45:04, Robert Sesek wrote:
> DCHECK(gpu_process_mailbox_.Verify()) ?

Done. Removed the DCHECK in GetGpuProcessMailbox() and moved it here instead as
it makes sense to check that we never have this service running with an invalid
mailbox.

FYI, also made gpu_process_mailbox_ const.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:124:
IOSurfaceManagerHostMsg_RegisterIOSurface msg;
On 2015/05/15 at 21:45:04, Robert Sesek wrote:
> This entire struct is the message. The struct declarations should have the
header, body, and mailbox fields in them.

Done by using mailbox_name[64] in the message struct instead of gpu::Mailbox.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:126: } register_io_surface;
On 2015/05/15 at 21:45:04, Robert Sesek wrote:
> These message structs should be in a common file, so that the client and
server can use the same declaration. It's easier to maintain that way.

Done.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:126: } register_io_surface;
On 2015/05/15 at 21:45:04, Robert Sesek wrote:
> These message structs should be in a common file, so that the client and
server can use the same declaration. It's easier to maintain that way.

Done.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:224: }
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> Registration gets a reply but unregistration does not? Why?

Registration needs to be synchronous to prevent the child process from trying to
acquire the IOSurface before it reached the browser.

Unregistration happens as a result of the child process who owns the iosurface
terminating or it notifying the browser that the iosurface is no longer used
(the renderer side of it has been deleted) this notification is asynchronous in
nature and there's no guarantee for how quickly it is actually removed from the
io surface manager on the browser side.

So a reply doesn't serve any purpose here and I prefer to avoid another
round-trip for performance reasons but I'm fine adding it for consistency if you
think that makes sense?

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:40: // acquire IOSurface
references.
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> |client_id| is the ___.

Done.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:46: // Get a unique unguessable
mailbox name that each GPU process can
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> a -> the, otherwise it's hard to understand why this shouldn't be named
"Generate". Also, isn't there only one GPU process, why "each"?

Done. Also changed the language to say "the" instead as that's less confusing.
There can be multiple GPU processes in the sense that if one crashes, we'll
spawn a new process. Maybe some other user of content/ is using multiple GPU
processes at the same time but chrome doesn't. I didn't want to make a 1 gpu
process/service assumption here but it also seemed overkill to try prevent one
gpu process from being able to unregister another GPU process's iosurfaces. 

Current code supports multiple GPU processes if necessary but doesn't keep them
in different IOSurface sandboxes.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:81: typedef std::pair<int, int>
IOSurfaceMapKey;
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> using IOSurfaceMapKey = std::pair<int, int>;

Done.

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:82: typedef
base::ScopedPtrHashMap<IOSurfaceMapKey,
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> using IOSurfaceMap = ...;

Done.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:7: #include <bsm/libbsm.h>
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> Unused.

Done.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:30: base::mac::ScopedMachSendRight
scoped_io_surface_right(
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> Do this after allocating the reply, since that can fail and this cannot.

Good call. Done.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:33: mach_port_t receive_port;
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> naming: reply_port

Done.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:42: union {
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> I'd avoid a union here.

hm, how? by using mach_msg_overwrite?

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:156: "%s.iosurfacemgr.%d",
base::mac::BaseBundleID(), getppid());
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> This could be centralized to IOSurfaceManager as a static, just like
MachBroker does. That way the same string literal does not need to be kept in
two places. It could take a bool is_child argument.

Done. See LookupServicePort in latest patch.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:162: return;
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> If this fails, then an instance of this class will continue to try and send
messages to a NULL port.

I changed how this is initialized in latest patch. Ptal.

https://codereview.chromium.org/1137453002/diff/210001/content/common/mac/io_...
File content/common/mac/io_surface_manager_messages.h (right):

https://codereview.chromium.org/1137453002/diff/210001/content/common/mac/io_...
content/common/mac/io_surface_manager_messages.h:19: struct
IOSurfaceManagerHostMsg_RegisterIOSurface {
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> Each of these messages should have the
> 
> mach_msg_header_t header
> mach_msg_body_t body
> gpu::Mailbox mailbox
> <data>
> 
> Rather than declaring those where you're calling mach_msg.

Done but gpu::Mailbox here gets ugly as it's not POD (need explicit ctor) so
changed it to mailbox_name[64].

https://codereview.chromium.org/1137453002/diff/210001/content/test/content_t...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1137453002/diff/210001/content/test/content_t...
content/test/content_test_suite.cc:120: typedef std::map<int,
linked_ptr<base::mac::ScopedMachSendRight>>
On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> Why is this a linked_ptr?

To be consistent with TestSurfaceTextureManager above. I assume you prefer
scoped_ptr so changed it to ScopedPtrHashMap. I'll deal with
TestSurfaceTextureManager in a separate patch.

[Robert Sesek, 2015-05-19 23:26:24]

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/210001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:224: }
On 2015/05/18 18:15:38, reveman wrote:
> On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> > Registration gets a reply but unregistration does not? Why?
> 
> Registration needs to be synchronous to prevent the child process from trying
to
> acquire the IOSurface before it reached the browser.
> 
> Unregistration happens as a result of the child process who owns the iosurface
> terminating or it notifying the browser that the iosurface is no longer used
> (the renderer side of it has been deleted) this notification is asynchronous
in
> nature and there's no guarantee for how quickly it is actually removed from
the
> io surface manager on the browser side.
> 
> So a reply doesn't serve any purpose here and I prefer to avoid another
> round-trip for performance reasons but I'm fine adding it for consistency if
you
> think that makes sense?

Makes sense. It's fine to leave this as a one-way message, but I'd leave a
comment about that.

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/210001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:42: union {
On 2015/05/18 18:15:39, reveman wrote:
> On 2015/05/15 at 21:45:05, Robert Sesek wrote:
> > I'd avoid a union here.
> 
> hm, how? by using mach_msg_overwrite?

No, just declare two variables not in a union.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:152: HandleRequest();
nit: Chrome indents blocks 4 spaces

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:189:
&reply.register_io_surface))
nit: braces required for multi-line body. Same on line 196.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:194: return;
Add a comment here that this is deliberately one-way, and why.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:198: return;
Won't this leave the remote caller hanging if this fails? Should you instead
send a reply with an error code?

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:205:
DCHECK(reply.header.msgh_size);
I don't think these DCHECKs are meaningful. The kernel will reject the message
if it's invalid.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:35: static mach_port_t
LookupServicePort(pid_t pid);
This should return in a scoper to ensure proper lifetime management.

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:40: } data = {{{0}}};
Here, I'd declare two separate variables for the request and reply.

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:127: return
IOSurfaceLookupFromMachPort(data.reply.msg.io_surface_port.name);
Does this function consume the port right?

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
File content/child/child_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
content/child/child_io_surface_manager_mac.h:30: // Set the service Mach port.
Who's managing the ownership of this port?

https://codereview.chromium.org/1137453002/diff/310001/content/common/mac/io_...
File content/common/mac/io_surface_manager_messages.h (right):

https://codereview.chromium.org/1137453002/diff/310001/content/common/mac/io_...
content/common/mac/io_surface_manager_messages.h:24: int8_t mailbox_name[64];
Perhaps gpu::Mailbox could expose a public typedef for this?

struct gpu::Mailbox {
  using Name = uint8_t[GL_MAILBOX_SIZE_CHROMIUM]
}

So that this could then just use:

gpu::Mailbox::Name mailbox_name;

I am not a gpu/ OWNER though, so should probably ask if that's okay. If not,
then this should at least use mailbox.h's GL_MAILBOX_SIZE_CHROMIUM constant.

[reveman, 2015-05-27 05:02:05]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[reveman, 2015-05-27 05:02:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org
Link to the patchset: https://codereview.chromium.org/1137453002/#ps330001
(title: "rsesek's review")

[commit-bot: I haz the power, 2015-05-27 05:02:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137453002/330001

[reveman, 2015-05-27 05:03:16]

reveman@chromium.org changed reviewers:
+ piman@chromium.org

[reveman, 2015-05-27 05:03:18]

+piman for gpu/

PTAL

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:152: HandleRequest();
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> nit: Chrome indents blocks 4 spaces

This is how cl format likes it. I'd rather not mess with that.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:189:
&reply.register_io_surface))
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> nit: braces required for multi-line body. Same on line 196.

Done.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:194: return;
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> Add a comment here that this is deliberately one-way, and why.

Done.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:198: return;
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> Won't this leave the remote caller hanging if this fails? Should you instead
send a reply with an error code?

Failure is a result of incorrect usage or a request from a process that is not
allowed to send Mach messages to the browser. I was thinking that it was best to
drop these requests on the floor instead of trying to send a reply in these
cases. Let me know if you think that's unwise.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:205:
DCHECK(reply.header.msgh_size);
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> I don't think these DCHECKs are meaningful. The kernel will reject the message
if it's invalid.

In case it wasn't clear, these are to protect against a bad Handle*Request()
implementation that doesn't set these properly. The idea was that we'd rather
crash here in a debug build than have the kernel silently ignore the message or
just a log message. I removed these checks from latest patch though as maybe
it's a bit excessive to have them here when the code we're checking for
correctness is in the same file.

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:40: } data = {{{0}}};
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> Here, I'd declare two separate variables for the request and reply.

Did not change this as discussed.

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:127: return
IOSurfaceLookupFromMachPort(data.reply.msg.io_surface_port.name);
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> Does this function consume the port right?

Good call. It does not based on the documentation:
https://developer.apple.com/library/mac/documentation/Miscellaneous/Reference...

Latest patch frees the port after creating an IOSurface reference.

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
File content/child/child_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1137453002/diff/310001/content/child/child_io...
content/child/child_io_surface_manager_mac.h:30: // Set the service Mach port.
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> Who's managing the ownership of this port?

Used to be the caller but I changed it in latest patch and added a comment.

https://codereview.chromium.org/1137453002/diff/310001/content/common/mac/io_...
File content/common/mac/io_surface_manager_messages.h (right):

https://codereview.chromium.org/1137453002/diff/310001/content/common/mac/io_...
content/common/mac/io_surface_manager_messages.h:24: int8_t mailbox_name[64];
On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> Perhaps gpu::Mailbox could expose a public typedef for this?
> 
> struct gpu::Mailbox {
>   using Name = uint8_t[GL_MAILBOX_SIZE_CHROMIUM]
> }
> 
> So that this could then just use:
> 
> gpu::Mailbox::Name mailbox_name;

Good idea. Done.

I also added an IOSurfaceManagerToken typedef to make it a bit less confusing.
We might want to add a base UnguessableId type to base/ as a follow up instead
of relying on gpu/ for this. The typedef seems OK for now as cc::SharedBitmap
does the same thing for cc:SharedBitmapId.

https://codereview.chromium.org/1137453002/diff/330001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1137453002/diff/330001/content/app/content_ma...
content/app/content_main_runner.cc:677: service_port.release());
Note: this could be a lot cleaner if move semantics were allowed with
ScopedMachSendRight. What's the reason ScopedMachSendRight inherits from
ScopedGeneric rather than just being a typedef?

[reveman, 2015-05-27 05:03:19]

reveman@chromium.org changed reviewers:
+ piman@chromium.org

[reveman, 2015-05-27 05:03:20]

+piman for gpu/

PTAL

[commit-bot: I haz the power, 2015-05-27 06:11:59]

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

[commit-bot: I haz the power, 2015-05-27 06:12:00]

Dry run: This issue passed the CQ dry run.

[piman, 2015-05-27 19:37:17]

LGTM for gpu. I know nothing about mach IPC so I'll let others evaluate the
rest, in particular the potential interaction/ordering wrt chrome IPCs.

[Robert Sesek, 2015-05-28 18:25:12]

Looking really good now. Just some nits now.

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/310001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:198: return;
On 2015/05/27 05:03:18, reveman wrote:
> On 2015/05/19 at 23:26:23, Robert Sesek wrote:
> > Won't this leave the remote caller hanging if this fails? Should you instead
> send a reply with an error code?
> 
> Failure is a result of incorrect usage or a request from a process that is not
> allowed to send Mach messages to the browser. I was thinking that it was best
to
> drop these requests on the floor instead of trying to send a reply in these
> cases. Let me know if you think that's unwise.

I think it's okay then. If there were non-misuse ways to message the server
(e.g. if we were actually interacting with IOSurfaces. Though the one potential
failure case would be a GPU process sending a Register message for an
already-existing <client id, surface id> pair.

https://codereview.chromium.org/1137453002/diff/330001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1137453002/diff/330001/content/app/content_ma...
content/app/content_main_runner.cc:677: service_port.release());
On 2015/05/27 05:03:18, reveman wrote:
> Note: this could be a lot cleaner if move semantics were allowed with
> ScopedMachSendRight. What's the reason ScopedMachSendRight inherits from
> ScopedGeneric rather than just being a typedef?

A lot of places use the implicit |operator mach_port_t()| conversion. We could
add move support to the Mach classes.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:197: // Unregister requests
are asynchronous and does not require a reply as
does -> do (S-V agreement)

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:198: // there is guarantee for
how quickly an IO surface is removed from
"is guarantee" — missing an article?

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:235: IOSurfaceMapKey
key(request.io_surface_id, request.client_id);
This is RegisterIOSurface()

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:259: IOSurfaceMapKey
key(request.io_surface_id, request.client_id);
This is UnregisterIOSurface()

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:91: // Stores the IOSurfces for
all GPU clients.
Describe what the key is, so it's clear that it's <io_surface_id, client_id>.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:106: // Mutex that guards
|io_surfaces_| and |child_process_ids_|.
Doesn't this also guard initialized_?

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac_unittest.cc (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac_unittest.cc:82: }
Since there's not much shared setup between these scope blocks, it's better to
create individual TEST_F()s for different cases. It makes it more clear what
specific behavior is broken when a test fails.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:509:
IOSurfaceManagerToken io_surface_manager_token_;
Comment, since the rest of the members have one.

https://codereview.chromium.org/1137453002/diff/330001/content/common/mac/io_...
File content/common/mac/io_surface_manager_token.h (right):

https://codereview.chromium.org/1137453002/diff/330001/content/common/mac/io_...
content/common/mac/io_surface_manager_token.h:6: #define
CONTENT_COMMON_MAC_IO_SURFACE_MANAGER_TOKEN_H_
I'm not sure this needs its own file. Can it go in io_surface_manager.h or
io_surface_manager_messages.h ?

[reveman, 2015-05-29 00:09:02]

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:197: // Unregister requests
are asynchronous and does not require a reply as
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> does -> do (S-V agreement)

Done.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:198: // there is guarantee for
how quickly an IO surface is removed from
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> "is guarantee" — missing an article?

Done.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:235: IOSurfaceMapKey
key(request.io_surface_id, request.client_id);
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> This is RegisterIOSurface()

The difference is no DCHECK and the input is a mach_port_t instead of an
IOSurfaceRef.

I could add:

void RegisterIOSurfaceMachPort(
    int io_surface_id,
    int client_id,
    scoped_ptr<new base::mac::ScopedMachSendRight> io_surface_mach_port)

and reuse that instead of doing io_surfaces_.add in two places but I prefer the
current patch.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:259: IOSurfaceMapKey
key(request.io_surface_id, request.client_id);
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> This is UnregisterIOSurface()

Same here. No DCHECK in this case.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:91: // Stores the IOSurfces for
all GPU clients.
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> Describe what the key is, so it's clear that it's <io_surface_id, client_id>.

Done.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.h:106: // Mutex that guards
|io_surfaces_| and |child_process_ids_|.
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> Doesn't this also guard initialized_?

True. Updated the comment.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac_unittest.cc (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac_unittest.cc:82: }
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> Since there's not much shared setup between these scope blocks, it's better to
create individual TEST_F()s for different cases. It makes it more clear what
specific behavior is broken when a test fails.

Done.

https://codereview.chromium.org/1137453002/diff/330001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:509:
IOSurfaceManagerToken io_surface_manager_token_;
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> Comment, since the rest of the members have one.

Done.

https://codereview.chromium.org/1137453002/diff/330001/content/common/mac/io_...
File content/common/mac/io_surface_manager_token.h (right):

https://codereview.chromium.org/1137453002/diff/330001/content/common/mac/io_...
content/common/mac/io_surface_manager_token.h:6: #define
CONTENT_COMMON_MAC_IO_SURFACE_MANAGER_TOKEN_H_
On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> I'm not sure this needs its own file. Can it go in io_surface_manager.h or
io_surface_manager_messages.h ?

I prefer a separate file. io_surface_manager.h is meant for clients of the
IOSurfaceManager interface. The token doesn't belong in that file as it's an
implementation detail and not a requirement to implement that interface.

I could move it to io_surface_manager_messages.h but then files such as
render_process_host_impl.h and child_process_messages.h which only care about
the token would have to include that file even though the definition of Mach
messages is not something the code in those files should be aware of.

[Robert Sesek, 2015-06-01 21:58:07]

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac_unittest.cc (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac_unittest.cc:82: }
On 2015/05/29 00:09:02, reveman wrote:
> On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> > Since there's not much shared setup between these scope blocks, it's better
to
> create individual TEST_F()s for different cases. It makes it more clear what
> specific behavior is broken when a test fails.
> 
> Done.

Really? There's still a bunch of different test cases running all under the same
TEST_F. Particularly in AcquireIOSurface.

https://codereview.chromium.org/1137453002/diff/350001/content/child/child_io...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/350001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:44:
DCHECK(service_port_.is_valid());
Move this DCHECK to the top of the function.

https://codereview.chromium.org/1137453002/diff/350001/content/child/child_th...
File content/child/child_thread_impl.h (right):

https://codereview.chromium.org/1137453002/diff/350001/content/child/child_th...
content/child/child_thread_impl.h:23: #if defined(OS_MACOSX) && !defined(OS_IOS)
Don't think the IOS bit is actually necessary since iOS may not compile
//content/child.

https://codereview.chromium.org/1137453002/diff/350001/content/common/child_p...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/1137453002/diff/350001/content/common/child_p...
content/common/child_process_messages.h:20: #if defined(OS_MACOSX) &&
!defined(OS_IOS)
Same bit about IOS.

[reveman, 2015-06-02 22:48:07]

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac_unittest.cc (right):

https://codereview.chromium.org/1137453002/diff/330001/content/browser/browse...
content/browser/browser_io_surface_manager_mac_unittest.cc:82: }
On 2015/06/01 21:58:06, Robert Sesek wrote:
> On 2015/05/29 00:09:02, reveman wrote:
> > On 2015/05/28 at 18:25:11, Robert Sesek wrote:
> > > Since there's not much shared setup between these scope blocks, it's
better
> to
> > create individual TEST_F()s for different cases. It makes it more clear what
> > specific behavior is broken when a test fails.
> > 
> > Done.
> 
> Really? There's still a bunch of different test cases running all under the
same
> TEST_F. Particularly in AcquireIOSurface.

Sorry, misunderstood your previous comment. Thought you were only talking about
how IOSurfaceRegistration was testing both RegisterIOSurface and
UnregisterIOSurface. I've split this up into a number of different tests in
latest patch. PTAL.

https://codereview.chromium.org/1137453002/diff/350001/content/child/child_io...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/350001/content/child/child_io...
content/child/child_io_surface_manager_mac.cc:44:
DCHECK(service_port_.is_valid());
On 2015/06/01 at 21:58:06, Robert Sesek wrote:
> Move this DCHECK to the top of the function.

Done. Moved all DCHECKs in this file to the top of functions instead of next to
where the member variable they check is used.

https://codereview.chromium.org/1137453002/diff/350001/content/child/child_th...
File content/child/child_thread_impl.h (right):

https://codereview.chromium.org/1137453002/diff/350001/content/child/child_th...
content/child/child_thread_impl.h:23: #if defined(OS_MACOSX) && !defined(OS_IOS)
On 2015/06/01 at 21:58:07, Robert Sesek wrote:
> Don't think the IOS bit is actually necessary since iOS may not compile
//content/child.

Done.

https://codereview.chromium.org/1137453002/diff/350001/content/common/child_p...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/1137453002/diff/350001/content/common/child_p...
content/common/child_process_messages.h:20: #if defined(OS_MACOSX) &&
!defined(OS_IOS)
On 2015/06/01 at 21:58:07, Robert Sesek wrote:
> Same bit about IOS.

Done.

[Robert Sesek, 2015-06-03 20:19:37]

LGTM

[reveman, 2015-06-03 20:24:58]

Robert, thanks for taking the time to review this. Current patch is a major
improvement to the initial version of this patch.

avi, ptal and stamp if latest patch looks good.

[Avi (use Gerrit), 2015-06-03 21:31:53]

One small nit; lgtm otherwise.

https://codereview.chromium.org/1137453002/diff/370001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/370001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:228: "Mach message token size
doesn't match expectation.");
COMPILE_ASSERT is just a wrapper around static_assert. Use static_assert here
and twice below.

[reveman, 2015-06-03 22:47:47]

reveman@chromium.org changed reviewers:
+ nasko@chromium.org

[reveman, 2015-06-03 22:47:49]

+nasko for content/common/*_messages.h

see
https://docs.google.com/document/d/1L8wZtEDtQ2S6dLfNcZHopMgZVzq2z7OrUCedXitTA...
for more details

https://codereview.chromium.org/1137453002/diff/370001/content/browser/browse...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1137453002/diff/370001/content/browser/browse...
content/browser/browser_io_surface_manager_mac.cc:228: "Mach message token size
doesn't match expectation.");
On 2015/06/03 at 21:31:53, Avi wrote:
> COMPILE_ASSERT is just a wrapper around static_assert. Use static_assert here
and twice below.

Done.

[nasko, 2015-06-03 23:40:00]

Rubberstamp LGTM for IPC, based on rsesek@'s review of this CL.

[reveman, 2015-06-04 00:00:24]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-06-04 00:00:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, piman@chromium.org, rsesek@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1137453002/#ps390001
(title: "static_assert")

[commit-bot: I haz the power, 2015-06-04 00:01:59]

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

[commit-bot: I haz the power, 2015-06-04 01:27:18]

Committed patchset #21 (id:390001)

[commit-bot: I haz the power, 2015-06-04 01:28:22]

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