Replace the CreateEvent/OpenEvent patches with their Nt counterparts like NtOpenEvent and NtCreateE…

Replace the CreateEvent/OpenEvent patches with their Nt counterparts like NtOpenEvent and NtCreateEvent.

Reason being :- We patch these APIS via the Export table patch which does not work with bound imports. This results in our
patched functions never getting called.

This should fix the GPU process hang with the XP presentation path.

BUG=305815
R=cpu@chromium.org, rvargas@chromium.org, cpu

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

[cpu_(ooo_6.6-7.5), 2013-10-22 01:27:40]

My only real comment is that we need to modify SyncPolicy::CreateEventAction as
well because we need to return the NTSTATUS instead of the GetLastError

Basically the if the intercept operates at the Nt level the action needs to
operate at the Nt level.

https://codereview.chromium.org/31933005/diff/1/sandbox/win/src/sync_intercep...
File sandbox/win/src/sync_interception.cc (right):

https://codereview.chromium.org/31933005/diff/1/sandbox/win/src/sync_intercep...
sandbox/win/src/sync_interception.cc:148: event_type == NotificationEvent,
this seems to map to 'manual reset' please check that is is correct.

[ananta, 2013-10-22 05:57:10]

I agree that in general if the intercept is operating at the Nt level that the
action needs to execute at the same level. I am not sure if it is worth it for
this use case. We would probably need to open the BaseNamedObjects for the
current session first and then pass it down in the object attributes to
NtOpen/CreateEvent. In any case we are returning the original ntstatus in case
of failure which should work in this case.

WDYT?

https://codereview.chromium.org/31933005/diff/1/sandbox/win/src/sync_intercep...
File sandbox/win/src/sync_interception.cc (right):

https://codereview.chromium.org/31933005/diff/1/sandbox/win/src/sync_intercep...
sandbox/win/src/sync_interception.cc:148: event_type == NotificationEvent,
On 2013/10/22 01:27:41, cpu wrote:
> this seems to map to 'manual reset' please check that is is correct.

Yes

[ananta, 2013-10-22 21:02:26]

On 2013/10/22 05:57:10, ananta wrote:
> I agree that in general if the intercept is operating at the Nt level that the
> action needs to execute at the same level. I am not sure if it is worth it for
> this use case. We would probably need to open the BaseNamedObjects for the
> current session first and then pass it down in the object attributes to
> NtOpen/CreateEvent. In any case we are returning the original ntstatus in case
> of failure which should work in this case.
> 
> WDYT?
> 
>
https://codereview.chromium.org/31933005/diff/1/sandbox/win/src/sync_intercep...
> File sandbox/win/src/sync_interception.cc (right):
> 
>
https://codereview.chromium.org/31933005/diff/1/sandbox/win/src/sync_intercep...
> sandbox/win/src/sync_interception.cc:148: event_type == NotificationEvent,
> On 2013/10/22 01:27:41, cpu wrote:
> > this seems to map to 'manual reset' please check that is is correct.
> 
> Yes

As per our discussion changed the event sync policy to use the NtCreateEvent and
NtOpenEvent functions.
PTAL

[ananta, 2013-10-22 22:44:06]

+rvargas

[cpu_(ooo_6.6-7.5), 2013-10-22 22:56:55]

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
File sandbox/win/src/sync_policy.cc (right):

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:99: // file as specified.
comment still says "file"

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:119: NtClose(object_directory);
instead of opening and closing the object_directory lets cache it on the
GetBaseNamedObjectsDirectory function.

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:128: return STATUS_SUCCESS;
I think we need to return the original status because it could be
STATUS_OBJECT_NAME_EXISTS or something like that.

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:170: return STATUS_SUCCESS;
same here.

[ananta, 2013-10-22 23:03:36]

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
File sandbox/win/src/sync_policy.cc (right):

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:99: // file as specified.
On 2013/10/22 22:56:55, cpu wrote:
> comment still says "file"

Done.

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:119: NtClose(object_directory);
On 2013/10/22 22:56:55, cpu wrote:
> instead of opening and closing the object_directory lets cache it on the
> GetBaseNamedObjectsDirectory function.

Done.

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:128: return STATUS_SUCCESS;
On 2013/10/22 22:56:55, cpu wrote:
> I think we need to return the original status because it could be
> STATUS_OBJECT_NAME_EXISTS or something like that.

Done.

https://codereview.chromium.org/31933005/diff/360001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:170: return STATUS_SUCCESS;
On 2013/10/22 22:56:55, cpu wrote:
> same here.

Done.

