Closed Bug 1271848 Opened 4 years ago Closed 4 years ago

Annotate CC faults with participant information

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 1026713
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.
Attachment #8751033 - Flags: feedback?(nfroyd)
(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 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.
Attachment #8751033 - Flags: feedback?(nfroyd) → feedback+
(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...
Bug 1183355 added the MOZ_CRASH annotation mechanism.
This avoids the NS_RUNTIMEABORT issue entirely by manually adding a crash annotation.
Attachment #8751820 - Flags: review?(bugs)
Attachment #8751030 - Attachment is obsolete: true
Attachment #8751033 - Attachment is obsolete: true
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.
Attachment #8751819 - Flags: review?(bugs) → review+
Attachment #8751820 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/5285237559da
https://hg.mozilla.org/mozilla-central/rev/79d28c84d3a9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
See Also: → 1277260
You need to log in before you can comment on or make changes to this bug.