Mojo: Fix wrapping of invalid SharedMemoryHandles.

Mojo: Fix wrapping of invalid SharedMemoryHandles.

When wrapping an invalid SharedMemoryHandle, we should always
just return a null ScopedSharedBufferHandle. Currently we return
a non-null ScopedSharedBufferHandle which wraps an invalid platform
handle. This will always fail to serialize and is generally bad.

Also fixes DiscardableSharedMemoryManager mojom to allow a null
shared buffer handle in the response to
AllocateLockedDiscardableSharedMemory. The existing client implementation
already handles the null case, so no additional code changes are required

BUG=674406
Committed: https://crrev.com/c11d6d1b41e00fe1e2e756ddbb601daa019a45cc
Cr-Commit-Position: refs/heads/master@{#438955}

[Ken Rockot(use gerrit already), 2016-12-15 19:09:34]

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

[commit-bot: I haz the power, 2016-12-15 19:09:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-12-15 19:10:19]

rockot@chromium.org changed reviewers:
+ tsepez@chromium.org, yzshen@chromium.org

[Ken Rockot(use gerrit already), 2016-12-15 19:10:20]

Yuzhu could you please look at //mojo, Tom at the mojom? Thanks!

[commit-bot: I haz the power, 2016-12-15 19:15:06]

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

[commit-bot: I haz the power, 2016-12-15 19:15:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ken Rockot(use gerrit already), 2016-12-15 20:15:26]

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

[commit-bot: I haz the power, 2016-12-15 20:16:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 20:21:06]

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

[commit-bot: I haz the power, 2016-12-15 20:21:07]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[yzshen1, 2016-12-15 20:26:43]

https://codereview.chromium.org/2583583002/diff/20001/mojo/public/cpp/system/...
File mojo/public/cpp/system/platform_handle.cc (right):

https://codereview.chromium.org/2583583002/diff/20001/mojo/public/cpp/system/...
mojo/public/cpp/system/platform_handle.cc:70: if (memory_handle ==
base::kInvalidPlatformFile)
They are not compatible for comparison?

[Ken Rockot(use gerrit already), 2016-12-15 20:29:12]

On POSIX, memory_handle is a PlatformFile (sadface)

On Thu, Dec 15, 2016 at 12:26 PM, <yzshen@chromium.org> wrote:

>
> https://codereview.chromium.org/2583583002/diff/20001/
> mojo/public/cpp/system/platform_handle.cc
> File mojo/public/cpp/system/platform_handle.cc (right):
>
> https://codereview.chromium.org/2583583002/diff/20001/
> mojo/public/cpp/system/platform_handle.cc#newcode70
> mojo/public/cpp/system/platform_handle.cc:70: if (memory_handle ==
> base::kInvalidPlatformFile)
> They are not compatible for comparison?
>
> https://codereview.chromium.org/2583583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-12-15 20:30:21]

Oh, oops - missing a .fd

On Thu, Dec 15, 2016 at 12:29 PM, Ken Rockot <rockot@chromium.org> wrote:

> On POSIX, memory_handle is a PlatformFile (sadface)
>
> On Thu, Dec 15, 2016 at 12:26 PM, <yzshen@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/2583583002/diff/20001/mojo/
>> public/cpp/system/platform_handle.cc
>> File mojo/public/cpp/system/platform_handle.cc (right):
>>
>> https://codereview.chromium.org/2583583002/diff/20001/mojo/
>> public/cpp/system/platform_handle.cc#newcode70
>> mojo/public/cpp/system/platform_handle.cc:70: if (memory_handle ==
>> base::kInvalidPlatformFile)
>> They are not compatible for comparison?
>>
>> https://codereview.chromium.org/2583583002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-12-15 20:31:04]

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

[Ken Rockot(use gerrit already), 2016-12-15 20:31:14]

Derp. It's a FileDescriptor not a PlatformFile. Fixed :)

On Thu, Dec 15, 2016 at 12:30 PM, Ken Rockot <rockot@chromium.org> wrote:

> Oh, oops - missing a .fd
>
> On Thu, Dec 15, 2016 at 12:29 PM, Ken Rockot <rockot@chromium.org> wrote:
>
>> On POSIX, memory_handle is a PlatformFile (sadface)
>>
>> On Thu, Dec 15, 2016 at 12:26 PM, <yzshen@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.org/2583583002/diff/20001/mojo/p
>>> ublic/cpp/system/platform_handle.cc
>>> File mojo/public/cpp/system/platform_handle.cc (right):
>>>
>>> https://codereview.chromium.org/2583583002/diff/20001/mojo/p
>>> ublic/cpp/system/platform_handle.cc#newcode70
>>> mojo/public/cpp/system/platform_handle.cc:70: if (memory_handle ==
>>> base::kInvalidPlatformFile)
>>> They are not compatible for comparison?
>>>
>>> https://codereview.chromium.org/2583583002/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-12-15 20:31:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-12-15 20:32:17]

LGTM if the bots are happy!

[Tom Sepez, 2016-12-15 21:04:17]

lgtm

[Ken Rockot(use gerrit already), 2016-12-15 21:05:28]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-12-15 21:06:37]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-12-15 21:07:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 22:16:58]

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

