Second round of sbox changes for 64 bit port...

Secound round of sbox changes for 64 bit port
- Handling pointer sized items
- Beefing up unit tests
- Beefing up integration tests
- Enabling Process, Thread and Token IPCs
- Making validation tests compile again

BUG=27218
TEST= unit tests included


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41481

[cpu_(ooo_6.6-7.5), 2010-03-09 22:43:33]

I am not done with this CL but I want your early feedback, specially the
approach of handling the void pointer sized elements in the IPC.

[rvargas (doing something else), 2010-03-10 01:04:06]

seems fine.

http://codereview.chromium.org/757001/diff/1/9
File sandbox/src/crosscall_client.h (right):

http://codereview.chromium.org/757001/diff/1/9#newcode5
sandbox/src/crosscall_client.h:5: #ifndef SANDBOX_SRC_CROSSCALL_CLIENT_H__
nit: update (and year)

http://codereview.chromium.org/757001/diff/1/9#newcode96
sandbox/src/crosscall_client.h:96: return &t_;
Shouldn't this be return t_; ?

http://codereview.chromium.org/757001/diff/1/2
File sandbox/src/crosscall_server.cc (right):

http://codereview.chromium.org/757001/diff/1/2#newcode198
sandbox/src/crosscall_server.cc:198: *param =
*(reinterpret_cast<void**>(start));
... and the number of indirections here would need adjustment.

http://codereview.chromium.org/757001/diff/1/5
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/1/5#newcode5
sandbox/src/ipc_unittest.cc:5: #include <windows.h>
I clearly see CloseHandle() already used in this file.

http://codereview.chromium.org/757001/diff/1/5#newcode218
sandbox/src/ipc_unittest.cc:218: // Check handling for regular 32 bit integers
used in Windows.
I would actually write a separate test and leave strings alone.

