Add shared bitmap managers for browser and renderer processes.

Add shared bitmap managers for browser and renderer processes.

The shared bitmap managers will allow software tiles to be allocated in shared memory, so delegated rendering could be used with them.

BUG=327220
R=danakj@chromium.org, jschuh@chromium.org, piman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256955

[danakj, 2014-01-30 22:59:01]

https://codereview.chromium.org/148243013/diff/40001/cc/resources/shared_bitm...
File cc/resources/shared_bitmap.cc (right):

https://codereview.chromium.org/148243013/diff/40001/cc/resources/shared_bitm...
cc/resources/shared_bitmap.cc:27: base::RandBytes(id.name, sizeof id.name);
nit: sizeof(id.name)

https://codereview.chromium.org/148243013/diff/40001/content/browser/composit...
File content/browser/compositor/image_transport_factory.cc (right):

https://codereview.chromium.org/148243013/diff/40001/content/browser/composit...
content/browser/compositor/image_transport_factory.cc:21:
ui::Compositor::SetSharedBitmapManager(HostSharedBitmapManager::current());
move to bottom of this method

https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
File content/child/child_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
content/child/child_shared_bitmap_manager.cc:28: cc::SharedBitmapId id =
cc::SharedBitmap::GenerateId();
It seems potentially problematic to me that the child is generating these ids,
can't it just ask for a size, and be returned an id?

https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
content/child/child_shared_bitmap_manager.cc:55: return
scoped_ptr<cc::SharedBitmap>();
Future: It looks like ResourceProvider::ReceiveFromChild currently calls this
unconditionally, when it's just going to give a NULL back. Maybe we could
instead delay that call to where the RP actually tries to use the pixels field
instead, and have this function NOTREACHED()?

https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
content/child/child_shared_bitmap_manager.cc:61: base::SharedMemoryHandle
handle_to_send = mem->handle();
Could the id here come from the browser where the handle came from?

https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
content/child/child_shared_bitmap_manager.cc:82: sender_->Send(new
ChildProcessHostMsg_DeletedSharedBitmap(handle->id()));
TRACE_EVENT here?

https://codereview.chromium.org/148243013/diff/40001/content/child/child_thre...
File content/child/child_thread.h (right):

https://codereview.chromium.org/148243013/diff/40001/content/child/child_thre...
content/child/child_thread.h:237: scoped_ptr<ChildSharedBitmapManager>
shared_bitmap_manager_;
move above the WeakPtrFactory?

https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
content/common/host_shared_bitmap_manager.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
You have this file in content/common/. Would a renderer that had an OOP iframe
in it end up owning a HostSharedBitmapManager, or is it just the browser process
that owns one of these? I was expecting the latter so this file's place was
surprising.

https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
content/common/host_shared_bitmap_manager.cc:33:
base::LazyInstance<HostSharedBitmapManager> g_shared_memory_manager =
Naive question maybe, as this seems to be the way things are often done in the
browser process.. but.. Is there someone that could own this instead of making
it a global?

https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
content/common/host_shared_bitmap_manager.cc:45: return
scoped_ptr<cc::SharedBitmap>();
NOTREACHED() and a comment?

https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
content/common/host_shared_bitmap_manager.cc:112: NOTREACHED() << "Cannot create
shared memory buffer";
Not a fan of NOTREACHED/DCHECK on a path that also tries to handle the case.
Then we can't write a test for this code path. Either LOG(FATAL) and stop trying
to handle the case, or LOG(ERROR) and handle it as you are, IMO.

https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
content/common/host_shared_bitmap_manager.cc:124: NOTREACHED() << "Cannot share
shared memory buffer";
Same here.

https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
File content/common/host_shared_bitmap_manager_unittest.cc (right):

