1250637 - Allow crash report annotation when in a child process and off main thread

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

We have a current limitation in CrashReporter::AnnotateCrashReport and CrashReporter::AppendAppNotesToCrashReport where we don't handle the child process annotation outside the main thread.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

As for the practical scenario - image decoding.

Comment 2

[Benjamin Smedberg]

Also related and may be fixed by the work aklotz is doing in bug 1236108. Aaron, can you comment?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: MozReview Request: Bug 1250637: Allow child process crash annotation off main thread. r=aklotz

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

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Initially, bug 1236108 is focusing on just-in-time annotations vs ahead-of-time annotations. By "just-in-time annotations" I am referring to annotations that are made at the time of the crash, like the size of the OOM-ing malloc or the flag that indicates whether or not we were currently in a GC.

Its WIP implementation involves sending that data over the same pipe as the rest of the crash report, meaning that we don't need to punch any additional holes in the sandbox or do any additional IPC turnarounds.

In the short term, once bug 1236108 is in place, you could probably just set up your annotations as just-in-time annotations. In the longer term, I think we'd probably want to unify the ahead-of-time and just-in-time annotations mechanism.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

This seems to still be an issue, with bug 1236108 fixed.  Do we need this patch anyway?

Comment 6

[(not currently active) Ted Mielczarek]

I think so. Sorry, Mozreview doesn't seem to have set the review flag in bugzilla, which is what I actually use to triage my review queue!

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8722619 [details]
MozReview Request: Bug 1250637: Allow child process crash annotation off main thread. r=aklotz

Didn't notice the review flag wasn't automatically set.

Comment 8

[(not currently active) Ted Mielczarek]

FYI, I'm intending to review this patch but I have to block off some time to read and understand it because threading is always hard. I will find some time tomorrow or Thursday to review it.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8722619 [details]
MozReview Request: Bug 1250637: Allow child process crash annotation off main thread. r=aklotz

I'm sorry, I haven't quite found time for this, and I'm leaving for a week of vacation. I think aklotz might be a good reviewer for this code.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8722619 [details]
MozReview Request: Bug 1250637: Allow child process crash annotation off main thread. r=aklotz

Sounds like a plan.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8722619 [details]
MozReview Request: Bug 1250637: Allow child process crash annotation off main thread. r=aklotz

https://reviewboard.mozilla.org/r/36141/#review47651

::: toolkit/crashreporter/nsExceptionHandler.cpp:1859
(Diff revision 1)
>      gDelayedAnnotations = new nsTArray<nsAutoPtr<DelayedNote> >();
>    }
>    gDelayedAnnotations->AppendElement(aNote);
>  }
>  
> +class CrashReporterHelperRunnable : public nsCancelableRunnable {

s/nsCancelableRunnable/nsRunnable/

::: toolkit/crashreporter/nsExceptionHandler.cpp:1876
(Diff revision 1)
> +    {}
> +
> +  NS_METHOD Run() override;
> +
> +private:
> +  nsAutoCString mKey;

s/nsAutoCString/nsCString/ for these member variables, please.

::: toolkit/crashreporter/nsExceptionHandler.cpp:2001
(Diff revision 1)
>    return AnnotateCrashReport(NS_LITERAL_CSTRING("Notes"), *notesField);
>  }
>  
> +nsresult CrashReporterHelperRunnable::Run()
> +{
> +  // We expect this to be in the child process' main thread.  If it isn't

Nit: please add a comma after "isn't"

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8722619 [details]
MozReview Request: Bug 1250637: Allow child process crash annotation off main thread. r=aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36141/diff/1-2/

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8722619 [details]
MozReview Request: Bug 1250637: Allow child process crash annotation off main thread. r=aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36141/diff/2-3/

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Aaron Klotz [:aklotz] from comment #11)
> ...
> >  
> > +class CrashReporterHelperRunnable : public nsCancelableRunnable {
> 
> s/nsCancelableRunnable/nsRunnable/

Aaron, change to nsRunnable or Runnable?

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

mozilla::Runnable due to khuey's latest changes.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6578e218bdba

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6578e218bdba