Add IPC handling for INVALID_HANDLE_VALUE on Win64 builds

Add IPC handling for INVALID_HANDLE_VALUE on Win64 builds

BUG=179693
R=cpu@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185826

[jschuh, 2013-03-03 07:14:39]

This one was fun.

[piman, 2013-03-03 16:50:18]

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc#newc...
ipc/ipc_message_utils.cc:767: if (reinterpret_cast<uint32>(INVALID_HANDLE_VALUE)
== temp)
why not simply reading and writing the data of the correct size?

[jschuh, 2013-03-03 17:15:04]

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc#newc...
ipc/ipc_message_utils.cc:767: if (reinterpret_cast<uint32>(INVALID_HANDLE_VALUE)
== temp)
On 2013/03/03 16:50:18, piman wrote:
> why not simply reading and writing the data of the correct size?

32-bits is the correct size because valid handles are required to fit in the
lower 32-bits, so you can pass them between 64-bit and 32-bit processes.   The
only quirk is that INVALID_HANDLE_VALUE is a sentinel value that's not really
intended to be passed between processes. However, our architecture relies on it,
so it makes sense for us to special case it and handle the conversion ourselves.

[piman, 2013-03-03 17:31:38]

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc#newc...
ipc/ipc_message_utils.cc:767: if (reinterpret_cast<uint32>(INVALID_HANDLE_VALUE)
== temp)
On 2013/03/03 17:15:05, Justin Schuh wrote:
> On 2013/03/03 16:50:18, piman wrote:
> > why not simply reading and writing the data of the correct size?
> 
> 32-bits is the correct size because valid handles are required to fit in the
> lower 32-bits, so you can pass them between 64-bit and 32-bit processes.   The
> only quirk is that INVALID_HANDLE_VALUE is a sentinel value that's not really
> intended to be passed between processes. However, our architecture relies on
it,
> so it makes sense for us to special case it and handle the conversion
ourselves.

Do we use IPC between 32 and 64 bit processes? Do we even have mixed bit-ness
processes?

It sounds like could have a bunch of other problems when we pass around data
structures, that end up having different sizes or packed differently.

[jschuh, 2013-03-03 17:41:24]

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc#newc...
ipc/ipc_message_utils.cc:767: if (reinterpret_cast<uint32>(INVALID_HANDLE_VALUE)
== temp)
On 2013/03/03 17:31:38, piman wrote:
> On 2013/03/03 17:15:05, Justin Schuh wrote:
> > On 2013/03/03 16:50:18, piman wrote:
> > > why not simply reading and writing the data of the correct size?
> > 
> > 32-bits is the correct size because valid handles are required to fit in the
> > lower 32-bits, so you can pass them between 64-bit and 32-bit processes.  
The
> > only quirk is that INVALID_HANDLE_VALUE is a sentinel value that's not
really
> > intended to be passed between processes. However, our architecture relies on
> it,
> > so it makes sense for us to special case it and handle the conversion
> ourselves.
> 
> Do we use IPC between 32 and 64 bit processes? Do we even have mixed bit-ness
> processes?

Absolutely. We have it today with NaCl, and off the top of my head we know we'll
have it with the 32-bit plugin container and possibly with the Firefox profile
importer.


> It sounds like could have a bunch of other problems when we pass around data
> structures, that end up having different sizes or packed differently.

Well, packed structures at the IPC layer are pure evil and should really not be
allowed for many other reasons. That said, the cases where we need to
communicate from 64-bit to 32-bit are narrow enough that it won't be a big
concern.

[cpu_(ooo_6.6-7.5), 2013-03-03 19:36:24]

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/12381066/diff/1/ipc/ipc_message_utils.cc#newc...
ipc/ipc_message_utils.cc:758: m->WriteUInt32(reinterpret_cast<uint32>(p));
why not do it here instead?

[cpu_(ooo_6.6-7.5), 2013-03-03 20:08:53]

(hit enter too soon) so the the values on the wire make sense.

[cpu_(ooo_6.6-7.5), 2013-03-03 23:51:00]

I retract my previous comment. I was confused.

Check
Include\shared\basetsd.h

For the best macro for the ocassion and use that.

[jschuh, 2013-03-03 23:57:11]

Carlos always wins. Now with double the Windows macro flavor.

[cpu_(ooo_6.6-7.5), 2013-03-04 00:05:27]

lgtm

[commit-bot: I haz the power, 2013-03-04 00:11:07]

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

[commit-bot: I haz the power, 2013-03-04 00:42:12]

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

[jschuh, 2013-03-04 02:29:06]

Committed patchset #3 manually as r185826 (presubmit successful).