Closed Bug 1124933 Opened 5 years ago Closed 5 years ago

Annotate crash reports when we kill the child process

Categories

(Core :: DOM: Content Processes, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
e10s m5+ ---
firefox38 --- fixed

People

(Reporter: mrbkap, Assigned: billm)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Blocks: killhard-win
Pretty sure this is the function in the parent that triggers all of these - 
http://mxr.mozilla.org/mozilla-central/search?string=KillHard
Assignee: nobody → jmathies
OS: Linux → All
(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.
Keywords: leave-open
I'd like to get a look at result codes for PContent related aborts.
Attachment #8557629 - Flags: review?(wmccloskey)
simple helpers for adding annotations to PContent aborts.
Attachment #8557890 - Attachment is obsolete: true
Attachment #8557892 - Flags: review?(wmccloskey)
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.
Attachment #8557892 - Flags: review?(wmccloskey) → review+
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.
Attachment #8557629 - Flags: review?(wmccloskey)
Attached patch killhard-reasonSplinter Review
This does what we discussed, Ben. Jim's other patch implements the "global variable" approach we talked about.
Attachment #8558199 - Flags: review?(bent.mozilla)
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.
Attachment #8558199 - Flags: review?(bent.mozilla) → review+
(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.
Assignee: jmathies → wmccloskey
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.
er, routing errors I mean
(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.
(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.
(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.
Oh, I see, it's only in top-level protocols. But yes, the text should cover that.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/7eb4e585f7f2
https://hg.mozilla.org/mozilla-central/rev/d87062815bb1
https://hg.mozilla.org/mozilla-central/rev/5fc092347f17
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.