Make SharedMemory track the size that was actually mapped

Make SharedMemory track the size that was actually mapped

In the process I also had to fix several dependencies, and
some major issues in transport DIBs.

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

[jschuh, 2013-03-18 01:09:25]

@brettw - base/shared_memory*
Windows is the only one with significant changes you need to look at. The rest
is mechanical.

@michaelbai - base/shared_memory_posix.cc + ui/surface/transport_dib_android.cc
To make sure that the android-specific stuff is correct.

@dalecurtis - content/browser/renderer_host/media
All mechanical callsite changes.

@kbr - ui/surface
Windows is the one with the significant changes here too.

[DaleCurtis, 2013-03-18 16:41:19]

media/ lgtm.

[Ken Russell (switch to Gerrit), 2013-03-18 16:49:01]

ui/surface => apatrick

[apatrick_chromium, 2013-03-18 17:32:10]

I looked at ui/surface. I'm not familiar with the TransportDIB code. Some
questions inline.

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib.cc
File ui/surface/transport_dib.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib....
ui/surface/transport_dib.cc:14: if (w <= 0 || h <= 0 || static_cast<size_t>(w) >
(kMaxSize / one_stride) ||
nit: braces around "return false".

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib....
ui/surface/transport_dib.cc:18: return (stride * h) <= size_;
Are we worried about (stride * h) overflowing or being negative on account of
that here?

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
File ui/surface/transport_dib_android.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
ui/surface/transport_dib_android.cc:66: if ((!memory() && !Map()) ||
!VerifyCanvasSize(w, h))
Do you want to evaluate VerifyCanvasSize(w, h) first so the short-circuit avoids
mapping unnecessarily?

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
File ui/surface/transport_dib_linux.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
ui/surface/transport_dib_linux.cc:95: if ((address_ == kInvalidAddress &&
!Map()) || !VerifyCanvasSize(w, h))
Do you want to evaluate VerifyCanvasSize(w, h) first so the short-circuit avoids
mapping unnecessarily?

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
File ui/surface/transport_dib_mac.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
ui/surface/transport_dib_mac.cc:64: if ((!memory() && !Map()) ||
!VerifyCanvasSize(w, h))
Do you want to evaluate VerifyCanvasSize(w, h) first so the short-circuit avoids
mapping unnecessarily?

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
File ui/surface/transport_dib_win.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
ui/surface/transport_dib_win.cc:86: size_ = memory_info.RegionSize -
(reinterpret_cast<char*>(canvas) -
From MSDN:

"RegionSize
The size of the region beginning at the base address in which all pages have
identical attributes, in bytes."

I don't know exactly what RegionSize means. If you this reviewed by someone who
understands what you're doing, add Carlos?

[brettw, 2013-03-18 19:39:07]

base lgtm

https://codereview.chromium.org/12537014/diff/67007/base/shared_memory_win.cc
File base/shared_memory_win.cc (right):

https://codereview.chromium.org/12537014/diff/67007/base/shared_memory_win.cc...
base/shared_memory_win.cc:29: requested_size_(0),
Is the ordering of these initializers correct? This ordering actually makes more
sense to me than what's in the header (I'd expect the two sizes to be next to
each other). Maybe fix that instead (although I guess this means you have to
update the other platforms).

[jschuh, 2013-03-21 01:08:03]

https://codereview.chromium.org/12537014/diff/67007/base/shared_memory_win.cc
File base/shared_memory_win.cc (right):

https://codereview.chromium.org/12537014/diff/67007/base/shared_memory_win.cc...
base/shared_memory_win.cc:29: requested_size_(0),
On 2013/03/18 19:39:07, brettw wrote:
> Is the ordering of these initializers correct? This ordering actually makes
more
> sense to me than what's in the header (I'd expect the two sizes to be next to
> each other). Maybe fix that instead (although I guess this means you have to
> update the other platforms).

