Switch gfx::Range to use uint32_t instead of size_t.

Switch gfx::Range to use uint32_t instead of size_t.

This is because size_t's size depends on the architecture and we send gfx::Range over IPC. We need IPCs to match as we're now going to support 32 and 64 bit processes communicating on Android.

This is split off from https://codereview.chromium.org/1619363002/.

BUG=581409
Committed: https://crrev.com/1f0b47c4a982ab2076590d95937b89f892bd4552
Cr-Commit-Position: refs/heads/master@{#374568}

[jam, 2016-02-06 00:44:10]

Description was changed from

==========
Switch gfx::Range to use uint32_t instead of size_t.

This is because size_t's size depends on the architecture and we send gfx::Range
over IPC. We need IPCs to match as we're now going to support 32 and 64 bit
processes communicating on Android.

This is split off from https://codereview.chromium.org/1619363002/.

BUG=581409
==========

to

==========
Switch gfx::Range to use uint32_t instead of size_t.

This is because size_t's size depends on the architecture and we send gfx::Range
over IPC. We need IPCs to match as we're now going to support 32 and 64 bit
processes communicating on Android.

This is split off from https://codereview.chromium.org/1619363002/.

BUG=581409
==========

[jam, 2016-02-06 00:44:10]

jam@chromium.org changed reviewers:
+ rsesek@chromium.org, tsepez@chromium.org

[jam, 2016-02-06 00:46:06]

Tom: ui\gfx\ipc\gfx_param_traits.cc
Rob: rest

I first tried switching only the traits to serialize as uint32_t, but that ran
into issues when the value was npos. In the 32 bit process, when deserializing
it would appear that overflow occurred. I didn't want to special case -1 since
that could be an actual error to send that much over IPC.

[Tom Sepez, 2016-02-08 17:29:43]

https://codereview.chromium.org/1671403002/diff/20001/ui/gfx/ipc/gfx_param_tr...
File ui/gfx/ipc/gfx_param_traits.cc (right):

https://codereview.chromium.org/1671403002/diff/20001/ui/gfx/ipc/gfx_param_tr...
ui/gfx/ipc/gfx_param_traits.cc:310: int start, end;
can we use a stdint type here?

[Robert Sesek, 2016-02-08 17:41:08]

https://codereview.chromium.org/1671403002/diff/20001/ui/gfx/range/range.cc
File ui/gfx/range/range.cc (right):

https://codereview.chromium.org/1671403002/diff/20001/ui/gfx/range/range.cc#n...
ui/gfx/range/range.cc:10: #include "base/format_macros.h"
Unused now that PRIuS is gone.

[jam, 2016-02-08 17:56:29]

https://codereview.chromium.org/1671403002/diff/20001/ui/gfx/ipc/gfx_param_tr...
File ui/gfx/ipc/gfx_param_traits.cc (right):

https://codereview.chromium.org/1671403002/diff/20001/ui/gfx/ipc/gfx_param_tr...
ui/gfx/ipc/gfx_param_traits.cc:310: int start, end;
On 2016/02/08 17:29:43, Tom Sepez wrote:
> can we use a stdint type here?

Done.

https://codereview.chromium.org/1671403002/diff/20001/ui/gfx/range/range.cc
File ui/gfx/range/range.cc (right):

https://codereview.chromium.org/1671403002/diff/20001/ui/gfx/range/range.cc#n...
ui/gfx/range/range.cc:10: #include "base/format_macros.h"
On 2016/02/08 17:41:07, Robert Sesek wrote:
> Unused now that PRIuS is gone.

Done.

[Robert Sesek, 2016-02-08 18:02:43]

LGTM

[Peter Kasting, 2016-02-09 00:52:38]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-02-09 00:52:38]

Not LGTM

We should have methods already to safely send size_ts over IPC (which force it
to 64 bits regardless of the native size of either end).  And the actual API of
Range definitely should be using size_t and not uint32_t from a conceptual point
of view -- things like range lengths are sizes.

[jabdelmalek, 2016-02-09 01:05:26]

On 2016/02/09 00:52:38, Peter Kasting wrote:
> Not LGTM
> 
> We should have methods already to safely send size_ts over IPC (which force it
> to 64 bits regardless of the native size of either end).  And the actual API
of
> Range definitely should be using size_t and not uint32_t from a conceptual
point
> of view -- things like range lengths are sizes.

