RenderHostImpl::AddRoute - Turn a DCHECK to CHECK()

RenderHostImpl::AddRoute - Turn a DCHECK to CHECK()

This is to narrow down the cause of a crash bug and
likely to crash. Please revert and let me know
if you see this in the wild.

R=nasko@chromium.org, kareng@chromium.org
BUG=381990, 415059
Committed: https://crrev.com/40e8a27bfd3c51ca92136c796d50e3f3a0d86c04
Cr-Commit-Position: refs/heads/master@{#299591}

[nasko, 2014-10-14 18:49:33]

https://codereview.chromium.org/657613003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/657613003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:925: << "Found Routing
ID conflicts: " << routing_id;
Should we add the previous CHECK we had too? I know the probability of this
failing without actually adding a duplicate is very low, but with the number of
users running the code you never know.

Also, let's log some more data (the RPH id, OS process id), so we have more to
look at in the crash reports.

[Hajime Morrita, 2014-10-14 19:36:56]

Thanks for the review. Updated. PTAL?

https://codereview.chromium.org/657613003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/657613003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:925: << "Found Routing
ID conflicts: " << routing_id;
On 2014/10/14 18:49:33, nasko wrote:
> Should we add the previous CHECK we had too? I know the probability of this
> failing without actually adding a duplicate is very low, but with the number
of
> users running the code you never know.
I worried to get as many crashes as before but true, we need it.
Added.

> 
> Also, let's log some more data (the RPH id, OS process id), so we have more to
> look at in the crash reports.
Add PID. Do we have log meessage in our crash report?
I wasn't aware of that. Could you point me where I can see it?

If we can see the dying message, we can do more. Not in this CL
but I'm thinking about passing base::Location from the caller
to AddRoute().

[nasko, 2014-10-14 20:54:16]

https://codereview.chromium.org/657613003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/657613003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:925: << "Found Routing
ID conflicts: " << routing_id;
On 2014/10/14 19:36:55, morrita wrote:
> On 2014/10/14 18:49:33, nasko wrote:
> > Should we add the previous CHECK we had too? I know the probability of this
> > failing without actually adding a duplicate is very low, but with the number
> of
> > users running the code you never know.
> I worried to get as many crashes as before but true, we need it.
> Added.

That's fine, at least we will know the relative amount of one type of crash vs
the other type.
 
> > 
> > Also, let's log some more data (the RPH id, OS process id), so we have more
to
> > look at in the crash reports.
> Add PID. Do we have log meessage in our crash report?
> I wasn't aware of that. Could you point me where I can see it?

Unfortunately we don't. We can use debug::Alias to ensure values we want to know
are on the stack and we can examine them.

> If we can see the dying message, we can do more. Not in this CL
> but I'm thinking about passing base::Location from the caller
> to AddRoute(). 

It will be nice to add base::Location, but AddRoute is a public interface method
and changing it doesn't seem right for a temp change to collect debug stats.

[Hajime Morrita, 2014-10-14 21:55:48]

PTAL?

On 2014/10/14 20:54:16, nasko wrote:
> 
> Unfortunately we don't. We can use debug::Alias to ensure values we want to
know
> are on the stack and we can examine them.

OK, Added an Alias() hoping it comes to minidump.

> 
> > If we can see the dying message, we can do more. Not in this CL
> > but I'm thinking about passing base::Location from the caller
> > to AddRoute(). 
> 
> It will be nice to add base::Location, but AddRoute is a public interface
method
> and changing it doesn't seem right for a temp change to collect debug stats.
That's fair. Will not do this.

[nasko, 2014-10-14 22:39:23]

LGTM

[Hajime Morrita, 2014-10-14 22:43:41]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-10-14 22:45:39]

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

[commit-bot: I haz the power, 2014-10-15 00:08:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-15 00:10:27]

Patchset 3 (id:??) landed as
https://crrev.com/40e8a27bfd3c51ca92136c796d50e3f3a0d86c04
Cr-Commit-Position: refs/heads/master@{#299591}

[rmcilroy, 2014-10-15 10:25:45]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/659763002/ by rmcilroy@chromium.org.

The reason for reverting is: This broke the perf bots, details in
crbug.com/423691

BUG=423691 .