1277260 - Annotate CCGraphBuilder::DescribeRefCountedNode() crashes with ClassName()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Andrew McCreight [:mccr8]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e144a5f4c38b2a7d9a4f5d24754a8fe46c63e98b

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1277260, part 1 - Make PtrInfo into a class and mark it final.

Review commit: https://reviewboard.mozilla.org/r/180904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180904/

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1277260, part 2 - Add and use method to annotate CC crashes with a class name.

Review commit: https://reviewboard.mozilla.org/r/180906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180906/

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909379 [details]
Bug 1277260, part 1 - Make PtrInfo into a class and mark it final.

https://reviewboard.mozilla.org/r/180904/#review186128

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909380 [details]
Bug 1277260, part 2 - Add and use method to annotate CC crashes with a class name.

https://reviewboard.mozilla.org/r/180906/#review186134

::: xpcom/base/nsCycleCollector.cpp:2399
(Diff revision 1)
>  NS_IMETHODIMP_(void)
>  CCGraphBuilder::DescribeRefCountedNode(nsrefcnt aRefCount, const char* aObjName)
>  {
> -  MOZ_RELEASE_ASSERT(aRefCount != 0, "CCed refcounted object has zero refcount");
> -  MOZ_RELEASE_ASSERT(aRefCount != UINT32_MAX, "CCed refcounted object has overflowing refcount");
> +  mCurrPi->AnnotatedReleaseAssert(aRefCount != 0,
> +                                  "CCed refcounted object has zero refcount");
> +  mCurrPi->AnnotatedReleaseAssert(aRefCount != UINT32_MAX,

Hmm,  not quite sure UINT32_MAX check is the right one anymore on 64bit systems at least, but no need to change here.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8909380 [details]
Bug 1277260, part 2 - Add and use method to annotate CC crashes with a class name.

Requesting data collection approval. This just annotates a crash report with the name of a C++ class, so I don't think there is anything particularly privacy sensitive.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8909380 [details]
Bug 1277260, part 2 - Add and use method to annotate CC crashes with a class name.

Maybe another data steward can take a look.

Comment 8

[[e-mail]]

:mccr8, can you answer the 8 questions in this form here?  
https://docs.google.com/document/d/1SSn5w8DfCSkHWJS8DNTd7ya82diWRxaDUFM5aL4UDDo/edit

This looks like it will be a very fast review with answers to those 8 questions.

Comment 9

[Andrew McCreight [:mccr8]]

1) What are the motivating questions you want to answer with this data?
Which C++ classes are involved in these cycle collector crashes?

2) Why does Mozilla need answers to this question?  Are there benefits for users? Do we need this information to address business requirements?
This information may allow me to fix crashes that users are experiencing.

3) List all proposed measurements.
High-level description: C++ classes involved in cycle collector crashes, to be included in crash reports.
Measurement type: Technical data.

4) How long will this data be collected?
Forever. However, this data is only collected if the user actually crashes in this way, so if I manage to somehow fix all of these crashes (though that's unlikely) we'd stop collecting data.

5) What populations do you need to these measurements for?
All channels.

6) Please provide a general description of how you will analyze this data.
I will look at crash reports, see which C++ classes we're hitting this assertion for, and then manually audit those classes to see if I can find any bugs that would cause this crash.

7) What alternative methods did you consider to answer this question? Why were they not sufficient?
I looked at some of these crash reports, but there's not enough information to solve them. Alternative approaches would be waiting for a fuzzing test case, or a URL that hits this crash found by our web spidering, but I haven't seen anything like that.

8) Can current instrumentation answer this question?
No.

9) Where do you intend to share the results of your analysis?
I will post anything I find in bug 1277280.

Comment 10

[Andrew McCreight [:mccr8]]

For the "supplementary details", I will be monitoring crash stats, which is where the data will be recorded.

Comment 11

[[e-mail]]

r+

1) Is there documentation that describes the schema for the ultimate data set available publicly, complete and accurate? 
Yes, standard Telemetry documentation.

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism)  
Yes, users can opt out of Telemetry in the normal fashion.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?
Permanent request, though I assume :mccr8 will handle it.

4) Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? 
This is type 1.

5) Is the data collection default-on or default-off? 
Default on for crashes. 

6) Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? 
No.

7) Is the data collection covered by the existing Firefox privacy notice? If unsure: escalate to legal.
Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) 
No

Comment 12

[[e-mail]]

Comment on attachment 8909380 [details]
Bug 1277260, part 2 - Add and use method to annotate CC crashes with a class name.

https://reviewboard.mozilla.org/r/180906/#review189436

Provided the data review in response to the NI.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36de9e91851f
part 1 - Make PtrInfo into a class and mark it final. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e331b951c347
part 2 - Add and use method to annotate CC crashes with a class name. r=rweiss+418169,smaug

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/36de9e91851f
https://hg.mozilla.org/mozilla-central/rev/e331b951c347