Add compile time checks against longs being used in IPC structs on 32 bit Android.

Add compile time checks against longs being used in IPC structs on 32 bit Android.

long is 4 bytes in 32 bit builds and 8 bytes in 64 bit builds so we don't want to send it between 32 and 64 bit processes. We can't remove the long IPC traits completely since types like uint64_t also use the long traits on Windows and 64 bit POSIX. So keep the traits for these platforms but remove it for others. This ensures that longs aren't sent over IPC between 32 and 64 bit  configs since the 32 bit build would have a compile error.

Also remove the size_t methods from Pickle. We can't add compile time checks for that since it's a typedef. A clang plugin will catch those cases.

BUG=581409
Committed: https://crrev.com/03d8a78d7333f1cce537596e4513356dbe2032fc
Cr-Commit-Position: refs/heads/master@{#374707}

[jam, 2016-01-22 15:20:44]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
==========

to

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jam, 2016-01-22 16:42:15]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
==========

[jam, 2016-01-22 20:23:35]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
==========

to

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jam, 2016-01-22 21:39:04]

Patchset #2 (id:20001) has been deleted

[jam, 2016-01-22 21:58:02]

Patchset #2 (id:30031) has been deleted

[jam, 2016-01-22 21:58:09]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
==========

[Dmitry Skiba, 2016-01-22 23:36:04]

dskiba@google.com changed reviewers:
+ dskiba@google.com

[Dmitry Skiba, 2016-01-22 23:36:04]

BTW, I commented out ParamTraits specializations for "long" and "unsigned long"
(which are as bad as size_t) and was able to find several places where they are
used, which are not listed here.

I think that we need to kill those specialization, along with 
WriteLongUsingDangerousNonPortableLessPersistableForm() method.

[Dmitry Skiba, 2016-01-22 23:40:16]

dskiba@google.com changed reviewers:
- dskiba@google.com

[Dmitry Skiba, 2016-01-22 23:40:17]

^ Please see comment above.

[jam, 2016-01-25 00:19:38]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
==========

to

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jam, 2016-01-25 01:24:32]

Patchset #3 (id:70001) has been deleted

[jam, 2016-01-25 17:41:05]

Patchset #3 (id:90001) has been deleted

[jam, 2016-01-25 17:41:55]

Patchset #3 (id:90001) has been deleted

[jam, 2016-01-25 17:53:21]

Patchset #3 (id:110001) has been deleted

[jam, 2016-01-26 17:39:21]

Patchset #3 (id:130001) has been deleted

[jam, 2016-01-26 19:02:13]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

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

[jam, 2016-01-27 16:14:08]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

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

to

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-01-27 16:14:09]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jam, 2016-01-27 16:14:41]

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

[commit-bot: I haz the power, 2016-01-27 16:15:19]

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

[jam, 2016-01-27 16:17:45]

Patchset #5 (id:190001) has been deleted

[jam, 2016-01-27 16:18:03]

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

[commit-bot: I haz the power, 2016-01-27 16:18:45]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2016-01-27 16:18:53]

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

[jam, 2016-01-27 17:14:18]

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

[commit-bot: I haz the power, 2016-01-27 17:15:45]

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

[jam, 2016-01-27 17:30:14]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix 64 bit browser connecting to 32 bit browser.

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

The fixes mainly involve removing size_t from any structure that is sent over
IPC, since it's 4 bytes in 32 bit builds and 8 bytes in 64 bit builds.

Additionally, we have to be able to send longs even though they are sized
differently, since types like uint64_t also use the long traits. This change
adds checked_cast to ensure that we don't have overflow when reading long. If
it's triggered, that'll inform us that the given parameter should be changed to
use uint64_t/int64_t.

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

[jam, 2016-01-27 17:30:15]

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

[jam, 2016-01-27 17:31:47]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit browser.

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

The fixes mainly involve removing size_t from any structure that is sent over
IPC, since it's 4 bytes in 32 bit builds and 8 bytes in 64 bit builds.