https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
content/common/host_shared_bitmap_manager_unittest.cc:27:
manager_->ChildAllocatedSharedBitmap(
This test skips AllocateSharedBitmapForChild, can you add a test for that?

https://codereview.chromium.org/148243013/diff/40001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/148243013/diff/40001/ui/compositor/compositor...
ui/compositor/compositor.cc:48: cc::SharedBitmapManager*
g_shared_bitmap_manager;
Can we pass it to ui::Compositor constructor instead?

[danakj, 2014-01-31 17:44:22]

https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
File content/child/child_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
content/child/child_shared_bitmap_manager.cc:55: return
scoped_ptr<cc::SharedBitmap>();
On 2014/01/30 22:59:01, danakj wrote:
> Future: It looks like ResourceProvider::ReceiveFromChild currently calls this
> unconditionally, when it's just going to give a NULL back. Maybe we could
> instead delay that call to where the RP actually tries to use the pixels field
> instead, and have this function NOTREACHED()?

dongseong pointed out that returning NULL here causes the ResourceProvider to
actually drop the resource in ReceiveFromChild. So if renderer is using this
(looks like it does), then it will be unable to receive and pass on software
resources without addressing this.

[jbauman, 2014-02-13 02:09:35]

PTAL.

On 2014/01/30 22:59:01, danakj wrote:
>
https://codereview.chromium.org/148243013/diff/40001/cc/resources/shared_bitm...
> File cc/resources/shared_bitmap.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/40001/cc/resources/shared_bitm...
> cc/resources/shared_bitmap.cc:27: base::RandBytes(id.name, sizeof id.name);
> nit: sizeof(id.name)
> 

Fixed.

>
https://codereview.chromium.org/148243013/diff/40001/content/browser/composit...
> File content/browser/compositor/image_transport_factory.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/browser/composit...
> content/browser/compositor/image_transport_factory.cc:21:
> ui::Compositor::SetSharedBitmapManager(HostSharedBitmapManager::current());
> move to bottom of this method
> 

Fixed.

>
https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
> File content/child/child_shared_bitmap_manager.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
> content/child/child_shared_bitmap_manager.cc:28: cc::SharedBitmapId id =
> cc::SharedBitmap::GenerateId();
> It seems potentially problematic to me that the child is generating these ids,
> can't it just ask for a size, and be returned an id?

On Windows that would cause an extra round-trip to the browser to ask for the ID
to be created. Not terrible, but I figure we might as well avoid it. Are there
any problems that could be caused by the child generating the IDs? I did however
change this to use crypto::RandBytes, to match what the GPU mailboxes are now
doing.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
> content/child/child_shared_bitmap_manager.cc:55: return
> scoped_ptr<cc::SharedBitmap>();
> Future: It looks like ResourceProvider::ReceiveFromChild currently calls this
> unconditionally, when it's just going to give a NULL back. Maybe we could
> instead delay that call to where the RP actually tries to use the pixels field
> instead, and have this function NOTREACHED()?

Ok, did that in the other patch.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
> content/child/child_shared_bitmap_manager.cc:61: base::SharedMemoryHandle
> handle_to_send = mem->handle();
> Could the id here come from the browser where the handle came from?
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/child/child_shar...
> content/child/child_shared_bitmap_manager.cc:82: sender_->Send(new
> ChildProcessHostMsg_DeletedSharedBitmap(handle->id()));
> TRACE_EVENT here?

Added.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/child/child_thre...
> File content/child/child_thread.h (right):
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/child/child_thre...
> content/child/child_thread.h:237: scoped_ptr<ChildSharedBitmapManager>
> shared_bitmap_manager_;
> move above the WeakPtrFactory?

Ok.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
> File content/common/host_shared_bitmap_manager.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
> content/common/host_shared_bitmap_manager.cc:1: // Copyright 2013 The Chromium
> Authors. All rights reserved.
> You have this file in content/common/. Would a renderer that had an OOP iframe
> in it end up owning a HostSharedBitmapManager, or is it just the browser
process
> that owns one of these? I was expecting the latter so this file's place was
> surprising.

All the ChildProcessHostImpl stuff is in content/common, so I think it makes
sense to have this live alongside that. Currently it's only used by
RenderMessageFilter, but if we want to use it from eg the GPU process then we'll
want it used from the ChildProcessHostImpl.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
> content/common/host_shared_bitmap_manager.cc:33:
> base::LazyInstance<HostSharedBitmapManager> g_shared_memory_manager =
> Naive question maybe, as this seems to be the way things are often done in the
> browser process.. but.. Is there someone that could own this instead of making
> it a global?

I can't think of anything, unfortunately. Most things in the browser process
seem to be per-window.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
> content/common/host_shared_bitmap_manager.cc:45: return
> scoped_ptr<cc::SharedBitmap>();
> NOTREACHED() and a comment?

No, this will happen when the browser compositor tries to create tiles for
itself. They don't need to be shared anywhere, so for efficiency it makes more
sense for it to allocate them using new[] itself.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
> content/common/host_shared_bitmap_manager.cc:112: NOTREACHED() << "Cannot
create
> shared memory buffer";
> Not a fan of NOTREACHED/DCHECK on a path that also tries to handle the case.
> Then we can't write a test for this code path. Either LOG(FATAL) and stop
trying
> to handle the case, or LOG(ERROR) and handle it as you are, IMO.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
> content/common/host_shared_bitmap_manager.cc:124: NOTREACHED() << "Cannot
share
> shared memory buffer";
> Same here.
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
> File content/common/host_shared_bitmap_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/40001/content/common/host_shar...
> content/common/host_shared_bitmap_manager_unittest.cc:27:
> manager_->ChildAllocatedSharedBitmap(
> This test skips AllocateSharedBitmapForChild, can you add a test for that?

Added TestCreateForChild.
> 
>
https://codereview.chromium.org/148243013/diff/40001/ui/compositor/compositor.cc
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/40001/ui/compositor/compositor...
> ui/compositor/compositor.cc:48: cc::SharedBitmapManager*
> g_shared_bitmap_manager;
> Can we pass it to ui::Compositor constructor instead?

The are a large number of function calls in ui/aura/ in between content/ and
when the compositor is actually created, and most of them don't care about the
SharedBitmapManager. I figure it makes sense to set this up along with the
context factory, which is conceptually similar and also global.

[danakj, 2014-02-14 20:16:47]

Ok I'm mostly good with this now. It's making more sense after a couple rounds
:) A few questions in the tests.

https://codereview.chromium.org/148243013/diff/160001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/148243013/diff/160001/cc/cc.gyp#newcode16
cc/cc.gyp:16: '<(DEPTH)/crypto/crypto.gyp:crypto',
This might change based on
https://codereview.chromium.org/165393003/diff/40001/crypto/random.h

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager.cc:45: // Bitmaps allocated in host
don't need to be shared to other processes, so
Ah ok. This causes RP to go down the new[] path instead. I'd prefer if this
class did the new[], and RP knew to call a different API. But then it needs to
know upfront that it is making a non-shareable thing. Let's see if we can make
this more clear in future CLs.

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager.cc:79: return
scoped_ptr<cc::SharedBitmap>();
Can this NOTREACHED()? This would mean the browser has a TextureMailbox with
shared memory, but it should not be using shared memory for things that it gives
to the compositor.

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
File content/common/host_shared_bitmap_manager_unittest.cc (right):

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager_unittest.cc:31: large_bitmap =
manager_->GetSharedBitmapFromId(gfx::Size(1024, 1024), id);
Is it not super weird that this size doesn't match the size the bitmap was
created with? Should this really be a valid use of the manager?

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager_unittest.cc:33:
EXPECT_TRUE(large_bitmap.get() == NULL);
This is 4mb of memory, we can't map 4 megabytes on non-posix?

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager_unittest.cc:135: handle,
base::GetCurrentProcessHandle(), id);
What about two different processes using the same id? Can you add a test for
that case?

