Add pipe implementation for OOP memory logging

Add pipe implementation for OOP memory logging

These pipes are not currently hooked up.

The sender pipe just connects to a known Windows named pipe and has a send capability. The receiver has a server that waits on Windows I/O completion ports for connections, then associates those connections with a receiver pipe.

We will investigate converting this to a Mojo EDK pipe in a second phase.

The pipes will need very different implementations per-platform, but we won't need to switch them. As a result, I put the Windows implementation in _win.* files, but didn't do any virtual base classes or anything and just named the classes without the Win suffix. In my previous implementation I did some more separation that I didn't find gave any more flexibility.

BUG=733336
Review-Url: https://codereview.chromium.org/2940003002
Cr-Commit-Position: refs/heads/master@{#479927}
Committed: https://chromium.googlesource.com/chromium/src/+/32f79a03c84b1be1ab5f1216112a1658fd864a54

[brettw, 2017-06-15 20:41:40]

Description was changed from

==========
Add sender pipes

BUG=733336
==========

to

==========
Add pipe implementation for OOP memory logging

These pipes are not currently hooked up.

The sender pipe just connects to a known Windows named pipe and has a send
capability. The receiver has a server that waits on Windows I/O completion ports
for connections, then associates those connections with a receiver pipe.

The pipes will need very different implementations per-platform, but we won't
need to switch them. As a result, I put the Windows implementation in _win.*
files, but didn't do any virtual base classes or anything and just named the
classes without the Win suffix. In my previous implementation I did some more
separation that I didn't find gave any more flexibility.

BUG=733336
==========

[brettw, 2017-06-15 20:41:40]

brettw@chromium.org changed reviewers:
+ ajwong@chromium.org, borisv@chromium.org

[brettw, 2017-06-15 20:41:58]

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

[commit-bot: I haz the power, 2017-06-15 20:42:49]

Dry run: CQ is trying da patch.

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

[brettw, 2017-06-15 20:58:33]

+some people CC

[brettw, 2017-06-15 21:09:02]

Description was changed from

==========
Add pipe implementation for OOP memory logging

These pipes are not currently hooked up.

The sender pipe just connects to a known Windows named pipe and has a send
capability. The receiver has a server that waits on Windows I/O completion ports
for connections, then associates those connections with a receiver pipe.

The pipes will need very different implementations per-platform, but we won't
need to switch them. As a result, I put the Windows implementation in _win.*
files, but didn't do any virtual base classes or anything and just named the
classes without the Win suffix. In my previous implementation I did some more
separation that I didn't find gave any more flexibility.

BUG=733336
==========

to

==========
Add pipe implementation for OOP memory logging

These pipes are not currently hooked up.

The sender pipe just connects to a known Windows named pipe and has a send
capability. The receiver has a server that waits on Windows I/O completion ports
for connections, then associates those connections with a receiver pipe.

We will investigate converting this to a Mojo EDK pipe in a second phase.

The pipes will need very different implementations per-platform, but we won't
need to switch them. As a result, I put the Windows implementation in _win.*
files, but didn't do any virtual base classes or anything and just named the
classes without the Win suffix. In my previous implementation I did some more
separation that I didn't find gave any more flexibility.

BUG=733336
==========

[awong, 2017-06-15 21:32:47]

didn't finish reviewing, but unclear I can finish before I have to go for the
week.

Do we really need refcoutned objects here? :(

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe_win.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.cc:41: bool
MemlogSenderPipe::Send(const void* data, size_t sz) {
Can we check fail that sz fits in a DWORD?

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.cc:47: DCHECK(bytes_written ==
static_cast<DWORD>(sz));
DCHECK_EQ

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_stream.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_stream.cc:10: const wchar_t kWindowsPipePrefix[]
= L"\\\\.\\pipe\\chrome_mem.";
gawd that's ugly.

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver_pipe_server_win.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.cc:20: // worked).
Cite source for this information?

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.cc:43: // process expects the
pipe to already have been created.
This is a great comment...but I don't see how this is the right place to put
this comment?  Should this be API documentation in the header explaining when
Start() be called?

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.cc:64:
PIPE_UNLIMITED_INSTANCES, kReadBufferSize, kReadBufferSize, 5000, NULL);
kBufferSize seems more appropriate if you're using the value for both
read/write.

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver_pipe_server_win.h (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.h:19: }
}  // namespace base

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.h:38:
~MemlogReceiverPipeServer();
virtual?

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.h:40: void
set_on_new_connection(NewConnectionCallback on_new_conn) {
on_new_conn -> on_new_connection

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.h:44: void Start();
Comment explaining what this does?

[awong, 2017-06-15 21:37:24]

Can you add brief class description comments?

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver_pipe_win.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_win.cc:112:
base::BindOnce(&MemlogStreamReceiver::OnStreamComplete, receiver_));
Why is this a BindOnce? I would have expected BindRepeating in the constructor?

[awong, 2017-06-15 21:41:59]

LGTM

Almost everything I have fits in nice to have. Since I'm out until monday, going
to preemptive LGTM and trust your judgement on if you need a second review.

[Boris Vidolov, 2017-06-15 21:51:00]

A couple comments for the conditional compiling for WIN

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe.h (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe.h:10: #if defined(OS_WIN)
Cam we avoid the conditional compilation by using the same pipe header and vary
only the .cc file?

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe_win.h (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.h:17: explicit
MemlogSenderPipe(const base::string16& pipe_id);
If the wideness of the characters is the concern, I have found
FilePath::StringType to be useful, avoiding the conditional (win/POSIX) header.
What are you thoughts on passing the pipes in POSIX - using integer file id?

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.h:27: HANDLE handle_;
Again, is it possible to use some higher level class from base::, or is this too
basic of functionality to do so?

[commit-bot: I haz the power, 2017-06-15 22:09:32]

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

[commit-bot: I haz the power, 2017-06-15 22:09:33]

Dry run: 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_...)

[brettw, 2017-06-15 22:12:22]

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe.h (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe.h:10: #if defined(OS_WIN)
On 2017/06/15 21:50:59, Boris Vidolov wrote:
> Cam we avoid the conditional compilation by using the same pipe header and
vary
> only the .cc file?

This is similar to a previous iteration I did that I wasn't very happy with. As
we discussed in Slack, it's hard to make decisions here without seeing what the
other implementations look like. We can tweak as those come in.

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe_win.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.cc:41: bool
MemlogSenderPipe::Send(const void* data, size_t sz) {
You asking me this made me realize that we can't call any logging code to this
function without hanging since this will be called from within malloc, and
logging mallocs (I ran into this a lot actually). I'll remove the DCHECK below.

Trying to add a check and crash here seems not that useful in practice. On
Windows we already limit the allocation size to less than this so couldn't ever
create a buffer this large. And if we do this, the worst thing that will happen
is a corrupt stream that the other end will close.

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe_win.h (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.h:17: explicit
MemlogSenderPipe(const base::string16& pipe_id);
On 2017/06/15 21:50:59, Boris Vidolov wrote:
> If the wideness of the characters is the concern, I have found
> FilePath::StringType to be useful, avoiding the conditional (win/POSIX)
header.
> What are you thoughts on passing the pipes in POSIX - using integer file id?

The Posix implementation will need an integer we parse out. We'll need to change
this around on Windows too to avoid problems with the sandbox. It's on my list
to change if we keep using native pipes.

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.h:27: HANDLE handle_;
On 2017/06/15 21:50:59, Boris Vidolov wrote:
> Again, is it possible to use some higher level class from base::, or is this
too
> basic of functionality to do so?

The main alternative is replacing with Mojo pipes which is what Albert keeps
asking about. I promised to look into this.

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver_pipe_server_win.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.cc:20: // worked).
On 2017/06/15 21:32:47, awong wrote:
> Cite source for this information?

Done.

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.cc:43: // process expects the
pipe to already have been created.
Thanks for noticing, the comment is wrong. It explains why it can't be done on a
background thread right before it posts to the background thread. I wrote that
when I thought this race was causing a problem.

I'll put a TODO in since this is suspicious, but I think this will be moot when
I change the process topology as promised (to avoid launching the browser from
the logging process).

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.cc:64:
PIPE_UNLIMITED_INSTANCES, kReadBufferSize, kReadBufferSize, 5000, NULL);
On 2017/06/15 21:32:47, awong wrote:
> kBufferSize seems more appropriate if you're using the value for both
> read/write.

Done.

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver_pipe_server_win.h (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.h:38:
~MemlogReceiverPipeServer();
On 2017/06/15 21:32:47, awong wrote:
> virtual?

This shouldn't be necessary. The whole reason base::RefCounted[ThreadSafe] has a
template argument of the current class is to permit refcounting of classes
without vtables.

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.h:40: void
set_on_new_connection(NewConnectionCallback on_new_conn) {
On 2017/06/15 21:32:47, awong wrote:
> on_new_conn -> on_new_connection

Done.

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_server_win.h:44: void Start();
On 2017/06/15 21:32:47, awong wrote:
> Comment explaining what this does?

Done.

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver_pipe_win.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver_pipe_win.cc:112:
base::BindOnce(&MemlogStreamReceiver::OnStreamComplete, receiver_));
On 2017/06/15 21:37:24, awong wrote:
> Why is this a BindOnce? I would have expected BindRepeating in the
constructor?

I don't follow. What do you mean by the constructor? This is just posting a task
to a thread.

[brettw, 2017-06-15 22:13:13]

Comments

[brettw, 2017-06-15 22:13:14]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-15 22:13:15]

The patchset sent to the CQ was uploaded after l-g-t-m from ajwong@chromium.org
Link to the patchset: https://codereview.chromium.org/2940003002/#ps40001
(title: "Comments")

[commit-bot: I haz the power, 2017-06-15 22:13:50]

CQ is trying da patch.

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

[Boris Vidolov, 2017-06-15 22:25:45]

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe_win.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.cc:32: handle_ =
::CreateFile(pipe_name.c_str(), GENERIC_READ | GENERIC_WRITE, 0,
Is it a bidirectional pipe? These are not fun, as I have found them to be error
prone. What do you think about 2 separate pipes? Maybe a control pipe from the
monitor process to tell the other process what to do and a pipe for the other
process to dump stats and errors.

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.cc:36: } while (handle_ ==
INVALID_HANDLE_VALUE);
Consider timeout. Maybe die after 30 seconds. The 'time' function could be good
enough, no fancy timeout needed.

[brettw, 2017-06-15 22:44:10]

The CQ bit was unchecked by brettw@chromium.org

[brettw, 2017-06-15 22:52:19]

Comments

[brettw, 2017-06-15 22:52:19]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-15 22:52:20]

The patchset sent to the CQ was uploaded after l-g-t-m from ajwong@chromium.org
Link to the patchset: https://codereview.chromium.org/2940003002/#ps60001
(title: "Comments")

[commit-bot: I haz the power, 2017-06-15 22:52:44]

CQ is trying da patch.

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

[brettw, 2017-06-15 22:53:10]

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe_win.cc (right):

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.cc:32: handle_ =
::CreateFile(pipe_name.c_str(), GENERIC_READ | GENERIC_WRITE, 0,
It is but I don't need that. I changed it to a single-directional pipe. The
change is also in the server when the pipe is created. The documentation implies
we should be OK as long as we specify FILE_READ_ATTRIBUTES here.

https://codereview.chromium.org/2940003002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.cc:36: } while (handle_ ==
INVALID_HANDLE_VALUE);
On 2017/06/15 22:25:45, Boris Vidolov wrote:
> Consider timeout. Maybe die after 30 seconds. The 'time' function could be
good
> enough, no fancy timeout needed.

Done.

[commit-bot: I haz the power, 2017-06-15 22:59:39]

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

[commit-bot: I haz the power, 2017-06-15 22:59:40]

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_...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Boris Vidolov, 2017-06-15 23:26:34]

lgtm

Only minor comments.

https://codereview.chromium.org/2940003002/diff/60001/chrome/common/profiling...
File chrome/common/profiling/memlog_sender_pipe_win.cc (right):

https://codereview.chromium.org/2940003002/diff/60001/chrome/common/profiling...
chrome/common/profiling/memlog_sender_pipe_win.cc:42: ::GetTickCount64() <
begin_ticks + kMaxWaitMS);
Minor optimization: you can calculate the max time outside of the loop, instead
of doing it on each loop iteration

[brettw, 2017-06-15 23:39:22]

Comment

[brettw, 2017-06-15 23:39:24]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-15 23:39:25]