Additionally, we have to be able to send longs even though they are sized
differently, since types like uint64_t also use the long traits. This change
adds checked_cast to ensure that we don't have overflow when reading long. If
it's triggered, that'll inform us that the given parameter should be changed to
use uint64_t/int64_t.

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

to

==========
Fix 64 bit browser connecting to 32 bit child processes and vice versa.

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

The fixes mainly involve removing size_t from any structure that is sent over
IPC, since it's 4 bytes in 32 bit builds and 8 bytes in 64 bit builds.

Additionally, we have to be able to send longs even though they are sized
differently, since types like uint64_t also use the long traits. This change
adds checked_cast to ensure that we don't have overflow when reading long. If
it's triggered, that'll inform us that the given parameter should be changed to
use uint64_t/int64_t.

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

[jam, 2016-01-27 17:36:19]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit child processes and vice versa.

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

The fixes mainly involve removing size_t from any structure that is sent over
IPC, since it's 4 bytes in 32 bit builds and 8 bytes in 64 bit builds.

Additionally, we have to be able to send longs even though they are sized
differently, since types like uint64_t also use the long traits. This change
adds checked_cast to ensure that we don't have overflow when reading long. If
it's triggered, that'll inform us that the given parameter should be changed to
use uint64_t/int64_t.

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

to

==========
Fix 64 bit browser connecting to 32 bit child processes and vice versa.

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

The fixes mainly involve removing size_t from any structure that is sent over
IPC, since it's 4 bytes in 32 bit builds and 8 bytes in 64 bit builds.

Additionally, we have to be able to send longs even though they are sized
differently, since types like uint64_t also use the long traits. This change
adds checked_cast to ensure that we don't have overflow when reading long. If
it's triggered, that'll inform us that the given parameter should be changed to
use uint64_t/int64_t.

BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-01-27 17:36:20]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit child processes and vice versa.

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

The fixes mainly involve removing size_t from any structure that is sent over
IPC, since it's 4 bytes in 32 bit builds and 8 bytes in 64 bit builds.

Additionally, we have to be able to send longs even though they are sized
differently, since types like uint64_t also use the long traits. This change
adds checked_cast to ensure that we don't have overflow when reading long. If
it's triggered, that'll inform us that the given parameter should be changed to
use uint64_t/int64_t.

BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix 64 bit browser connecting to 32 bit child processes and vice versa.

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

The fixes mainly involve removing size_t from any structure that is sent over
IPC, since it's 4 bytes in 32 bit builds and 8 bytes in 64 bit builds.

Additionally, we have to be able to send longs even though they are sized
differently, since types like uint64_t also use the long traits. This change
adds checked_cast to ensure that we don't have overflow when reading long. If
it's triggered, that'll inform us that the given parameter should be changed to
use uint64_t/int64_t.

BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jam, 2016-01-27 17:36:26]

Patchset #6 (id:230001) has been deleted

[jam, 2016-01-27 17:36:34]

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

[commit-bot: I haz the power, 2016-01-27 17:37:14]

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

[jam, 2016-01-27 17:38:19]

Dana: base & cc
Tom: everything else

I'm still awaiting final confirmation that this is a mode we will definitely
support. But until then, I figured it's good to have this reviewed and be ready
to land. Once we get confirmation:
1) land this cl
2) send PSA to chromium-dev about ensuring not using size_t's
3) send email to security reviewers to add this to list of things to check for
when looking at IPC files
4) Dmitry will work on clang plugin to automatically catch new instances of
size_t

[Tom Sepez, 2016-01-27 18:23:21]

Its going to take me some time to prove to myself that none of the new
uint32_t's overflow in obnoxious ways.  In the mean time, I've one comment:

https://codereview.chromium.org/1619363002/diff/250001/base/pickle.cc
File base/pickle.cc (right):

https://codereview.chromium.org/1619363002/diff/250001/base/pickle.cc#newcode100
base/pickle.cc:100: *result = base::checked_cast<long>(result_int64);
Maybe we return FALSE if the value is too big to fit rather than faulting.  We
could then handle this case along the lines of any other failure to deserialize.