[commit-bot: I haz the power, 2016-12-15 22:17:00]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-12-15 22:23:13]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-12-15 22:24:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 23:41:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481840593458540,
"parent_rev": "c56392b5d154d59b10ca6566f0e5dff1a5f9e61b", "commit_rev":
"09898e6a9d882cbbaa5de5688df5bb3253da1200"}

[commit-bot: I haz the power, 2016-12-15 23:41:50]

Description was changed from

==========
Mojo: Fix wrapping of invalid SharedMemoryHandles.

When wrapping an invalid SharedMemoryHandle, we should always
just return a null ScopedSharedBufferHandle. Currently we return
a non-null ScopedSharedBufferHandle which wraps an invalid platform
handle. This will always fail to serialize and is generally bad.

Also fixes DiscardableSharedMemoryManager mojom to allow a null
shared buffer handle in the response to
AllocateLockedDiscardableSharedMemory. The existing client implementation
already handles the null case, so no additional code changes are required

BUG=674406
==========

to

==========
Mojo: Fix wrapping of invalid SharedMemoryHandles.

When wrapping an invalid SharedMemoryHandle, we should always
just return a null ScopedSharedBufferHandle. Currently we return
a non-null ScopedSharedBufferHandle which wraps an invalid platform
handle. This will always fail to serialize and is generally bad.

Also fixes DiscardableSharedMemoryManager mojom to allow a null
shared buffer handle in the response to
AllocateLockedDiscardableSharedMemory. The existing client implementation
already handles the null case, so no additional code changes are required

BUG=674406

Review-Url: https://codereview.chromium.org/2583583002
==========

[commit-bot: I haz the power, 2016-12-15 23:41:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-15 23:45:57]

Description was changed from

==========
Mojo: Fix wrapping of invalid SharedMemoryHandles.

When wrapping an invalid SharedMemoryHandle, we should always
just return a null ScopedSharedBufferHandle. Currently we return
a non-null ScopedSharedBufferHandle which wraps an invalid platform
handle. This will always fail to serialize and is generally bad.

Also fixes DiscardableSharedMemoryManager mojom to allow a null
shared buffer handle in the response to
AllocateLockedDiscardableSharedMemory. The existing client implementation
already handles the null case, so no additional code changes are required

BUG=674406

Review-Url: https://codereview.chromium.org/2583583002
==========

to

==========
Mojo: Fix wrapping of invalid SharedMemoryHandles.

When wrapping an invalid SharedMemoryHandle, we should always
just return a null ScopedSharedBufferHandle. Currently we return
a non-null ScopedSharedBufferHandle which wraps an invalid platform
handle. This will always fail to serialize and is generally bad.

Also fixes DiscardableSharedMemoryManager mojom to allow a null
shared buffer handle in the response to
AllocateLockedDiscardableSharedMemory. The existing client implementation
already handles the null case, so no additional code changes are required

BUG=674406

Committed: https://crrev.com/c11d6d1b41e00fe1e2e756ddbb601daa019a45cc
Cr-Commit-Position: refs/heads/master@{#438955}
==========

[commit-bot: I haz the power, 2016-12-15 23:45:58]

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