ipc: Write end-to-end tests for SharedMemoryHandle brokering on Mac.

ipc: Write end-to-end tests for SharedMemoryHandle brokering on Mac.

BUG=535711
Committed: https://crrev.com/b82097cc7ecb8082b325013c25761ac39a222984
Cr-Commit-Position: refs/heads/master@{#353628}

[erikchen, 2015-10-09 22:08:23]

erikchen@chromium.org changed reviewers:
+ tsepez@chromium.org

[erikchen, 2015-10-09 22:08:24]

tsepez: Please review.

[Tom Sepez, 2015-10-09 22:38:48]

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
File ipc/attachment_broker_mac_unittest.cc (right):

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:27: const char kDataBuffer2[] = "Katy
Perry is so cool.";
nit: some may frown on this level of colloquialism and or/endorsement.  Dunno.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:151: char* buffer =
static_cast<char*>(malloc(contents.size()));
nit: scoped pointers of some flavor would help here.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:209: char* start =
static_cast<char*>(shared_memory1->memory());
You may need volatiles and memory barriers here, in theory, to get consistent
resuls.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:211: if (memcmp(shared_memory1->memory(),
shared_memory2->memory(),
Would like a better test, eg. blat a known string into memory 1, then compare
against the string from memory2.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:292: IPC::Message get_first_message() {
return messages_[0]; }
these two should probably assert !empty.

https://codereview.chromium.org/1397023002/diff/40001/ipc/ipc_test_messages.h
File ipc/ipc_test_messages.h (right):

https://codereview.chromium.org/1397023002/diff/40001/ipc/ipc_test_messages.h...
ipc/ipc_test_messages.h:20: #define IPC_MESSAGE_START IPCTestMsgStart
Do we need this redefinition (line 10)?

https://codereview.chromium.org/1397023002/diff/40001/ipc/ipc_test_messages.h...
ipc/ipc_test_messages.h:34: #endif  // defined(OS_WIN)
OS_MAXCOSX

[erikchen, 2015-10-09 23:14:44]

tsepez: Please review.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
File ipc/attachment_broker_mac_unittest.cc (right):

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:27: const char kDataBuffer2[] = "Katy
Perry is so cool.";
On 2015/10/09 22:38:47, Tom Sepez wrote:
> nit: some may frown on this level of colloquialism and or/endorsement.  Dunno.

I kept the flippancy but removed the proper nouns.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:151: char* buffer =
static_cast<char*>(malloc(contents.size()));
On 2015/10/09 22:38:47, Tom Sepez wrote:
> nit: scoped pointers of some flavor would help here.

Done.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:209: char* start =
static_cast<char*>(shared_memory1->memory());
On 2015/10/09 22:38:47, Tom Sepez wrote:
> You may need volatiles and memory barriers here, in theory, to get consistent
> resuls.

Given that the store/load are happening on the same thread, I'd imagine that
memory barriers are not required on x86. If you want, I can ask thakis@ or mark@
to take a look at this.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:211: if (memcmp(shared_memory1->memory(),
shared_memory2->memory(),
On 2015/10/09 22:38:47, Tom Sepez wrote:
> Would like a better test, eg. blat a known string into memory 1, then compare
> against the string from memory2.

Done.

https://codereview.chromium.org/1397023002/diff/40001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:292: IPC::Message get_first_message() {
return messages_[0]; }
On 2015/10/09 22:38:47, Tom Sepez wrote:
> these two should probably assert !empty.

Done.

https://codereview.chromium.org/1397023002/diff/40001/ipc/ipc_test_messages.h
File ipc/ipc_test_messages.h (right):

https://codereview.chromium.org/1397023002/diff/40001/ipc/ipc_test_messages.h...
ipc/ipc_test_messages.h:20: #define IPC_MESSAGE_START IPCTestMsgStart
On 2015/10/09 22:38:47, Tom Sepez wrote:
> Do we need this redefinition (line 10)?

The #define must happen after 
 #include "ipc/ipc_message_macros.h"

Line 10 resides in an OS_WIN block. I moved the #include and #define to the top
of the file.

https://codereview.chromium.org/1397023002/diff/40001/ipc/ipc_test_messages.h...
ipc/ipc_test_messages.h:34: #endif  // defined(OS_WIN)
On 2015/10/09 22:38:47, Tom Sepez wrote:
> OS_MAXCOSX

Done.

[Tom Sepez, 2015-10-12 15:52:22]

Almost.

https://codereview.chromium.org/1397023002/diff/80001/ipc/attachment_broker_m...
File ipc/attachment_broker_mac_unittest.cc (right):

https://codereview.chromium.org/1397023002/diff/80001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:29: const char kDataBuffer4[] = "Red
potato is best potato.";
Red potato is indeed best potato.

https://codereview.chromium.org/1397023002/diff/80001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:211: memcpy(shared_memory1->memory(),
known_string, strlen(known_string));
You'll want this memcpy to happen in the non-error case.

[erikchen, 2015-10-12 16:58:19]

tsepez: PTAL

https://codereview.chromium.org/1397023002/diff/80001/ipc/attachment_broker_m...
File ipc/attachment_broker_mac_unittest.cc (right):

https://codereview.chromium.org/1397023002/diff/80001/ipc/attachment_broker_m...
ipc/attachment_broker_mac_unittest.cc:211: memcpy(shared_memory1->memory(),
known_string, strlen(known_string));
On 2015/10/12 15:52:21, Tom Sepez wrote:
> You'll want this memcpy to happen in the non-error case.

Whoops, done.

[Tom Sepez, 2015-10-12 18:26:20]

lgtm

[erikchen, 2015-10-12 19:09:30]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-10-12 19:11:22]

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

[commit-bot: I haz the power, 2015-10-12 19:16:01]

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

[commit-bot: I haz the power, 2015-10-12 19:16:02]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  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...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2015-10-12 19:26:49]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-10-12 19:26:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1397023002/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-10-12 19:28:18]

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

[commit-bot: I haz the power, 2015-10-12 20:06:50]

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

[commit-bot: I haz the power, 2015-10-12 20:06:50]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[erikchen, 2015-10-12 20:15:16]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-10-12 20:15:16]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1397023002/#ps160001
(title: "Fix compile error on windows.")

[commit-bot: I haz the power, 2015-10-12 20:15:37]

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

[commit-bot: I haz the power, 2015-10-12 21:02:08]

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

[commit-bot: I haz the power, 2015-10-12 21:02:09]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[erikchen, 2015-10-12 21:06:54]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-10-12 21:06:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1397023002/#ps180001
(title: "More win compile errors.")

[commit-bot: I haz the power, 2015-10-12 21:08:01]

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

[commit-bot: I haz the power, 2015-10-12 21:50:41]

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

[commit-bot: I haz the power, 2015-10-12 21:50:42]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[erikchen, 2015-10-12 22:08:05]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-10-12 22:08:05]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1397023002/#ps200001
(title: "More Win compile errors.")

[commit-bot: I haz the power, 2015-10-12 22:11:02]

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

[commit-bot: I haz the power, 2015-10-12 23:28:04]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-10-12 23:29:12]

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