https://codereview.chromium.org/1619363002/diff/250001/base/pickle.cc#newcode127
base/pickle.cc:127: // parameter to use uint64_t.
Same here.

[jam, 2016-01-27 18:42:55]

https://codereview.chromium.org/1619363002/diff/250001/base/pickle.cc
File base/pickle.cc (right):

https://codereview.chromium.org/1619363002/diff/250001/base/pickle.cc#newcode100
base/pickle.cc:100: *result = base::checked_cast<long>(result_int64);
On 2016/01/27 18:23:20, Tom Sepez wrote:
> Maybe we return FALSE if the value is too big to fit rather than faulting.  We
> could then handle this case along the lines of any other failure to
deserialize.

good question: i wanted to avoid this because it leads to silent failures. i.e.
the processes might not crash, but a feature now isn't working correctly. it's
not the fault of developers, since it's not clear that when they sent a long (or
other types that are typedef'd to it) that there's a 32<>64 bit case where
overflow might happen. In that case, getting a crash (hopefully on our bots, but
if not, then on canary/dev/beta and hopefully not stable!) will be a good signal
for us to fix the code to use a type that won't overflow between 32 and 64 bits.

[commit-bot: I haz the power, 2016-01-27 18:48:13]

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

[commit-bot: I haz the power, 2016-01-27 18:48:14]

Dry run: This issue passed the CQ dry run.

[jam, 2016-01-28 17:15:21]

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

[commit-bot: I haz the power, 2016-01-28 17:16:22]

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

[commit-bot: I haz the power, 2016-01-28 19:18:42]

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

[commit-bot: I haz the power, 2016-01-28 19:18:43]

Dry run: This issue passed the CQ dry run.

[jam, 2016-02-04 04:57:17]

Patchset #10 (id:330001) has been deleted

[jam, 2016-02-04 05:14:51]

Patchset #10 (id:350001) has been deleted

[jam, 2016-02-04 05:29:58]

Patchset #10 (id:370001) has been deleted

[jam, 2016-02-04 05:30:10]

Patchset #9 (id:310001) has been deleted

[jam, 2016-02-04 21:32:07]

Patchset #15 (id:510001) has been deleted

[jam, 2016-02-04 21:32:19]

Patchset #14 (id:490001) has been deleted

[jam, 2016-02-04 21:33:05]

Patchset #13 (id:470001) has been deleted

[jam, 2016-02-04 21:33:24]

Patchset #12 (id:450001) has been deleted

[jam, 2016-02-04 21:33:26]

Patchset #11 (id:430001) has been deleted

[jam, 2016-02-04 21:33:30]

Patchset #10 (id:410001) has been deleted

[jam, 2016-02-04 21:33:36]

Patchset #9 (id:390001) has been deleted

[jam, 2016-02-05 20:53:55]

Patchset #16 (id:670001) has been deleted

[jam, 2016-02-05 20:53:57]

Patchset #15 (id:650001) has been deleted

[Peter Kasting, 2016-02-09 00:56:30]

The concerns I expressed on https://codereview.chromium.org/1671403002/ also
apply here.  Please do not remove size_ts from the places you're removing them
-- they should stay as size_ts conceptually.

We should already have methods to safely send size_ts over IPC, as I've
definitely serialized such things before.  These methods should force the size_t
to 64 bits and then checked_cast to a size_t on the receiving end.  If somehow
these don't exist (got removed, I'm misremembering, etc.) the right thing to do
is to add such methods so that size_ts are automatically serialized safely.

[jabdelmalek, 2016-02-09 01:00:47]

On 2016/02/09 00:56:30, Peter Kasting wrote:
> The concerns I expressed on https://codereview.chromium.org/1671403002/ also
> apply here.  Please do not remove size_ts from the places you're removing them
> -- they should stay as size_ts conceptually.
> 
> We should already have methods to safely send size_ts over IPC, as I've
> definitely serialized such things before.  These methods should force the
size_t
> to 64 bits and then checked_cast to a size_t on the receiving end.  If somehow
> these don't exist (got removed, I'm misremembering, etc.) the right thing to
do
> is to add such methods so that size_ts are automatically serialized safely.