I'm lazy, so I'll match the header.

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib.cc
File ui/surface/transport_dib.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib....
ui/surface/transport_dib.cc:14: if (w <= 0 || h <= 0 || static_cast<size_t>(w) >
(kMaxSize / one_stride) ||
On 2013/03/18 17:32:10, apatrick_chromium wrote:
> nit: braces around "return false".

Done.

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib....
ui/surface/transport_dib.cc:18: return (stride * h) <= size_;
On 2013/03/18 17:32:10, apatrick_chromium wrote:
> Are we worried about (stride * h) overflowing or being negative on account of
> that here?

The checks above verify it's positive and won't overflow. This last check just
verifies it's in bounds.

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
File ui/surface/transport_dib_android.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
ui/surface/transport_dib_android.cc:66: if ((!memory() && !Map()) ||
!VerifyCanvasSize(w, h))
On 2013/03/18 17:32:10, apatrick_chromium wrote:
> Do you want to evaluate VerifyCanvasSize(w, h) first so the short-circuit
avoids
> mapping unnecessarily?

No, because we won't get the actual size until we map it. Also, this constraint
is verified before the values are sent, but we do it here solely to protect
against a malicious sandboxed process.

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
File ui/surface/transport_dib_linux.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
ui/surface/transport_dib_linux.cc:95: if ((address_ == kInvalidAddress &&
!Map()) || !VerifyCanvasSize(w, h))
On 2013/03/18 17:32:10, apatrick_chromium wrote:
> Do you want to evaluate VerifyCanvasSize(w, h) first so the short-circuit
avoids
> mapping unnecessarily?

Same answer as Android.

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
File ui/surface/transport_dib_mac.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
ui/surface/transport_dib_mac.cc:64: if ((!memory() && !Map()) ||
!VerifyCanvasSize(w, h))
On 2013/03/18 17:32:10, apatrick_chromium wrote:
> Do you want to evaluate VerifyCanvasSize(w, h) first so the short-circuit
avoids
> mapping unnecessarily?

Same answer as Android.

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
File ui/surface/transport_dib_win.cc (right):

https://codereview.chromium.org/12537014/diff/67007/ui/surface/transport_dib_...
ui/surface/transport_dib_win.cc:86: size_ = memory_info.RegionSize -
(reinterpret_cast<char*>(canvas) -
On 2013/03/18 17:32:10, apatrick_chromium wrote:
> From MSDN:
> 
> "RegionSize
> The size of the region beginning at the base address in which all pages have
> identical attributes, in bytes."
> 
> I don't know exactly what RegionSize means. If you this reviewed by someone
who
> understands what you're doing, add Carlos?

I will add the @cpu for sanity checking.

[jschuh, 2013-03-21 01:08:25]

Adding @cpu to vouch for Windows.

[cpu_(ooo_6.6-7.5), 2013-03-21 17:14:22]

https://codereview.chromium.org/12537014/diff/97002/base/shared_memory_win.cc
File base/shared_memory_win.cc (right):

https://codereview.chromium.org/12537014/diff/97002/base/shared_memory_win.cc...
base/shared_memory_win.cc:19: }
I only see one usage and it that case address == allocationBase, right?

Do you have other uses planned?

[cpu_(ooo_6.6-7.5), 2013-03-21 17:33:40]

https://codereview.chromium.org/12537014/diff/97002/ui/surface/transport_dib_...
File ui/surface/transport_dib_win.cc (right):

https://codereview.chromium.org/12537014/diff/97002/ui/surface/transport_dib_...
ui/surface/transport_dib_win.cc:86: size_ = memory_info.RegionSize -
(reinterpret_cast<char*>(canvas) -
do we have any guarantee that skia::CreatePlatformCanvas returns a pointer
something inside the section?

I mean could skia::PlatformCanvas be

class PlatformCanvas {
  int foo_;
  void* section_;
};

[apatrick_chromium, 2013-03-21 18:01:13]

Thanks for the clarification. Over to cpu. LGTM for my part.

[jschuh, 2013-03-21 19:32:05]

https://codereview.chromium.org/12537014/diff/97002/base/shared_memory_win.cc
File base/shared_memory_win.cc (right):

https://codereview.chromium.org/12537014/diff/97002/base/shared_memory_win.cc...
base/shared_memory_win.cc:19: }
On 2013/03/21 17:14:22, cpu wrote:
> I only see one usage and it that case address == allocationBase, right?
> 
> Do you have other uses planned?

Oh, there is only one use at this point so I can move it inline. However,
MapAt() doesn't need to align address to AllocationBase, and the cost of the
subtraction is negligible against the cost of mapping. So, I'd rather keep that
part, just to be safe.

https://codereview.chromium.org/12537014/diff/97002/ui/surface/transport_dib_...
File ui/surface/transport_dib_win.cc (right):

https://codereview.chromium.org/12537014/diff/97002/ui/surface/transport_dib_...
ui/surface/transport_dib_win.cc:86: size_ = memory_info.RegionSize -
(reinterpret_cast<char*>(canvas) -
On 2013/03/21 17:33:40, cpu wrote:
> do we have any guarantee that skia::CreatePlatformCanvas returns a pointer
> something inside the section?

As written now, yes. It just ends up calling CreateDIBSection, which maps the
section as the canvas. I'm not sure what we could do to future-proof this,
beyond what I'm already doing by checking with the VirtualQuery after the
mapping. And someone was opposed to me using native API calls to perform any
more intensive verification. ;)

[cpu_(ooo_6.6-7.5), 2013-03-21 20:02:27]

https://codereview.chromium.org/12537014/diff/97002/ui/surface/transport_dib_...
File ui/surface/transport_dib_win.cc (right):

https://codereview.chromium.org/12537014/diff/97002/ui/surface/transport_dib_...
ui/surface/transport_dib_win.cc:86: size_ = memory_info.RegionSize -
(reinterpret_cast<char*>(canvas) -
But what does the subtraction of line 86 and 87 do then? 

Seems we need a dcheck in the skia::CreatePlatformCanvas function.


On 2013/03/21 19:32:06, Justin Schuh wrote:
> On 2013/03/21 17:33:40, cpu wrote:
> > do we have any guarantee that skia::CreatePlatformCanvas returns a pointer
> > something inside the section?
> 
> As written now, yes. It just ends up calling CreateDIBSection, which maps the
> section as the canvas. I'm not sure what we could do to future-proof this,
> beyond what I'm already doing by checking with the VirtualQuery after the
> mapping. And someone was opposed to me using native API calls to perform any
> more intensive verification. ;)

[cpu_(ooo_6.6-7.5), 2013-03-21 20:06:15]

https://codereview.chromium.org/12537014/diff/97002/base/shared_memory_win.cc
File base/shared_memory_win.cc (right):

https://codereview.chromium.org/12537014/diff/97002/base/shared_memory_win.cc...
base/shared_memory_win.cc:19: }
standalone is fine.