[jbauman, 2014-02-18 19:31:35]

https://codereview.chromium.org/148243013/diff/160001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/148243013/diff/160001/cc/cc.gyp#newcode16
cc/cc.gyp:16: '<(DEPTH)/crypto/crypto.gyp:crypto',
On 2014/02/14 20:16:48, danakj wrote:
> This might change based on
> https://codereview.chromium.org/165393003/diff/40001/crypto/random.h

Looks like I should change this back to use base::RandBytes, as that's what
mailboxes are doing now.

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager.cc:79: return
scoped_ptr<cc::SharedBitmap>();
On 2014/02/14 20:16:48, danakj wrote:
> Can this NOTREACHED()? This would mean the browser has a TextureMailbox with
> shared memory, but it should not be using shared memory for things that it
gives
> to the compositor.

Sure.

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
File content/common/host_shared_bitmap_manager_unittest.cc (right):

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager_unittest.cc:31: large_bitmap =
manager_->GetSharedBitmapFromId(gfx::Size(1024, 1024), id);
On 2014/02/14 20:16:48, danakj wrote:
> Is it not super weird that this size doesn't match the size the bitmap was
> created with? Should this really be a valid use of the manager?

It's not valid at all, so I'm testing that mapping this fails in some way (so at
least we're not mapping some unrelated memory in this case).

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager_unittest.cc:135: handle,
base::GetCurrentProcessHandle(), id);
On 2014/02/14 20:16:48, danakj wrote:
> What about two different processes using the same id? Can you add a test for
> that case?

