Add a flag to dump IPC messages sent from the renderer to the browser.

Add a flag to dump IPC messages sent from the renderer to the browser.

BUG=450268
Committed: https://crrev.com/e97877bc4f3132e644c4ce88554daafcfb5a284d
Cr-Commit-Position: refs/heads/master@{#319514}

[Martin Barbella, 2015-03-03 21:11:25]

WIP. Not yet ready for review.

[Martin Barbella, 2015-03-05 18:08:31]

mbarbella@chromium.org changed reviewers:
+ inferno@chromium.org

[inferno, 2015-03-05 18:46:48]

first round of comments, will review again.

https://codereview.chromium.org/975903002/diff/100001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/975903002/diff/100001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:472:
LoadExternalIPCDumper(dump_directory));
put LoadExternalIPCDumper(dump_directory) in a local and use that.

https://codereview.chromium.org/975903002/diff/100001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/975903002/diff/100001/ipc/ipc_channel_proxy.c...
ipc/ipc_channel_proxy.cc:338: : context_(context), did_init_(false),
outgoing_message_filter_(NULL) {
each var on its own line.

https://codereview.chromium.org/975903002/diff/100001/ipc/ipc_channel_proxy.c...
ipc/ipc_channel_proxy.cc:338: : context_(context), did_init_(false),
outgoing_message_filter_(NULL) {
why not put this var inside ifdef, regular builds don't need this.

https://codereview.chromium.org/975903002/diff/100001/ipc/ipc_channel_proxy.c...
ipc/ipc_channel_proxy.cc:415: if (outgoing_message_filter())
Please add a comment explaining what its purpose is. also, this code behind
ifdef.

https://codereview.chromium.org/975903002/diff/100001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/975903002/diff/100001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:60: // Interface for a filter to be imposed on outgoing
messages which can
All code ifdefed.

https://codereview.chromium.org/975903002/diff/100001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:246: OutgoingMessageFilter* outgoing_message_filter() {
() const {

https://codereview.chromium.org/975903002/diff/100001/tools/ipc_fuzzer/dump/m...
File tools/ipc_fuzzer/dump/message_dump.cc (right):

https://codereview.chromium.org/975903002/diff/100001/tools/ipc_fuzzer/dump/m...
tools/ipc_fuzzer/dump/message_dump.cc:1: #include "base/files/file_path.h"
Missing copyright notice.

https://codereview.chromium.org/975903002/diff/100001/tools/ipc_fuzzer/dump/m...
tools/ipc_fuzzer/dump/message_dump.cc:9: #define MESSAGE_DUMP_EXPORT
__declspec(dllexport)
Can you use IPC_EXPORT or similar ?

https://codereview.chromium.org/975903002/diff/100001/tools/ipc_fuzzer/dump/m...
tools/ipc_fuzzer/dump/message_dump.cc:18: public:
2 spaces indent, not 1

https://codereview.chromium.org/975903002/diff/100001/tools/ipc_fuzzer/dump/m...
tools/ipc_fuzzer/dump/message_dump.cc:23: dump_directory_.Append(pid_string +
FILE_PATH_LITERAL(".ipcdump"));
please use a getter for dump_directory_, like dump_directory() const {

https://codereview.chromium.org/975903002/diff/100001/tools/ipc_fuzzer/dump/m...
tools/ipc_fuzzer/dump/message_dump.cc:38: MessageVector messages_;
indents issue.

[inferno, 2015-03-05 22:11:59]

https://codereview.chromium.org/975903002/diff/200001/chrome/common/external_...
File chrome/common/external_ipc_dumper.cc (right):

https://codereview.chromium.org/975903002/diff/200001/chrome/common/external_...
chrome/common/external_ipc_dumper.cc:13: const char kSetDumpDirectoryEntryName[]
= "SetDumpDirectory";
newline before this line.

https://codereview.chromium.org/975903002/diff/200001/chrome/common/external_...
chrome/common/external_ipc_dumper.cc:13: const char kSetDumpDirectoryEntryName[]
= "SetDumpDirectory";
this line should come after kFilterEntryName.

https://codereview.chromium.org/975903002/diff/200001/chrome/common/external_...
chrome/common/external_ipc_dumper.cc:15: #if defined(OS_WIN)
newline before this line.

https://codereview.chromium.org/975903002/diff/200001/chrome/common/external_...
chrome/common/external_ipc_dumper.cc:16: #define LIBRARY_NAME
FILE_PATH_LITERAL("ipc_message_dump.dll")
s/LIBRARY_NAME/IPC_MESSAGE_DUMP_MODULE/LIB/similar

https://codereview.chromium.org/975903002/diff/200001/chrome/common/external_...
chrome/common/external_ipc_dumper.cc:27: LOG(ERROR) << "PathService::Get
failed.";
Too cryptic error message. Something like "Ipc dump module not found or similar
?

https://codereview.chromium.org/975903002/diff/200001/chrome/common/external_...
chrome/common/external_ipc_dumper.cc:46:
set_directory_entry_point(dump_directory);
Should be LOG(ERROR) on failures for line 45 and line 50.

https://codereview.chromium.org/975903002/diff/200001/tools/ipc_fuzzer/dump/m...
File tools/ipc_fuzzer/dump/message_dump.cc (right):

https://codereview.chromium.org/975903002/diff/200001/tools/ipc_fuzzer/dump/m...
tools/ipc_fuzzer/dump/message_dump.cc:23:
IntToStringType(base::Process::Current().Pid());
base::Process::Current().Pid() is base::ProcessId. On windows, this is an dword
(unsigned int), so you need to use base::UIntToString16. Please also put a
comment in the #define above.

maybe s/IntToStringType/PidToStringType

[inferno, 2015-03-05 22:54:52]

lgtm with nit.

https://codereview.chromium.org/975903002/diff/220001/chrome/common/external_...
File chrome/common/external_ipc_dumper.cc (right):

https://codereview.chromium.org/975903002/diff/220001/chrome/common/external_...
chrome/common/external_ipc_dumper.cc:50: LOG(ERROR) <<
kSetDumpDirectoryEntryName
else needs { }. or better way is to follow the same "if (!" and return result on
error.

https://codereview.chromium.org/975903002/diff/220001/chrome/common/external_...
chrome/common/external_ipc_dumper.cc:58: LOG(ERROR) << kFilterEntryName << " not
exported by message dump module.";
or better way is to follow the same "if (!" and return result on

[Martin Barbella, 2015-03-06 17:42:30]

mbarbella@chromium.org changed reviewers:
+ jam@chromium.org

[Martin Barbella, 2015-03-06 17:42:31]

jam@, could you please take a look at this or suggest another reviewer?

This is based on https://codereview.chromium.org/6711024/

[jam, 2015-03-06 19:18:54]

rubberstamp lgtm, deferring to inferno's review

[Martin Barbella, 2015-03-06 19:19:37]

The CQ bit was checked by mbarbella@chromium.org

[Martin Barbella, 2015-03-06 19:19:38]

The patchset sent to the CQ was uploaded after l-g-t-m from inferno@chromium.org
Link to the patchset: https://codereview.chromium.org/975903002/#ps230001
(title: "Improve error handling in external_ipc_dumper.cc")

[commit-bot: I haz the power, 2015-03-06 19:21:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/975903002/230001

[Martin Barbella, 2015-03-06 19:41:44]

The CQ bit was unchecked by mbarbella@chromium.org

[Martin Barbella, 2015-03-06 19:50:13]

The CQ bit was checked by mbarbella@chromium.org

[Martin Barbella, 2015-03-06 19:50:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
inferno@chromium.org
Link to the patchset: https://codereview.chromium.org/975903002/#ps250001
(title: "Remove BUILD.gn changes, since the IPC fuzzer doesn't work with gn")

[commit-bot: I haz the power, 2015-03-06 19:50:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/975903002/250001

[commit-bot: I haz the power, 2015-03-06 21:51:31]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-03-06 21:52:05]

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

[inferno, 2015-03-08 18:05:55]

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