http://codereview.chromium.org/757001/diff/1/5#newcode538
sandbox/src/ipc_unittest.cc:538: static const IPCCall call_one = {
Wrong indentation. I find it easier to understand if non trivial code is out of
the class declaration.

http://codereview.chromium.org/757001/diff/1/5#newcode567
sandbox/src/ipc_unittest.cc:567: return true;
Shouldn't return false?

[Nicolas Sylvain, 2010-03-10 01:13:26]

http://codereview.chromium.org/757001/diff/1/9
File sandbox/src/crosscall_client.h (right):

http://codereview.chromium.org/757001/diff/1/9#newcode103
sandbox/src/crosscall_client.h:103: return true;
return false?

http://codereview.chromium.org/757001/diff/1/9#newcode108
sandbox/src/crosscall_client.h:108: return sizeof(void*);
why not sizeof(t_) ?

http://codereview.chromium.org/757001/diff/1/5
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/1/5#newcode218
sandbox/src/ipc_unittest.cc:218: // Check handling for regular 32 bit integers
used in Windows.
On 2010/03/10 01:04:06, rvargas wrote:
> I would actually write a separate test and leave strings alone.

yeah, this test is getting big. And it seems like the same pattern comes over
and over. I wonder if it is possible to create some utility functions to help

http://codereview.chromium.org/757001/diff/1/5#newcode555
sandbox/src/ipc_unittest.cc:555: virtual bool SetupService(InterceptionManager*
manager, int service) {
unamed params?

[rvargas (doing something else), 2010-03-10 01:20:42]

http://codereview.chromium.org/757001/diff/1/5
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/1/5#newcode555
sandbox/src/ipc_unittest.cc:555: virtual bool SetupService(InterceptionManager*
manager, int service) {
On 2010/03/10 01:13:26, Nicolas Sylvain wrote:
> unamed params?

I actually prefer keeping the names... and I think lint complains otherwise.

[Nicolas Sylvain, 2010-03-10 01:38:51]

On Tue, Mar 9, 2010 at 5:20 PM, <rvargas@chromium.org> wrote:

>
> http://codereview.chromium.org/757001/diff/1/5
> File sandbox/src/ipc_unittest.cc (right):
>
> http://codereview.chromium.org/757001/diff/1/5#newcode555
> sandbox/src/ipc_unittest.cc:555: virtual bool
> SetupService(InterceptionManager* manager, int service) {
> On 2010/03/10 01:13:26, Nicolas Sylvain wrote:
>
>> unamed params?
>>
>
> I actually prefer keeping the names... and I think lint complains
> otherwise.

should we use the UNUSED_PARAM_THINGY ? Does that even compile in release?


>
>
> http://codereview.chromium.org/757001
>

[rvargas (doing something else), 2010-03-10 01:52:50]

> should we use the UNUSED_PARAM_THINGY ? Does that even compile in release?
> 

We could. We actually stopped using that (adding more) when we integrated the
code into chrome (vsprop) so we dropped the warning level from 4 to 3, and there
were no more warnings.

[cpu_(ooo_6.6-7.5), 2010-03-10 05:46:47]

Uploaded new version, I think I addressed all comments.

http://codereview.chromium.org/757001/diff/1/9
File sandbox/src/crosscall_client.h (right):

http://codereview.chromium.org/757001/diff/1/9#newcode5
sandbox/src/crosscall_client.h:5: #ifndef SANDBOX_SRC_CROSSCALL_CLIENT_H__
On 2010/03/10 01:04:06, rvargas wrote:
> nit: update (and year)

Done.

http://codereview.chromium.org/757001/diff/1/9#newcode96
sandbox/src/crosscall_client.h:96: return &t_;
On 2010/03/10 01:04:06, rvargas wrote:
> Shouldn't this be return t_; ?

No, unless we want to make a special case in some other places. It is used as
the starting address of the element.

http://codereview.chromium.org/757001/diff/1/9#newcode103
sandbox/src/crosscall_client.h:103: return true;
On 2010/03/10 01:13:26, Nicolas Sylvain wrote:
> return false?

I think is correct as is. See the specialization of the template for
CopyHelper<InOutCountedBuffer> in this same file for the counterexample.

http://codereview.chromium.org/757001/diff/1/9#newcode108
sandbox/src/crosscall_client.h:108: return sizeof(void*);
On 2010/03/10 01:13:26, Nicolas Sylvain wrote:
> why not sizeof(t_) ?

Done.

http://codereview.chromium.org/757001/diff/1/2
File sandbox/src/crosscall_server.cc (right):

http://codereview.chromium.org/757001/diff/1/2#newcode198
sandbox/src/crosscall_server.cc:198: *param =
*(reinterpret_cast<void**>(start));
On 2010/03/10 01:04:06, rvargas wrote:
> ... and the number of indirections here would need adjustment.

Yep and in at least another place. Imho I find it more logical this way.

http://codereview.chromium.org/757001/diff/1/5
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/1/5#newcode5
sandbox/src/ipc_unittest.cc:5: #include <windows.h>
On 2010/03/10 01:04:06, rvargas wrote:
> I clearly see CloseHandle() already used in this file.

Yes, it is being included via one of the sharedmem headers, I'll remove it then.

http://codereview.chromium.org/757001/diff/1/5#newcode218
sandbox/src/ipc_unittest.cc:218: // Check handling for regular 32 bit integers
used in Windows.
On 2010/03/10 01:04:06, rvargas wrote:
> I would actually write a separate test and leave strings alone.

Done.

http://codereview.chromium.org/757001/diff/1/5#newcode218
sandbox/src/ipc_unittest.cc:218: // Check handling for regular 32 bit integers
used in Windows.
On 2010/03/10 01:13:26, Nicolas Sylvain wrote:
> On 2010/03/10 01:04:06, rvargas wrote:
> > I would actually write a separate test and leave strings alone.
>

I re-factored a bunch, it reduced the size of the file by about 100 lines. I
rather not spend more time kicking this horse.

> yeah, this test is getting big. And it seems like the same pattern comes over
> and over. I wonder if it is possible to create some utility functions to help
>

[Nicolas Sylvain, 2010-03-10 18:19:20]

LGTM

[rvargas (doing something else), 2010-03-10 19:48:52]

http://codereview.chromium.org/757001/diff/1/9
File sandbox/src/crosscall_client.h (right):

http://codereview.chromium.org/757001/diff/1/9#newcode5
sandbox/src/crosscall_client.h:5: #ifndef SANDBOX_SRC_CROSSCALL_CLIENT_H__
On 2010/03/10 05:46:47, cpu wrote:
> On 2010/03/10 01:04:06, rvargas wrote:
> > nit: update (and year)
> 
> Done.

You said it was a work in progress... so I meant in every file you are touching.

http://codereview.chromium.org/757001/diff/1/9#newcode96
sandbox/src/crosscall_client.h:96: return &t_;
On 2010/03/10 05:46:47, cpu wrote:
> On 2010/03/10 01:04:06, rvargas wrote:
> > Shouldn't this be return t_; ?
> 
> No, unless we want to make a special case in some other places. It is used as
> the starting address of the element.
> 

I'm having second thoughts on the whole approach. Let's talk.

[rvargas (doing something else), 2010-03-11 20:53:35]

I'm convinced then.

http://codereview.chromium.org/757001/diff/1/5
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/1/5#newcode24
sandbox/src/ipc_unittest.cc:24: 
What do you have against empty lines? :)

http://codereview.chromium.org/757001/diff/16001/17004#newcode68
sandbox/src/ipc_unittest.cc:68: channel.channel_base = base_start;
nit: spaces around *

http://codereview.chromium.org/757001/diff/16001/17004#newcode84
sandbox/src/ipc_unittest.cc:84: EXPECT_EQ(kFreeChannel,
client_control->channels[5].state);
ditto

http://codereview.chromium.org/757001/diff/16001/17004#newcode153
sandbox/src/ipc_unittest.cc:153: 
ditto

http://codereview.chromium.org/757001/diff/16001/17004#newcode232
sandbox/src/ipc_unittest.cc:232: // Check handling for windows HANDLES.
and here (I'll stop at this one)

http://codereview.chromium.org/757001/diff/16001/17004#newcode254
sandbox/src/ipc_unittest.cc:254: ASSERT_TRUE(NULL != param_addr);
nit: sizeof(dw)?

http://codereview.chromium.org/757001/diff/16001/17004#newcode256
sandbox/src/ipc_unittest.cc:256: type = INVALID_TYPE;
you just tested this two lines ago... remove that one?

http://codereview.chromium.org/757001/diff/16001/17004#newcode257
sandbox/src/ipc_unittest.cc:257: param_addr = actual_params->GetRawParameter(1,
&param_size, &type);
nit: sizeof(dw) instead of param_size (or assert param_size)

http://codereview.chromium.org/757001/diff/16001/17004#newcode270
sandbox/src/ipc_unittest.cc:270: for (size_t ix = 0; ix !=
client_control->channels_count; ++ix) {
same here (etc)

http://codereview.chromium.org/757001/diff/16001/17004#newcode279
sandbox/src/ipc_unittest.cc:279: // First a sanity test with a well formed
parameter object.
sizeof(h)

http://codereview.chromium.org/757001/diff/16001/17004#newcode286
sandbox/src/ipc_unittest.cc:286: size_t out_size = 0;
ditto (etc)

http://codereview.chromium.org/757001/diff/16001/17004#newcode525
sandbox/src/ipc_unittest.cc:525: delete []
reinterpret_cast<char*>(client_control);
nit: sounds weird

http://codereview.chromium.org/757001/diff/16001/17004#newcode603
sandbox/src/ipc_unittest.cc:603: NULL, NULL, channel_size, NULL,
reinterpret_cast<char*>(client_control),
nit: there is a mixture of delete [] and delete[] in this file. I think the
latter is what we want. Could you fix them?

[cpu_(ooo_6.6-7.5), 2010-03-12 00:17:39]

Ok folks this is it. CL ready for review.

Some things to note:
The code you already reviewed is the same.
The new stuff is:
1- enabling IPC and policy for process, token a thread items
2- enabling the tests of #1
3- nits from rvargas
4- Some extra checks in the integration tests
5- change so validation tests compile in 64 bits as two exports of the dll were
clashing with kernel32 exports.

http://codereview.chromium.org/757001/diff/16001/17004
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/16001/17004#newcode68
sandbox/src/ipc_unittest.cc:68: IPCControl* client_control = MakeChannels(12*64,
4096, &channel_start);
On 2010/03/11 20:53:35, rvargas wrote:
> nit: spaces around *

Done.

http://codereview.chromium.org/757001/diff/16001/17004#newcode84
sandbox/src/ipc_unittest.cc:84: MakeChannels(kIPCChannelSize, 4096*2,
&base_start);
On 2010/03/11 20:53:35, rvargas wrote:
> ditto

Done.

http://codereview.chromium.org/757001/diff/16001/17004#newcode153
sandbox/src/ipc_unittest.cc:153: MakeChannels(kIPCChannelSize, 4096*4,
&base_start);
On 2010/03/11 20:53:35, rvargas wrote:
> ditto

Done.

http://codereview.chromium.org/757001/diff/16001/17004#newcode232
sandbox/src/ipc_unittest.cc:232: MakeChannels(kIPCChannelSize, 4096*4,
&base_start);
On 2010/03/11 20:53:35, rvargas wrote:
> and here (I'll stop at this one)

Done.

http://codereview.chromium.org/757001/diff/16001/17004#newcode254
sandbox/src/ipc_unittest.cc:254: EXPECT_EQ(4, param_size);
On 2010/03/11 20:53:35, rvargas wrote:
> nit: sizeof(dw)?

Done.

http://codereview.chromium.org/757001/diff/16001/17004#newcode256
sandbox/src/ipc_unittest.cc:256: ASSERT_TRUE(NULL != param_addr);
On 2010/03/11 20:53:35, rvargas wrote:
> you just tested this two lines ago... remove that one?

Done.

http://codereview.chromium.org/757001/diff/16001/17004#newcode257
sandbox/src/ipc_unittest.cc:257: EXPECT_EQ(0, memcmp(&dw, param_addr,
param_size));
On 2010/03/11 20:53:35, rvargas wrote:
> nit: sizeof(dw) instead of param_size (or assert param_size)

There are asserts of size like in lines 254 or 267, 279

http://codereview.chromium.org/757001/diff/16001/17004#newcode525
sandbox/src/ipc_unittest.cc:525: // The testing IPC dispatcher has two IPC
handlers with the same signature
On 2010/03/11 20:53:35, rvargas wrote:
> nit: sounds weird

Done.

http://codereview.chromium.org/757001/diff/16001/17004#newcode603
sandbox/src/ipc_unittest.cc:603: delete []
reinterpret_cast<char*>(client_control);
On 2010/03/11 20:53:35, rvargas wrote:
> nit: there is a mixture of delete [] and delete[] in this file. I think the
> latter is what we want. Could you fix them?

Done.

[Nicolas Sylvain, 2010-03-12 00:49:07]

http://codereview.chromium.org/757001/diff/47001/48004
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/47001/48004#newcode376
sandbox/src/ipc_unittest.cc:376: IPCControl* client_control =
MakeChannels(channel_size, 4096 * 2, &base_start);
80 chars

http://codereview.chromium.org/757001/diff/47001/48005
File sandbox/src/sharedmem_ipc_server.h (right):

http://codereview.chromium.org/757001/diff/47001/48005#newcode15
sandbox/src/sharedmem_ipc_server.h:15: #include
"testing/gtest/include/gtest/gtest_prod.h"
I don't remember seeing this include before. Can't we include like gtest.h or
something like that?

http://codereview.chromium.org/757001/diff/47001/48009
File sandbox/tests/validation_tests/commands.cc (right):

http://codereview.chromium.org/757001/diff/47001/48009#newcode90
sandbox/tests/validation_tests/commands.cc:90: SBOX_TESTS_COMMAND int
OpenProcessCmd(int argc, wchar_t **argv) {
did you actually understand why it was failing? On my local machine i fixed it
that way too... but I did not understand why.  The namespace does not work?

[rvargas (doing something else), 2010-03-12 01:06:48]

http://codereview.chromium.org/757001/diff/16001/17004
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/16001/17004#newcode257
sandbox/src/ipc_unittest.cc:257: EXPECT_EQ(0, memcmp(&dw, param_addr,
param_size));
On 2010/03/12 00:17:39, cpu wrote:
> On 2010/03/11 20:53:35, rvargas wrote:
> > nit: sizeof(dw) instead of param_size (or assert param_size)
> 
> There are asserts of size like in lines 254 or 267, 279

Those are not asserts. If they fail, you'll keep on going and this line may
crash the test.

http://codereview.chromium.org/757001/diff/47001/48012
File sandbox/src/process_thread_policy.cc (right):

http://codereview.chromium.org/757001/diff/47001/48012#newcode196
sandbox/src/process_thread_policy.cc:196: HANDLE *handle) {
and in this file.

http://codereview.chromium.org/757001/diff/47001/48016
File sandbox/src/process_thread_policy.h (right):

http://codereview.chromium.org/757001/diff/47001/48016#newcode65
sandbox/src/process_thread_policy.h:65: HANDLE *handle);
nit: Do you mind fixing this in this file :)

http://codereview.chromium.org/757001/diff/47001/48011
File sandbox/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/757001/diff/47001/48011#newcode79
sandbox/src/sandbox_policy_base.cc:79:
ipc_targets_[IPC_NTOPENPROCESSTOKENEX_TAG] = dispatcher;
I would not move this code out of the ifdef at this point. It's not like it can
run anyway if we don't have the whole table filled in (unless you also change
that code)... and it is more noise when looking at changes between cls.

http://codereview.chromium.org/757001/diff/47001/48011#newcode108
sandbox/src/sandbox_policy_base.cc:108: delete
ipc_targets_[IPC_NTOPENTHREAD_TAG];
same here

[cpu_(ooo_6.6-7.5), 2010-03-12 02:35:53]

new version, addressing the comments, please look again.

http://codereview.chromium.org/757001/diff/16001/17004
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/757001/diff/16001/17004#newcode257
sandbox/src/ipc_unittest.cc:257: EXPECT_EQ(0, memcmp(&dw, param_addr,
param_size));
On 2010/03/12 01:06:48, rvargas wrote:
> On 2010/03/12 00:17:39, cpu wrote:
> > On 2010/03/11 20:53:35, rvargas wrote:
> > > nit: sizeof(dw) instead of param_size (or assert param_size)
> > 
> > There are asserts of size like in lines 254 or 267, 279
> 
> Those are not asserts. If they fail, you'll keep on going and this line may
> crash the test.
> 
> 

Done.

http://codereview.chromium.org/757001/diff/47001/48004#newcode376
sandbox/src/ipc_unittest.cc:376: const size_t channel_size = kIPCChannelSize;
On 2010/03/12 00:49:07, Nicolas Sylvain wrote:
> 80 chars

Done.

http://codereview.chromium.org/757001/diff/47001/48012
File sandbox/src/process_thread_policy.cc (right):

http://codereview.chromium.org/757001/diff/47001/48012#newcode196
sandbox/src/process_thread_policy.cc:196: HANDLE *handle) {
On 2010/03/12 01:06:48, rvargas wrote:
> and in this file.

Done.

http://codereview.chromium.org/757001/diff/47001/48016
File sandbox/src/process_thread_policy.h (right):

http://codereview.chromium.org/757001/diff/47001/48016#newcode65
sandbox/src/process_thread_policy.h:65: HANDLE *handle);
On 2010/03/12 01:06:48, rvargas wrote:
> nit: Do you mind fixing this in this file :)

Done.

http://codereview.chromium.org/757001/diff/47001/48011
File sandbox/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/757001/diff/47001/48011#newcode79
sandbox/src/sandbox_policy_base.cc:79:
ipc_targets_[IPC_NTOPENPROCESSTOKENEX_TAG] = dispatcher;
On 2010/03/12 01:06:48, rvargas wrote:
> I would not move this code out of the ifdef at this point. It's not like it
can
> run anyway if we don't have the whole table filled in (unless you also change

It works lets keep it.
> that code)... and it is more noise when looking at changes between cls.

http://codereview.chromium.org/757001/diff/47001/48005
File sandbox/src/sharedmem_ipc_server.h (right):

http://codereview.chromium.org/757001/diff/47001/48005#newcode15
sandbox/src/sharedmem_ipc_server.h:15: #include
"testing/gtest/include/gtest/gtest_prod.h"
On 2010/03/12 00:49:07, Nicolas Sylvain wrote:
> I don't remember seeing this include before. Can't we include like gtest.h or
> something like that?

Apparently this way is the recommended way, check for example
chrome/trunk/src/base/pickle.cc

http://codereview.chromium.org/757001/diff/47001/48009
File sandbox/tests/validation_tests/commands.cc (right):

http://codereview.chromium.org/757001/diff/47001/48009#newcode90
sandbox/tests/validation_tests/commands.cc:90: SBOX_TESTS_COMMAND int
OpenProcessCmd(int argc, wchar_t **argv) {
On 2010/03/12 00:49:07, Nicolas Sylvain wrote:
> did you actually understand why it was failing? On my local machine i fixed it
> that way too... but I did not understand why.  The namespace does not work?

Yeah, so when you export, it ignores the namespace so it classes with
kernel32!OpenProcess.

[rvargas (doing something else), 2010-03-12 02:47:27]

LGTM

[Nicolas Sylvain, 2010-03-12 21:38:50]

On Thu, Mar 11, 2010 at 6:35 PM, <cpu@chromium.org> wrote:

> new version, addressing the comments, please look again.
>
>
>
>
>
> http://codereview.chromium.org/757001/diff/16001/17004
> File sandbox/src/ipc_unittest.cc (right):
>
> http://codereview.chromium.org/757001/diff/16001/17004#newcode257
> sandbox/src/ipc_unittest.cc:257: EXPECT_EQ(0, memcmp(&dw, param_addr,
> param_size));
> On 2010/03/12 01:06:48, rvargas wrote:
>
>> On 2010/03/12 00:17:39, cpu wrote:
>> > On 2010/03/11 20:53:35, rvargas wrote:
>> > > nit: sizeof(dw) instead of param_size (or assert param_size)
>> >
>> > There are asserts of size like in lines 254 or 267, 279
>>
>
>  Those are not asserts. If they fail, you'll keep on going and this
>>
> line may
>
>> crash the test.
>>
>
>
>
> Done.
>
> http://codereview.chromium.org/757001/diff/47001/48004#newcode376
> sandbox/src/ipc_unittest.cc:376: const size_t channel_size =
> kIPCChannelSize;
> On 2010/03/12 00:49:07, Nicolas Sylvain wrote:
>
>> 80 chars
>>
>
> Done.
>
>
> http://codereview.chromium.org/757001/diff/47001/48012
> File sandbox/src/process_thread_policy.cc (right):
>
> http://codereview.chromium.org/757001/diff/47001/48012#newcode196
> sandbox/src/process_thread_policy.cc:196: HANDLE *handle) {
> On 2010/03/12 01:06:48, rvargas wrote:
>
>> and in this file.
>>
>
> Done.
>
>
> http://codereview.chromium.org/757001/diff/47001/48016
> File sandbox/src/process_thread_policy.h (right):
>
> http://codereview.chromium.org/757001/diff/47001/48016#newcode65
> sandbox/src/process_thread_policy.h:65: HANDLE *handle);
> On 2010/03/12 01:06:48, rvargas wrote:
>
>> nit: Do you mind fixing this in this file :)
>>
>
> Done.
>
>
> http://codereview.chromium.org/757001/diff/47001/48011
> File sandbox/src/sandbox_policy_base.cc (right):
>
> http://codereview.chromium.org/757001/diff/47001/48011#newcode79
> sandbox/src/sandbox_policy_base.cc:79:
> ipc_targets_[IPC_NTOPENPROCESSTOKENEX_TAG] = dispatcher;
> On 2010/03/12 01:06:48, rvargas wrote:
>
>> I would not move this code out of the ifdef at this point. It's not
>>
> like it can
>
>> run anyway if we don't have the whole table filled in (unless you also
>>
> change
>
> It works lets keep it.
>
>  that code)... and it is more noise when looking at changes between
>>
> cls.
>
> http://codereview.chromium.org/757001/diff/47001/48005
> File sandbox/src/sharedmem_ipc_server.h (right):
>
> http://codereview.chromium.org/757001/diff/47001/48005#newcode15
> sandbox/src/sharedmem_ipc_server.h:15: #include
> "testing/gtest/include/gtest/gtest_prod.h"
> On 2010/03/12 00:49:07, Nicolas Sylvain wrote:
>
>> I don't remember seeing this include before. Can't we include like
>>
> gtest.h or
>
>> something like that?
>>
>
> Apparently this way is the recommended way, check for example
> chrome/trunk/src/base/pickle.cc
>
>
> http://codereview.chromium.org/757001/diff/47001/48009
> File sandbox/tests/validation_tests/commands.cc (right):
>
> http://codereview.chromium.org/757001/diff/47001/48009#newcode90
> sandbox/tests/validation_tests/commands.cc:90: SBOX_TESTS_COMMAND int
> OpenProcessCmd(int argc, wchar_t **argv) {
> On 2010/03/12 00:49:07, Nicolas Sylvain wrote:
>
>> did you actually understand why it was failing? On my local machine i
>>
> fixed it
>
>> that way too... but I did not understand why.  The namespace does not
>>
> work?
>
> Yeah, so when you export, it ignores the namespace so it classes with
> kernel32!OpenProcess.

Any idea why it was not failing before?

 i would lgtm, but then i saw that it turned the buildbot red ;) Not lgtm !

>
>
> http://codereview.chromium.org/757001
>