The patchset sent to the CQ was uploaded after l-g-t-m from borisv@chromium.org,
ajwong@chromium.org
Link to the patchset: https://codereview.chromium.org/2940003002/#ps80001
(title: "Comment")

[commit-bot: I haz the power, 2017-06-15 23:39:45]

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, 2017-06-15 23:46:15]

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

[commit-bot: I haz the power, 2017-06-15 23:46:18]

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...)

[brettw, 2017-06-16 01:17:31]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2017-06-16 01:17:56]

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, 2017-06-16 02:30:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497575851149800,
"parent_rev": "14018bdd55e93ab2d95c1bacc7788bf9b7d97f39", "commit_rev":
"32f79a03c84b1be1ab5f1216112a1658fd864a54"}

[commit-bot: I haz the power, 2017-06-16 02:31:09]

Description was changed from

==========
Add pipe implementation for OOP memory logging

These pipes are not currently hooked up.

The sender pipe just connects to a known Windows named pipe and has a send
capability. The receiver has a server that waits on Windows I/O completion ports
for connections, then associates those connections with a receiver pipe.

We will investigate converting this to a Mojo EDK pipe in a second phase.

The pipes will need very different implementations per-platform, but we won't
need to switch them. As a result, I put the Windows implementation in _win.*
files, but didn't do any virtual base classes or anything and just named the
classes without the Win suffix. In my previous implementation I did some more
separation that I didn't find gave any more flexibility.

BUG=733336
==========

to

==========
Add pipe implementation for OOP memory logging

These pipes are not currently hooked up.

The sender pipe just connects to a known Windows named pipe and has a send
capability. The receiver has a server that waits on Windows I/O completion ports
for connections, then associates those connections with a receiver pipe.

We will investigate converting this to a Mojo EDK pipe in a second phase.

The pipes will need very different implementations per-platform, but we won't
need to switch them. As a result, I put the Windows implementation in _win.*
files, but didn't do any virtual base classes or anything and just named the
classes without the Win suffix. In my previous implementation I did some more
separation that I didn't find gave any more flexibility.

BUG=733336

Review-Url: https://codereview.chromium.org/2940003002
Cr-Commit-Position: refs/heads/master@{#479927}
Committed:
https://chromium.googlesource.com/chromium/src/+/32f79a03c84b1be1ab5f1216112a...
==========

[commit-bot: I haz the power, 2017-06-16 02:31:11]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/32f79a03c84b1be1ab5f1216112a...