I don't think we have two valid process handles here, so I can't figure out a
way to test that. It shouldn't matter in the current implementation, though.

[danakj, 2014-02-18 19:57:28]

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
File content/common/host_shared_bitmap_manager_unittest.cc (right):

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager_unittest.cc:31: large_bitmap =
manager_->GetSharedBitmapFromId(gfx::Size(1024, 1024), id);
On 2014/02/18 19:31:35, jbauman wrote:
> On 2014/02/14 20:16:48, danakj wrote:
> > Is it not super weird that this size doesn't match the size the bitmap was
> > created with? Should this really be a valid use of the manager?
> 
> It's not valid at all, so I'm testing that mapping this fails in some way (so
at
> least we're not mapping some unrelated memory in this case).

But on posix you do end up mapping something that will later crash. Why don't we
guard this inside the manager to always return NULL?

https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
content/common/host_shared_bitmap_manager_unittest.cc:135: handle,
base::GetCurrentProcessHandle(), id);
On 2014/02/18 19:31:35, jbauman wrote:
> On 2014/02/14 20:16:48, danakj wrote:
> > What about two different processes using the same id? Can you add a test for
> > that case?
> 
> I don't think we have two valid process handles here, so I can't figure out a
> way to test that. It shouldn't matter in the current implementation, though.

I was hoping there's some way to generate fake process handles. Do we have
something like that?

[jbauman, 2014-02-18 20:39:19]

PTAL.

I also discovered that when delegated rendering is disabled (by default still)
we still hit HostSharedBitmapManager::GetBitmapForSharedMemory for the output
from a child software renderer, so putting NOTREACHED there won't work.