let's have the conversation in only one place, so I'll comment there.

[jam, 2016-02-10 02:40:54]

Patchset #16 (id:710001) has been deleted

[jam, 2016-02-10 16:19:06]

Description was changed from

==========
Fix 64 bit browser connecting to 32 bit child processes and vice versa.

To test, build 32 & 64 bit Chrome and launch 64 bit binary with
--browser-subprocess-path=path/to/32/bit/chrome

The fixes mainly involve removing size_t from any structure that is sent over
IPC, since it's 4 bytes in 32 bit builds and 8 bytes in 64 bit builds.

Additionally, we have to be able to send longs even though they are sized
differently, since types like uint64_t also use the long traits. This change
adds checked_cast to ensure that we don't have overflow when reading long. If
it's triggered, that'll inform us that the given parameter should be changed to
use uint64_t/int64_t.

BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add compile time checks against longs being used in IPC structs on 32 bit
Android.

long is 4 bytes in 32 bit builds and 8 bytes in 64 bit builds so we don't want
to send it between 32 and 64 bit processes. We can't remove the long IPC traits
completely since types like uint64_t also use the long traits on Windows and 64
bit POSIX. So keep the traits for these platforms but remove it for others. This
ensures that longs aren't sent over IPC between 32 and 64 bit  configs since the
32 bit build would have a compile error.

Also remove the size_t methods from Pickle. We can't add compile time checks for
that since it's a typedef. A clang plugin will catch those cases.

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

[jam, 2016-02-10 16:19:06]

jam@chromium.org changed reviewers:
- danakj@chromium.org, pkasting@chromium.org

[jam, 2016-02-10 16:22:58]

Patchset #17 (id:750001) has been deleted

[jam, 2016-02-10 16:42:59]

Patchset #17 (id:770001) has been deleted

[jam, 2016-02-10 16:54:04]

Patchset #17 (id:790001) has been deleted

[jam, 2016-02-10 17:02:14]

Tom: ptal. This change is now reduced a lot, since I moved the size_t and long
removal to other cls that have landed.

[Tom Sepez, 2016-02-10 18:00:07]

lgtm

https://codereview.chromium.org/1619363002/diff/810001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/1619363002/diff/810001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:196: #if defined(OS_WIN) || defined(OS_LINUX) ||
defined(ARCH_CPU_ARM64)
nit: you might want to define a IPC_ALLOW_LONG here for the given OSes, then use
#ifdef IPC_ALLOW_LONG both here and in the CC file, so that if the OS list
changes, we only have to update one place.

[jam, 2016-02-10 18:19:22]

The CQ bit was checked by jam@chromium.org

[jam, 2016-02-10 18:19:41]

thanks for all these reviews

https://codereview.chromium.org/1619363002/diff/810001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/1619363002/diff/810001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:196: #if defined(OS_WIN) || defined(OS_LINUX) ||
defined(ARCH_CPU_ARM64)
On 2016/02/10 18:00:07, Tom Sepez wrote:
> nit: you might want to define a IPC_ALLOW_LONG here for the given OSes, then
use
> #ifdef IPC_ALLOW_LONG both here and in the CC file, so that if the OS list
> changes, we only have to update one place.

i think in practice this won't really change. if it turns out to be a headache
though, I can do this

[commit-bot: I haz the power, 2016-02-10 18:21:10]

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

[commit-bot: I haz the power, 2016-02-10 20:13:52]

Description was changed from

==========
Add compile time checks against longs being used in IPC structs on 32 bit
Android.