On 2013/03/21 19:32:06, Justin Schuh wrote:
> On 2013/03/21 17:14:22, cpu wrote:
> > I only see one usage and it that case address == allocationBase, right?
> > 
> > Do you have other uses planned?
> 
> Oh, there is only one use at this point so I can move it inline. However,
> MapAt() doesn't need to align address to AllocationBase, and the cost of the
> subtraction is negligible against the cost of mapping. So, I'd rather keep
that
> part, just to be safe.

[jschuh, 2013-03-26 22:09:24]

https://codereview.chromium.org/12537014/diff/97002/ui/surface/transport_dib_...
File ui/surface/transport_dib_win.cc (right):

https://codereview.chromium.org/12537014/diff/97002/ui/surface/transport_dib_...
ui/surface/transport_dib_win.cc:86: size_ = memory_info.RegionSize -
(reinterpret_cast<char*>(canvas) -
I think I over-complicated things. We can just calculate the size here since we
know that either the canvas is valid or creating it failed.

[cpu_(ooo_6.6-7.5), 2013-03-26 23:31:10]

lgtm

[commit-bot: I haz the power, 2013-03-26 23:45:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/12537014/111005

[commit-bot: I haz the power, 2013-03-27 05:26:02]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-03-27 21:06:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/12537014/151002

[commit-bot: I haz the power, 2013-03-27 21:58:11]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-03-27 23:31:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/12537014/151002

[commit-bot: I haz the power, 2013-03-28 02:02:23]

Change committed as 191098