On 2014/02/18 19:57:28, danakj wrote:
>
https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
> File content/common/host_shared_bitmap_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
> content/common/host_shared_bitmap_manager_unittest.cc:31: large_bitmap =
> manager_->GetSharedBitmapFromId(gfx::Size(1024, 1024), id);
> On 2014/02/18 19:31:35, jbauman wrote:
> > On 2014/02/14 20:16:48, danakj wrote:
> > > Is it not super weird that this size doesn't match the size the bitmap was
> > > created with? Should this really be a valid use of the manager?
> > 
> > It's not valid at all, so I'm testing that mapping this fails in some way
(so
> at
> > least we're not mapping some unrelated memory in this case).
> 
> But on posix you do end up mapping something that will later crash. Why don't
we
> guard this inside the manager to always return NULL?
> 
Ok, I changed this to keep track of the size requested by the renderer and
disallow mapping more than that. This doesn't protect against renderers that
actually map less than that size, but there doesn't seem to be any way to really
protect against that (the renderer can always call ftruncate on the fd in the
worst case).

>
https://codereview.chromium.org/148243013/diff/160001/content/common/host_sha...
> content/common/host_shared_bitmap_manager_unittest.cc:135: handle,
> base::GetCurrentProcessHandle(), id);
> On 2014/02/18 19:31:35, jbauman wrote:
> > On 2014/02/14 20:16:48, danakj wrote:
> > > What about two different processes using the same id? Can you add a test
for
> > > that case?
> > 
> > I don't think we have two valid process handles here, so I can't figure out
a
> > way to test that. It shouldn't matter in the current implementation, though.
> 
> I was hoping there's some way to generate fake process handles. Do we have
> something like that?

Not that I can see. It would have to be a legitimate process handle, though, or
else it wouldn't work on Windows.

[piman, 2014-02-18 21:38:20]

https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
File content/child/child_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
content/child/child_shared_bitmap_manager.cc:27: size_t memory_size =
cc::SharedBitmap::GetSizeInBytes(size);
Do we need to protect against overflow here? Can content try to create
layers/quads that are 33kx32k?

https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
content/child/child_shared_bitmap_manager.cc:38:
make_scoped_ptr(ChildThread::AllocateSharedMemory(memory_size, sender_));
nit: or memory.reset(ChildThread::AllocateSharedMemory(memory_size, sender_)) to
fit on one line.

https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
content/child/child_shared_bitmap_manager.cc:41: base::SharedMemoryHandle
handle_to_send = memory->handle();
Is that safe?

Assume the child (renderer) does like:

scoped_ptr<cc::SharedBitmap> bitmap = AllocateSharedBitmap(...);
SendFrameThatUsesBitmap(bitmap);
bitmap.reset();


Then by the time the browser receives the
ChildProcessHostMsg_AllocatedSharedBitmap, the handle may have been deleted in
the child, and the browser will try to map an invalid handle (are handle values
recycled on Windows? That'd be even scarier).

https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
content/child/child_shared_bitmap_manager.cc:49: base::Unretained(this))));
What makes Unretained safe?

https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
content/child/child_shared_bitmap_manager.cc:73: base::Unretained(this))));
What makes Unretained safe?

https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
File content/child/child_shared_bitmap_manager.h (right):

https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
content/child/child_shared_bitmap_manager.h:33: scoped_refptr<ThreadSafeSender>
sender_;
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
content/common/host_shared_bitmap_manager.cc:64: size_t bitmap_size =
cc::SharedBitmap::GetSizeInBytes(size);
We probably want overflow protection here.
Otherwise the renderer could cause OOB reads in the browser, if it sends a quad
that's, say, 33kx32k, with a 1MB resource or whatever.

https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
File content/common/host_shared_bitmap_manager.h (right):

https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
content/common/host_shared_bitmap_manager.h:56: typedef
std::map<cc::SharedBitmapId, scoped_refptr<BitmapData> > BitmapMap;
nit: hash_map is probably faster (and uses less memory).

https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
content/common/host_shared_bitmap_manager.h:59: typedef
std::map<base::ProcessHandle, std::set<cc::SharedBitmapId> >
nit: hash_map, hash_set

[jbauman, 2014-02-21 01:45:48]

On 2014/02/18 21:38:20, piman (OOO back 2014-3-4) wrote:
>
https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
> File content/child/child_shared_bitmap_manager.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
> content/child/child_shared_bitmap_manager.cc:27: size_t memory_size =
> cc::SharedBitmap::GetSizeInBytes(size);
> Do we need to protect against overflow here? Can content try to create
> layers/quads that are 33kx32k?

