1271848 - Annotate CC faults with participant information

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8] (PTO until 12-2)]

It turns out, as seen in bug 1266882, that we do hit cycle collector faults sometimes. We should annotate the crash report with information about the participant.

One easy way to do this is:
1. Add a virtual method on the participant that returns a string naming the class.
2. Mash that into a string and crash with it.

As a bonus, (1) will let us easily fix the poor CC logging we get for DETHs.

Comment 1

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 1 - Add ClassName() method to the CC participant.

Comment 2

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 2 - Annotate invalid refcount crashes with CC participant information.

Comment 3

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8751033 [details] [diff] [review]
part 2 - Annotate invalid refcount crashes with CC participant information.

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

Nathan, maybe you have some ideas here. I want to generate a string dynamically, then pass it to a crash function so that the crash report gets annotated. However, NS_RUNTIMEABORT does not work in the child process (bug 1259255), and MOZ_CRASH requires a string literal argument because it mashes in some other stuff around the string. Would it be reasonable to add a variant of MOZ_CRASH that doesn't take a literal argument? Or should I do something else? Fixing bug 1259255 could be a huge rathole for me, but it might also be very easy.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Nathan, maybe you have some ideas here. I want to generate a string
> dynamically, then pass it to a crash function so that the crash report gets
> annotated. However, NS_RUNTIMEABORT does not work in the child process (bug
> 1259255), and MOZ_CRASH requires a string literal argument because it mashes
> in some other stuff around the string. Would it be reasonable to add a
> variant of MOZ_CRASH that doesn't take a literal argument? Or should I do
> something else? Fixing bug 1259255 could be a huge rathole for me, but it
> might also be very easy.

Adding an internal-ish-only MOZ_CRASH that doesn't take a literal argument would be easy enough.  It's not clear to me whether we can do crash annotations from the child process generally, which is the really important bit for your purposes here: can we do that, and it's just that trying to do crash annotations from the child process within crashing IPC code doesn't work all that well, so we have to be careful there?

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8751033 [details] [diff] [review]
part 2 - Annotate invalid refcount crashes with CC participant information.

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

Whoops, should have set the appropriate flags along with commenting.

Comment 6

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> It's not clear to me whether we can do crash
> annotations from the child process generally, which is the really important
> bit for your purposes here: can we do that, and it's just that trying to do
> crash annotations from the child process within crashing IPC code doesn't
> work all that well, so we have to be careful there?

I think it works, thanks to patches people have landed, there's just the specific issue of a failure during an urgent IPC message and then you try to send a lower priority message, and it hangs. Or that's my impression from bug 1049275. It feels like we should have some NS_RUNTIMEABORT variant that's safe to call from IPC...

Comment 7

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Bug 1183355 added the MOZ_CRASH annotation mechanism.

Comment 8

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 1 - Add ClassName() method to the CC participant.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64d214392179

Comment 9

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 2 - Annotate invalid refcount crashes with CC participant information.

This avoids the NS_RUNTIMEABORT issue entirely by manually adding a crash annotation.

Comment 10

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Note that this changes the behavior so that it will no longer crash if pi->mInternalRefs == pi->mRefCount, but an earlier check should catch that so it isn't possible here.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5285237559da
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d28c84d3a9

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5285237559da
https://hg.mozilla.org/mozilla-central/rev/79d28c84d3a9