long is 4 bytes in 32 bit builds and 8 bytes in 64 bit builds so we don't want
to send it between 32 and 64 bit processes. We can't remove the long IPC traits
completely since types like uint64_t also use the long traits on Windows and 64
bit POSIX. So keep the traits for these platforms but remove it for others. This
ensures that longs aren't sent over IPC between 32 and 64 bit  configs since the
32 bit build would have a compile error.

Also remove the size_t methods from Pickle. We can't add compile time checks for
that since it's a typedef. A clang plugin will catch those cases.

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

to

==========
Add compile time checks against longs being used in IPC structs on 32 bit
Android.

long is 4 bytes in 32 bit builds and 8 bytes in 64 bit builds so we don't want
to send it between 32 and 64 bit processes. We can't remove the long IPC traits
completely since types like uint64_t also use the long traits on Windows and 64
bit POSIX. So keep the traits for these platforms but remove it for others. This
ensures that longs aren't sent over IPC between 32 and 64 bit  configs since the
32 bit build would have a compile error.

Also remove the size_t methods from Pickle. We can't add compile time checks for
that since it's a typedef. A clang plugin will catch those cases.

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

[commit-bot: I haz the power, 2016-02-10 20:13:53]

Committed patchset #17 (id:810001)

[hartmanng, 2016-02-10 22:29:37]

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

[hartmanng, 2016-02-10 22:29:38]

I believe this broke Android x64 Builder (dbg)

https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde...
- specifically
https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde...


[1299/35202] CXX obj/components/webcrypto/webcrypto/util.o
FAILED: /b/build/goma/gomacc
../../third_party/android_tools/ndk/toolchains/x86_64-4.9/prebuilt/linux-x86_64/bin/x86_64-linux-android-g++
-MMD -MF obj/components/tracing/tracing/tracing_messages.o.d
-DTRACING_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2
-DENABLE_NOTIFICATIONS -DENABLE_BROWSER_CDMS -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1
-DDONT_EMBED_BUILD_METADATA -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC
-DENABLE_WEBRTC=1 -DDISABLE_NACL -DENABLE_CONFIGURATION_POLICY
-DENABLE_SUPERVISED_USERS=1 -DENABLE_AUTOFILL_DIALOG=1 -DUSE_PROPRIETARY_CODECS
-DVIDEO_HOLE=1 -DSAFE_BROWSING_DB_REMOTE -DCHROMIUM_BUILD
-DENABLE_MEDIA_ROUTER=1 -DENABLE_WEBVR -DFIELDTRIAL_TESTING_ENABLED
-D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -D__GNU_SOURCE=1 -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -I../.. -Igen -fno-strict-aliasing
--param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -pipe
-ffunction-sections -fno-short-enums -finline-limit=64 -m64 -march=x86-64 -Wall
-Werror -Wno-unused-local-typedefs -Wno-missing-field-initializers
-Wno-unused-parameter -Os -fdata-sections -ffunction-sections
-fomit-frame-pointer -g1
--sysroot=../../third_party/android_tools/ndk/platforms/android-21/arch-x86_64
-fvisibility=hidden -fno-threadsafe-statics -fvisibility-inlines-hidden
-std=gnu++11 -Wno-narrowing -fno-rtti
-isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include
-isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/include
-isystem../../third_party/android_tools/ndk/sources/android/support/include
-fno-exceptions -c ../../components/tracing/tracing_messages.cc -o
obj/components/tracing/tracing/tracing_messages.o
In file included from ../../ipc/ipc_message_templates.h:16:0,
                 from ../../ipc/ipc_message_macros.h:207,
                 from ../../components/tracing/tracing_messages.h:18,
                 from ../../components/tracing/tracing_messages.cc:7:
../../ipc/ipc_message_utils.h: In instantiation of 'void
IPC::WriteParam(base::Pickle*, const P&) [with P = long unsigned int]':
../../components/tracing/tracing_messages.h:32:1:   required from here
../../ipc/ipc_message_utils.h:88:59: error: 'Write' is not a member of
'IPC::ParamTraits<long unsigned int>'
   ParamTraits<Type>::Write(m, static_cast<const Type& >(p));
                                                           ^