Good point, can do. It was possible in the past for video to create very large
layers, but I think that's been fixed. So it should only happen with malicious
renderers.
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
> content/child/child_shared_bitmap_manager.cc:38:
> make_scoped_ptr(ChildThread::AllocateSharedMemory(memory_size, sender_));
> nit: or memory.reset(ChildThread::AllocateSharedMemory(memory_size, sender_))
to
> fit on one line.
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
> content/child/child_shared_bitmap_manager.cc:41: base::SharedMemoryHandle
> handle_to_send = memory->handle();
> Is that safe?
> 
> Assume the child (renderer) does like:
> 
> scoped_ptr<cc::SharedBitmap> bitmap = AllocateSharedBitmap(...);
> SendFrameThatUsesBitmap(bitmap);
> bitmap.reset();
> 
> 
> Then by the time the browser receives the
> ChildProcessHostMsg_AllocatedSharedBitmap, the handle may have been deleted in
> the child, and the browser will try to map an invalid handle (are handle
values
> recycled on Windows? That'd be even scarier).

Well, it's no worse than what we current do with the legacy 2d or with existing
software compositor path....
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
> content/child/child_shared_bitmap_manager.cc:49: base::Unretained(this))));
> What makes Unretained safe?
> 
The RenderWidget will be shut down before the RenderThreadImpl, causing this
callback to be called then.

>
https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
> content/child/child_shared_bitmap_manager.cc:73: base::Unretained(this))));
> What makes Unretained safe?

Same
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
> File content/child/child_shared_bitmap_manager.h (right):
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/child/child_sha...
> content/child/child_shared_bitmap_manager.h:33:
scoped_refptr<ThreadSafeSender>
> sender_;
> nit: DISALLOW_COPY_AND_ASSIGN
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
> File content/common/host_shared_bitmap_manager.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
> content/common/host_shared_bitmap_manager.cc:64: size_t bitmap_size =
> cc::SharedBitmap::GetSizeInBytes(size);
> We probably want overflow protection here.
> Otherwise the renderer could cause OOB reads in the browser, if it sends a
quad
> that's, say, 33kx32k, with a 1MB resource or whatever.
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
> File content/common/host_shared_bitmap_manager.h (right):
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
> content/common/host_shared_bitmap_manager.h:56: typedef
> std::map<cc::SharedBitmapId, scoped_refptr<BitmapData> > BitmapMap;
> nit: hash_map is probably faster (and uses less memory).
> 
>
https://codereview.chromium.org/148243013/diff/280001/content/common/host_sha...
> content/common/host_shared_bitmap_manager.h:59: typedef
> std::map<base::ProcessHandle, std::set<cc::SharedBitmapId> >
> nit: hash_map, hash_set

[danakj, 2014-02-21 16:24:55]

On Thu, Feb 20, 2014 at 8:45 PM, <jbauman@chromium.org> wrote:

> On 2014/02/18 21:38:20, piman (OOO back 2014-3-4) wrote:
>
> https://codereview.chromium.org/148243013/diff/280001/
> content/child/child_shared_bitmap_manager.cc
>
>> File content/child/child_shared_bitmap_manager.cc (right):
>>
>
>
> https://codereview.chromium.org/148243013/diff/280001/
> content/child/child_shared_bitmap_manager.cc#newcode27
>
>> content/child/child_shared_bitmap_manager.cc:27: size_t memory_size =
>> cc::SharedBitmap::GetSizeInBytes(size);
>> Do we need to protect against overflow here? Can content try to create
>> layers/quads that are 33kx32k?
>>
>
> Good point, can do. It was possible in the past for video to create very
> large
> layers, but I think that's been fixed. So it should only happen with
> malicious
> renderers.
>
>
>
> https://codereview.chromium.org/148243013/diff/280001/
> content/child/child_shared_bitmap_manager.cc#newcode38
>
>> content/child/child_shared_bitmap_manager.cc:38:
>> make_scoped_ptr(ChildThread::AllocateSharedMemory(memory_size, sender_));
>> nit: or memory.reset(ChildThread::AllocateSharedMemory(memory_size,
>> sender_))
>>
> to
>
>> fit on one line.
>>
>
>
> https://codereview.chromium.org/148243013/diff/280001/
> content/child/child_shared_bitmap_manager.cc#newcode41
>
>> content/child/child_shared_bitmap_manager.cc:41: base::SharedMemoryHandle
>> handle_to_send = memory->handle();
>> Is that safe?
>>
>
>  Assume the child (renderer) does like:
>>
>
>  scoped_ptr<cc::SharedBitmap> bitmap = AllocateSharedBitmap(...);
>> SendFrameThatUsesBitmap(bitmap);
>> bitmap.reset();
>>
>
>
>  Then by the time the browser receives the
>> ChildProcessHostMsg_AllocatedSharedBitmap, the handle may have been
>> deleted in
>> the child, and the browser will try to map an invalid handle (are handle
>>
> values
>
>> recycled on Windows? That'd be even scarier).
>>
>
> Well, it's no worse than what we current do with the legacy 2d or with
> existing
> software compositor path....
>
>
>
> https://codereview.chromium.org/148243013/diff/280001/
> content/child/child_shared_bitmap_manager.cc#newcode49
>
>> content/child/child_shared_bitmap_manager.cc:49:
>> base::Unretained(this))));
>> What makes Unretained safe?
>>
>
> The RenderWidget will be shut down before the RenderThreadImpl, causing
> this
> callback to be called then.



Can document this in a comment?


>
>
>
> https://codereview.chromium.org/148243013/diff/280001/
> content/child/child_shared_bitmap_manager.cc#newcode73
>
>> content/child/child_shared_bitmap_manager.cc:73:
>> base::Unretained(this))));
>> What makes Unretained safe?
>>
>
> Same


And this?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2014-02-25 00:46:18]

PTAL.

[piman, 2014-03-05 22:15:54]

LGTM, thanks for the extra tests!

[danakj, 2014-03-05 22:25:18]

LGTM2

[jbauman, 2014-03-05 23:30:40]

jschuh@, could I get a security review of the new IPC messages for creating and
destroying shared memory?

[jschuh, 2014-03-08 00:58:10]

Sorry for getting to this late. I missed it.

https://codereview.chromium.org/148243013/diff/470001/cc/resources/shared_bit...
File cc/resources/shared_bitmap.cc (right):

https://codereview.chromium.org/148243013/diff/470001/cc/resources/shared_bit...
cc/resources/shared_bitmap.cc:30: *size_in_bytes = size.GetArea() * 4;
I think the base::CheckedNumeric template would make the math and overflow
checks clearer.

