1124933 - Annotate crash reports when we kill the child process

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

When we kill the child process, it generates a crash report with the stack in the child but no information about the state in the parent. Bug 1110498 would help fix this for sure, but another thing we could do would be to manually annotate the crash report when we know that we're about to intentionally kill the child process and gain additional information that way.

Comment 1

[Jim Mathies [:jimm]]

Pretty sure this is the function in the parent that triggers all of these - 
http://mxr.mozilla.org/mozilla-central/search?string=KillHard

Comment 2

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #31)
> I wonder if we should change the signature for RecvFoo functions so that
> you're required to return something more explicit. It would look something
> like:
>   return Success;
> or
>   return Failure("this is why it failed");
> 
> Then we would annotate the crash dump with the failure message.
> 
> This would be a lot of grunt work, but it might be worth it given all these
> crashes.

We can cover this here. I've filed individual bugs on a few worst offenders, see the parent bug.

Comment 3

[Jim Mathies [:jimm]]

Attachment: log error Result for PContent

I'd like to get a look at result codes for PContent related aborts.

Comment 4

[Jim Mathies [:jimm]]

Attachment: Add KillHard annotation helpers

Comment 5

[Jim Mathies [:jimm]]

Attachment: Add KillHard annotation helpers

simple helpers for adding annotations to PContent aborts.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8557892 [details] [diff] [review]
Add KillHard annotation helpers

Review of attachment 8557892 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.h
@@ +243,5 @@
> +    /**
> +     * API for adding a crash reporter annotation that provides a reason
> +     * for a listener request to abort the child.
> +     */
> +    bool IsKillHardAnnotationSet() { return !!mKillHardAnnotation.Length(); }

Please use IsEmpty() on the string.

@@ +245,5 @@
> +     * for a listener request to abort the child.
> +     */
> +    bool IsKillHardAnnotationSet() { return !!mKillHardAnnotation.Length(); }
> +    const nsCString& GetKillHardAnnotation() { return mKillHardAnnotation; }
> +    void SetKillHardAnnotation(const nsCString* aReason) {

Why not use |const nsACString&|? Passing null here seems like a pretty bad idea.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8557629 [details] [diff] [review]
log error Result for PContent

Review of attachment 8557629 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this will be too helpful. I wrote a different patch that includes more information.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: killhard-reason

This does what we discussed, Ben. Jim's other patch implements the "global variable" approach we talked about.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8558199 [details] [diff] [review]
killhard-reason

Review of attachment 8558199 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.cpp
@@ +3268,5 @@
>                  NS_LITERAL_CSTRING("additional_minidumps"),
>                  additionalDumps);
> +
> +            if (aReason) {
> +                nsAutoCString reason(aReason);

Nit: nsDependentCString

::: ipc/glue/MessageChannel.cpp
@@ +1539,5 @@
>  
>      PrintErrorMessage(mSide, aChannelName, errorMsg);
>  
>      MonitorAutoUnlock unlock(*mMonitor);
> +    mListener->OnProcessingError(MsgDropped, nullptr);

Might be good to have something here so that people don't have to sometimes worry about null-checking the reason parameter.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #7)
> Comment on attachment 8557629 [details] [diff] [review]
> log error Result for PContent
> 
> Review of attachment 8557629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this will be too helpful. I wrote a different patch that
> includes more information.

I'm in need of the Result code so that I can filter routing from handler errors. In you current patch this information is getting dropped in ContentParent::ProcessingError.

Comment 11

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7eb4e585f7f2

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc092347f17

I included data about MsgDropped. I'm 99% sure we'll never see that, but it can't hurt.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

er, routing errors I mean

Comment 14

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #13)
> er, routing errors I mean

What if OnMessageReceived returns MsgRouteError? I don't see where you pick that up and report it uniquely. These errors a big chunk of the aborts we're seeing.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #14)
> (In reply to Bill McCloskey (:billm) from comment #13)
> > er, routing errors I mean
> 
> What if OnMessageReceived returns MsgRouteError? I don't see where you pick
> that up and report it uniquely. These errors a big chunk of the aborts we're
> seeing.

ctually I think this might be ok, it looks like the entire error message gets dumped into 'reason' in MaybeHandleError, and that has some text in it that differentiates the error type. I should be able to work with that.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Jim Mathies [:jimm] from comment #14)
> (In reply to Bill McCloskey (:billm) from comment #13)
> > er, routing errors I mean
> 
> What if OnMessageReceived returns MsgRouteError? I don't see where you pick
> that up and report it uniquely. These errors a big chunk of the aborts we're
> seeing.

I don't think that can happen. OnMessageReceived only appears to return MsgValueError and MsgProcessingError.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, I see, it's only in top-level protocols. But yes, the text should cover that.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7eb4e585f7f2
https://hg.mozilla.org/mozilla-central/rev/d87062815bb1
https://hg.mozilla.org/mozilla-central/rev/5fc092347f17