content: Change how a bad message from a child is logged.

content: Change how a bad message from a child is logged.

Instead of always logging the message ID, log a custom error string.
This is necessary for when the messages are converted to mojo messages,
where the messages do not have a corresponding ID.

BUG=643746
Review-Url: https://codereview.chromium.org/2624613003
Cr-Commit-Position: refs/heads/master@{#442790}
Committed: https://chromium.googlesource.com/chromium/src/+/73f39a6659c2a0f5f576b83cf1ccdb3ce704e4b7

[sadrul, 2017-01-10 18:59:10]

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

[commit-bot: I haz the power, 2017-01-10 18:59:46]

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

[sadrul, 2017-01-10 19:00:12]

sadrul@chromium.org changed reviewers:
+ creis@chromium.org

[sadrul, 2017-01-10 19:00:13]

Hi! I considered just removing the error log altogether, since we now also log a
stack-trace, but decided to keep the logging. I am happy to remove it if you
think that's better. PTAL. Thank you!

[sadrul, 2017-01-10 19:35:32]

On 2017/01/10 19:00:13, sadrul wrote:
> Hi! I considered just removing the error log altogether, since we now also log
a
> stack-trace, but decided to keep the logging. I am happy to remove it if you
> think that's better. PTAL. Thank you!

For reference, https://crrev.com/321598 is when the logging was first added, and
https://crrev.com/324003 is when the stack-trace was added.
https://codereview.chromium.org/2612623002/ is where I am converting chrome-ipc
messages for GPU to use mojoms, where we stop having the message-ids.

[sadrul, 2017-01-10 19:42:19]

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

[commit-bot: I haz the power, 2017-01-10 19:42:44]

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, 2017-01-10 21:28:31]

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

[commit-bot: I haz the power, 2017-01-10 21:28:32]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Charlie Reis, 2017-01-10 23:06:43]

creis@chromium.org changed reviewers:
+ wfh@chromium.org

[Charlie Reis, 2017-01-10 23:06:44]

I'm more familiar with the BadMessage::ReceivedBadMessage cases than this one. 
Do you know why this doesn't use ReceivedBadMessage?  (That has a lot more
support, like special magic signatures for the crash dumps and crash keys
recording the enum value.)

Anyway, I'm fine with this, though I agree that there doesn't seem to be a
strong need for the log statement at all, and I think it may be better to remove
it.  (Given that this is #19 on the top browser crash list for M55, I'm guessing
that it would be better to diagnose using things like stacks and crash keys than
log messages.)

LGTM.  Please get wfh@'s approval as well, in case he knows of times the log is
useful.  Thanks!

https://codereview.chromium.org/2624613003/diff/20001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2624613003/diff/20001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:388: LOG(ERROR) <<
"Terminating child process for bad IPC message:" << error;
nit: Space after colon.

https://codereview.chromium.org/2624613003/diff/20001/content/browser/browser...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/2624613003/diff/20001/content/browser/browser...
content/browser/browser_child_process_host_impl.h:95: // Terminates the process
and logs an stack trace after a bad message was
nit: s/an/a/

[Will Harris, 2017-01-10 23:17:50]

this is fine, the log is not parsed by anything I know of, and as long as the
string is still visble in the stack then in future we can parse it. lgtm

[sadrul, 2017-01-11 02:27:49]

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

[commit-bot: I haz the power, 2017-01-11 02:28:47]

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

[sadrul, 2017-01-11 02:29:13]

Addressed the nits. Sounds like it's useful to have the message. So kept that.

https://codereview.chromium.org/2624613003/diff/20001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2624613003/diff/20001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:388: LOG(ERROR) <<
"Terminating child process for bad IPC message:" << error;
On 2017/01/10 23:06:43, Charlie Reis wrote:
> nit: Space after colon.

Done.

https://codereview.chromium.org/2624613003/diff/20001/content/browser/browser...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/2624613003/diff/20001/content/browser/browser...
content/browser/browser_child_process_host_impl.h:95: // Terminates the process
and logs an stack trace after a bad message was
On 2017/01/10 23:06:43, Charlie Reis wrote:
> nit: s/an/a/

Done.

[commit-bot: I haz the power, 2017-01-11 03:33:12]

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

[commit-bot: I haz the power, 2017-01-11 03:33:12]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-01-11 03:35:09]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2017-01-11 03:35:10]

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

[commit-bot: I haz the power, 2017-01-11 03:35:44]

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-01-11 04:07:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484105709905180,
"parent_rev": "ef3843675cfd07788b1a0f12c17302a1fc01496d", "commit_rev":
"73f39a6659c2a0f5f576b83cf1ccdb3ce704e4b7"}

[commit-bot: I haz the power, 2017-01-11 04:08:32]

Description was changed from

==========
content: Change how a bad message from a child is logged.

Instead of always logging the message ID, log a custom error string.
This is necessary for when the messages are converted to mojo messages,
where the messages do not have a corresponding ID.

BUG=643746
==========

to

==========
content: Change how a bad message from a child is logged.

Instead of always logging the message ID, log a custom error string.
This is necessary for when the messages are converted to mojo messages,
where the messages do not have a corresponding ID.

BUG=643746

Review-Url: https://codereview.chromium.org/2624613003
Cr-Commit-Position: refs/heads/master@{#442790}
Committed:
https://chromium.googlesource.com/chromium/src/+/73f39a6659c2a0f5f576b83cf1cc...
==========

[commit-bot: I haz the power, 2017-01-11 04:08:33]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/73f39a6659c2a0f5f576b83cf1cc...