https://codereview.chromium.org/148243013/diff/470001/content/common/host_sha...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/148243013/diff/470001/content/common/host_sha...
content/common/host_shared_bitmap_manager.cc:72: if
(!data->memory->Map(data->buffer_size)) {
Just want to verify this is the only place we map the memory in the privileged
process.

[jbauman, 2014-03-10 20:00:36]

On 2014/03/08 00:58:10, Justin Schuh wrote:
> Sorry for getting to this late. I missed it.
> 
>
https://codereview.chromium.org/148243013/diff/470001/cc/resources/shared_bit...
> File cc/resources/shared_bitmap.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/470001/cc/resources/shared_bit...
> cc/resources/shared_bitmap.cc:30: *size_in_bytes = size.GetArea() * 4;
> I think the base::CheckedNumeric template would make the math and overflow
> checks clearer.

Thanks, I didn't realize that that existed. Fixed.
> 
>
https://codereview.chromium.org/148243013/diff/470001/content/common/host_sha...
> File content/common/host_shared_bitmap_manager.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/470001/content/common/host_sha...
> content/common/host_shared_bitmap_manager.cc:72: if
> (!data->memory->Map(data->buffer_size)) {
> Just want to verify this is the only place we map the memory in the privileged
> process.

Yes, that's right.

[jschuh, 2014-03-10 22:18:53]

https://codereview.chromium.org/148243013/diff/490001/cc/resources/shared_bit...
File cc/resources/shared_bitmap.cc (right):

https://codereview.chromium.org/148243013/diff/490001/cc/resources/shared_bit...
cc/resources/shared_bitmap.cc:23: base::CheckedNumeric<int> s = size.width();
Do you still want a check that height and width are greater than zero in here
somewhere? You'll catch any overflows, but couldn't you still get a negative or
zero size?

[jbauman, 2014-03-11 21:15:43]

On 2014/03/10 22:18:53, Justin Schuh wrote:
>
https://codereview.chromium.org/148243013/diff/490001/cc/resources/shared_bit...
> File cc/resources/shared_bitmap.cc (right):
> 
>
https://codereview.chromium.org/148243013/diff/490001/cc/resources/shared_bit...
> cc/resources/shared_bitmap.cc:23: base::CheckedNumeric<int> s = size.width();
> Do you still want a check that height and width are greater than zero in here
> somewhere? You'll catch any overflows, but couldn't you still get a negative
or
> zero size?

Good point; I added a check for that.

[jschuh, 2014-03-11 21:42:33]

On 2014/03/11 21:15:43, jbauman wrote:
> On 2014/03/10 22:18:53, Justin Schuh wrote:
> >
>
https://codereview.chromium.org/148243013/diff/490001/cc/resources/shared_bit...
> > File cc/resources/shared_bitmap.cc (right):
> > 
> >
>
https://codereview.chromium.org/148243013/diff/490001/cc/resources/shared_bit...
> > cc/resources/shared_bitmap.cc:23: base::CheckedNumeric<int> s =
size.width();
> > Do you still want a check that height and width are greater than zero in
here
> > somewhere? You'll catch any overflows, but couldn't you still get a negative
> or
> > zero size?
> 
> Good point; I added a check for that.

lgtm

[jbauman, 2014-03-11 21:45:57]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-03-11 22:05:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/148243013/530001

[commit-bot: I haz the power, 2014-03-11 23:19:56]

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

[commit-bot: I haz the power, 2014-03-11 23:19:57]

Try jobs failed on following builders: linux_chromium_clang_dbg

[jbauman, 2014-03-11 23:21:40]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-03-11 23:26:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/148243013/530001

[commit-bot: I haz the power, 2014-03-12 01:48:25]

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

[commit-bot: I haz the power, 2014-03-12 01:48:26]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[jschuh, 2014-03-12 02:58:18]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2014-03-12 02:59:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/148243013/530001

[commit-bot: I haz the power, 2014-03-12 06:05:35]

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

[commit-bot: I haz the power, 2014-03-12 06:05:36]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[jbauman, 2014-03-13 19:44:17]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-03-13 19:49:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/148243013/550001

[commit-bot: I haz the power, 2014-03-13 21:13:31]

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

[commit-bot: I haz the power, 2014-03-13 21:13:32]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[danakj, 2014-03-13 21:27:22]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-13 21:28:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/148243013/550001

[commit-bot: I haz the power, 2014-03-13 22:52:57]

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

[commit-bot: I haz the power, 2014-03-13 22:52:58]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[jbauman, 2014-03-13 23:04:56]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:05:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/148243013/570001

[commit-bot: I haz the power, 2014-03-13 23:24:23]

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

[commit-bot: I haz the power, 2014-03-13 23:24:24]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[jbauman, 2014-03-13 23:30:14]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:31:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/148243013/570001

[commit-bot: I haz the power, 2014-03-13 23:35:59]

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

[commit-bot: I haz the power, 2014-03-13 23:36:00]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[jbauman, 2014-03-13 23:59:53]

Committed patchset #12 manually as r256955 (presubmit successful).