[rvargas (doing something else), 2013-10-23 01:36:57]

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors.h (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
sandbox/win/src/interceptors.h:42: OPEN_EVENT_ID,
nit: invert the order

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors_64.cc (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
sandbox/win/src/interceptors_64.cc:252: SANDBOX_INTERCEPT NTSTATUS WINAPI
TargetNtOpenEvent64(
nit: follow the declaration order

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors_64.h (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
sandbox/win/src/interceptors_64.h:163: PHANDLE event_handle, DWORD
desired_access,
nit: I would argue for ACCESS_MASK here

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
File sandbox/win/src/sync_interception.cc (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.cc:22: void* memory = GetGlobalIPCMemory();
nit: pass the pointer from the caller?

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.cc:33: ResultCode code = CrossCall(ipc,
IPC_CREATEEVENT_TAG, name, manual_reset,
We should modify the IPC definition to use an int for event_type and remove
manual_reset

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.cc:40: BOOL inherit_handle,
remove the argument

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.cc:61: NTSTATUS WINAPI
TargetNtOpenEvent(NtOpenEventFunction orig_OpenEvent,
nit: follow the declaration order

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
File sandbox/win/src/sync_interception.h (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.h:39: DWORD desired_access,
nit: ACCESS_MASK

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_pol...
File sandbox/win/src/sync_policy.cc (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:118: UNICODE_STRING unicode_event_name = {0};
nit: = {};

[ananta, 2013-10-23 06:20:18]

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors.h (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
sandbox/win/src/interceptors.h:42: OPEN_EVENT_ID,
On 2013/10/23 01:36:57, rvargas wrote:
> nit: invert the order

Done.

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors_64.cc (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
sandbox/win/src/interceptors_64.cc:252: SANDBOX_INTERCEPT NTSTATUS WINAPI
TargetNtOpenEvent64(
On 2013/10/23 01:36:57, rvargas wrote:
> nit: follow the declaration order

Done.

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors_64.h (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/intercep...
sandbox/win/src/interceptors_64.h:163: PHANDLE event_handle, DWORD
desired_access,
On 2013/10/23 01:36:57, rvargas wrote:
> nit: I would argue for ACCESS_MASK here

Done.

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
File sandbox/win/src/sync_interception.cc (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.cc:22: void* memory = GetGlobalIPCMemory();
On 2013/10/23 01:36:57, rvargas wrote:
> nit: pass the pointer from the caller?

Done.

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.cc:33: ResultCode code = CrossCall(ipc,
IPC_CREATEEVENT_TAG, name, manual_reset,
On 2013/10/23 01:36:57, rvargas wrote:
> We should modify the IPC definition to use an int for event_type and remove
> manual_reset

Done.

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.cc:40: BOOL inherit_handle,
On 2013/10/23 01:36:57, rvargas wrote:
> remove the argument

Done.

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.cc:61: NTSTATUS WINAPI
TargetNtOpenEvent(NtOpenEventFunction orig_OpenEvent,
On 2013/10/23 01:36:57, rvargas wrote:
> nit: follow the declaration order

Done.

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
File sandbox/win/src/sync_interception.h (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_int...
sandbox/win/src/sync_interception.h:39: DWORD desired_access,
On 2013/10/23 01:36:57, rvargas wrote:
> nit: ACCESS_MASK

Done.

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_pol...
File sandbox/win/src/sync_policy.cc (right):

https://codereview.chromium.org/31933005/diff/120001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:118: UNICODE_STRING unicode_event_name = {0};
On 2013/10/23 01:36:57, rvargas wrote:
> nit: = {};

Done.

[rvargas (doing something else), 2013-10-23 18:07:02]

LGTM

https://codereview.chromium.org/31933005/diff/730001/sandbox/win/src/sync_pol...
File sandbox/win/src/sync_policy.cc (right):

https://codereview.chromium.org/31933005/diff/730001/sandbox/win/src/sync_pol...
sandbox/win/src/sync_policy.cc:125: &local_handle, EVENT_ALL_ACCESS,
&object_attributes,
nit: move args to the previous line

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

lgtm

[ananta, 2013-10-23 21:23:12]

Committed patchset #9 manually as r230512 (presubmit successful).

[falken, 2013-10-24 04:18:14]

On 2013/10/23 21:23:12, ananta wrote:
> Committed patchset #9 manually as r230512 (presubmit successful).

I think this causes the following tests to fail:
SyncPolicyTest.TestEvent
SyncPolicyTest.TestEventReadOnly 

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[ananta, 2013-10-24 06:25:18]

On 2013/10/24 04:18:14, falken wrote:
> On 2013/10/23 21:23:12, ananta wrote:
> > Committed patchset #9 manually as r230512 (presubmit successful).
> 
> I think this causes the following tests to fail:
> SyncPolicyTest.TestEvent
> SyncPolicyTest.TestEventReadOnly 
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...

Sorry for causing this. Will reland after fixing the tests.

Thanks
Ananta