Refactor IPC fuzzer.

Refactor IPC fuzzer.

1. New directory structure to accomodate replay, mutate and an upcoming message_lib components.
2. Break ipc_fuzzer_main.cc into replay.cc + replay_process.[cc|h].

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

[aedla, 2013-12-05 14:43:49]

tsepez@, could you please take a look?

[Tom Sepez, 2013-12-05 19:05:49]

Nice.  Mostly nits.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/mutate/muta...
File tools/ipc_fuzzer/mutate/mutate.cc (right):

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/mutate/muta...
tools/ipc_fuzzer/mutate/mutate.cc:6: return 0;
nit: put a // TODO(aedla): Implement mutator. or similar here.
nit: return EXIT_SUCCESS, just because (include stdlib.h)

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
File tools/ipc_fuzzer/replay/replay.cc (right):

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay.cc:10: replay.OpenChannel();
nit: can OpenChannel fail?

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay.cc:13: return 0;
return EXIT_FAILURE or 1 to indicate failure.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay.cc:16: return 0;
nit: return EXIT_SUCCESS just because its more portable.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
File tools/ipc_fuzzer/replay/replay_process.cc (right):

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.cc:13: #include
"base/posix/global_descriptors.h"
Ok.  Would be nice if we didn't need posix-specific stuff, but I guess this is
conditionalized in the .gyp files, right? Yet there's an ifdef posix below. 
Maybe this needs an ifdef posix as well?

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.cc:72: break;
return false here with error.  This is the only way data < end, below, no?

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.cc:120: channel_->Send(message);
check return value.  this could fail.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
File tools/ipc_fuzzer/replay/replay_process.h (right):

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:4: 
Please put an include guard macro in this file.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:25: void Initialize(int argc, const
char **argv);
nit: probably needs to be a bool to fail execution if someone passes wacky flags
...

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:28: void OpenChannel();
Nit: probably needs to be a bool to return status.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:31: bool OpenTestcase();
Nit: // returns true on success, false otherwise.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:49: scoped_ptr<base::MemoryMappedFile>
testcase_map_;
nit: testcase_mapped_file_, otherwise the name sounds like a std::map.

[aedla, 2013-12-05 23:16:20]

Thanks for the review.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/mutate/muta...
File tools/ipc_fuzzer/mutate/mutate.cc (right):

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/mutate/muta...
tools/ipc_fuzzer/mutate/mutate.cc:6: return 0;
On 2013/12/05 19:05:49, Tom Sepez wrote:
> nit: put a // TODO(aedla): Implement mutator. or similar here.
> nit: return EXIT_SUCCESS, just because (include stdlib.h)

Done.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
File tools/ipc_fuzzer/replay/replay.cc (right):

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay.cc:10: replay.OpenChannel();
On 2013/12/05 19:05:49, Tom Sepez wrote:
> nit: can OpenChannel fail?

It can't. Well, at least for POSIX, the channel has already been set up for us
before fork. If it hasn't been set up, we can't see the error at this point.
GlobalDescriptors::Get() will DLOG(FATAL) and we'll die later in
ReplayProcess::OnChannelError().

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay.cc:13: return 0;
On 2013/12/05 19:05:49, Tom Sepez wrote:
> return EXIT_FAILURE or 1 to indicate failure.

Done.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay.cc:16: return 0;
On 2013/12/05 19:05:49, Tom Sepez wrote:
> nit: return EXIT_SUCCESS just because its more portable.

Done.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
File tools/ipc_fuzzer/replay/replay_process.cc (right):

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.cc:13: #include
"base/posix/global_descriptors.h"
On 2013/12/05 19:05:49, Tom Sepez wrote:
> Ok.  Would be nice if we didn't need posix-specific stuff, but I guess this is
> conditionalized in the .gyp files, right? Yet there's an ifdef posix below. 
> Maybe this needs an ifdef posix as well?

Yes, I made ipc_fuzzer target linux-only. I would really like to stick to POSIX.
Windows doesn't support --renderer-cmd-prefix. But even without that, I probably
won't have the time  or motivation to support Windows.

Removed the ifdef below.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.cc:72: break;
On 2013/12/05 19:05:49, Tom Sepez wrote:
> return false here with error.  This is the only way data < end, below, no?

Ah, good catch. Another way to data < end below was !message_tail. Changed that
case to directly error out as well.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.cc:120: channel_->Send(message);
On 2013/12/05 19:05:49, Tom Sepez wrote:
> check return value.  this could fail.

ChannelProxy::Send() doesn't actually return false. But yeah, that could change.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
File tools/ipc_fuzzer/replay/replay_process.h (right):

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:4: 
On 2013/12/05 19:05:49, Tom Sepez wrote:
> Please put an include guard macro in this file.

Good catch.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:25: void Initialize(int argc, const
char **argv);
On 2013/12/05 19:05:49, Tom Sepez wrote:
> nit: probably needs to be a bool to fail execution if someone passes wacky
flags
> ...

Done.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:28: void OpenChannel();
On 2013/12/05 19:05:49, Tom Sepez wrote:
> Nit: probably needs to be a bool to return status.

OpenChannel() can't really fail. Wrote a bit longer in a replay.cc comment.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:31: bool OpenTestcase();
On 2013/12/05 19:05:49, Tom Sepez wrote:
> Nit: // returns true on success, false otherwise.

Done.

https://codereview.chromium.org/106163003/diff/1/tools/ipc_fuzzer/replay/repl...
tools/ipc_fuzzer/replay/replay_process.h:49: scoped_ptr<base::MemoryMappedFile>
testcase_map_;
On 2013/12/05 19:05:49, Tom Sepez wrote:
> nit: testcase_mapped_file_, otherwise the name sounds like a std::map.

Went with mapped_testcase_.

[Tom Sepez, 2013-12-06 00:56:45]

lgtm

[commit-bot: I haz the power, 2013-12-06 10:30:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aedla@chromium.org/106163003/20001

[commit-bot: I haz the power, 2013-12-06 12:46:56]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-06 13:19:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aedla@chromium.org/106163003/20001

[commit-bot: I haz the power, 2013-12-06 14:15:56]

Change committed as 239206