../../ipc/ipc_message_utils.h: In instantiation of 'bool IPC::ReadParam(const
base::Pickle*, base::PickleIterator*, P*) [with P = long unsigned int]':
../../components/tracing/tracing_messages.h:32:1:   required from here
../../ipc/ipc_message_utils.h:96:70: error: 'Read' is not a member of
'IPC::ParamTraits<long unsigned int>'
   return ParamTraits<Type>::Read(m, iter, reinterpret_cast<Type* >(p));
                                                                      ^
../../ipc/ipc_message_utils.h: In instantiation of 'void IPC::LogParam(const P&,
std::__1::string*) [with P = long unsigned int; std::__1::string =
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> >]':
../../components/tracing/tracing_messages.h:32:1:   required from here
../../ipc/ipc_message_utils.h:102:57: error: 'Log' is not a member of
'IPC::ParamTraits<long unsigned int>'
   ParamTraits<Type>::Log(static_cast<const Type& >(p), l);
                                                         ^
../../ipc/ipc_message_utils.h: In function 'bool IPC::ReadParam(const
base::Pickle*, base::PickleIterator*, P*) [with P = long unsigned int]':
../../ipc/ipc_message_utils.h:97:1: error: control reaches end of non-void
function [-Werror=return-type]
 }
 ^
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.

[jam, 2016-02-10 22:47:39]

On 2016/02/10 22:29:38, hartmanng wrote:
> I believe this broke Android x64 Builder (dbg)
> 
>
https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde...
> - specifically
>
https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde...
> 
> 
> [1299/35202] CXX obj/components/webcrypto/webcrypto/util.o
> FAILED: /b/build/goma/gomacc
>
../../third_party/android_tools/ndk/toolchains/x86_64-4.9/prebuilt/linux-x86_64/bin/x86_64-linux-android-g++
> -MMD -MF obj/components/tracing/tracing/tracing_messages.o.d
> -DTRACING_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2
> -DENABLE_NOTIFICATIONS -DENABLE_BROWSER_CDMS -DENABLE_PRINTING=1
> -DENABLE_BASIC_PRINTING=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1
> -DDONT_EMBED_BUILD_METADATA -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1
-DNO_TCMALLOC
> -DENABLE_WEBRTC=1 -DDISABLE_NACL -DENABLE_CONFIGURATION_POLICY
> -DENABLE_SUPERVISED_USERS=1 -DENABLE_AUTOFILL_DIALOG=1
-DUSE_PROPRIETARY_CODECS
> -DVIDEO_HOLE=1 -DSAFE_BROWSING_DB_REMOTE -DCHROMIUM_BUILD
> -DENABLE_MEDIA_ROUTER=1 -DENABLE_WEBVR -DFIELDTRIAL_TESTING_ENABLED
> -D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H -D__STDC_CONSTANT_MACROS
> -D__STDC_FORMAT_MACROS -D__GNU_SOURCE=1 -D_DEBUG
-DDYNAMIC_ANNOTATIONS_ENABLED=1
> -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -I../.. -Igen -fno-strict-aliasing
> --param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -pipe
> -ffunction-sections -fno-short-enums -finline-limit=64 -m64 -march=x86-64
-Wall
> -Werror -Wno-unused-local-typedefs -Wno-missing-field-initializers
> -Wno-unused-parameter -Os -fdata-sections -ffunction-sections
> -fomit-frame-pointer -g1
> --sysroot=../../third_party/android_tools/ndk/platforms/android-21/arch-x86_64
> -fvisibility=hidden -fno-threadsafe-statics -fvisibility-inlines-hidden
> -std=gnu++11 -Wno-narrowing -fno-rtti
>
-isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include
>
-isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/include
> -isystem../../third_party/android_tools/ndk/sources/android/support/include
> -fno-exceptions -c ../../components/tracing/tracing_messages.cc -o
> obj/components/tracing/tracing/tracing_messages.o
> In file included from ../../ipc/ipc_message_templates.h:16:0,
>                  from ../../ipc/ipc_message_macros.h:207,
>                  from ../../components/tracing/tracing_messages.h:18,
>                  from ../../components/tracing/tracing_messages.cc:7:
> ../../ipc/ipc_message_utils.h: In instantiation of 'void
> IPC::WriteParam(base::Pickle*, const P&) [with P = long unsigned int]':
> ../../components/tracing/tracing_messages.h:32:1:   required from here
> ../../ipc/ipc_message_utils.h:88:59: error: 'Write' is not a member of
> 'IPC::ParamTraits<long unsigned int>'
>    ParamTraits<Type>::Write(m, static_cast<const Type& >(p));
>                                                            ^
> ../../ipc/ipc_message_utils.h: In instantiation of 'bool IPC::ReadParam(const
> base::Pickle*, base::PickleIterator*, P*) [with P = long unsigned int]':
> ../../components/tracing/tracing_messages.h:32:1:   required from here
> ../../ipc/ipc_message_utils.h:96:70: error: 'Read' is not a member of
> 'IPC::ParamTraits<long unsigned int>'
>    return ParamTraits<Type>::Read(m, iter, reinterpret_cast<Type* >(p));
>                                                                       ^
> ../../ipc/ipc_message_utils.h: In instantiation of 'void IPC::LogParam(const
P&,
> std::__1::string*) [with P = long unsigned int; std::__1::string =
> std::__1::basic_string<char, std::__1::char_traits<char>,
> std::__1::allocator<char> >]':
> ../../components/tracing/tracing_messages.h:32:1:   required from here
> ../../ipc/ipc_message_utils.h:102:57: error: 'Log' is not a member of
> 'IPC::ParamTraits<long unsigned int>'
>    ParamTraits<Type>::Log(static_cast<const Type& >(p), l);
>                                                          ^
> ../../ipc/ipc_message_utils.h: In function 'bool IPC::ReadParam(const
> base::Pickle*, base::PickleIterator*, P*) [with P = long unsigned int]':
> ../../ipc/ipc_message_utils.h:97:1: error: control reaches end of non-void
> function [-Werror=return-type]
>  }
>  ^
> cc1plus: all warnings being treated as errors
> ninja: build stopped: subcommand failed.