Hey Peter, there's been a lot of discussion on the best way to solve this bug
over the last few weeks. The consensus was that the safest way was to remove all
size_ts and long from IPC messages, and structs that are sent over IPC. 581409
captures a bunch most of that discussion (but not all, some of it was in person
with Dmitry, Justin and Tom.

The problem with size_ts is that when used in a struct, they are typede'd to
long or int depending on the architecture. The IPC macros don't know what the
original intent is. We considered sending 64 bit integers for numbers always,
but then this fails because you don't know whent o enforce overflow checks
because of things like npos (and others places that are sending -2, -3 etc...
cast as size_t).

[Peter Kasting, 2016-02-09 02:19:15]

On 2016/02/09 01:05:26, jabdelmalek wrote:
> On 2016/02/09 00:52:38, Peter Kasting wrote:
> > Not LGTM
> > 
> > We should have methods already to safely send size_ts over IPC (which force
it
> > to 64 bits regardless of the native size of either end).  And the actual API
> of
> > Range definitely should be using size_t and not uint32_t from a conceptual
> point
> > of view -- things like range lengths are sizes.
> 
> Hey Peter, there's been a lot of discussion on the best way to solve this bug
> over the last few weeks. The consensus was that the safest way was to remove
all
> size_ts and long from IPC messages, and structs that are sent over IPC. 581409
> captures a bunch most of that discussion (but not all, some of it was in
person
> with Dmitry, Justin and Tom.
> 
> The problem with size_ts is that when used in a struct, they are typede'd to
> long or int depending on the architecture. The IPC macros don't know what the
> original intent is. We considered sending 64 bit integers for numbers always,
> but then this fails because you don't know whent o enforce overflow checks
> because of things like npos (and others places that are sending -2, -3 etc...
> cast as size_t).

Using signal values like -2, -3, etc. is almost always inadvisable, and we
should seek to fix code that does that to instead use separate fields/flags to
communicate this information.  In the cases I've encountered this, this has
always ended up making the resulting code clearer and more difficult to misuse. 
This should be very rare in Chromium code today in any case.

If this is ensured, then npos can be special-cased, a la:

  size_t deserialized = (serialized == static_cast<uint64_t>(-1)) ?
      static_cast<size_t>(-1) : checked_cast<size_t>(serialized);

This should handle everything I'm aware of.

Removing size_ts from structs and APIs has dangers and problems that, IMO, go
well beyond the above.  Concerns about introducing potential overflows are an
obvious example, but using non-size_ts also obscures the meaning of certain code
(does this code use a uint32_t because the 32-bitness of the value is a critical
part of the API contract?), as well as making interaction with standard
libraries more difficult, especially as we try to turn on more compiler warnings
about truncating conversions and the like.

These issues are things I'm particularly aware of as I've been spending a great
deal of time over the past couple of years trying to fix precisely these issues
in our codebase, and almost invariably, more consistent use of size_t for all
purposes for which it's suited results in simpler, less buggy code, which also
generates fewer warnings.  This also allows us to be consistent about
encouraging (explicitly, or implicitly as people read existing coding patterns)
the use of size_t in such cases.

I would be happy to chat with any concerned/invested parties further about all
this stuff; I've probably had my head in the size_t issues more than anyone
except possibly Justin, so hopefully I can be helpful.

[Tom Sepez, 2016-02-09 03:10:37]

On 2016/02/09 02:19:15, Peter Kasting wrote:
> On 2016/02/09 01:05:26, jabdelmalek wrote:
> > On 2016/02/09 00:52:38, Peter Kasting wrote:
> > > Not LGTM
> > > 
> > > We should have methods already to safely send size_ts over IPC (which
force
> it
> > > to 64 bits regardless of the native size of either end).  And the actual
API
> > of
> > > Range definitely should be using size_t and not uint32_t from a conceptual
> > point
> > > of view -- things like range lengths are sizes.
> > 
> > Hey Peter, there's been a lot of discussion on the best way to solve this
bug
> > over the last few weeks. The consensus was that the safest way was to remove
> all
> > size_ts and long from IPC messages, and structs that are sent over IPC.
581409
> > captures a bunch most of that discussion (but not all, some of it was in
> person
> > with Dmitry, Justin and Tom.
> > 
> > The problem with size_ts is that when used in a struct, they are typede'd to
> > long or int depending on the architecture. The IPC macros don't know what
the
> > original intent is. We considered sending 64 bit integers for numbers
always,
> > but then this fails because you don't know whent o enforce overflow checks
> > because of things like npos (and others places that are sending -2, -3
etc...
> > cast as size_t).
> 
> Using signal values like -2, -3, etc. is almost always inadvisable, and we
> should seek to fix code that does that to instead use separate fields/flags to
> communicate this information.  In the cases I've encountered this, this has
> always ended up making the resulting code clearer and more difficult to
misuse. 
> This should be very rare in Chromium code today in any case.
> 
> If this is ensured, then npos can be special-cased, a la:
> 
>   size_t deserialized = (serialized == static_cast<uint64_t>(-1)) ?
>       static_cast<size_t>(-1) : checked_cast<size_t>(serialized);
> 
> This should handle everything I'm aware of.
> 
> Removing size_ts from structs and APIs has dangers and problems that, IMO, go
> well beyond the above.  Concerns about introducing potential overflows are an
> obvious example, but using non-size_ts also obscures the meaning of certain
code
> (does this code use a uint32_t because the 32-bitness of the value is a
critical
> part of the API contract?), as well as making interaction with standard
> libraries more difficult, especially as we try to turn on more compiler
warnings
> about truncating conversions and the like.
> 
> These issues are things I'm particularly aware of as I've been spending a
great
> deal of time over the past couple of years trying to fix precisely these
issues
> in our codebase, and almost invariably, more consistent use of size_t for all
> purposes for which it's suited results in simpler, less buggy code, which also
> generates fewer warnings.  This also allows us to be consistent about
> encouraging (explicitly, or implicitly as people read existing coding
patterns)
> the use of size_t in such cases.
> 
> I would be happy to chat with any concerned/invested parties further about all
> this stuff; I've probably had my head in the size_t issues more than anyone
> except possibly Justin, so hopefully I can be helpful.

So ... one thought on going forward is that we might need to define a type which
is large enough to hold the largest size_t on any platform we've yet
encountered, whose value would be the same on all platforms. A max_size_t as it
were.

[jam, 2016-02-09 04:36:28]

On 2016/02/09 02:19:15, Peter Kasting wrote:
> On 2016/02/09 01:05:26, jabdelmalek wrote:
> > On 2016/02/09 00:52:38, Peter Kasting wrote:
> > > Not LGTM
> > > 
> > > We should have methods already to safely send size_ts over IPC (which
force
> it
> > > to 64 bits regardless of the native size of either end).  And the actual
API
> > of
> > > Range definitely should be using size_t and not uint32_t from a conceptual
> > point
> > > of view -- things like range lengths are sizes.
> > 
> > Hey Peter, there's been a lot of discussion on the best way to solve this
bug
> > over the last few weeks. The consensus was that the safest way was to remove
> all
> > size_ts and long from IPC messages, and structs that are sent over IPC.
581409
> > captures a bunch most of that discussion (but not all, some of it was in
> person
> > with Dmitry, Justin and Tom.
> > 
> > The problem with size_ts is that when used in a struct, they are typede'd to
> > long or int depending on the architecture. The IPC macros don't know what
the
> > original intent is. We considered sending 64 bit integers for numbers
always,
> > but then this fails because you don't know whent o enforce overflow checks
> > because of things like npos (and others places that are sending -2, -3
etc...
> > cast as size_t).
> 
> Using signal values like -2, -3, etc. is almost always inadvisable, and we
> should seek to fix code that does that to instead use separate fields/flags to
> communicate this information.  In the cases I've encountered this, this has
> always ended up making the resulting code clearer and more difficult to
misuse. 
> This should be very rare in Chromium code today in any case.

Yes I agree, this is not ideal. However, the goal here was to not have silent
failures. Trying to catch every last usage of -2 or -3 etc would be a game of
whack-a-mole. Removing size_ts and longs and enforcing this with a clang plugin
gives us a compile time way of ensuring we don't have these kind of bugs instead
of relying on code inspection.

> 
> If this is ensured, then npos can be special-cased, a la:
> 
>   size_t deserialized = (serialized == static_cast<uint64_t>(-1)) ?
>       static_cast<size_t>(-1) : checked_cast<size_t>(serialized);
> 
> This should handle everything I'm aware of.

this was all discussed in person and in the bug btw.. we still decided to go
with removing size_ts. note that this is actually not new, 6 years ago a PSA was
sent to chromium-dev about not using size_t in IPC anymore:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MjZye589qqE/LRdYQ...

> 
> Removing size_ts from structs and APIs has dangers and problems that, IMO, go
> well beyond the above.  Concerns about introducing potential overflows are an
> obvious example,

this is taken care of by using checked_cast

> but using non-size_ts also obscures the meaning of certain code
> (does this code use a uint32_t because the 32-bitness of the value is a
critical
> part of the API contract?),

I think this is more theoretical.. If someone wanted to see why this happened
for these structs, it's like any code archeology.


> as well as making interaction with standard
> libraries more difficult, especially as we try to turn on more compiler
warnings
> about truncating conversions and the like.

checked_cast helps as per above.

Also note that this is the last of the changes to remove size_t (see bug for
full list). The next change was to remove the size_t methods from base::Pickle.
Over the last 6 years, only a small number of size_ts crept in the codebase. The
vast majority of structs can still use size_t.
> 
> These issues are things I'm particularly aware of as I've been spending a
great
> deal of time over the past couple of years trying to fix precisely these
issues
> in our codebase, and almost invariably, more consistent use of size_t for all
> purposes for which it's suited results in simpler, less buggy code, which also
> generates fewer warnings.  This also allows us to be consistent about
> encouraging (explicitly, or implicitly as people read existing coding
patterns)
> the use of size_t in such cases.
> 
> I would be happy to chat with any concerned/invested parties further about all
> this stuff; I've probably had my head in the size_t issues more than anyone
> except possibly Justin, so hopefully I can be helpful.

If it helps, fyi Justin and I had discussed this specific issue in depth and he
agreed with removing size_ts.

[jam, 2016-02-09 04:46:11]

On 2016/02/09 04:36:28, jam wrote:
> On 2016/02/09 02:19:15, Peter Kasting wrote:
> > On 2016/02/09 01:05:26, jabdelmalek wrote:
> > > On 2016/02/09 00:52:38, Peter Kasting wrote:
> > > > Not LGTM
> > > > 
> > > > We should have methods already to safely send size_ts over IPC (which
> force
> > it
> > > > to 64 bits regardless of the native size of either end).  And the actual
> API
> > > of
> > > > Range definitely should be using size_t and not uint32_t from a
conceptual
> > > point
> > > > of view -- things like range lengths are sizes.
> > > 
> > > Hey Peter, there's been a lot of discussion on the best way to solve this
> bug
> > > over the last few weeks. The consensus was that the safest way was to
remove
> > all
> > > size_ts and long from IPC messages, and structs that are sent over IPC.
> 581409
> > > captures a bunch most of that discussion (but not all, some of it was in
> > person
> > > with Dmitry, Justin and Tom.
> > > 
> > > The problem with size_ts is that when used in a struct, they are typede'd
to
> > > long or int depending on the architecture. The IPC macros don't know what
> the
> > > original intent is. We considered sending 64 bit integers for numbers
> always,
> > > but then this fails because you don't know whent o enforce overflow checks
> > > because of things like npos (and others places that are sending -2, -3
> etc...
> > > cast as size_t).
> > 
> > Using signal values like -2, -3, etc. is almost always inadvisable, and we
> > should seek to fix code that does that to instead use separate fields/flags
to
> > communicate this information.  In the cases I've encountered this, this has
> > always ended up making the resulting code clearer and more difficult to
> misuse. 
> > This should be very rare in Chromium code today in any case.
> 
> Yes I agree, this is not ideal. However, the goal here was to not have silent
> failures. Trying to catch every last usage of -2 or -3 etc would be a game of
> whack-a-mole. Removing size_ts and longs and enforcing this with a clang
plugin
> gives us a compile time way of ensuring we don't have these kind of bugs
instead
> of relying on code inspection.
> 
> > 
> > If this is ensured, then npos can be special-cased, a la:
> > 
> >   size_t deserialized = (serialized == static_cast<uint64_t>(-1)) ?
> >       static_cast<size_t>(-1) : checked_cast<size_t>(serialized);
> > 
> > This should handle everything I'm aware of.
> 
> this was all discussed in person and in the bug btw.. we still decided to go
> with removing size_ts. note that this is actually not new, 6 years ago a PSA
was
> sent to chromium-dev about not using size_t in IPC anymore:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MjZye589qqE/LRdYQ...
> 
> > 
> > Removing size_ts from structs and APIs has dangers and problems that, IMO,
go
> > well beyond the above.  Concerns about introducing potential overflows are
an
> > obvious example,
> 
> this is taken care of by using checked_cast
> 
> > but using non-size_ts also obscures the meaning of certain code
> > (does this code use a uint32_t because the 32-bitness of the value is a
> critical
> > part of the API contract?),
> 
> I think this is more theoretical.. If someone wanted to see why this happened
> for these structs, it's like any code archeology.
> 
> 
> > as well as making interaction with standard
> > libraries more difficult, especially as we try to turn on more compiler
> warnings
> > about truncating conversions and the like.
> 
> checked_cast helps as per above.
> 
> Also note that this is the last of the changes to remove size_t (see bug for
> full list). The next change was to remove the size_t methods from
base::Pickle.
> Over the last 6 years, only a small number of size_ts crept in the codebase.
The
> vast majority of structs can still use size_t.
> > 
> > These issues are things I'm particularly aware of as I've been spending a
> great
> > deal of time over the past couple of years trying to fix precisely these
> issues
> > in our codebase, and almost invariably, more consistent use of size_t for
all
> > purposes for which it's suited results in simpler, less buggy code, which
also
> > generates fewer warnings.  This also allows us to be consistent about
> > encouraging (explicitly, or implicitly as people read existing coding
> patterns)
> > the use of size_t in such cases.
> > 
> > I would be happy to chat with any concerned/invested parties further about
all
> > this stuff; I've probably had my head in the size_t issues more than anyone
> > except possibly Justin, so hopefully I can be helpful.
> 
> If it helps, fyi Justin and I had discussed this specific issue in depth and
he
> agreed with removing size_ts.

to quote https://code.google.com/p/chromium/issues/detail?id=581409#c16

jschuh@chromium.org
"We discussed it for a while, and the only viable solution seems to be
eliminating the ambiguous types from the IPC layer. Even if you have the
sender's type information, special cases like npos are going to mess you up.
Better to just send unambiguous, known size values than to roll the dice."

[Peter Kasting, 2016-02-09 06:35:34]

On 2016/02/09 04:36:28, jam wrote:
> On 2016/02/09 02:19:15, Peter Kasting wrote:
> > Using signal values like -2, -3, etc. is almost always inadvisable, and we
> > should seek to fix code that does that to instead use separate fields/flags
to
> > communicate this information.  In the cases I've encountered this, this has
> > always ended up making the resulting code clearer and more difficult to
> misuse. 
> > This should be very rare in Chromium code today in any case.
> 
> Yes I agree, this is not ideal. However, the goal here was to not have silent
> failures. Trying to catch every last usage of -2 or -3 etc would be a game of
> whack-a-mole. Removing size_ts and longs and enforcing this with a clang
plugin
> gives us a compile time way of ensuring we don't have these kind of bugs
instead
> of relying on code inspection.

It's only whack a mole if there are very many uses of this.  I would be happy to
volunteer to audit all affected types and remove -2s, -3s, etc.

In general, I don't want my comments to imply an unfunded mandate -- proper
usage of size_t is something I'm passionate about and have spent a ton of time
on, and I'm very much willing to contribute further engineering effort to doing
so.

I am willing to go so far as to own this entire issue and do all the work on it.

> > If this is ensured, then npos can be special-cased, a la:
> > 
> >   size_t deserialized = (serialized == static_cast<uint64_t>(-1)) ?
> >       static_cast<size_t>(-1) : checked_cast<size_t>(serialized);
> > 
> > This should handle everything I'm aware of.
> 
> this was all discussed in person and in the bug btw.. we still decided to go
> with removing size_ts.

Right.  I can't quite say "I respectfully disagree" without making sure I've
talked with Justin to understand his concerns, but I'm deeply concerned with
this move.

As more of Chrome becomes Mojo-ized, talking over IPC will be increasingly
common.  If I were convinced that this avoidance of size_ts was restricted to a
very small number of esoteric types, it would be easier to contemplate.  But as
we see in this change, this decision is impacting types like gfx::Range that are
broadly important, e.g. to text selection models like the omnibox uses.  These
sorts of changes are not only alarming on their own, they have followon effects
in the rest of the codebase, again, especially as I try to fix the numerous
warnings (which we disable) that our code currently generates from type
mismatches.  Most developers are not yet aware of these issues since we're not
at the point of turning the warnings on, and further don't really understand the
semantics of size_t and why it means something very different from either
uint32_t or uint64_t.  The effect of these changes is going to be to (a)
decrease usage of size_t in general and (b) where (a) does not happen, increase
the number of warnings and problems with the code.

> note that this is actually not new, 6 years ago a PSA was
> sent to chromium-dev about not using size_t in IPC anymore:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MjZye589qqE/LRdYQ...

Right, but the message there was really "no unsized types" (not int, for
example) in anything over IPC -- with an implied corollary that the types sent
over IPc would be very specific serialization types.

As we can see here, that's not really the case; there are various other "not
specific to IPC" types being affected.

> > Removing size_ts from structs and APIs has dangers and problems that, IMO,
go
> > well beyond the above.  Concerns about introducing potential overflows are
an
> > obvious example,
> 
> this is taken care of by using checked_cast

Only if you checked_cast in every case a size_t is stored in a uint32_t.  The
warnings that alert you to that cause thousands of errors today and are thus
disabled.  (That's why it's taking me years to fix them.)  This will result in
introducing more over time, no matter how careful you happen to be in this
particular change.

This also has both performance and, more importantly, readability implications. 
In addition to the unappealing nature of inserting casts all over the codebase,
a checked_cast implies two things:

(1) It's theoretically possible for the source value to exceed the target size
(2) If that happens, it's a fatal error

Neither of these is necessarily obvious given a piece of code, and knowing
whether a cast is needed, and what kind, is not always trivial.  The unfortunate
effect is that normally casts get inserted in places that don't need them, and
then later those casts are read as a sign that the code needs them, which leads
to calling, refactoring etc. that code with that constraint in mind.  (Similar
to extraneous null checks.)

> > but using non-size_ts also obscures the meaning of certain code
> > (does this code use a uint32_t because the 32-bitness of the value is a
> critical
> > part of the API contract?),
> 
> I think this is more theoretical.. If someone wanted to see why this happened
> for these structs, it's like any code archeology.

When I encounter a class whose API uses a uint32_t, my reaction is not to
immediately do a recursive blame on all the uint32_ts in hopes of tracking back
to a change where the code used a previous type, so I can understand the change.
 I attempt to use the code as it is now.

My concern is not with understanding the history of how an API got to be a
certain way, it's with specific usage problems such as the ones I mention above.
 These are not theoretical concerns.  I have submitted tens of thousands of
lines of code changes so far fixing problems, not only with size_t but with our
use of varying sized types in general, and people run into numerous problems
with them.  Like const, sized types are viral; this led to, for example, the
entire WebRTC codebase using uint16_t and int16_t for purposes which, after
months of untangling, turned out not only to not require 16-bit values but be
far clearer and safer not using them.

As previously noted, due to how we currently build, I doubt there are any other
developers who have spent anywhere near the amount of time on this issue that I
have, so I am not filled with confidence that, on seeing the usage of particular
sized types, they will understand what to do with them; I frequently don't know
the right answer without specific study either.

> > as well as making interaction with standard
> > libraries more difficult, especially as we try to turn on more compiler
> warnings
> > about truncating conversions and the like.
> 
> checked_cast helps as per above.

Problems noted above.

> Also note that this is the last of the changes to remove size_t (see bug for
> full list). The next change was to remove the size_t methods from
base::Pickle.
> Over the last 6 years, only a small number of size_ts crept in the codebase.
The
> vast majority of structs can still use size_t.

I think I would like to understand more about this, in particular the long-term
likely effects of Mojo in this regard.

If we could somehow draw a line around particular classes that must be IPC-safe
-- e.g. annotate them as USED_IN_IPC or whatever, and have the compiler enforce
restrictions like "all types are sized" -- and if we could, as much as possible,
keep these to low-level serialization functions, and make the public-facing APIs
of structs like Range support normal type usage, that would likely be one
possible good end state.

I suspect this would be significantly more work than ensuring IPC can properly
and safely serialize size_t.

> > I would be happy to chat with any concerned/invested parties further about
all
> > this stuff; I've probably had my head in the size_t issues more than anyone
> > except possibly Justin, so hopefully I can be helpful.
> 
> If it helps, fyi Justin and I had discussed this specific issue in depth and
he
> agreed with removing size_ts.

Right, and I don't want to force you to rehash things pointlessly, or demand
that the world kowtow to my opinions or something.  But I just happened to be
talking to Justin last week about a conversation he had on an unrelated issue
where a decision was made that it turned out may not have been right after he
heard more detail from me, so I wouldn't mind a shot of chatting with him to see
if we can reach a point where all of us are happy.

I definitely saw Justin's comment on the bug, and I think it implies a different
understanding of the problem space than I have, so I'm willing to believe I'm
missing something here.

Let me try and come in tomorrow so I can chat with him about this, and if so
I'll update this afterwards.

[Tom Sepez, 2016-02-09 17:23:07]

Lemme give you an LGTM on the mechanics of the change, so you're not hung up
once you reach agreement.

[jam, 2016-02-09 17:31:41]

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

[jam, 2016-02-09 17:33:05]

The CQ bit was unchecked by jam@chromium.org

[commit-bot: I haz the power, 2016-02-09 17:33:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1671403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1671403002/40001

[Peter Kasting, 2016-02-09 20:07:48]

Hey John, Justin and I talked, and he said to tell you he's "officially back on
the fence".  We'd like to talk with you more before moving forward with this and
your other changes.

I would also like to note that what you're doing here is at best questionable in
the Google style guide.  While limited exceptions for unsigned sized types like
uint32_t are allowed, this would basically be acceptable for a type restricted
to serialization/marshalling, where the type's only semantic is "exactly 32 bits
over the wire".  For types used in more general-purpose contexts such as
gfx::Range, you would need to use something like int, or else have a public
Range type that uses size_t and a serialization type behind it, only for IPC,
using a sized type.

You note that you don't have time to respond to me in detail, in which case I
reiterate my willingness to take over this change and all the other work in this
series and leave you free to do better things with your time.

[Peter Kasting, 2016-02-09 23:51:31]

John and I chatted briefly, and his primary concern is authors sending a >4 GB
value from a 64-bit to 32-bit process, but doing it so rarely that catching it
at runtime might be difficult/lead to uncaught bugs.  The hope is that forcing
consumers of IPC structs to interact with sized types forces them to think about
this.

I checked into the omnibox code affected by this gfx::Range change.  There are
definitely cases this doesn't add casts to where we store size_ts into what's
now a uint32_t.  For example, OmniboxViewViews::SetForcedQuery() does this:

  const size_t start = current_text.find_first_not_of(base::kWhitespaceUTF16);
  ...
    SelectRange(gfx::Range(current_text.size(), start + 1));

Or SetWindowTextAndCaretPos(), which itself is called in a variety of places:

  // |caret_pos| is a size_t argument to the function
  const gfx::Range range(caret_pos, caret_pos);

There are more such cases in this file alone.  These worry me since if the
compiler hasn't forced you to modify them, I don't know how many there are, and
I don't know how dangerous they are.

Also, if our general pattern is to fix this sort of thing by static_casting, it
seems like we still have the problem where it's easy for an author to write code
that stuffs a >32-bit value into a 32-bit object by casting, but because the
commonly-used cast there is not a checked_cast, we no longer have the runtime
check that serializing as size_t would at least provide.

So on the one hand, this sort of rule might force people to consider an
important question up front and get it right more often.  OTOH, this might lead
to the compiler silently truncating and thus people having neither compile-time
NOR runtime warning about this; or, in cases where the compiler does request a
cast, if code commonly static_casts around such cases, sloppy authors who don't
think hard about the effects of their code may be encouraged to simply add
another static_cast, which has the same issues.

So I am torn, because while scrupulous authors + pedantic compilers would
definitely result in this change being safer than my alternative, I worry that
the world we actually have might be safer with the guaranteed-runtime check at
the IPC boundary.

[jam, 2016-02-10 00:12:48]

On 2016/02/09 23:51:31, Peter Kasting wrote:
> John and I chatted briefly, and his primary concern is authors sending a >4 GB
> value from a 64-bit to 32-bit process, but doing it so rarely that catching it
> at runtime might be difficult/lead to uncaught bugs.  The hope is that forcing
> consumers of IPC structs to interact with sized types forces them to think
about
> this.
> 
> I checked into the omnibox code affected by this gfx::Range change.  There are
> definitely cases this doesn't add casts to where we store size_ts into what's
> now a uint32_t.  For example, OmniboxViewViews::SetForcedQuery() does this:
> 
>   const size_t start = current_text.find_first_not_of(base::kWhitespaceUTF16);
>   ...
>     SelectRange(gfx::Range(current_text.size(), start + 1));
> 
> Or SetWindowTextAndCaretPos(), which itself is called in a variety of places:
> 
>   // |caret_pos| is a size_t argument to the function
>   const gfx::Range range(caret_pos, caret_pos);
> 
> There are more such cases in this file alone.  These worry me since if the
> compiler hasn't forced you to modify them, I don't know how many there are,
and
> I don't know how dangerous they are.
> 
> Also, if our general pattern is to fix this sort of thing by static_casting,
it
> seems like we still have the problem where it's easy for an author to write
code
> that stuffs a >32-bit value into a 32-bit object by casting, but because the
> commonly-used cast there is not a checked_cast, we no longer have the runtime
> check that serializing as size_t would at least provide.
> 
> So on the one hand, this sort of rule might force people to consider an
> important question up front and get it right more often.  OTOH, this might
lead
> to the compiler silently truncating and thus people having neither
compile-time
> NOR runtime warning about this; or, in cases where the compiler does request a
> cast, if code commonly static_casts around such cases, sloppy authors who
don't
> think hard about the effects of their code may be encouraged to simply add
> another static_cast, which has the same issues.
> 
> So I am torn, because while scrupulous authors + pedantic compilers would
> definitely result in this change being safer than my alternative, I worry that
> the world we actually have might be safer with the guaranteed-runtime check at
> the IPC boundary.

I'd like to add something that maybe wasn't mentioned: in picking the type to
replace size_ts with, the context is very important. i.e. in the range case, we
know that it's inconceivable that we will have a range that's > uintmax.
similarly for arrays where we send the array size first. i don't see this as a
source of bugs in practice.

similarly, in places where we send things like memory usage, we would pick
uint64_t because we know that it could be larger than uint32max.

[Peter Kasting, 2016-02-10 00:19:35]

I asked Justin to chime in here, but I don't know if he will.

As for me, I am too conflicted to make a clear recommendation for one over the
other.

However, assuming you continue on your current course, I would like to request
that you add some comments to the gfx::Range header, and for all other classes
you've modified, to the effect of "This uses uint32_t where one might expect
size_t because it is serialized over IPC.  See <...> for more information on why
IPC structs must use sized values, and how you should interact with them." 
Hopefully <...> is a comment in the IPC code somewhere or on the Chromium dev
site or wiki that can cover some of the details in the conversations we've had,
such as specific pitfalls.

This should help to prevent people from trying to convert this back to size_t in
the future (as I might otherwise try!), help avoid making people think they
should pervade non-size_t types across the codebase, and in general make for
more awareness of the restrictions here.

With that, LGTM to whatever you decide to do.

[jam, 2016-02-10 00:22:56]

On 2016/02/10 00:19:35, Peter Kasting wrote:
> I asked Justin to chime in here, but I don't know if he will.
> 
> As for me, I am too conflicted to make a clear recommendation for one over the
> other.
> 
> However, assuming you continue on your current course, I would like to request
> that you add some comments to the gfx::Range header, and for all other classes
> you've modified, to the effect of "This uses uint32_t where one might expect
> size_t because it is serialized over IPC.  See <...> for more information on
why
> IPC structs must use sized values, and how you should interact with them." 
> Hopefully <...> is a comment in the IPC code somewhere or on the Chromium dev
> site or wiki that can cover some of the details in the conversations we've
had,
> such as specific pitfalls.
> 
> This should help to prevent people from trying to convert this back to size_t
in
> the future (as I might otherwise try!), help avoid making people think they
> should pervade non-size_t types across the codebase, and in general make for
> more awareness of the restrictions here.
> 
> With that, LGTM to whatever you decide to do.

Thanks Peter. I'll add comments to all these classes.

if in the future we get some data that this strategy was not correct, I'll own
up to correcting all these classes.

[jam, 2016-02-10 00:23:08]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-02-10 00:26:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1671403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1671403002/40001

[commit-bot: I haz the power, 2016-02-10 01:49:32]

Description was changed from

==========
Switch gfx::Range to use uint32_t instead of size_t.

This is because size_t's size depends on the architecture and we send gfx::Range
over IPC. We need IPCs to match as we're now going to support 32 and 64 bit
processes communicating on Android.

This is split off from https://codereview.chromium.org/1619363002/.

BUG=581409
==========

to

==========
Switch gfx::Range to use uint32_t instead of size_t.

This is because size_t's size depends on the architecture and we send gfx::Range
over IPC. We need IPCs to match as we're now going to support 32 and 64 bit
processes communicating on Android.

This is split off from https://codereview.chromium.org/1619363002/.

BUG=581409
==========

[commit-bot: I haz the power, 2016-02-10 01:49:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-10 01:50:54]

Description was changed from

==========
Switch gfx::Range to use uint32_t instead of size_t.

This is because size_t's size depends on the architecture and we send gfx::Range
over IPC. We need IPCs to match as we're now going to support 32 and 64 bit
processes communicating on Android.

This is split off from https://codereview.chromium.org/1619363002/.

BUG=581409
==========

to

==========
Switch gfx::Range to use uint32_t instead of size_t.

This is because size_t's size depends on the architecture and we send gfx::Range
over IPC. We need IPCs to match as we're now going to support 32 and 64 bit
processes communicating on Android.

This is split off from https://codereview.chromium.org/1619363002/.

BUG=581409

Committed: https://crrev.com/1f0b47c4a982ab2076590d95937b89f892bd4552
Cr-Commit-Position: refs/heads/master@{#374568}
==========

[commit-bot: I haz the power, 2016-02-10 01:50:55]

Patchset 3 (id:??) landed as
https://crrev.com/1f0b47c4a982ab2076590d95937b89f892bd4552
Cr-Commit-Position: refs/heads/master@{#374568}