looking

[commit-bot: I haz the power, 2016-02-16 22:30:37]

Description was changed from

==========
Add compile time checks against longs being used in IPC structs on 32 bit
Android.

long is 4 bytes in 32 bit builds and 8 bytes in 64 bit builds so we don't want
to send it between 32 and 64 bit processes. We can't remove the long IPC traits
completely since types like uint64_t also use the long traits on Windows and 64
bit POSIX. So keep the traits for these platforms but remove it for others. This
ensures that longs aren't sent over IPC between 32 and 64 bit  configs since the
32 bit build would have a compile error.

Also remove the size_t methods from Pickle. We can't add compile time checks for
that since it's a typedef. A clang plugin will catch those cases.

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

to

==========
Add compile time checks against longs being used in IPC structs on 32 bit
Android.

long is 4 bytes in 32 bit builds and 8 bytes in 64 bit builds so we don't want
to send it between 32 and 64 bit processes. We can't remove the long IPC traits
completely since types like uint64_t also use the long traits on Windows and 64
bit POSIX. So keep the traits for these platforms but remove it for others. This
ensures that longs aren't sent over IPC between 32 and 64 bit  configs since the
32 bit build would have a compile error.

Also remove the size_t methods from Pickle. We can't add compile time checks for
that since it's a typedef. A clang plugin will catch those cases.

BUG=581409

Committed: https://crrev.com/03d8a78d7333f1cce537596e4513356dbe2032fc
Cr-Commit-Position: refs/heads/master@{#374707}
==========

[commit-bot: I haz the power, 2016-02-16 22:30:38]

Patchset 17 (id:??) landed as
https://crrev.com/03d8a78d7333f1cce537596e4513356dbe2032fc
Cr-Commit-Position: refs/heads/master@{#374707}